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 0b10d81..38f057f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,21 +52,51 @@ 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), 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 - 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 + - 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 + 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 + - 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 + 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/.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 new file mode 100644 index 0000000..4d1cb93 --- /dev/null +++ b/.goreleaser.yaml @@ -0,0 +1,50 @@ +# 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 + +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 + env: + - 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] + 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 + - src: internal/ocrbin/bin/LICENSE + dst: LICENSE.open-code-review + +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/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 f235f48..ad85527 100644 --- a/README.md +++ b/README.md @@ -15,19 +15,22 @@ 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 `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): @@ -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, 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. Example `.pruefbyte.yml`: ```yaml +llm: + provider: anthropic + model: claude-sonnet-5 ocr: effort: high exclude: ["**/generated/**"] @@ -116,15 +122,94 @@ 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 + +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 +mise use -g github:feinarbyte/pruefbyte +``` + +**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 ``` -`--dry-run` prints the discussions instead of posting them. `pruefbyte config print` shows the effective configuration. +**Windows** (PowerShell), latest release into `%LOCALAPPDATA%\Programs\pruefbyte`: + +```powershell +$arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') { 'arm64' } else { 'amd64' } +$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') +``` + +Each release also lists the archives with a `checksums.txt` +([releases](https://github.com/feinarbyte/pruefbyte/releases)). + +**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 +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`; 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`. +`pruefbyte config print` shows the effective configuration. ## Development @@ -135,11 +220,21 @@ 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`. + +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: @@ -148,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 | @@ -158,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 new file mode 100644 index 0000000..89937b8 --- /dev/null +++ b/cmd/pruefbyte/local.go @@ -0,0 +1,209 @@ +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: %w", dir, err) + } + 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, head); 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 := newRunner(base.OCR.Binary) + 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 == "" { + 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 + 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) + } + } + 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" { + if out.Findings == nil { + out.Findings = []ocr.Comment{} // "findings": [], not null + } + 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..8a421fc --- /dev/null +++ b/cmd/pruefbyte/local_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "context" + "errors" + "fmt" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/feinarbyte/pruefbyte/internal/gitutil" +) + +// 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") + } +} + +// 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 4a242d3..dbe81d4 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -9,21 +9,34 @@ 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/ocrbin" + "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,10 +80,17 @@ 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) }, + 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 } @@ -85,6 +105,73 @@ 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. +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 + } + repoProvider := "" // llm.provider as the repository file chose it + 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) + 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 + // 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)) + 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) + } + } + } + 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() + } +} + func configCmd(g *globalFlags) *cobra.Command { cmd := &cobra.Command{Use: "config", Short: "Inspect configuration"} var repoFile string @@ -190,7 +277,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 } @@ -204,31 +291,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..097475b 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"` @@ -106,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, @@ -133,7 +136,19 @@ 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) + } + // 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) } @@ -177,10 +192,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..55a978f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -166,3 +166,41 @@ func TestEnvBadValue(t *testing.T) { t.Fatal("non-numeric int accepted") } } + +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.ApplyRepo(repo); err != nil { + t.Fatal(err) + } + if cfg.LLM.Provider != "anthropic" || cfg.LLM.Model != "claude-sonnet-5" { + t.Errorf("llm = %+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 5e02e22..a262101 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,135 @@ 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 copy of the index, so the real index, HEAD and branches stay +// 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 + } + defer os.RemoveAll(tmp) + 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 + } + 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 +} + +// 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) + 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..4c5d57a 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, head); 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, head) + 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..c626c13 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,37 @@ 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","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 got := userCredential(p, provider); got != want { + t.Errorf("%s: got %+v, want %+v", provider, got, want) + } + } + if got := userCredential(filepath.Join(t.TempDir(), "missing.json"), "anthropic"); got != (UserCred{}) { + 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) + } + // 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/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..569738b 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 { @@ -108,10 +108,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 +159,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 { @@ -341,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/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 new file mode 100644 index 0000000..8301f64 --- /dev/null +++ b/internal/ocr/usercfg.go @@ -0,0 +1,79 @@ +package ocr + +import ( + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "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. It is +// the zero value when there is none. +func UserCredential(provider string) UserCred { + home, err := os.UserHomeDir() + if err != nil { + return UserCred{} + } + return userCredential(filepath.Join(home, ".opencodereview", "config.json"), provider) +} + +func userCredential(path, provider string) UserCred { + data, err := os.ReadFile(path) + if err != nil { + return UserCred{} + } + var doc struct { + Providers map[string]UserCred `json:"providers"` + CustomProviders map[string]UserCred `json:"custom_providers"` + } + if json.Unmarshal(data, &doc) != nil { + return UserCred{} + } + if config.IsBuiltinProvider(provider) { + return doc.Providers[provider] + } + return doc.CustomProviders[provider] +} + +// 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() + 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() + 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/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/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..42abfc2 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 ( @@ -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 new file mode 100644 index 0000000..9d71c3d --- /dev/null +++ b/internal/review/local.go @@ -0,0 +1,130 @@ +package review + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "unicode" + + "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 + } + 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 + if errors.As(err, &re) { + err = fmt.Errorf("%w (see ocr's output above)", re.Err) + } + return nil, fmt.Errorf("%w: %w", ErrReviewFailed, err) + } + // 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 := 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(plain(badge(c)), "*"); b != "" { + head += " " + b + } + 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) + } + 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, 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", 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 { + 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..7a14dbd 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -12,10 +12,11 @@ import ( "path" "path/filepath" "strings" + "time" - "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. @@ -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() @@ -171,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 } @@ -234,23 +223,53 @@ 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 } +// 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) { + 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 == "" { + 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..37b24ff 100644 --- a/pruefbyte.example.yml +++ b/pruefbyte.example.yml @@ -7,6 +7,10 @@ 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. + # 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 @@ -19,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 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)