From 8f5322b64a307e159b48af11380190bdb9e65971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Tue, 6 Oct 2026 15:52:44 +0200 Subject: [PATCH 1/4] Add pruefbyte local and installable releases `pruefbyte local` reviews the current branch the way the CI job reviews its merge request and prints the findings instead of posting them. It shares the CI's config loading (.pruefbyte.yml and OCR rule files at the merge base), rule merging, OCR options, severity filtering and the fail_on_severity exit code. By default it reviews the working tree too, via a snapshot commit built with a throwaway index, so nothing in the repository changes. The API key falls back to the developer's own OCR config and then to the provider's env var. .pruefbyte.yml may now set llm.provider (OCR built-ins only), so provider and model live in the repository and GitLab variables hold only secrets. Installation: the module is now github.com/feinarbyte/pruefbyte so `go install` works, and v* tags publish GoReleaser binaries for Linux, macOS and Windows on amd64 and arm64. Every CI run builds them as a snapshot. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 33 ++++-- .goreleaser.yaml | 40 +++++++ README.md | 112 +++++++++++++++--- cmd/pruefbyte/local.go | 187 ++++++++++++++++++++++++++++++ cmd/pruefbyte/local_test.go | 161 +++++++++++++++++++++++++ cmd/pruefbyte/main.go | 91 ++++++++++----- cmd/pruefbyte/main_test.go | 10 ++ go.mod | 2 +- internal/config/config.go | 20 +++- internal/config/config_test.go | 20 ++++ internal/gitutil/git.go | 113 ++++++++++++++++++ internal/gitutil/git_test.go | 69 +++++++++++ internal/ocr/ocr_test.go | 17 ++- internal/ocr/rules.go | 2 +- internal/ocr/runner.go | 20 +++- internal/ocr/usercfg.go | 44 +++++++ internal/review/diff.go | 4 +- internal/review/diff_test.go | 4 +- internal/review/filter.go | 4 +- internal/review/format.go | 2 +- internal/review/local.go | 116 ++++++++++++++++++ internal/review/review.go | 34 +++--- internal/review/review_test.go | 6 +- pruefbyte.example.yml | 2 + templates/pruefbyte.gitlab-ci.yml | 6 +- 25 files changed, 1027 insertions(+), 92 deletions(-) create mode 100644 .goreleaser.yaml create mode 100644 cmd/pruefbyte/local.go create mode 100644 cmd/pruefbyte/local_test.go create mode 100644 internal/ocr/usercfg.go create mode 100644 internal/review/local.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b10d81..69ef25a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,21 +52,40 @@ jobs: - run: go test -race -coverprofile=coverage.out ./... - run: go tool cover -func=coverage.out | tail -1 + # Builds every release binary (linux, macOS, windows; amd64, arm64) from the + # same config the release job uses, so a broken release shows up in the PR. 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 + - uses: goreleaser/goreleaser-action@v7 + with: + version: v2.18.2 + args: build --snapshot --clean + + # Publishes the binaries for `pruefbyte local` as a GitHub release on v* tags. + release: + if: startsWith(github.ref, 'refs/tags/v') + needs: [lint, test, build] + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 # the changelog needs the previous tag + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + - uses: goreleaser/goreleaser-action@v7 + with: + version: v2.18.2 + args: release --clean env: - CGO_ENABLED: "0" - GOOS: linux - GOARCH: ${{ matrix.goarch }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} image: needs: [lint, test, build] diff --git a/.goreleaser.yaml b/.goreleaser.yaml new file mode 100644 index 0000000..fcc8d56 --- /dev/null +++ b/.goreleaser.yaml @@ -0,0 +1,40 @@ +# Release binaries for `pruefbyte local` on developer machines. CI runs this on +# v* tags; try it locally with: goreleaser release --snapshot --clean +version: 2 + +project_name: pruefbyte + +builds: + - main: ./cmd/pruefbyte + binary: pruefbyte + env: + - CGO_ENABLED=0 + flags: + - -trimpath + ldflags: + - -s -w -X main.version={{ .Version }} + goos: [linux, darwin, windows] + goarch: [amd64, arm64] + +archives: + - formats: [tar.gz] + format_overrides: + - goos: windows + formats: [zip] + # No version in the name, so .../releases/latest/download/pruefbyte_linux_arm64.tar.gz + # always gets the newest release; the README's install commands rely on this. + name_template: "{{ .ProjectName }}_{{ .Os }}_{{ .Arch }}" + files: + - LICENSE + - README.md + +checksum: + name_template: checksums.txt + +changelog: + use: github + sort: asc + +release: + footer: | + Container image: `ghcr.io/feinarbyte/pruefbyte:{{ .Version }}` (linux/amd64, linux/arm64). diff --git a/README.md b/README.md index f235f48..cc2c49d 100644 --- a/README.md +++ b/README.md @@ -15,13 +15,16 @@ from a dedicated bot account. ## 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): +2. **CI/CD variables** (group level, masked). Only secrets go here: | 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` | + + Provider, model and all review settings go into the repository's `.pruefbyte.yml` + (see [Configuration](#configuration)), so that `pruefbyte local` reviews with exactly + the same settings. A `PRUEFBYTE_LLM_MODEL` or similar CI variable would override the + file in CI only, and local runs would no longer match. 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 @@ -57,13 +60,16 @@ Settings are layered; later layers win: 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. +The repository file may set `llm.provider` (OCR built-in providers only), `llm.model`, `ocr.*` (except `binary` and `extra_args`) and `review.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there: custom providers with their own `llm.url` belong in the global file. 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 +llm: + provider: anthropic + model: claude-sonnet-5 ocr: effort: high exclude: ["**/generated/**"] @@ -116,15 +122,89 @@ Overriding `rules:` replaces the template's list, so keep its first two entries | 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 +## Local review + +Run the CI review on your machine before you push, so the bot has nothing left to say: + +```sh +pruefbyte local +``` + +It reviews what your merge request will contain: everything from the merge base with +the target branch (default: origin's HEAD; or e.g. `--target origin/develop`) up to your +working tree, including staged, unstaged and untracked files. Your index, branch and +files stay untouched. `--committed` reviews only commits, which is exactly what CI sees +after a push. Findings print in the terminal (`--format json` for tools), and nothing +is posted. + +The settings are the CI's, read the same way and from the same places: `.pruefbyte.yml` +and OCR rule files at the target branch, with the same rule merging, excludes, effort, +provider and model, and the same `review.min_severity` / `review.categories` filtering. +A finding that reaches `review.fail_on_severity` exits 3, as in CI, so the command +works as a pre-push hook. If you edit `.pruefbyte.yml` on your branch, the run tells +you that CI, and so the local run, uses the target branch's version until your change +is merged. One difference remains: CI gives OCR the merge request's title and +description as background; locally the branch name and commit messages stand in. + +The API key is the first one found of: +1. the env var named by `llm.api_key_env` (`PRUEFBYTE_LLM_API_KEY`), +2. your own OCR setup (`~/.opencodereview/config.json`: `api_key` or `api_key_cmd`), +3. the provider's env var, e.g. `ANTHROPIC_API_KEY`. + +No GitLab token is needed. If your CI uses a global config file (`PRUEFBYTE_CONFIG`), +pass the same file with `--config`. + +### Install + +pruefbyte needs OpenCodeReview's `ocr` on your PATH: + +```sh +npm install -g @alibaba-group/open-code-review # or: brew install open-code-review +``` + +Then install pruefbyte itself, in whichever way suits you. + +**Linux / macOS**, latest release into `~/.local/bin`: ```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 +os=$(uname -s | tr '[:upper:]' '[:lower:]'); arch=$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/') +mkdir -p ~/.local/bin +curl -fsSL "https://github.com/feinarbyte/pruefbyte/releases/latest/download/pruefbyte_${os}_${arch}.tar.gz" \ + | tar -xz -C ~/.local/bin pruefbyte +``` + +**Windows** (PowerShell), latest release into `%LOCALAPPDATA%\pruefbyte`: + +```powershell +$arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') { 'arm64' } else { 'amd64' } +$dir = "$env:LOCALAPPDATA\pruefbyte"; $zip = "$env:TEMP\pruefbyte.zip" +Invoke-WebRequest "https://github.com/feinarbyte/pruefbyte/releases/latest/download/pruefbyte_windows_$arch.zip" -OutFile $zip +Expand-Archive $zip $dir -Force; Remove-Item $zip +[Environment]::SetEnvironmentVariable('Path', "$([Environment]::GetEnvironmentVariable('Path', 'User'));$dir", 'User') ``` -`--dry-run` prints the discussions instead of posting them. `pruefbyte config print` shows the effective configuration. +Each release also lists the archives with a `checksums.txt` +([releases](https://github.com/feinarbyte/pruefbyte/releases)). + +**With Go** 1.25 or newer: + +```sh +go install github.com/feinarbyte/pruefbyte/cmd/pruefbyte@latest +``` + +**With Docker**, with `ocr` included and nothing to install. Run it as yourself so +new git objects in your repository stay yours: + +```sh +docker run --rm -it --user "$(id -u):$(id -g)" -v "$PWD:/repo" -w /repo \ + -e PRUEFBYTE_LLM_API_KEY ghcr.io/feinarbyte/pruefbyte pruefbyte local +``` + +Check the install with `pruefbyte version`. + +To try the CI path against a real merge request without posting, set +`PRUEFBYTE_GITLAB_TOKEN` and run `pruefbyte review --project group/project --mr 42 --dry-run`. +`pruefbyte config print` shows the effective configuration. ## Development @@ -135,11 +215,15 @@ 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. +GitHub Actions (`.github/workflows/ci.yml`) runs these checks plus `go mod tidy` on +pushes to `main` and `v*` tags and on every pull request. It also builds all release +binaries (Linux, macOS and Windows; amd64 and arm64) with GoReleaser +(`.goreleaser.yaml`). Once these 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. + +To release, push a tag such as `v0.1.0`. CI then publishes the image tags `0.1.0` +and `0.1`, plus a GitHub release with the binaries and checksums. Try the release +build locally with `goreleaser release --snapshot --clean`. Layout: diff --git a/cmd/pruefbyte/local.go b/cmd/pruefbyte/local.go new file mode 100644 index 0000000..bed3ca6 --- /dev/null +++ b/cmd/pruefbyte/local.go @@ -0,0 +1,187 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + + "github.com/feinarbyte/pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/gitutil" + "github.com/feinarbyte/pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/review" +) + +type localFlags struct { + repoDir string + target string + committed bool + format string +} + +func localCmd(g *globalFlags) *cobra.Command { + f := &localFlags{} + cmd := &cobra.Command{ + Use: "local", + Short: "Review your branch locally with the same settings as CI", + Long: `Review the current branch the way the CI job will review its merge request, +and print the findings instead of posting them. + +The review covers everything from the merge base with the target branch up to +your working tree: commits, staged and unstaged changes and untracked files. +--committed limits it to commits, which is exactly what CI sees after a push. +Settings come from the same places as in CI: the global config (--config), +.pruefbyte.yml and OCR rule files at the target branch, and PRUEFBYTE_* env vars. + +The API key comes from the env var named by llm.api_key_env +(PRUEFBYTE_LLM_API_KEY), else from your own OCR setup +(~/.opencodereview/config.json), else from the provider's env var such as +ANTHROPIC_API_KEY. No GitLab token is needed. + +Exits 3 if a finding reaches review.fail_on_severity, as in CI.`, + RunE: func(cmd *cobra.Command, _ []string) error { + return runLocal(cmd.Context(), cmd.OutOrStdout(), g, f) + }, + } + cmd.Flags().StringVar(&f.repoDir, "repo", "", "repository to review (default: the one containing the current directory)") + cmd.Flags().StringVar(&f.target, "target", "", "branch the merge request goes into (default: origin's HEAD, e.g. origin/main)") + cmd.Flags().BoolVar(&f.committed, "committed", false, "review only committed changes, ignoring the working tree") + cmd.Flags().StringVar(&f.format, "format", "text", "output format: text or json") + return cmd +} + +func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFlags) error { + if f.format != "text" && f.format != "json" { + return fmt.Errorf("--format must be text or json, got %q", f.format) + } + base, err := baseConfig(g) + if err != nil { + return err + } + dir := f.repoDir + if dir == "" { + dir = "." + } + if dir, err = filepath.Abs(dir); err != nil { + return err + } + repo := gitutil.Repo{Dir: dir} + top, err := repo.Toplevel(ctx) + if err != nil { + return fmt.Errorf("%s is not inside a git repository", dir) + } + repo.Dir = top + + target := f.target + if target == "" { + if target, err = repo.DefaultTarget(ctx); err != nil { + return err + } + } + head, err := repo.Head(ctx) + if err != nil { + return err + } + mergeBase, err := repo.MergeBase(ctx, target, head) + if err != nil { + return err + } + to := head + if !f.committed { + if to, err = repo.Snapshot(ctx); err != nil { + return fmt.Errorf("snapshotting the working tree: %w", err) + } + } + if mergeBase == to { + fmt.Fprintf(os.Stderr, "[pruefbyte] nothing to review: no changes against %s\n", target) + return nil + } + + // In CI the merge request's title and description are OCR background; locally + // the branch name and commit messages stand in for them. + branch, _ := repo.Branch(ctx) + log, _ := repo.Log(ctx, mergeBase, head) + + apiKey := os.Getenv(base.LLM.APIKeyEnv) + 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 + } + scope := "commits and working tree" + if f.committed { + scope = "commits only" + } + fmt.Fprintf(os.Stderr, "[pruefbyte] %s, %s\n", version, strings.SplitN(v, "\n", 2)[0]) + fmt.Fprintf(os.Stderr, "[pruefbyte] reviewing %s against %s (merge base %.8s, %s); this can take a few minutes\n", + branchOr(branch, head), target, mergeBase, scope) + + deps := review.Deps{ + OCR: runner, + Log: os.Stderr, + LoadConfig: configLoader(g, repo, true), + PrepareOCR: func(ctx context.Context, cfg config.Config) error { + key := apiKey + if key == "" { + var cmd string + key, cmd = ocr.UserCredential(cfg.LLM.Provider) + runner.APIKeyCmd = cmd + if key != "" || cmd != "" { + fmt.Fprintf(os.Stderr, "[pruefbyte] using the %s API key from your OCR config\n", cfg.LLM.Provider) + } + } + return runner.Configure(ctx, cfg.LLM, key, cfg.OCR.Language) + }, + ReadFileAt: repo.ShowFile, + } + out, err := review.Local(ctx, deps, review.LocalTarget{ + BaseSHA: mergeBase, HeadSHA: to, Title: branch, Description: log, + }, repo.Dir) + if err != nil { + return err + } + + if f.format == "json" { + enc := json.NewEncoder(stdout) + enc.SetIndent("", " ") + if err := enc.Encode(localJSON{ + Status: out.Result.Status, Model: out.Result.LLM.Model, Base: mergeBase, Head: to, + Findings: out.Findings, Gate: out.GateReason, + }); err != nil { + return err + } + } else { + review.WriteLocal(stdout, out) + } + if out.GateReason != "" { + return &exitCodeError{code: exitGate, err: errors.New(out.GateReason)} + } + return nil +} + +type localJSON struct { + Status string `json:"status"` + Model string `json:"model,omitempty"` + Base string `json:"base"` + Head string `json:"head"` + Findings []ocr.Comment `json:"findings"` + Gate string `json:"fail_on_severity_reached,omitempty"` +} + +func branchOr(branch, head string) string { + if branch != "" { + return branch + } + return head[:min(8, len(head))] +} diff --git a/cmd/pruefbyte/local_test.go b/cmd/pruefbyte/local_test.go new file mode 100644 index 0000000..4251a50 --- /dev/null +++ b/cmd/pruefbyte/local_test.go @@ -0,0 +1,161 @@ +package main + +import ( + "context" + "errors" + "fmt" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestLocalReviewsLikeCI runs the same branch through `review` (CI) and `local` +// and checks that OCR gets the same arguments, rules and provider settings. +func TestLocalReviewsLikeCI(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + repo := t.TempDir() + gitCmd(t, repo, "init", "-q", "-b", "main") + os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\n\nfunc main() {}\n"), 0o644) + // Provider and model live in the repository, next to the review settings. + os.WriteFile(filepath.Join(repo, ".pruefbyte.yml"), []byte(`llm: + provider: anthropic + model: claude-sonnet-5 +ocr: + effort: high + exclude: ["gen/**"] + rules: + - path: "**/*.go" + rule: "Wrap errors with %w." +review: + min_severity: medium + fail_on_severity: critical +`), 0o644) + os.MkdirAll(filepath.Join(repo, ".opencodereview"), 0o755) + os.WriteFile(filepath.Join(repo, ".opencodereview", "rule.json"), []byte(`{"rules":[{"path":"**/*.md","rule":"Docs in English."}]}`), 0o644) + gitCmd(t, repo, "add", ".") + gitCmd(t, repo, "commit", "-qm", "base") + base := gitCmd(t, repo, "rev-parse", "HEAD") + gitCmd(t, repo, "switch", "-qc", "feature") + os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\n\nvar m map[string]int\nfunc main() {}\n"), 0o644) + gitCmd(t, repo, "commit", "-qam", "Add a map") + head := gitCmd(t, repo, "rev-parse", "HEAD") + + srv := &fakeServer{base: base, head: head} + ts := httptest.NewServer(srv) + defer ts.Close() + + dir := t.TempDir() + // The global config only says where ocr is; everything else comes from the repo. + global := filepath.Join(dir, "pruefbyte.yml") + os.WriteFile(global, []byte(fmt.Sprintf("ocr:\n binary: %q\n", 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"}]}` + for k, v := range map[string]string{ + "FAKE_OCR": "1", "FAKE_OCR_RESULT": result, "FAKE_OCR_ARGS": filepath.Join(dir, "args.txt"), + "PRUEFBYTE_LLM_API_KEY": "sk-ant-test", "PRUEFBYTE_GITLAB_TOKEN": "glpat-bot", + } { + t.Setenv(k, v) + } + ciEnv := map[string]string{ + "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, "CI_MERGE_REQUEST_SOURCE_BRANCH_SHA": "", + } + type capture struct{ args, rule, settings string } + run := func(name string, args ...string) (capture, string, error) { + t.Helper() + prefix := filepath.Join(dir, name) + t.Setenv("FAKE_OCR_CAPTURE", prefix) + var stdout strings.Builder + cmd := rootCmd() + cmd.SetArgs(append(args, "--config", global)) + cmd.SetOut(&stdout) + err := cmd.ExecuteContext(context.Background()) + a, _ := os.ReadFile(filepath.Join(dir, "args.txt")) + r, _ := os.ReadFile(prefix + ".rule.json") + s, _ := os.ReadFile(prefix + ".settings.txt") + return capture{string(a), string(r), string(s)}, stdout.String(), err + } + + for k, v := range ciEnv { + t.Setenv(k, v) + } + ciCap, _, err := run("ci", "review") + var ec *exitCodeError + if !errors.As(err, &ec) || ec.code != exitGate { + t.Fatalf("CI run: want gate exit, got %v", err) + } + for k := range ciEnv { + t.Setenv(k, "") // a developer's machine has no CI variables + } + localCap, stdout, err := run("local", "local", "--repo", repo, "--target", "main", "--committed") + if !errors.As(err, &ec) || ec.code != exitGate { + t.Fatalf("local run: want the same gate exit as CI, got %v", err) + } + + // Paths of per-run temp files differ; everything else must be identical. + normalize := func(args string) string { + parts := strings.Split(args, "\n") + for i := 1; i < len(parts); i++ { + switch parts[i-1] { + case "--output", "--rule", "--background-file": + parts[i] = "" + } + } + return strings.Join(parts, "\n") + } + if a, b := normalize(ciCap.args), normalize(localCap.args); a != b { + t.Errorf("ocr arguments differ\nCI:\n%s\nlocal:\n%s", a, b) + } + if ciCap.rule == "" || ciCap.rule != localCap.rule { + t.Errorf("rule files differ\nCI:\n%s\nlocal:\n%s", ciCap.rule, localCap.rule) + } + if !strings.Contains(ciCap.rule, "Wrap errors") || !strings.Contains(ciCap.rule, "Docs in English") { + t.Errorf("rules not merged:\n%s", ciCap.rule) + } + if ciCap.settings == "" || ciCap.settings != localCap.settings { + t.Errorf("ocr settings differ\nCI:\n%s\nlocal:\n%s", ciCap.settings, localCap.settings) + } + for _, want := range []string{"--from\n" + base, "--to\n" + head, "--provider\nanthropic", "--model\nclaude-sonnet-5", "--effort\nhigh"} { + if !strings.Contains(localCap.args, want) { + t.Errorf("local args missing %q:\n%s", want, localCap.args) + } + } + // Same filtering as CI: the low finding is below min_severity. + for _, want := range []string{"main.go:3", "Critical · bug", "Writing to a nil map panics.", "1 more below review.min_severity", "CI would fail"} { + if !strings.Contains(stdout, want) { + t.Errorf("local output misses %q:\n%s", want, stdout) + } + } + if strings.Contains(stdout, "Nitpick") { + t.Errorf("filtered finding printed:\n%s", stdout) + } + + // By default uncommitted work is reviewed too, without touching the repository. + os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\n\nvar m = map[string]int{}\nfunc main() {}\n"), 0o644) + os.WriteFile(filepath.Join(repo, "new.go"), []byte("package main\n"), 0o644) + status := gitCmd(t, repo, "status", "--porcelain") + wipCap, _, _ := run("wip", "local", "--repo", repo, "--target", "main") + to := "" + parts := strings.Split(wipCap.args, "\n") + for i, p := range parts { + if p == "--to" && i+1 < len(parts) { + to = parts[i+1] + } + } + if to == "" || to == head { + t.Fatalf("working tree not reviewed: --to %q", to) + } + if got := gitCmd(t, repo, "show", to+":new.go"); got != "package main" { + t.Errorf("untracked file missing from the reviewed snapshot: %q", got) + } + if gitCmd(t, repo, "rev-parse", "HEAD") != head || gitCmd(t, repo, "status", "--porcelain") != status { + t.Error("local review changed the repository") + } +} diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index 4a242d3..f40a7e4 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -9,21 +9,33 @@ import ( "os" "os/signal" "path/filepath" + "runtime/debug" "strings" "syscall" "github.com/spf13/cobra" - "pruefbyte/internal/ci" - "pruefbyte/internal/config" - "pruefbyte/internal/gitlab" - "pruefbyte/internal/gitutil" - "pruefbyte/internal/ocr" - "pruefbyte/internal/review" + "github.com/feinarbyte/pruefbyte/internal/ci" + "github.com/feinarbyte/pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/gitutil" + "github.com/feinarbyte/pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/review" ) +// version is set by release builds (-X main.version=...). `go install ...@vX.Y.Z` +// builds get it from the module version instead. var version = "dev" +func init() { + if version != "dev" { + return + } + if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "" && bi.Main.Version != "(devel)" { + version = strings.TrimPrefix(bi.Main.Version, "v") + } +} + // Exit codes. const ( exitError = 1 @@ -67,7 +79,7 @@ func rootCmd() *cobra.Command { } 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{ + root.AddCommand(reviewCmd(g), localCmd(g), configCmd(g), &cobra.Command{ Use: "version", Short: "Print the version", Run: func(cmd *cobra.Command, _ []string) { fmt.Fprintln(cmd.OutOrStdout(), "pruefbyte", version) }, @@ -85,6 +97,42 @@ func baseConfig(g *globalFlags) (config.Config, error) { return cfg, cfg.ApplyEnv(os.LookupEnv) } +// configLoader returns the effective config for a review whose base commit is +// known: defaults < global file < repository file at the base commit < env. +// CI and local runs both use it, so they review with the same settings. +func configLoader(g *globalFlags, repo gitutil.Repo, local bool) func(context.Context, string) (config.Config, error) { + return 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 local { + // CI reads the file at the merge request's base, so edits on the branch + // only apply once merged. Say so instead of silently ignoring them. + work, err := os.ReadFile(filepath.Join(repo.Dir, config.RepoConfigFile)) + if (err == nil) != found || string(work) != string(data) { + fmt.Fprintf(os.Stderr, "[pruefbyte] note: your %s differs from the target branch's; CI uses the target's until your change is merged, and so does this run\n", config.RepoConfigFile) + } + } + } + if err := cfg.ApplyEnv(os.LookupEnv); err != nil { + return cfg, err + } + return cfg, cfg.Validate() + } +} + func configCmd(g *globalFlags) *cobra.Command { cmd := &cobra.Command{Use: "config", Short: "Inspect configuration"} var repoFile string @@ -204,31 +252,10 @@ func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { 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() - }, + GitLab: api, + OCR: runner, + Log: os.Stderr, + LoadConfig: configLoader(g, repo, false), 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) diff --git a/cmd/pruefbyte/main_test.go b/cmd/pruefbyte/main_test.go index fb7eb0a..9fd0965 100644 --- a/cmd/pruefbyte/main_test.go +++ b/cmd/pruefbyte/main_test.go @@ -50,6 +50,16 @@ func fakeOCR(args []string) int { } } _ = os.WriteFile(os.Getenv("FAKE_OCR_ARGS"), []byte(strings.Join(args, "\n")), 0o600) + if capture := os.Getenv("FAKE_OCR_CAPTURE"); capture != "" { + for i, a := range args { + if a == "--rule" { + rule, _ := os.ReadFile(args[i+1]) + _ = os.WriteFile(capture+".rule.json", rule, 0o600) + } + } + settings, _ := os.ReadFile(filepath.Join(os.Getenv("HOME"), "settings.txt")) + _ = os.WriteFile(capture+".settings.txt", settings, 0o600) + } _ = os.WriteFile(out, []byte(os.Getenv("FAKE_OCR_RESULT")), 0o600) return 0 } diff --git a/go.mod b/go.mod index 8461b24..220e5ed 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module pruefbyte +module github.com/feinarbyte/pruefbyte go 1.25.14 diff --git a/internal/config/config.go b/internal/config/config.go index 3878ae9..b285022 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -85,7 +85,10 @@ type Review struct { // ocr.binary, ocr.extra_args) stays under the operator's control. type repoConfig struct { LLM struct { - Model string `yaml:"model"` + // Provider must be an OCR built-in: their endpoints are fixed by OCR, so the + // repository can pick a vendor but cannot point the API key at its own URL. + Provider string `yaml:"provider"` + Model string `yaml:"model"` } `yaml:"llm"` OCR struct { Effort string `yaml:"effort"` @@ -133,7 +136,16 @@ func (c *Config) ApplyGlobal(data []byte) error { func (c *Config) ApplyRepo(data []byte) error { var probe repoConfig if err := decodeStrict(data, &probe); err != nil { - return fmt.Errorf("%s: %w (only llm.model, ocr.*, and review.* are allowed here, except ocr.binary and ocr.extra_args)", RepoConfigFile, err) + return fmt.Errorf("%s: %w (only llm.provider, llm.model, ocr.* and review.* are allowed here, except ocr.binary and ocr.extra_args)", RepoConfigFile, err) + } + if p := probe.LLM.Provider; p != "" { + if !IsBuiltinProvider(p) { + return fmt.Errorf("%s: llm.provider %q is not an OCR built-in provider; custom providers (llm.url, llm.protocol) can only be set in the global config", RepoConfigFile, p) + } + if p != c.LLM.Provider { + // An endpoint configured for the global provider does not belong to this one. + c.LLM.URL, c.LLM.Protocol = "", "" + } } return decodeStrict(data, c) } @@ -177,10 +189,10 @@ var protocols = map[string]bool{"anthropic": true, "openai": true, "openai-respo func (c Config) Validate() error { var errs []error if c.LLM.Provider == "" { - errs = append(errs, errors.New("llm.provider is required")) + errs = append(errs, errors.New("llm.provider is required: set it in .pruefbyte.yml or the global config")) } if c.LLM.Model == "" { - errs = append(errs, errors.New("llm.model is required")) + errs = append(errs, errors.New("llm.model is required: set it in .pruefbyte.yml or the global config")) } if !IsBuiltinProvider(c.LLM.Provider) && c.LLM.Provider != "" { if !protocols[c.LLM.Protocol] { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index eec6c4e..cfb0c3c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -166,3 +166,23 @@ func TestEnvBadValue(t *testing.T) { t.Fatal("non-numeric int accepted") } } + +func TestRepoFileSetsBuiltinProvider(t *testing.T) { + cfg := Default() + if err := cfg.ApplyGlobal([]byte("llm:\n provider: gateway\n protocol: openai\n url: https://gw.internal/v1\n model: x\n")); err != nil { + t.Fatal(err) + } + if err := cfg.ApplyRepo([]byte("llm:\n provider: anthropic\n model: claude-sonnet-5\n")); err != nil { + t.Fatal(err) + } + if cfg.LLM.Provider != "anthropic" || cfg.LLM.Model != "claude-sonnet-5" { + t.Errorf("llm = %+v", cfg.LLM) + } + // The gateway's endpoint must not be used as anthropic's URL. + if cfg.LLM.URL != "" || cfg.LLM.Protocol != "" { + t.Errorf("endpoint of the global provider kept: %+v", cfg.LLM) + } + if err := cfg.Validate(); err != nil { + t.Error(err) + } +} diff --git a/internal/gitutil/git.go b/internal/gitutil/git.go index 5e02e22..f81c46d 100644 --- a/internal/gitutil/git.go +++ b/internal/gitutil/git.go @@ -5,15 +5,24 @@ import ( "bytes" "context" "fmt" + "os" "os/exec" + "path/filepath" "strings" ) type Repo struct{ Dir string } func (r Repo) run(ctx context.Context, args ...string) ([]byte, error) { + return r.runEnv(ctx, nil, args...) +} + +func (r Repo) runEnv(ctx context.Context, env []string, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, "git", append([]string{"-c", "safe.directory=*"}, args...)...) cmd.Dir = r.Dir + if env != nil { + cmd.Env = append(os.Environ(), env...) + } var stderr bytes.Buffer cmd.Stderr = &stderr out, err := cmd.Output() @@ -65,3 +74,107 @@ func (r Repo) EnsureCommits(ctx context.Context, fetchRefs []string, shas ...str } return nil } + +func (r Repo) revParse(ctx context.Context, rev string) (string, error) { + out, err := r.run(ctx, "rev-parse", "--verify", "--quiet", rev+"^{commit}") + return strings.TrimSpace(string(out)), err +} + +// DefaultTarget guesses the branch merge requests go into: origin's HEAD, else +// origin/main or origin/master, else a local main or master. +func (r Repo) DefaultTarget(ctx context.Context) (string, error) { + if out, err := r.run(ctx, "symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"); err == nil { + if ref := strings.TrimSpace(string(out)); ref != "" { + return ref, nil + } + } + for _, ref := range []string{"origin/main", "origin/master", "main", "master"} { + if _, err := r.revParse(ctx, ref); err == nil { + return ref, nil + } + } + return "", fmt.Errorf("cannot tell the target branch: pass --target (e.g. origin/main)") +} + +// MergeBase returns the commit a merge request from HEAD into target would be +// reviewed from, like GitLab's diff base. +func (r Repo) MergeBase(ctx context.Context, target, head string) (string, error) { + if _, err := r.revParse(ctx, target); err != nil { + return "", fmt.Errorf("target %q is not a commit in %s (fetch it, or pass --target)", target, r.Dir) + } + out, err := r.run(ctx, "merge-base", target, head) + if err != nil { + return "", fmt.Errorf("no common ancestor of %s and %s: %w", target, head, err) + } + return strings.TrimSpace(string(out)), nil +} + +// Head returns the commit HEAD points to. +func (r Repo) Head(ctx context.Context) (string, error) { + sha, err := r.revParse(ctx, "HEAD") + if err != nil { + return "", fmt.Errorf("%s has no commits yet", r.Dir) + } + return sha, nil +} + +// Snapshot returns a commit holding the working tree as it is now: tracked +// changes, staged or not, plus untracked files that are not ignored. It uses a +// throwaway index, so the real index, HEAD and branches stay untouched; the +// commit is unreferenced and git eventually prunes it. Without changes it +// returns HEAD itself. +func (r Repo) Snapshot(ctx context.Context) (string, error) { + head, err := r.Head(ctx) + if err != nil { + return "", err + } + tmp, err := os.MkdirTemp("", "pruefbyte-index-") + if err != nil { + return "", err + } + defer os.RemoveAll(tmp) + env := []string{"GIT_INDEX_FILE=" + filepath.Join(tmp, "index")} + if _, err := r.runEnv(ctx, env, "read-tree", head); err != nil { + return "", err + } + if _, err := r.runEnv(ctx, env, "add", "--all", "--", "."); err != nil { + return "", err + } + out, err := r.runEnv(ctx, env, "write-tree") + if err != nil { + return "", err + } + tree := strings.TrimSpace(string(out)) + if out, err := r.run(ctx, "rev-parse", head+"^{tree}"); err == nil && strings.TrimSpace(string(out)) == tree { + return head, nil + } + out, err = r.runEnv(ctx, []string{ + "GIT_AUTHOR_NAME=pruefbyte", "GIT_AUTHOR_EMAIL=pruefbyte@localhost", + "GIT_COMMITTER_NAME=pruefbyte", "GIT_COMMITTER_EMAIL=pruefbyte@localhost", + }, "commit-tree", tree, "-p", head, "-m", "pruefbyte local review snapshot") + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + +// Log returns the subjects and bodies of the commits in base..head, oldest first. +func (r Repo) Log(ctx context.Context, base, head string) (string, error) { + out, err := r.run(ctx, "log", "--reverse", "--format=%B", base+".."+head) + return strings.TrimSpace(string(out)), err +} + +// Toplevel returns the root of the working tree containing r.Dir. +func (r Repo) Toplevel(ctx context.Context) (string, error) { + out, err := r.run(ctx, "rev-parse", "--show-toplevel") + if err != nil { + return "", err + } + return filepath.FromSlash(strings.TrimSpace(string(out))), nil +} + +// Branch returns the current branch name, or "" when HEAD is detached. +func (r Repo) Branch(ctx context.Context) (string, error) { + out, err := r.run(ctx, "symbolic-ref", "--quiet", "--short", "HEAD") + return strings.TrimSpace(string(out)), err +} diff --git a/internal/gitutil/git_test.go b/internal/gitutil/git_test.go index bafd77b..e65b334 100644 --- a/internal/gitutil/git_test.go +++ b/internal/gitutil/git_test.go @@ -54,3 +54,72 @@ func TestShowFileAndEnsureCommits(t *testing.T) { t.Error("EnsureCommits with an unknown commit and no remote should fail") } } + +func TestSnapshotAndMergeBase(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + dir := t.TempDir() + git(t, dir, "init", "-q", "-b", "main") + os.WriteFile(filepath.Join(dir, "a.txt"), []byte("one\n"), 0o644) + os.WriteFile(filepath.Join(dir, ".gitignore"), []byte("ignored.txt\n"), 0o644) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "base") + base := git(t, dir, "rev-parse", "HEAD") + git(t, dir, "switch", "-qc", "feature") + os.WriteFile(filepath.Join(dir, "a.txt"), []byte("two\n"), 0o644) + git(t, dir, "commit", "-qam", "change a") + head := git(t, dir, "rev-parse", "HEAD") + + r := Repo{Dir: dir} + ctx := context.Background() + if mb, err := r.MergeBase(ctx, "main", "HEAD"); err != nil || mb != base { + t.Fatalf("MergeBase = %s, %v; want %s", mb, err, base) + } + if target, err := r.DefaultTarget(ctx); err != nil || target != "main" { + t.Errorf("DefaultTarget = %q, %v", target, err) + } + if _, err := r.MergeBase(ctx, "nope", "HEAD"); err == nil { + t.Error("unknown target accepted") + } + if snap, err := r.Snapshot(ctx); err != nil || snap != head { + t.Errorf("clean tree: Snapshot = %s, %v; want HEAD", snap, err) + } + + // Unstaged, staged and untracked changes are all in the snapshot; ignored files are not. + os.WriteFile(filepath.Join(dir, "a.txt"), []byte("three\n"), 0o644) + os.WriteFile(filepath.Join(dir, "staged.txt"), []byte("s\n"), 0o644) + git(t, dir, "add", "staged.txt") + os.WriteFile(filepath.Join(dir, "new.txt"), []byte("n\n"), 0o644) + os.WriteFile(filepath.Join(dir, "ignored.txt"), []byte("i\n"), 0o644) + statusBefore := git(t, dir, "status", "--porcelain") + snap, err := r.Snapshot(ctx) + if err != nil || snap == head { + t.Fatalf("Snapshot = %s, %v", snap, err) + } + if got := git(t, dir, "show", snap+":a.txt"); got != "three" { + t.Errorf("a.txt in snapshot = %q", got) + } + files := git(t, dir, "ls-tree", "--name-only", snap) + for _, want := range []string{"staged.txt", "new.txt"} { + if !strings.Contains(files, want) { + t.Errorf("%s missing from snapshot: %s", want, files) + } + } + if strings.Contains(files, "ignored.txt") { + t.Error("ignored file in snapshot") + } + if parent := git(t, dir, "rev-parse", snap+"^"); parent != head { + t.Errorf("snapshot parent = %s, want HEAD", parent) + } + // Nothing about the user's repository changed. + if now := git(t, dir, "rev-parse", "HEAD"); now != head { + t.Error("HEAD moved") + } + if after := git(t, dir, "status", "--porcelain"); after != statusBefore { + t.Errorf("status changed:\nbefore %q\nafter %q", statusBefore, after) + } + if log, err := r.Log(ctx, base, head); err != nil || log != "change a" { + t.Errorf("Log = %q, %v", log, err) + } +} diff --git a/internal/ocr/ocr_test.go b/internal/ocr/ocr_test.go index 5750f09..e5d57ed 100644 --- a/internal/ocr/ocr_test.go +++ b/internal/ocr/ocr_test.go @@ -7,7 +7,7 @@ import ( "strings" "testing" - "pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/config" ) func TestParseResult(t *testing.T) { @@ -241,3 +241,18 @@ func TestExtraBodyWithNonStringKeys(t *testing.T) { t.Errorf("extra_body = %q", got) } } + +func TestUserCredential(t *testing.T) { + p := filepath.Join(t.TempDir(), "config.json") + os.WriteFile(p, []byte(`{"provider":"anthropic","providers":{"anthropic":{"api_key":"sk-user"},"openai":{"api_key_cmd":"op read x"}}, + "custom_providers":{"gw":{"api_key":"gw-key"}}}`), 0o600) + cases := map[string][2]string{"anthropic": {"sk-user", ""}, "openai": {"", "op read x"}, "gw": {"gw-key", ""}, "deepseek": {"", ""}} + for provider, want := range cases { + if k, c := userCredential(p, provider); k != want[0] || c != want[1] { + t.Errorf("%s: got %q %q, want %q", provider, k, c, want) + } + } + if k, c := userCredential(filepath.Join(t.TempDir(), "missing.json"), "anthropic"); k != "" || c != "" { + t.Error("missing config should give nothing") + } +} diff --git a/internal/ocr/rules.go b/internal/ocr/rules.go index f6eacff..d376082 100644 --- a/internal/ocr/rules.go +++ b/internal/ocr/rules.go @@ -7,7 +7,7 @@ import ( "slices" "strings" - "pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/config" ) // RuleFile is OCR's rule.json schema. diff --git a/internal/ocr/runner.go b/internal/ocr/runner.go index 0dcf7c5..6ed2b08 100644 --- a/internal/ocr/runner.go +++ b/internal/ocr/runner.go @@ -19,7 +19,7 @@ import ( "syscall" "time" - "pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/config" ) type Runner struct { @@ -32,6 +32,8 @@ type Runner struct { // SecretEnv names env vars ocr must not inherit, such as the renamed // gitlab.token_env and llm.api_key_env. SecretEnv []string + // APIKeyCmd is used as the provider's api_key_cmd when no API key is given. + APIKeyCmd string } // NewRunner creates a runner with a fresh temporary home directory. Call Close to remove it. @@ -108,10 +110,7 @@ func (r *Runner) command(ctx context.Context, args ...string) (*exec.Cmd, error) // 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 - } + section := providerSection(llm.Provider) var kv [][2]string add := func(k, v string) { if v != "" { @@ -162,6 +161,14 @@ func ConfigSettings(llm config.LLM, apiKey, language string) ([][2]string, error return kv, nil } +// providerSection is the config.json section holding a provider's settings. +func providerSection(provider string) string { + if config.IsBuiltinProvider(provider) { + return "providers." + provider + } + return "custom_providers." + provider +} + // 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 { @@ -195,6 +202,9 @@ func (r *Runner) Configure(ctx context.Context, llm config.LLM, apiKey, language if err != nil { return err } + if apiKey == "" && r.APIKeyCmd != "" { + settings = append(settings, [2]string{providerSection(llm.Provider) + ".api_key_cmd", r.APIKeyCmd}) + } 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]) diff --git a/internal/ocr/usercfg.go b/internal/ocr/usercfg.go new file mode 100644 index 0000000..7f4711f --- /dev/null +++ b/internal/ocr/usercfg.go @@ -0,0 +1,44 @@ +package ocr + +import ( + "encoding/json" + "os" + "path/filepath" + + "github.com/feinarbyte/pruefbyte/internal/config" +) + +// UserCredential returns the API key, or the command producing it, that the +// user's own OCR setup (~/.opencodereview/config.json) has for provider. Local +// runs use it so a developer who already runs OCR needs no extra setup. Both are +// empty when there is none. +func UserCredential(provider string) (apiKey, apiKeyCmd string) { + home, err := os.UserHomeDir() + if err != nil { + return "", "" + } + return userCredential(filepath.Join(home, ".opencodereview", "config.json"), provider) +} + +func userCredential(path, provider string) (string, string) { + data, err := os.ReadFile(path) + if err != nil { + return "", "" + } + type cred struct { + APIKey string `json:"api_key"` + APIKeyCmd string `json:"api_key_cmd"` + } + var doc struct { + Providers map[string]cred `json:"providers"` + CustomProviders map[string]cred `json:"custom_providers"` + } + if json.Unmarshal(data, &doc) != nil { + return "", "" + } + c := doc.CustomProviders[provider] + if config.IsBuiltinProvider(provider) { + c = doc.Providers[provider] + } + return c.APIKey, c.APIKeyCmd +} diff --git a/internal/review/diff.go b/internal/review/diff.go index f629b09..d17140d 100644 --- a/internal/review/diff.go +++ b/internal/review/diff.go @@ -5,8 +5,8 @@ import ( "strconv" "strings" - "pruefbyte/internal/gitlab" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) diff --git a/internal/review/diff_test.go b/internal/review/diff_test.go index 27be819..3fd3ede 100644 --- a/internal/review/diff_test.go +++ b/internal/review/diff_test.go @@ -4,8 +4,8 @@ import ( "strings" "testing" - "pruefbyte/internal/gitlab" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) func TestParsePatch(t *testing.T) { diff --git a/internal/review/filter.go b/internal/review/filter.go index b690b0a..f4e1a09 100644 --- a/internal/review/filter.go +++ b/internal/review/filter.go @@ -5,8 +5,8 @@ import ( "sort" "strings" - "pruefbyte/internal/config" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) // filterFindings drops findings below review.min_severity or outside diff --git a/internal/review/format.go b/internal/review/format.go index d66a92c..b7bce73 100644 --- a/internal/review/format.go +++ b/internal/review/format.go @@ -7,7 +7,7 @@ import ( "regexp" "strings" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) const ( diff --git a/internal/review/local.go b/internal/review/local.go new file mode 100644 index 0000000..6901e7c --- /dev/null +++ b/internal/review/local.go @@ -0,0 +1,116 @@ +package review + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/ocr" +) + +// LocalTarget is what a local run reviews: the changes from BaseSHA (where the +// merge request would branch off) to HeadSHA. Title and Description stand in for +// the merge request's, which OCR gets as background. +type LocalTarget struct { + BaseSHA, HeadSHA string + Title, Description string +} + +type LocalOutcome struct { + Result *ocr.Result + // Findings are what CI would publish, after review.min_severity and + // review.categories, most severe first. + Findings []ocr.Comment + GateReason string +} + +// Local reviews a target the way Run reviews a merge request, using the same +// config (read at the base commit), rule files and filters, but publishes +// nothing. d.GitLab is not used. +func Local(ctx context.Context, d Deps, t LocalTarget, repoDir string) (*LocalOutcome, error) { + cfg, err := d.LoadConfig(ctx, t.BaseSHA) + if err != nil { + return nil, err + } + if d.PrepareOCR != nil { + if err := d.PrepareOCR(ctx, cfg); err != nil { + return nil, err + } + } + mr := &gitlab.MR{BaseSHA: t.BaseSHA, StartSHA: t.BaseSHA, HeadSHA: t.HeadSHA, Title: t.Title, Description: t.Description} + ro, err := reviewOptions(ctx, cfg, mr, 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 { + _ = d.SaveResult(raw) + } + if err == nil && res.Failed() { + err = fmt.Errorf("ocr reported status %q: %s", res.Status, res.Message) + } + if err != nil { + // ocr's output has already streamed to d.Log; repeating it would only add noise. + var re *ocr.RunError + if errors.As(err, &re) { + err = fmt.Errorf("%w (see ocr's output above)", re.Err) + } + return nil, fmt.Errorf("%w: %w", ErrReviewFailed, err) + } + findings := filterFindings(res.Comments, cfg.Review) + return &LocalOutcome{Result: res, Findings: findings, GateReason: gateReason(findings, cfg.Review)}, nil +} + +// WriteLocal prints a local review's findings for a terminal. +func WriteLocal(w io.Writer, o *LocalOutcome) { + for _, c := range o.Findings { + head := c.Path + switch { + case c.StartLine > 0 && c.EndLine > c.StartLine: + head += fmt.Sprintf(":%d-%d", c.StartLine, c.EndLine) + case c.EndLine > 0: + head += fmt.Sprintf(":%d", c.EndLine) + case c.StartLine > 0: + head += fmt.Sprintf(":%d", c.StartLine) + } + if b := strings.Trim(badge(c), "*"); b != "" { + head += " " + b + } + fmt.Fprintf(w, "%s\n%s\n", head, indent(strings.TrimSpace(c.Content), " ")) + if code := strings.TrimRight(c.SuggestionCode, "\n"); code != "" { + fmt.Fprintf(w, "\n Suggested change:\n%s\n", indent(code, " ")) + } + fmt.Fprintln(w) + } + hidden := len(o.Result.Comments) - len(o.Findings) + if len(o.Findings) == 0 && hidden == 0 { + fmt.Fprintln(w, "No findings.") + } else { + fmt.Fprintf(w, "%d finding(s)", len(o.Findings)) + if hidden > 0 { + fmt.Fprintf(w, ", %d more below review.min_severity or outside review.categories", hidden) + } + fmt.Fprintln(w, ".") + } + if !o.Result.Complete() { + fmt.Fprintf(w, "Warning: OCR's review was incomplete (status %s); CI may report more.\n", o.Result.Status) + } + if o.GateReason != "" { + fmt.Fprintf(w, "CI would fail this merge request: %s\n", o.GateReason) + } +} + +func indent(s, prefix string) string { + lines := strings.Split(s, "\n") + for i, l := range lines { + if l != "" { + lines[i] = prefix + l + } + } + return strings.Join(lines, "\n") +} diff --git a/internal/review/review.go b/internal/review/review.go index 2453ed3..5a39242 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -13,9 +13,9 @@ import ( "path/filepath" "strings" - "pruefbyte/internal/config" - "pruefbyte/internal/gitlab" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) // Reviewer runs the review engine. *ocr.Runner implements it. @@ -234,23 +234,29 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { } } - 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 - } - } - } + out.GateReason = gateReason(findings, cfg.Review) + out.GateFailed = out.GateReason != "" if st.Failed > 0 { return out, fmt.Errorf("%d comment(s) could not be posted", st.Failed) } return out, nil } +// gateReason explains why review.fail_on_severity trips for these findings, or "". +func gateReason(findings []ocr.Comment, r config.Review) string { + if r.FailOnSeverity == "" { + return "" + } + gate := config.SeverityRank(r.FailOnSeverity) + for _, c := range findings { + if config.SeverityRank(c.Severity) >= gate { + return fmt.Sprintf("%s finding in %s (%s): review.fail_on_severity is %s", + strings.ToLower(c.Severity), c.Path, lineLabel(c), r.FailOnSeverity) + } + } + return "" +} + 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, diff --git a/internal/review/review_test.go b/internal/review/review_test.go index a111e8d..9696536 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -11,9 +11,9 @@ import ( gl "gitlab.com/gitlab-org/api/client-go" - "pruefbyte/internal/config" - "pruefbyte/internal/gitlab" - "pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/config" + "github.com/feinarbyte/pruefbyte/internal/gitlab" + "github.com/feinarbyte/pruefbyte/internal/ocr" ) const botID = 42 diff --git a/pruefbyte.example.yml b/pruefbyte.example.yml index 27cb56c..2a9a0e1 100644 --- a/pruefbyte.example.yml +++ b/pruefbyte.example.yml @@ -7,6 +7,8 @@ gitlab: token_env: PRUEFBYTE_GITLAB_TOKEN # env var holding the bot's PAT llm: + # provider and model are better set in the repository's .pruefbyte.yml, where + # `pruefbyte local` finds them too; only custom providers (url, protocol) need this file. 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 diff --git a/templates/pruefbyte.gitlab-ci.yml b/templates/pruefbyte.gitlab-ci.yml index a3c7167..c8b9e73 100644 --- a/templates/pruefbyte.gitlab-ci.yml +++ b/templates/pruefbyte.gitlab-ci.yml @@ -15,11 +15,11 @@ # 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): +# Required CI/CD variables (masked, ideally set at group level), secrets only: # 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 +# Provider, model and review settings belong in the repository's .pruefbyte.yml, +# so `pruefbyte local` on a developer machine reviews exactly like this job. # Optional: # PRUEFBYTE_CONFIG path to a global config file (a "File" type CI variable works well) # PRUEFBYTE_IMAGE image to run (default below) From 217af40f443c84c6cc86cbe5a7681dd3b621f1fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Tue, 6 Oct 2026 16:00:18 +0200 Subject: [PATCH 2/4] Address review findings for local review - A repository may set llm.provider only when the global config leaves it unset, so a merged .pruefbyte.yml cannot send the operator's shared API key to another vendor. - Run api_key_cmd from the developer's OCR config in pruefbyte with the real environment (keychains, pass, ~/ paths), not under ocr's private HOME. - Snapshot starts from a copy of the real index (sparse checkouts, no full rehash); CRLF-insensitive .pruefbyte.yml change note; JSON output always has a findings array; keep the real Toplevel error. - Share the OCR invocation between CI and local runs, and the line range formatting between comments and terminal output. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 2 +- cmd/pruefbyte/local.go | 16 ++++++++++--- cmd/pruefbyte/main.go | 4 +++- internal/config/config.go | 9 ++++--- internal/config/config_test.go | 36 +++++++++++++++++++++------- internal/gitutil/git.go | 43 +++++++++++++++++++++++++++++----- internal/ocr/ocr_test.go | 12 ++++++++++ internal/ocr/runner.go | 5 ---- internal/ocr/usercfg.go | 38 ++++++++++++++++++++++++++++++ internal/review/format.go | 20 ++++++++++++---- internal/review/local.go | 22 +++++------------ internal/review/review.go | 30 +++++++++++++++--------- pruefbyte.example.yml | 2 ++ 13 files changed, 180 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index cc2c49d..cda4b89 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ Settings are layered; later layers win: 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 set `llm.provider` (OCR built-in providers only), `llm.model`, `ocr.*` (except `binary` and `extra_args`) and `review.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there: custom providers with their own `llm.url` belong in the global file. +The repository file may set `llm.provider` (OCR built-in providers only, and only when the global config does not set one: a provider the operator names keeps the shared API key with that vendor), `llm.model`, `ocr.*` (except `binary` and `extra_args`) and `review.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there: custom providers with their own `llm.url` belong in the global file. 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. diff --git a/cmd/pruefbyte/local.go b/cmd/pruefbyte/local.go index bed3ca6..f2b3a59 100644 --- a/cmd/pruefbyte/local.go +++ b/cmd/pruefbyte/local.go @@ -74,7 +74,7 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla repo := gitutil.Repo{Dir: dir} top, err := repo.Toplevel(ctx) if err != nil { - return fmt.Errorf("%s is not inside a git repository", dir) + return fmt.Errorf("%s is not inside a git repository: %w", dir, err) } repo.Dir = top @@ -136,8 +136,15 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla if key == "" { var cmd string key, cmd = ocr.UserCredential(cfg.LLM.Provider) - runner.APIKeyCmd = cmd - if key != "" || cmd != "" { + if key == "" && cmd != "" { + // ocr would run the command under pruefbyte's private HOME, where + // keychains, pass or ~/ files are not found; run it here instead. + var err error + if key, err = ocr.RunKeyCommand(ctx, cmd); err != nil { + return fmt.Errorf("api_key_cmd for %s from your OCR config: %w", cfg.LLM.Provider, err) + } + } + if key != "" { fmt.Fprintf(os.Stderr, "[pruefbyte] using the %s API key from your OCR config\n", cfg.LLM.Provider) } } @@ -153,6 +160,9 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla } if f.format == "json" { + if out.Findings == nil { + out.Findings = []ocr.Comment{} // "findings": [], not null + } enc := json.NewEncoder(stdout) enc.SetIndent("", " ") if err := enc.Encode(localJSON{ diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index f40a7e4..00e16d4 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -120,8 +120,10 @@ func configLoader(g *globalFlags, repo gitutil.Repo, local bool) func(context.Co if local { // CI reads the file at the merge request's base, so edits on the branch // only apply once merged. Say so instead of silently ignoring them. + // Compare without CRs: with core.autocrlf the checkout has CRLF, the blob LF. work, err := os.ReadFile(filepath.Join(repo.Dir, config.RepoConfigFile)) - if (err == nil) != found || string(work) != string(data) { + lf := func(b []byte) string { return strings.ReplaceAll(string(b), "\r\n", "\n") } + if (err == nil) != found || lf(work) != lf(data) { fmt.Fprintf(os.Stderr, "[pruefbyte] note: your %s differs from the target branch's; CI uses the target's until your change is merged, and so does this run\n", config.RepoConfigFile) } } diff --git a/internal/config/config.go b/internal/config/config.go index b285022..34d930b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -142,9 +142,12 @@ func (c *Config) ApplyRepo(data []byte) error { if !IsBuiltinProvider(p) { return fmt.Errorf("%s: llm.provider %q is not an OCR built-in provider; custom providers (llm.url, llm.protocol) can only be set in the global config", RepoConfigFile, p) } - if p != c.LLM.Provider { - // An endpoint configured for the global provider does not belong to this one. - c.LLM.URL, c.LLM.Protocol = "", "" + // The operator's API key (and extra headers or body) are meant for the + // operator's provider. A global config that names one keeps it, so a merged + // .pruefbyte.yml cannot send that key to another vendor; repositories choose + // the provider only when the global config leaves it open. + if c.LLM.Provider != "" && p != c.LLM.Provider { + return fmt.Errorf("%s: llm.provider %q: the global config sets llm.provider %q, which a repository cannot change; leave llm.provider out of the global config to let repositories choose", RepoConfigFile, p, c.LLM.Provider) } } return decodeStrict(data, c) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index cfb0c3c..55a978f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -167,22 +167,40 @@ func TestEnvBadValue(t *testing.T) { } } -func TestRepoFileSetsBuiltinProvider(t *testing.T) { +func TestRepoFileProvider(t *testing.T) { + repo := []byte("llm:\n provider: anthropic\n model: claude-sonnet-5\n") + + // The global config leaves the provider open: the repository chooses. cfg := Default() - if err := cfg.ApplyGlobal([]byte("llm:\n provider: gateway\n protocol: openai\n url: https://gw.internal/v1\n model: x\n")); err != nil { - t.Fatal(err) - } - if err := cfg.ApplyRepo([]byte("llm:\n provider: anthropic\n model: claude-sonnet-5\n")); err != nil { + if err := cfg.ApplyRepo(repo); err != nil { t.Fatal(err) } if cfg.LLM.Provider != "anthropic" || cfg.LLM.Model != "claude-sonnet-5" { t.Errorf("llm = %+v", cfg.LLM) } - // The gateway's endpoint must not be used as anthropic's URL. - if cfg.LLM.URL != "" || cfg.LLM.Protocol != "" { - t.Errorf("endpoint of the global provider kept: %+v", cfg.LLM) - } if err := cfg.Validate(); err != nil { t.Error(err) } + + // Naming the provider the global config already uses is fine. + cfg = Default() + cfg.LLM.Provider = "anthropic" + if err := cfg.ApplyRepo(repo); err != nil { + t.Errorf("same provider rejected: %v", err) + } + + // A global config that names a provider keeps it: the operator's key must not + // go to another vendor, whether built-in or a custom gateway. + for _, global := range []string{ + "llm:\n provider: openrouter\n", + "llm:\n provider: gateway\n protocol: openai\n url: https://gw.internal/v1\n", + } { + cfg = Default() + if err := cfg.ApplyGlobal([]byte(global)); err != nil { + t.Fatal(err) + } + if err := cfg.ApplyRepo(repo); err == nil || !strings.Contains(err.Error(), "cannot change") { + t.Errorf("repository switched the provider from %q: %v", cfg.LLM.Provider, err) + } + } } diff --git a/internal/gitutil/git.go b/internal/gitutil/git.go index f81c46d..f6facff 100644 --- a/internal/gitutil/git.go +++ b/internal/gitutil/git.go @@ -120,9 +120,9 @@ func (r Repo) Head(ctx context.Context) (string, error) { // Snapshot returns a commit holding the working tree as it is now: tracked // changes, staged or not, plus untracked files that are not ignored. It uses a -// throwaway index, so the real index, HEAD and branches stay untouched; the -// commit is unreferenced and git eventually prunes it. Without changes it -// returns HEAD itself. +// throwaway copy of the index, so the real index, HEAD and branches stay +// untouched; the commit is unreferenced and git eventually prunes it. Without +// changes it returns HEAD itself. func (r Repo) Snapshot(ctx context.Context) (string, error) { head, err := r.Head(ctx) if err != nil { @@ -133,9 +133,15 @@ func (r Repo) Snapshot(ctx context.Context) (string, error) { return "", err } defer os.RemoveAll(tmp) - env := []string{"GIT_INDEX_FILE=" + filepath.Join(tmp, "index")} - if _, err := r.runEnv(ctx, env, "read-tree", head); err != nil { - return "", err + index := filepath.Join(tmp, "index") + env := []string{"GIT_INDEX_FILE=" + index} + // Starting from a copy of the real index keeps its stat data, so `git add` + // only rehashes files that changed, and its skip-worktree bits, so files + // outside a sparse checkout are not recorded as deleted. + if err := r.copyIndex(ctx, index); err != nil { + if _, err := r.runEnv(ctx, env, "read-tree", head); err != nil { + return "", err + } } if _, err := r.runEnv(ctx, env, "add", "--all", "--", "."); err != nil { return "", err @@ -158,6 +164,31 @@ func (r Repo) Snapshot(ctx context.Context) (string, error) { return strings.TrimSpace(string(out)), nil } +// copyIndex copies the repository's index file to dst. +func (r Repo) copyIndex(ctx context.Context, dst string) error { + out, err := r.run(ctx, "rev-parse", "--git-path", "index") + if err != nil { + return err + } + src := filepath.FromSlash(strings.TrimSpace(string(out))) + if !filepath.IsAbs(src) { + src = filepath.Join(r.Dir, src) + } + st, err := os.Stat(src) + if err != nil { + return err + } + data, err := os.ReadFile(src) + if err != nil { + return err + } + if err := os.WriteFile(dst, data, 0o600); err != nil { //nolint:gosec // G703: dst is in pruefbyte's own temp dir + return err + } + // git's racy-clean check compares entries with the index file's mtime. + return os.Chtimes(dst, st.ModTime(), st.ModTime()) +} + // Log returns the subjects and bodies of the commits in base..head, oldest first. func (r Repo) Log(ctx context.Context, base, head string) (string, error) { out, err := r.run(ctx, "log", "--reverse", "--format=%B", base+".."+head) diff --git a/internal/ocr/ocr_test.go b/internal/ocr/ocr_test.go index e5d57ed..4d573c1 100644 --- a/internal/ocr/ocr_test.go +++ b/internal/ocr/ocr_test.go @@ -256,3 +256,15 @@ func TestUserCredential(t *testing.T) { t.Error("missing config should give nothing") } } + +func TestRunKeyCommand(t *testing.T) { + ctx := context.Background() + if key, err := RunKeyCommand(ctx, "echo sk-from-cmd"); err != nil || key != "sk-from-cmd" { + t.Errorf("got %q, %v", key, err) + } + for _, bad := range []string{"exit 3", "echo a && echo b"} { + if key, err := RunKeyCommand(ctx, bad); err == nil { + t.Errorf("%q: accepted %q", bad, key) + } + } +} diff --git a/internal/ocr/runner.go b/internal/ocr/runner.go index 6ed2b08..b50ca7c 100644 --- a/internal/ocr/runner.go +++ b/internal/ocr/runner.go @@ -32,8 +32,6 @@ type Runner struct { // SecretEnv names env vars ocr must not inherit, such as the renamed // gitlab.token_env and llm.api_key_env. SecretEnv []string - // APIKeyCmd is used as the provider's api_key_cmd when no API key is given. - APIKeyCmd string } // NewRunner creates a runner with a fresh temporary home directory. Call Close to remove it. @@ -202,9 +200,6 @@ func (r *Runner) Configure(ctx context.Context, llm config.LLM, apiKey, language if err != nil { return err } - if apiKey == "" && r.APIKeyCmd != "" { - settings = append(settings, [2]string{providerSection(llm.Provider) + ".api_key_cmd", r.APIKeyCmd}) - } 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]) diff --git a/internal/ocr/usercfg.go b/internal/ocr/usercfg.go index 7f4711f..e4a9e35 100644 --- a/internal/ocr/usercfg.go +++ b/internal/ocr/usercfg.go @@ -1,9 +1,15 @@ package ocr import ( + "context" "encoding/json" + "errors" "os" + "os/exec" "path/filepath" + "runtime" + "strings" + "time" "github.com/feinarbyte/pruefbyte/internal/config" ) @@ -42,3 +48,35 @@ func userCredential(path, provider string) (string, string) { } return c.APIKey, c.APIKeyCmd } + +// RunKeyCommand runs an api_key_cmd the way OCR does: through the platform's +// shell (sh, or cmd.exe on Windows), with the terminal's stdin and stderr so +// prompts such as pinentry or Touch ID work, within 60 seconds, and requiring a +// single non-empty line of output. +func RunKeyCommand(ctx context.Context, command string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) + defer cancel() + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + cmd = exec.CommandContext(ctx, "cmd.exe", "/c", command) + } else { + cmd = exec.CommandContext(ctx, "sh", "-c", command) + } + cmd.Stdin, cmd.Stderr = os.Stdin, os.Stderr + cmd.WaitDelay = 5 * time.Second // a daemon left holding stdout must not block forever + out, err := cmd.Output() + if err != nil { + return "", err + } + if len(out) > 64<<10 { + return "", errors.New("output is larger than 64 KiB") + } + key := strings.TrimSpace(string(out)) + switch { + case key == "": + return "", errors.New("printed nothing") + case strings.ContainsAny(key, "\r\n"): + return "", errors.New("printed more than one line") + } + return key, nil +} diff --git a/internal/review/format.go b/internal/review/format.go index b7bce73..42abfc2 100644 --- a/internal/review/format.go +++ b/internal/review/format.go @@ -63,15 +63,27 @@ func badge(c ocr.Comment) string { } func lineLabel(c ocr.Comment) string { + switch start, end := lineSpan(c); { + case end > start: + return fmt.Sprintf("lines %d–%d", start, end) + case start > 0: + return fmt.Sprintf("line %d", start) + } + return "" +} + +// lineSpan returns the lines a finding covers: start == end for a single line, +// both 0 when it names none. +func lineSpan(c ocr.Comment) (start, end int) { switch { case c.StartLine > 0 && c.EndLine > c.StartLine: - return fmt.Sprintf("lines %d–%d", c.StartLine, c.EndLine) + return c.StartLine, c.EndLine case c.EndLine > 0: - return fmt.Sprintf("line %d", c.EndLine) + return c.EndLine, c.EndLine case c.StartLine > 0: - return fmt.Sprintf("line %d", c.StartLine) + return c.StartLine, c.StartLine } - return "" + return 0, 0 } // fence returns a backtick fence longer than any backtick run inside code. diff --git a/internal/review/local.go b/internal/review/local.go index 6901e7c..d7fc05d 100644 --- a/internal/review/local.go +++ b/internal/review/local.go @@ -45,15 +45,7 @@ func Local(ctx context.Context, d Deps, t LocalTarget, repoDir string) (*LocalOu 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 { - _ = d.SaveResult(raw) - } - if err == nil && res.Failed() { - err = fmt.Errorf("ocr reported status %q: %s", res.Status, res.Message) - } + res, err := runOCR(ctx, d, ro, cfg.OCR.Timeout) if err != nil { // ocr's output has already streamed to d.Log; repeating it would only add noise. var re *ocr.RunError @@ -70,13 +62,11 @@ func Local(ctx context.Context, d Deps, t LocalTarget, repoDir string) (*LocalOu func WriteLocal(w io.Writer, o *LocalOutcome) { for _, c := range o.Findings { head := c.Path - switch { - case c.StartLine > 0 && c.EndLine > c.StartLine: - head += fmt.Sprintf(":%d-%d", c.StartLine, c.EndLine) - case c.EndLine > 0: - head += fmt.Sprintf(":%d", c.EndLine) - case c.StartLine > 0: - head += fmt.Sprintf(":%d", c.StartLine) + switch start, end := lineSpan(c); { + case end > start: + head += fmt.Sprintf(":%d-%d", start, end) + case start > 0: + head += fmt.Sprintf(":%d", start) } if b := strings.Trim(badge(c), "*"); b != "" { head += " " + b diff --git a/internal/review/review.go b/internal/review/review.go index 5a39242..5890ad0 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -12,6 +12,7 @@ import ( "path" "path/filepath" "strings" + "time" "github.com/feinarbyte/pruefbyte/internal/config" "github.com/feinarbyte/pruefbyte/internal/gitlab" @@ -127,17 +128,7 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { 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) - } + res, err := runOCR(ctx, d, ro, cfg.OCR.Timeout) if err != nil { if cfg.Review.PostFailures { msg := err.Error() @@ -242,6 +233,23 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { return out, nil } +// runOCR runs the review within timeout, keeps OCR's raw result if asked to, +// and turns a result OCR reports as failed into an error. +func runOCR(ctx context.Context, d Deps, ro ocr.ReviewOptions, timeout time.Duration) (*ocr.Result, error) { + reviewCtx, cancel := context.WithTimeout(ctx, timeout) + res, raw, err := d.OCR.Review(reviewCtx, ro) + cancel() + if raw != nil && d.SaveResult != nil { + if serr := d.SaveResult(raw); serr != nil { + fmt.Fprintf(d.Log, "[pruefbyte] warning: saving OCR result: %v\n", serr) + } + } + if err == nil && res.Failed() { + err = fmt.Errorf("ocr reported status %q: %s", res.Status, res.Message) + } + return res, err +} + // gateReason explains why review.fail_on_severity trips for these findings, or "". func gateReason(findings []ocr.Comment, r config.Review) string { if r.FailOnSeverity == "" { diff --git a/pruefbyte.example.yml b/pruefbyte.example.yml index 2a9a0e1..96bdb63 100644 --- a/pruefbyte.example.yml +++ b/pruefbyte.example.yml @@ -9,6 +9,8 @@ gitlab: llm: # provider and model are better set in the repository's .pruefbyte.yml, where # `pruefbyte local` finds them too; only custom providers (url, protocol) need this file. + # A provider set here is fixed: repositories cannot switch the shared API key to + # another vendor. Leave it out to let each repository choose. 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 From 3bce7f9b369a4e801d09624d1417c0515b2e2bac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Tue, 6 Oct 2026 18:31:26 +0200 Subject: [PATCH 3/4] Embed OCR in release binaries Release builds (GoReleaser, build tag embedocr) now carry the ocr binary for their platform, gzip-compressed, so a single download is all a developer needs for `pruefbyte local`. A before hook runs `go run ./internal/ocrbin/fetch`, which downloads the OCR release pinned in internal/ocrbin/VERSION for all six platforms and checks each against OCR's sha256sum.txt. On first use pruefbyte unpacks its copy into the user cache directory, verified against the same checksum. ocr.binary now defaults to automatic: a configured binary, else the built-in copy, else ocr from the PATH. The built-in copy comes before PATH so local runs use exactly the OCR version CI uses. The Docker image reads the same VERSION file, release archives ship OCR's license as LICENSE.open-code-review, and CI caches the downloads. Plain go build/test/install and the Docker image embed nothing. Co-Authored-By: Claude Opus 5.5 (1M context) --- .dockerignore | 2 + .github/workflows/ci.yml | 15 ++- .gitignore | 3 + .goreleaser.yaml | 10 ++ Dockerfile | 6 +- README.md | 37 ++++-- cmd/pruefbyte/local.go | 2 +- cmd/pruefbyte/main.go | 30 ++++- internal/config/config.go | 2 +- internal/ocr/runner.go | 2 +- internal/ocrbin/VERSION | 1 + internal/ocrbin/embed_darwin_amd64.go | 13 +++ internal/ocrbin/embed_darwin_arm64.go | 13 +++ internal/ocrbin/embed_linux_amd64.go | 13 +++ internal/ocrbin/embed_linux_arm64.go | 13 +++ internal/ocrbin/embed_windows_amd64.go | 13 +++ internal/ocrbin/embed_windows_arm64.go | 13 +++ internal/ocrbin/fetch/main.go | 152 +++++++++++++++++++++++++ internal/ocrbin/ocrbin.go | 119 +++++++++++++++++++ internal/ocrbin/ocrbin_test.go | 65 +++++++++++ pruefbyte.example.yml | 2 +- 21 files changed, 505 insertions(+), 21 deletions(-) create mode 100644 internal/ocrbin/VERSION create mode 100644 internal/ocrbin/embed_darwin_amd64.go create mode 100644 internal/ocrbin/embed_darwin_arm64.go create mode 100644 internal/ocrbin/embed_linux_amd64.go create mode 100644 internal/ocrbin/embed_linux_arm64.go create mode 100644 internal/ocrbin/embed_windows_amd64.go create mode 100644 internal/ocrbin/embed_windows_arm64.go create mode 100644 internal/ocrbin/fetch/main.go create mode 100644 internal/ocrbin/ocrbin.go create mode 100644 internal/ocrbin/ocrbin_test.go diff --git a/.dockerignore b/.dockerignore index f9980df..e717a1a 100644 --- a/.dockerignore +++ b/.dockerignore @@ -2,3 +2,5 @@ .serena .pruefbyte *.exe +internal/ocrbin/bin +dist diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69ef25a..38f057f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,8 +52,9 @@ jobs: - run: go test -race -coverprofile=coverage.out ./... - run: go tool cover -func=coverage.out | tail -1 - # Builds every release binary (linux, macOS, windows; amd64, arm64) from the - # same config the release job uses, so a broken release shows up in the PR. + # Builds every release binary (linux, macOS, windows; amd64, arm64), with ocr + # embedded, from the same config the release job uses, so a broken release + # shows up in the PR. build: runs-on: ubuntu-latest steps: @@ -61,6 +62,11 @@ jobs: - uses: actions/setup-go@v7 with: go-version-file: go.mod + - name: Cache OCR binaries embedded in release builds + uses: actions/cache@v6 + with: + path: internal/ocrbin/bin + key: ocrbin-${{ hashFiles('internal/ocrbin/VERSION', 'internal/ocrbin/fetch/**') }} - uses: goreleaser/goreleaser-action@v7 with: version: v2.18.2 @@ -80,6 +86,11 @@ jobs: - uses: actions/setup-go@v7 with: go-version-file: go.mod + - name: Cache OCR binaries embedded in release builds + uses: actions/cache@v6 + with: + path: internal/ocrbin/bin + key: ocrbin-${{ hashFiles('internal/ocrbin/VERSION', 'internal/ocrbin/fetch/**') }} - uses: goreleaser/goreleaser-action@v7 with: version: v2.18.2 diff --git a/.gitignore b/.gitignore index bd04c21..ff3f74b 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,9 @@ coverage.* /.pruefbyte/ /.ocr/ +# OCR binaries downloaded for release builds (go run ./internal/ocrbin/fetch) +/internal/ocrbin/bin/ + # Local configuration and secrets .env .env.* diff --git a/.goreleaser.yaml b/.goreleaser.yaml index fcc8d56..4d1cb93 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -4,6 +4,12 @@ version: 2 project_name: pruefbyte +before: + hooks: + # Downloads the OCR release pinned in internal/ocrbin/VERSION for every + # platform, checksum-verified, for embedding below. + - go run ./internal/ocrbin/fetch + builds: - main: ./cmd/pruefbyte binary: pruefbyte @@ -11,6 +17,8 @@ builds: - CGO_ENABLED=0 flags: - -trimpath + tags: + - embedocr # include ocr, so the download is all a developer needs ldflags: - -s -w -X main.version={{ .Version }} goos: [linux, darwin, windows] @@ -27,6 +35,8 @@ archives: files: - LICENSE - README.md + - src: internal/ocrbin/bin/LICENSE + dst: LICENSE.open-code-review checksum: name_template: checksums.txt diff --git a/Dockerfile b/Dockerfile index 2e86a97..82c14a3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -16,11 +16,15 @@ 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 +# The OCR version is pinned in internal/ocrbin/VERSION, which the release +# binaries embed too, so the image and `pruefbyte local` run the same ocr. +# tr also drops the CR a Windows checkout (core.autocrlf) adds. +COPY internal/ocrbin/VERSION /tmp/OCR_VERSION # 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 \ + && OCR_VERSION="$(tr -d '[:space:]' < /tmp/OCR_VERSION)" \ && arch="${TARGETARCH:-amd64}" \ && cd /tmp \ && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/opencodereview-linux-${arch}" \ diff --git a/README.md b/README.md index cda4b89..ad85527 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ from a dedicated bot account. 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 \ + docker buildx build --platform linux/amd64,linux/arm64 \ -t registry.example.com/tools/pruefbyte:latest --push . ``` 4. **Pipeline.** Include the template in each project (or in a shared CI config): @@ -156,14 +156,16 @@ pass the same file with `--config`. ### Install -pruefbyte needs OpenCodeReview's `ocr` on your PATH: +Release binaries include OpenCodeReview (`ocr`), the exact version CI uses, so a +single download is all you need. On first use pruefbyte unpacks it into your user cache +directory. Pick whichever install suits you: + +**With mise:** ```sh -npm install -g @alibaba-group/open-code-review # or: brew install open-code-review +mise use -g github:feinarbyte/pruefbyte ``` -Then install pruefbyte itself, in whichever way suits you. - **Linux / macOS**, latest release into `~/.local/bin`: ```sh @@ -173,11 +175,11 @@ curl -fsSL "https://github.com/feinarbyte/pruefbyte/releases/latest/download/pru | tar -xz -C ~/.local/bin pruefbyte ``` -**Windows** (PowerShell), latest release into `%LOCALAPPDATA%\pruefbyte`: +**Windows** (PowerShell), latest release into `%LOCALAPPDATA%\Programs\pruefbyte`: ```powershell $arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') { 'arm64' } else { 'amd64' } -$dir = "$env:LOCALAPPDATA\pruefbyte"; $zip = "$env:TEMP\pruefbyte.zip" +$dir = "$env:LOCALAPPDATA\Programs\pruefbyte"; $zip = "$env:TEMP\pruefbyte.zip" Invoke-WebRequest "https://github.com/feinarbyte/pruefbyte/releases/latest/download/pruefbyte_windows_$arch.zip" -OutFile $zip Expand-Archive $zip $dir -Force; Remove-Item $zip [Environment]::SetEnvironmentVariable('Path', "$([Environment]::GetEnvironmentVariable('Path', 'User'));$dir", 'User') @@ -186,10 +188,12 @@ Expand-Archive $zip $dir -Force; Remove-Item $zip Each release also lists the archives with a `checksums.txt` ([releases](https://github.com/feinarbyte/pruefbyte/releases)). -**With Go** 1.25 or newer: +**With Go** 1.25 or newer. This builds from source without OCR, so `ocr` must be on +your PATH as well: ```sh go install github.com/feinarbyte/pruefbyte/cmd/pruefbyte@latest +npm install -g @alibaba-group/open-code-review # or: brew install open-code-review ``` **With Docker**, with `ocr` included and nothing to install. Run it as yourself so @@ -200,7 +204,8 @@ docker run --rm -it --user "$(id -u):$(id -g)" -v "$PWD:/repo" -w /repo \ -e PRUEFBYTE_LLM_API_KEY ghcr.io/feinarbyte/pruefbyte pruefbyte local ``` -Check the install with `pruefbyte version`. +Check the install with `pruefbyte version`; it also says whether `ocr` is built in. +An `ocr.binary` setting overrides the built-in copy. To try the CI path against a real merge request without posting, set `PRUEFBYTE_GITLAB_TOKEN` and run `pruefbyte review --project group/project --mr 42 --dry-run`. @@ -225,6 +230,12 @@ To release, push a tag such as `v0.1.0`. CI then publishes the image tags `0.1.0 and `0.1`, plus a GitHub release with the binaries and checksums. Try the release build locally with `goreleaser release --snapshot --clean`. +The OCR version is pinned in `internal/ocrbin/VERSION`, for both the Docker image and +the release binaries. To update OCR, change that file. Release builds use the +`embedocr` build tag and embed the gzip-compressed `ocr` that +`go run ./internal/ocrbin/fetch` downloads and checks against OCR's published +checksums. Plain `go build` and `go test` need neither. + Layout: | Path | Purpose | @@ -232,6 +243,7 @@ Layout: | `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/ocrbin` | the pinned OCR version; the `ocr` embedded in release builds (`fetch` downloads it) | | `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 | @@ -242,6 +254,7 @@ OCR is used as a subprocess. Its Go packages all live under `internal/`, so they pruefbyte is released under the [MIT License](LICENSE). -The Docker image also bundles the OpenCodeReview (`ocr`) binary, which is -licensed under the [Apache License 2.0](https://github.com/alibaba/open-code-review/blob/main/LICENSE). -Both license texts are in the image under `/usr/share/licenses/`. +The Docker image and the release binaries also bundle the OpenCodeReview (`ocr`) +binary, which is licensed under the [Apache License 2.0](https://github.com/alibaba/open-code-review/blob/main/LICENSE). +Both license texts are in the image under `/usr/share/licenses/`, and in each release +archive as `LICENSE` and `LICENSE.open-code-review`. diff --git a/cmd/pruefbyte/local.go b/cmd/pruefbyte/local.go index f2b3a59..dfef3f7 100644 --- a/cmd/pruefbyte/local.go +++ b/cmd/pruefbyte/local.go @@ -109,7 +109,7 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla log, _ := repo.Log(ctx, mergeBase, head) apiKey := os.Getenv(base.LLM.APIKeyEnv) - runner, err := ocr.NewRunner(base.OCR.Binary, os.Stderr) + runner, err := newRunner(base.OCR.Binary) if err != nil { return err } diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index 00e16d4..a4ba622 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -20,6 +20,7 @@ import ( "github.com/feinarbyte/pruefbyte/internal/gitlab" "github.com/feinarbyte/pruefbyte/internal/gitutil" "github.com/feinarbyte/pruefbyte/internal/ocr" + "github.com/feinarbyte/pruefbyte/internal/ocrbin" "github.com/feinarbyte/pruefbyte/internal/review" ) @@ -82,7 +83,14 @@ func rootCmd() *cobra.Command { root.AddCommand(reviewCmd(g), localCmd(g), configCmd(g), &cobra.Command{ Use: "version", Short: "Print the version", - Run: func(cmd *cobra.Command, _ []string) { fmt.Fprintln(cmd.OutOrStdout(), "pruefbyte", version) }, + Run: func(cmd *cobra.Command, _ []string) { + fmt.Fprintln(cmd.OutOrStdout(), "pruefbyte", version) + if ocrbin.Available() { + fmt.Fprintln(cmd.OutOrStdout(), "ocr", ocrbin.Version(), "(built in)") + } else { + fmt.Fprintln(cmd.OutOrStdout(), "ocr: not built in; uses ocr.binary or ocr from the PATH (tested with", ocrbin.Version()+")") + } + }, }) return root } @@ -97,6 +105,24 @@ func baseConfig(g *globalFlags) (config.Config, error) { return cfg, cfg.ApplyEnv(os.LookupEnv) } +// newRunner prepares ocr: ocr.binary if configured, else the copy built into +// release binaries, else ocr from the PATH. The built-in copy comes first so a +// local run uses exactly the OCR version CI does. +func newRunner(configured string) (*ocr.Runner, error) { + binary := configured + if binary == "" && ocrbin.Available() { + p, err := ocrbin.Path() + if err != nil { + return nil, fmt.Errorf("preparing the built-in ocr: %w (set ocr.binary to use another one)", err) + } + binary = p + } + if binary == "" { + binary = "ocr" + } + return ocr.NewRunner(binary, os.Stderr) +} + // configLoader returns the effective config for a review whose base commit is // known: defaults < global file < repository file at the base commit < env. // CI and local runs both use it, so they review with the same settings. @@ -240,7 +266,7 @@ func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { api = gitlab.DryRun{API: client, Out: os.Stdout} } - runner, err := ocr.NewRunner(base.OCR.Binary, os.Stderr) + runner, err := newRunner(base.OCR.Binary) if err != nil { return err } diff --git a/internal/config/config.go b/internal/config/config.go index 34d930b..097475b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -109,7 +109,7 @@ func Default() 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", + Binary: "", // automatic: see newRunner in cmd/pruefbyte Effort: "medium", Timeout: 30 * time.Minute, Concurrency: 8, diff --git a/internal/ocr/runner.go b/internal/ocr/runner.go index b50ca7c..569738b 100644 --- a/internal/ocr/runner.go +++ b/internal/ocr/runner.go @@ -346,7 +346,7 @@ 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) + return "", fmt.Errorf("%w: %q (use a pruefbyte release binary, which includes ocr, or install @alibaba-group/open-code-review, or set ocr.binary)", ErrNotFound, r.Binary) } cmd, err := r.command(ctx, "version") if err != nil { diff --git a/internal/ocrbin/VERSION b/internal/ocrbin/VERSION new file mode 100644 index 0000000..68d89df --- /dev/null +++ b/internal/ocrbin/VERSION @@ -0,0 +1 @@ +v1.12.10 diff --git a/internal/ocrbin/embed_darwin_amd64.go b/internal/ocrbin/embed_darwin_amd64.go new file mode 100644 index 0000000..04d00c0 --- /dev/null +++ b/internal/ocrbin/embed_darwin_amd64.go @@ -0,0 +1,13 @@ +//go:build embedocr && darwin && amd64 + +package ocrbin + +import _ "embed" + +//go:embed bin/darwin_amd64/ocr.gz +var embeddedGz []byte + +//go:embed bin/darwin_amd64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/embed_darwin_arm64.go b/internal/ocrbin/embed_darwin_arm64.go new file mode 100644 index 0000000..d3e01ed --- /dev/null +++ b/internal/ocrbin/embed_darwin_arm64.go @@ -0,0 +1,13 @@ +//go:build embedocr && darwin && arm64 + +package ocrbin + +import _ "embed" + +//go:embed bin/darwin_arm64/ocr.gz +var embeddedGz []byte + +//go:embed bin/darwin_arm64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/embed_linux_amd64.go b/internal/ocrbin/embed_linux_amd64.go new file mode 100644 index 0000000..fb590ed --- /dev/null +++ b/internal/ocrbin/embed_linux_amd64.go @@ -0,0 +1,13 @@ +//go:build embedocr && linux && amd64 + +package ocrbin + +import _ "embed" + +//go:embed bin/linux_amd64/ocr.gz +var embeddedGz []byte + +//go:embed bin/linux_amd64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/embed_linux_arm64.go b/internal/ocrbin/embed_linux_arm64.go new file mode 100644 index 0000000..1754656 --- /dev/null +++ b/internal/ocrbin/embed_linux_arm64.go @@ -0,0 +1,13 @@ +//go:build embedocr && linux && arm64 + +package ocrbin + +import _ "embed" + +//go:embed bin/linux_arm64/ocr.gz +var embeddedGz []byte + +//go:embed bin/linux_arm64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/embed_windows_amd64.go b/internal/ocrbin/embed_windows_amd64.go new file mode 100644 index 0000000..46c7ab9 --- /dev/null +++ b/internal/ocrbin/embed_windows_amd64.go @@ -0,0 +1,13 @@ +//go:build embedocr && windows && amd64 + +package ocrbin + +import _ "embed" + +//go:embed bin/windows_amd64/ocr.gz +var embeddedGz []byte + +//go:embed bin/windows_amd64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/embed_windows_arm64.go b/internal/ocrbin/embed_windows_arm64.go new file mode 100644 index 0000000..555d9b5 --- /dev/null +++ b/internal/ocrbin/embed_windows_arm64.go @@ -0,0 +1,13 @@ +//go:build embedocr && windows && arm64 + +package ocrbin + +import _ "embed" + +//go:embed bin/windows_arm64/ocr.gz +var embeddedGz []byte + +//go:embed bin/windows_arm64/ocr.sha256 +var embeddedSum string + +func init() { compressed, sum = embeddedGz, embeddedSum } diff --git a/internal/ocrbin/fetch/main.go b/internal/ocrbin/fetch/main.go new file mode 100644 index 0000000..69cb10b --- /dev/null +++ b/internal/ocrbin/fetch/main.go @@ -0,0 +1,152 @@ +// Command fetch downloads the OCR release pinned in internal/ocrbin/VERSION for +// every platform pruefbyte ships, checks each binary against the release's +// sha256sum.txt and stores it gzip-compressed for embedding: +// +// internal/ocrbin/bin/_/ocr.gz the binary, gzipped +// internal/ocrbin/bin/_/ocr.sha256 its checksum +// internal/ocrbin/bin/LICENSE OCR's license, shipped with releases +// +// GoReleaser runs it before release builds: go run ./internal/ocrbin/fetch +// Binaries already present with the right checksum are not downloaded again. +package main + +import ( + "bufio" + "bytes" + "compress/gzip" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "time" +) + +const repo = "https://github.com/alibaba/open-code-review" + +// Platforms pruefbyte releases for; keep in sync with .goreleaser.yaml and embed_*.go. +var platforms = []string{"linux_amd64", "linux_arm64", "darwin_amd64", "darwin_arm64", "windows_amd64", "windows_arm64"} + +func main() { + if err := run("internal/ocrbin"); err != nil { + fmt.Fprintln(os.Stderr, "fetch ocr:", err) + os.Exit(1) + } +} + +var client = &http.Client{Timeout: 5 * time.Minute} + +func run(pkgDir string) error { + v, err := os.ReadFile(filepath.Join(pkgDir, "VERSION")) + if err != nil { + return err + } + version := strings.TrimSpace(string(v)) + sums, err := download(fmt.Sprintf("%s/releases/download/%s/sha256sum.txt", repo, version)) + if err != nil { + return err + } + want := parseSums(sums) + binDir := filepath.Join(pkgDir, "bin") + fetched := false + for _, p := range platforms { + asset := "opencodereview-" + strings.ReplaceAll(p, "_", "-") + if strings.HasPrefix(p, "windows") { + asset += ".exe" + } + sum := want[asset] + if sum == "" { + return fmt.Errorf("%s has no checksum in %s's sha256sum.txt", asset, version) + } + dir := filepath.Join(binDir, p) + // Check the payload itself, not just the recorded checksum, so a truncated + // ocr.gz (an interrupted run, a stale CI cache) is downloaded again. + if cur, err := os.ReadFile(filepath.Join(dir, "ocr.sha256")); err == nil && strings.TrimSpace(string(cur)) == sum { + if got, err := gzSHA256(filepath.Join(dir, "ocr.gz")); err == nil && got == sum { + fmt.Printf("ocr %s %s: up to date\n", version, p) + continue + } + } + fetched = true + data, err := download(fmt.Sprintf("%s/releases/download/%s/%s", repo, version, asset)) + if err != nil { + return err + } + h := sha256.Sum256(data) + if got := hex.EncodeToString(h[:]); got != sum { + return fmt.Errorf("%s: checksum %s, want %s", asset, got, sum) + } + var gz bytes.Buffer + zw, _ := gzip.NewWriterLevel(&gz, gzip.BestCompression) + if _, err := zw.Write(data); err != nil { + return err + } + if err := zw.Close(); err != nil { + return err + } + if err := os.MkdirAll(dir, 0o750); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "ocr.gz"), gz.Bytes(), 0o600); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "ocr.sha256"), []byte(sum+"\n"), 0o600); err != nil { + return err + } + fmt.Printf("ocr %s %s: %.1f MB, %.1f MB compressed\n", version, p, float64(len(data))/1e6, float64(gz.Len())/1e6) + } + licensePath := filepath.Join(binDir, "LICENSE") + if _, err := os.Stat(licensePath); err == nil && !fetched { + return nil // binaries unchanged, so the license of their release is already here + } + license, err := download(fmt.Sprintf("https://raw.githubusercontent.com/alibaba/open-code-review/%s/LICENSE", version)) + if err != nil { + return err + } + return os.WriteFile(licensePath, license, 0o600) //nolint:gosec // G703: a fixed path in the repository +} + +// gzSHA256 returns the hex sha256 of a gzip file's uncompressed content. +func gzSHA256(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + zr, err := gzip.NewReader(f) + if err != nil { + return "", err + } + h := sha256.New() + if _, err := io.Copy(h, zr); err != nil { //nolint:gosec // G110: our own download, checked against its checksum + return "", err + } + return hex.EncodeToString(h.Sum(nil)), nil +} + +// parseSums reads " " lines. +func parseSums(data []byte) map[string]string { + out := map[string]string{} + sc := bufio.NewScanner(bytes.NewReader(data)) + for sc.Scan() { + if f := strings.Fields(sc.Text()); len(f) == 2 { + out[strings.TrimPrefix(f[1], "*")] = strings.ToLower(f[0]) + } + } + return out +} + +func download(url string) ([]byte, error) { + resp, err := client.Get(url) + if err != nil { + return nil, err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("GET %s: %s", url, resp.Status) + } + return io.ReadAll(resp.Body) +} diff --git a/internal/ocrbin/ocrbin.go b/internal/ocrbin/ocrbin.go new file mode 100644 index 0000000..65e819e --- /dev/null +++ b/internal/ocrbin/ocrbin.go @@ -0,0 +1,119 @@ +// Package ocrbin carries the OpenCodeReview (ocr) binary inside release builds +// of pruefbyte, so a single download is all a developer needs. +// +// Release builds use the embedocr build tag and embed the gzip-compressed ocr +// for their platform, fetched by ./internal/ocrbin/fetch. Other builds (go +// build, go install, the Docker image, which installs ocr itself) embed nothing +// and use an ocr from the PATH. +package ocrbin + +import ( + "bytes" + "compress/gzip" + "crypto/sha256" + _ "embed" + "encoding/hex" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "runtime" + "strings" +) + +//go:embed VERSION +var versionFile string + +// Set by the platform's embed_*.go file in embedocr builds. +var ( + compressed []byte // gzip of the ocr binary + sum string // hex sha256 of the uncompressed binary, from OCR's release checksums +) + +// Version is the OCR release pruefbyte is built and tested against, e.g. v1.12.10. +func Version() string { return strings.TrimSpace(versionFile) } + +// Available reports whether this build carries an ocr binary. +func Available() bool { return len(compressed) > 0 } + +// Path returns the embedded ocr, unpacked into the user cache directory on first +// use and verified against its release checksum. +func Path() (string, error) { + if !Available() { + return "", errors.New("this pruefbyte build does not include ocr") + } + // No fallback to the shared temp directory: another user could own the + // directory there and swap the binary between its check and its use. + dir, err := os.UserCacheDir() + if err != nil { + return "", fmt.Errorf("no user cache directory to unpack ocr into: %w", err) + } + return extract(filepath.Join(dir, "pruefbyte"), compressed, sum) +} + +func extract(cacheDir string, gz []byte, want string) (string, error) { + want = strings.ToLower(strings.TrimSpace(want)) // the embedded file ends with a newline + if len(want) < 12 { + return "", errors.New("embedded ocr has no checksum") + } + name := "ocr" + if runtime.GOOS == "windows" { + name = "ocr.exe" + } + dir := filepath.Join(cacheDir, "ocr-"+Version()+"-"+want[:12]) + target := filepath.Join(dir, name) + if got, err := fileSHA256(target); err == nil && got == want { + return target, nil + } + if err := os.MkdirAll(dir, 0o750); err != nil { + return "", err + } + // Unpack next to the target and rename, so concurrent runs never see a + // half-written binary. + tmp, err := os.CreateTemp(dir, name+".tmp-*") + if err != nil { + return "", err + } + defer os.Remove(tmp.Name()) + zr, err := gzip.NewReader(bytes.NewReader(gz)) + if err != nil { + _ = tmp.Close() + return "", fmt.Errorf("embedded ocr: %w", err) + } + h := sha256.New() + _, err = io.Copy(io.MultiWriter(tmp, h), zr) //nolint:gosec // G110: the archive is our own build input, checked against its checksum below + if cerr := tmp.Close(); err == nil { + err = cerr + } + if err != nil { + return "", fmt.Errorf("unpacking ocr into %s: %w", dir, err) + } + if got := hex.EncodeToString(h.Sum(nil)); got != want { + return "", fmt.Errorf("embedded ocr has checksum %s, want %s", got, want) + } + if err := os.Chmod(tmp.Name(), 0o755); err != nil { //nolint:gosec // G302: an executable must be executable + return "", err + } + if err := os.Rename(tmp.Name(), target); err != nil { + // Another run may have put it there first (Windows cannot replace a file in use). + if got, serr := fileSHA256(target); serr == nil && got == want { + return target, nil + } + return "", err + } + return target, nil +} + +func fileSHA256(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", err + } + return hex.EncodeToString(h.Sum(nil)), nil +} diff --git a/internal/ocrbin/ocrbin_test.go b/internal/ocrbin/ocrbin_test.go new file mode 100644 index 0000000..d235c80 --- /dev/null +++ b/internal/ocrbin/ocrbin_test.go @@ -0,0 +1,65 @@ +package ocrbin + +import ( + "bytes" + "compress/gzip" + "crypto/sha256" + "encoding/hex" + "os" + "runtime" + "strings" + "testing" +) + +func gzipped(t *testing.T, data []byte) []byte { + t.Helper() + var b bytes.Buffer + zw := gzip.NewWriter(&b) + zw.Write(data) + zw.Close() + return b.Bytes() +} + +func TestExtract(t *testing.T) { + bin := []byte("#!/bin/sh\necho fake ocr\n") + h := sha256.Sum256(bin) + sum := hex.EncodeToString(h[:]) + cache := t.TempDir() + + p, err := extract(cache, gzipped(t, bin), sum+"\n") // as embedded from ocr.sha256 + if err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(p); !bytes.Equal(got, bin) { + t.Errorf("unpacked %q", got) + } + if !strings.Contains(p, "ocr-"+Version()+"-"+sum[:12]) { + t.Errorf("path %s does not name version and checksum", p) + } + if fi, err := os.Stat(p); err != nil || runtime.GOOS != "windows" && fi.Mode().Perm()&0o100 == 0 { + t.Errorf("not executable: %v %v", fi.Mode(), err) + } + + // Reused while intact; replaced when the cached copy was changed. + if p2, err := extract(cache, gzipped(t, bin), sum); err != nil || p2 != p { + t.Errorf("second extract: %s, %v", p2, err) + } + os.WriteFile(p, []byte("tampered"), 0o755) + if _, err := extract(cache, gzipped(t, bin), sum); err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(p); !bytes.Equal(got, bin) { + t.Error("tampered cache copy was not replaced") + } + + // A payload that does not match its checksum is refused. + if _, err := extract(t.TempDir(), gzipped(t, []byte("other")), sum); err == nil { + t.Error("checksum mismatch accepted") + } +} + +func TestVersionPinned(t *testing.T) { + if v := Version(); !strings.HasPrefix(v, "v") || strings.ContainsAny(v, " \n") { + t.Errorf("VERSION = %q", v) + } +} diff --git a/pruefbyte.example.yml b/pruefbyte.example.yml index 96bdb63..37b24ff 100644 --- a/pruefbyte.example.yml +++ b/pruefbyte.example.yml @@ -23,7 +23,7 @@ llm: aws_profile: "" ocr: - binary: ocr + binary: "" # empty: the ocr built into release binaries, else ocr on the PATH effort: medium # low | medium | high (review rounds) language: English # language of the comments timeout: 30m # whole OCR run From 7b5314fe2d9bc7f4b003980723818adfa7f40a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Wed, 7 Oct 2026 15:18:07 +0200 Subject: [PATCH 4/4] Address third review of local review and embedded OCR - Windows api_key_cmd: build the cmd.exe command line ourselves so quoted arguments survive. - Do not use a key from the developer's OCR config when it belongs to a different endpoint than this review uses. - Error when PRUEFBYTE_LLM_PROVIDER overrides the provider a repository's .pruefbyte.yml sets, instead of mixing provider and model. - Strip control characters from model text printed to the terminal. - Local output: drop repeated findings like CI, show OCR warnings, label the hidden count correctly. - Snapshot takes the HEAD the caller already resolved. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/pruefbyte/local.go | 20 ++++++++++++---- cmd/pruefbyte/local_test.go | 26 ++++++++++++++++++++ cmd/pruefbyte/main.go | 11 +++++++++ internal/gitutil/git.go | 11 ++++----- internal/gitutil/git_test.go | 4 ++-- internal/ocr/ocr_test.go | 17 +++++++++---- internal/ocr/shell_other.go | 13 ++++++++++ internal/ocr/shell_windows.go | 20 ++++++++++++++++ internal/ocr/usercfg.go | 45 ++++++++++++++++------------------- internal/review/local.go | 38 +++++++++++++++++++++++------ internal/review/review.go | 11 ++++++--- 11 files changed, 164 insertions(+), 52 deletions(-) create mode 100644 internal/ocr/shell_other.go create mode 100644 internal/ocr/shell_windows.go diff --git a/cmd/pruefbyte/local.go b/cmd/pruefbyte/local.go index dfef3f7..89937b8 100644 --- a/cmd/pruefbyte/local.go +++ b/cmd/pruefbyte/local.go @@ -94,7 +94,7 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla } to := head if !f.committed { - if to, err = repo.Snapshot(ctx); err != nil { + if to, err = repo.Snapshot(ctx, head); err != nil { return fmt.Errorf("snapshotting the working tree: %w", err) } } @@ -134,9 +134,21 @@ func runLocal(ctx context.Context, stdout io.Writer, g *globalFlags, f *localFla PrepareOCR: func(ctx context.Context, cfg config.Config) error { key := apiKey if key == "" { - var cmd string - key, cmd = ocr.UserCredential(cfg.LLM.Provider) - if key == "" && cmd != "" { + uc := ocr.UserCredential(cfg.LLM.Provider) + // A key the user's setup sends to its own endpoint (a gateway, a proxy) + // must not go to the endpoint this review uses instead. + if (uc.APIKey != "" || uc.APIKeyCmd != "") && uc.URL != "" && + strings.TrimRight(uc.URL, "/") != strings.TrimRight(cfg.LLM.URL, "/") { + endpoint := cfg.LLM.URL + if endpoint == "" { + endpoint = "the provider's default endpoint" + } + fmt.Fprintf(os.Stderr, "[pruefbyte] not using the %s API key from your OCR config: it is for %s, but this review uses %s\n", + cfg.LLM.Provider, uc.URL, endpoint) + uc = ocr.UserCred{} + } + key = uc.APIKey + if cmd := uc.APIKeyCmd; key == "" && cmd != "" { // ocr would run the command under pruefbyte's private HOME, where // keychains, pass or ~/ files are not found; run it here instead. var err error diff --git a/cmd/pruefbyte/local_test.go b/cmd/pruefbyte/local_test.go index 4251a50..8a421fc 100644 --- a/cmd/pruefbyte/local_test.go +++ b/cmd/pruefbyte/local_test.go @@ -10,6 +10,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/feinarbyte/pruefbyte/internal/gitutil" ) // TestLocalReviewsLikeCI runs the same branch through `review` (CI) and `local` @@ -159,3 +161,27 @@ review: t.Error("local review changed the repository") } } + +// A provider pinned by env var, like one in the global config, cannot be +// switched by the repository; its llm.model would not fit the other provider. +func TestRepoProviderVersusEnv(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + repo := t.TempDir() + gitCmd(t, repo, "init", "-q", "-b", "main") + os.WriteFile(filepath.Join(repo, ".pruefbyte.yml"), []byte("llm:\n provider: anthropic\n model: claude-sonnet-5\n"), 0o644) + gitCmd(t, repo, "add", ".") + gitCmd(t, repo, "commit", "-qm", "base") + base := gitCmd(t, repo, "rev-parse", "HEAD") + load := configLoader(&globalFlags{repoConfig: true}, gitutil.Repo{Dir: repo}, false) + + t.Setenv("PRUEFBYTE_LLM_PROVIDER", "anthropic") + if _, err := load(context.Background(), base); err != nil { + t.Errorf("same provider rejected: %v", err) + } + t.Setenv("PRUEFBYTE_LLM_PROVIDER", "openai") + if _, err := load(context.Background(), base); err == nil || !strings.Contains(err.Error(), "PRUEFBYTE_LLM_PROVIDER") { + t.Errorf("env provider silently replaced the repository's: %v", err) + } +} diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index a4ba622..dbe81d4 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -132,6 +132,7 @@ func configLoader(g *globalFlags, repo gitutil.Repo, local bool) func(context.Co if err := cfg.LoadGlobalFile(g.configPath); err != nil { return cfg, err } + repoProvider := "" // llm.provider as the repository file chose it if g.repoConfig { data, found, err := repo.ShowFile(ctx, baseSHA, config.RepoConfigFile) if err != nil { @@ -139,9 +140,13 @@ func configLoader(g *globalFlags, repo gitutil.Repo, local bool) func(context.Co } if found { fmt.Fprintf(os.Stderr, "[pruefbyte] using %s from %.8s\n", config.RepoConfigFile, baseSHA) + before := cfg.LLM.Provider if err := cfg.ApplyRepo(data); err != nil { return cfg, err } + if cfg.LLM.Provider != before { + repoProvider = cfg.LLM.Provider + } } if local { // CI reads the file at the merge request's base, so edits on the branch @@ -157,6 +162,12 @@ func configLoader(g *globalFlags, repo gitutil.Repo, local bool) func(context.Co if err := cfg.ApplyEnv(os.LookupEnv); err != nil { return cfg, err } + // Like a provider in the global config, one set by env var is fixed. Say so + // rather than run the repository's llm.model against another provider. + if repoProvider != "" && cfg.LLM.Provider != repoProvider { + return cfg, fmt.Errorf("%s: llm.provider %q: %sLLM_PROVIDER sets llm.provider %q, which a repository cannot change; unset it to let repositories choose", + config.RepoConfigFile, repoProvider, config.EnvPrefix, cfg.LLM.Provider) + } return cfg, cfg.Validate() } } diff --git a/internal/gitutil/git.go b/internal/gitutil/git.go index f6facff..a262101 100644 --- a/internal/gitutil/git.go +++ b/internal/gitutil/git.go @@ -121,13 +121,10 @@ func (r Repo) Head(ctx context.Context) (string, error) { // Snapshot returns a commit holding the working tree as it is now: tracked // changes, staged or not, plus untracked files that are not ignored. It uses a // throwaway copy of the index, so the real index, HEAD and branches stay -// untouched; the commit is unreferenced and git eventually prunes it. Without -// changes it returns HEAD itself. -func (r Repo) Snapshot(ctx context.Context) (string, error) { - head, err := r.Head(ctx) - if err != nil { - return "", err - } +// untouched; the commit is unreferenced and git eventually prunes it. head is +// the commit HEAD points to (see Head), the snapshot's parent; without changes +// Snapshot returns head itself. +func (r Repo) Snapshot(ctx context.Context, head string) (string, error) { tmp, err := os.MkdirTemp("", "pruefbyte-index-") if err != nil { return "", err diff --git a/internal/gitutil/git_test.go b/internal/gitutil/git_test.go index e65b334..4c5d57a 100644 --- a/internal/gitutil/git_test.go +++ b/internal/gitutil/git_test.go @@ -82,7 +82,7 @@ func TestSnapshotAndMergeBase(t *testing.T) { if _, err := r.MergeBase(ctx, "nope", "HEAD"); err == nil { t.Error("unknown target accepted") } - if snap, err := r.Snapshot(ctx); err != nil || snap != head { + if snap, err := r.Snapshot(ctx, head); err != nil || snap != head { t.Errorf("clean tree: Snapshot = %s, %v; want HEAD", snap, err) } @@ -93,7 +93,7 @@ func TestSnapshotAndMergeBase(t *testing.T) { os.WriteFile(filepath.Join(dir, "new.txt"), []byte("n\n"), 0o644) os.WriteFile(filepath.Join(dir, "ignored.txt"), []byte("i\n"), 0o644) statusBefore := git(t, dir, "status", "--porcelain") - snap, err := r.Snapshot(ctx) + snap, err := r.Snapshot(ctx, head) if err != nil || snap == head { t.Fatalf("Snapshot = %s, %v", snap, err) } diff --git a/internal/ocr/ocr_test.go b/internal/ocr/ocr_test.go index 4d573c1..c626c13 100644 --- a/internal/ocr/ocr_test.go +++ b/internal/ocr/ocr_test.go @@ -245,14 +245,17 @@ func TestExtraBodyWithNonStringKeys(t *testing.T) { func TestUserCredential(t *testing.T) { p := filepath.Join(t.TempDir(), "config.json") os.WriteFile(p, []byte(`{"provider":"anthropic","providers":{"anthropic":{"api_key":"sk-user"},"openai":{"api_key_cmd":"op read x"}}, - "custom_providers":{"gw":{"api_key":"gw-key"}}}`), 0o600) - cases := map[string][2]string{"anthropic": {"sk-user", ""}, "openai": {"", "op read x"}, "gw": {"gw-key", ""}, "deepseek": {"", ""}} + "custom_providers":{"gw":{"api_key":"gw-key","url":"https://gw.internal/v1"}}}`), 0o600) + cases := map[string]UserCred{ + "anthropic": {APIKey: "sk-user"}, "openai": {APIKeyCmd: "op read x"}, + "gw": {APIKey: "gw-key", URL: "https://gw.internal/v1"}, "deepseek": {}, + } for provider, want := range cases { - if k, c := userCredential(p, provider); k != want[0] || c != want[1] { - t.Errorf("%s: got %q %q, want %q", provider, k, c, want) + if got := userCredential(p, provider); got != want { + t.Errorf("%s: got %+v, want %+v", provider, got, want) } } - if k, c := userCredential(filepath.Join(t.TempDir(), "missing.json"), "anthropic"); k != "" || c != "" { + if got := userCredential(filepath.Join(t.TempDir(), "missing.json"), "anthropic"); got != (UserCred{}) { t.Error("missing config should give nothing") } } @@ -262,6 +265,10 @@ func TestRunKeyCommand(t *testing.T) { if key, err := RunKeyCommand(ctx, "echo sk-from-cmd"); err != nil || key != "sk-from-cmd" { t.Errorf("got %q, %v", key, err) } + // Quotes inside the command reach the shell as written (cmd.exe ignores Go's \" escaping). + if key, err := RunKeyCommand(ctx, `echo "sk quoted"`); err != nil || key != `"sk quoted"` && key != "sk quoted" { + t.Errorf("quoted: got %q, %v", key, err) + } for _, bad := range []string{"exit 3", "echo a && echo b"} { if key, err := RunKeyCommand(ctx, bad); err == nil { t.Errorf("%q: accepted %q", bad, key) diff --git a/internal/ocr/shell_other.go b/internal/ocr/shell_other.go new file mode 100644 index 0000000..e391f52 --- /dev/null +++ b/internal/ocr/shell_other.go @@ -0,0 +1,13 @@ +//go:build !windows + +package ocr + +import ( + "context" + "os/exec" +) + +// shellCommand runs command through sh. +func shellCommand(ctx context.Context, command string) *exec.Cmd { + return exec.CommandContext(ctx, "sh", "-c", command) +} diff --git a/internal/ocr/shell_windows.go b/internal/ocr/shell_windows.go new file mode 100644 index 0000000..be865da --- /dev/null +++ b/internal/ocr/shell_windows.go @@ -0,0 +1,20 @@ +//go:build windows + +package ocr + +import ( + "context" + "os/exec" + "syscall" +) + +// shellCommand runs command through cmd.exe. Go's argument escaping (\") means +// nothing to cmd.exe, so a command with quotes inside, such as +// op read "op://vault/item/key", would reach the program mangled. The command +// line is built by hand instead, as Node's shell option does: /s strips only the +// outer quotes and passes the rest as written. +func shellCommand(ctx context.Context, command string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "cmd.exe") + cmd.SysProcAttr = &syscall.SysProcAttr{CmdLine: `cmd.exe /d /s /c "` + command + `"`} + return cmd +} diff --git a/internal/ocr/usercfg.go b/internal/ocr/usercfg.go index e4a9e35..8301f64 100644 --- a/internal/ocr/usercfg.go +++ b/internal/ocr/usercfg.go @@ -5,48 +5,50 @@ import ( "encoding/json" "errors" "os" - "os/exec" "path/filepath" - "runtime" "strings" "time" "github.com/feinarbyte/pruefbyte/internal/config" ) +// UserCred is what the user's own OCR setup has for a provider. +type UserCred struct { + APIKey string `json:"api_key"` + APIKeyCmd string `json:"api_key_cmd"` // command printing the key + // URL is the endpoint the key is for, when the user's setup overrides the + // provider's default. + URL string `json:"url"` +} + // UserCredential returns the API key, or the command producing it, that the // user's own OCR setup (~/.opencodereview/config.json) has for provider. Local -// runs use it so a developer who already runs OCR needs no extra setup. Both are -// empty when there is none. -func UserCredential(provider string) (apiKey, apiKeyCmd string) { +// runs use it so a developer who already runs OCR needs no extra setup. It is +// the zero value when there is none. +func UserCredential(provider string) UserCred { home, err := os.UserHomeDir() if err != nil { - return "", "" + return UserCred{} } return userCredential(filepath.Join(home, ".opencodereview", "config.json"), provider) } -func userCredential(path, provider string) (string, string) { +func userCredential(path, provider string) UserCred { data, err := os.ReadFile(path) if err != nil { - return "", "" - } - type cred struct { - APIKey string `json:"api_key"` - APIKeyCmd string `json:"api_key_cmd"` + return UserCred{} } var doc struct { - Providers map[string]cred `json:"providers"` - CustomProviders map[string]cred `json:"custom_providers"` + Providers map[string]UserCred `json:"providers"` + CustomProviders map[string]UserCred `json:"custom_providers"` } if json.Unmarshal(data, &doc) != nil { - return "", "" + return UserCred{} } - c := doc.CustomProviders[provider] if config.IsBuiltinProvider(provider) { - c = doc.Providers[provider] + return doc.Providers[provider] } - return c.APIKey, c.APIKeyCmd + return doc.CustomProviders[provider] } // RunKeyCommand runs an api_key_cmd the way OCR does: through the platform's @@ -56,12 +58,7 @@ func userCredential(path, provider string) (string, string) { func RunKeyCommand(ctx context.Context, command string) (string, error) { ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() - var cmd *exec.Cmd - if runtime.GOOS == "windows" { - cmd = exec.CommandContext(ctx, "cmd.exe", "/c", command) - } else { - cmd = exec.CommandContext(ctx, "sh", "-c", command) - } + cmd := shellCommand(ctx, command) cmd.Stdin, cmd.Stderr = os.Stdin, os.Stderr cmd.WaitDelay = 5 * time.Second // a daemon left holding stdout must not block forever out, err := cmd.Output() diff --git a/internal/review/local.go b/internal/review/local.go index d7fc05d..9d71c3d 100644 --- a/internal/review/local.go +++ b/internal/review/local.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "strings" + "unicode" "github.com/feinarbyte/pruefbyte/internal/gitlab" "github.com/feinarbyte/pruefbyte/internal/ocr" @@ -54,25 +55,33 @@ func Local(ctx context.Context, d Deps, t LocalTarget, repoDir string) (*LocalOu } return nil, fmt.Errorf("%w: %w", ErrReviewFailed, err) } - findings := filterFindings(res.Comments, cfg.Review) + // CI posts a finding OCR reports twice in one run only once. + var findings []ocr.Comment + seen := map[string]bool{} + for _, c := range filterFindings(res.Comments, cfg.Review) { + if key := repeatKey(c, fingerprint(c)); !seen[key] { + seen[key] = true + findings = append(findings, c) + } + } return &LocalOutcome{Result: res, Findings: findings, GateReason: gateReason(findings, cfg.Review)}, nil } // WriteLocal prints a local review's findings for a terminal. func WriteLocal(w io.Writer, o *LocalOutcome) { for _, c := range o.Findings { - head := c.Path + head := plain(c.Path) switch start, end := lineSpan(c); { case end > start: head += fmt.Sprintf(":%d-%d", start, end) case start > 0: head += fmt.Sprintf(":%d", start) } - if b := strings.Trim(badge(c), "*"); b != "" { + if b := strings.Trim(plain(badge(c)), "*"); b != "" { head += " " + b } - fmt.Fprintf(w, "%s\n%s\n", head, indent(strings.TrimSpace(c.Content), " ")) - if code := strings.TrimRight(c.SuggestionCode, "\n"); code != "" { + fmt.Fprintf(w, "%s\n%s\n", head, indent(strings.TrimSpace(plain(c.Content)), " ")) + if code := strings.TrimRight(plain(c.SuggestionCode), "\n"); code != "" { fmt.Fprintf(w, "\n Suggested change:\n%s\n", indent(code, " ")) } fmt.Fprintln(w) @@ -83,18 +92,33 @@ func WriteLocal(w io.Writer, o *LocalOutcome) { } else { fmt.Fprintf(w, "%d finding(s)", len(o.Findings)) if hidden > 0 { - fmt.Fprintf(w, ", %d more below review.min_severity or outside review.categories", hidden) + fmt.Fprintf(w, ", %d more below review.min_severity, outside review.categories, empty or repeated", hidden) } fmt.Fprintln(w, ".") } + // CI lists these in its summary note. + for _, wn := range o.Result.Warnings { + fmt.Fprintf(w, "OCR warning: %s\n", plain(strings.TrimSpace(wn.File+" "+strings.Join(strings.Fields(wn.Message), " ")))) + } if !o.Result.Complete() { - fmt.Fprintf(w, "Warning: OCR's review was incomplete (status %s); CI may report more.\n", o.Result.Status) + fmt.Fprintf(w, "Warning: OCR's review was incomplete (status %s); CI may report more.\n", plain(o.Result.Status)) } if o.GateReason != "" { fmt.Fprintf(w, "CI would fail this merge request: %s\n", o.GateReason) } } +// plain drops control characters other than tab and newline from model output, +// so a finding cannot move the cursor, recolor or retitle the terminal. +func plain(s string) string { + return strings.Map(func(r rune) rune { + if r == '\n' || r == '\t' || !unicode.IsControl(r) { + return r + } + return -1 + }, s) +} + func indent(s, prefix string) string { lines := strings.Split(s, "\n") for i, l := range lines { diff --git a/internal/review/review.go b/internal/review/review.go index 5890ad0..7a14dbd 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -162,9 +162,7 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { 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), " "))) + key := repeatKey(c, fp) if seen[key] { continue } @@ -233,6 +231,13 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { return out, nil } +// repeatKey identifies a finding reported twice in one run. Two findings share a +// fingerprint (fp) when they quote the same code; only an identical report at +// the same place is the same finding twice. +func repeatKey(c ocr.Comment, fp string) string { + return fmt.Sprintf("%s:%d-%d:%s", fp, c.StartLine, c.EndLine, strings.ToLower(strings.Join(strings.Fields(c.Content), " "))) +} + // runOCR runs the review within timeout, keeps OCR's raw result if asked to, // and turns a result OCR reports as failed into an error. func runOCR(ctx context.Context, d Deps, ro ocr.ReviewOptions, timeout time.Duration) (*ocr.Result, error) {