From 2b4d02c75c7df7c3b9732697e3138e2335a3dad3 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 24 Sep 2026 14:57:14 +0300 Subject: [PATCH 1/7] Refresh models, isolate runners, classify failures - Add Opus 5.5, Fable 5.1, GPT-6 and deepseek-flash to price tables and model lists; patch retired model ids in runner profiles - Add an OpenAI Responses API provider for GPT-6 in the direct runner; custom base URLs (Azure, LiteLLM) keep chat completions - Pass reasoning effort to DeepSeek and raise its output cap to 128K - Update the codex runner: effort, per-call usage on resume, and a --codex-sandbox flag for containers where bubblewrap can't start - Isolate CLI runners from the reviewed checkout: claude loads user settings only and no MCP servers; opencode runs without project config, formatters and LSP, and refuses a checkout that ships .opencode/ or opencode.json(c), whose plugins it would execute - Switch opencode to --auto - Classify run outcomes (status + reason) for all runners, report them with cost in debug bundles, and export reviewer_runs_total and reviewer_run_cost_usd_total - Retry transient provider errors with one shared policy; turn off the Anthropic SDK's own retries so attempts don't stack - Warn the direct runner before its round limit, add grace rounds and a maxRounds runner-profile param (--max-rounds) - Fix direct runner diff refs in GitLab CI (bare branch names) - Fix cost reporting to include failed runs, the Step 2 retry and failed judge attempts - Fix debug bundle uploads capped at 2MB by the global body limit - Keep debug artifacts gzip-compressed in memory; clip bundle metadata - Handle SIGTERM in reviewctl: a cancelled job is not a failure - Update go-openai to v1.42.1 --- README.md | 39 +- cfg/local.toml.dist | 5 + cmd/reviewctl/README.md | 15 +- cmd/reviewctl/main.go | 46 +- docs/patches/2026-09-23-retired-models.sql | 32 + frontend/src/api/vt.generated.ts | 4 +- .../src/vt/pages/projects/ProjectsPage.vue | 12 +- .../runner-profiles/RunnerProfileFormPage.vue | 50 +- go.mod | 2 +- go.sum | 4 +- pkg/app/app.go | 22 +- pkg/app/handlers.go | 28 +- pkg/app/handlers_test.go | 32 + pkg/app/metrics.go | 2 + pkg/db/model_params.go | 4 +- pkg/debug/handler.go | 148 ++- pkg/debug/handler_test.go | 113 ++- pkg/debug/storage.go | 81 +- pkg/debug/storage_test.go | 61 +- pkg/debug/templates/bundle.html | 6 +- pkg/debug/templates/list.html | 11 +- pkg/rest/rest.go | 11 +- pkg/rest/rest_test.go | 6 +- pkg/reviewctl/reviewctl.go | 3 +- pkg/reviewctl/reviewctl_zenrpc.go | 7 +- pkg/reviewer/ctl/config.go | 32 +- pkg/reviewer/ctl/config_test.go | 8 +- pkg/reviewer/ctl/ctl.go | 63 +- pkg/reviewer/ctl/ctl_test.go | 115 +++ pkg/reviewer/ctl/env.go | 26 +- pkg/reviewer/ctl/env_test.go | 9 + pkg/reviewer/ctl/panel.go | 16 +- pkg/reviewer/ctl/prompt.go | 3 +- .../ctl/reviewctlclient/client.generated.go | 2 + pkg/reviewer/ctl/skip_detect_test.go | 6 +- pkg/reviewer/ctl/upload.go | 14 + pkg/reviewer/ctl/upload_test.go | 6 + pkg/reviewer/direct/errors.go | 105 ++ pkg/reviewer/direct/loop.go | 111 ++- pkg/reviewer/direct/loop_test.go | 108 +- pkg/reviewer/direct/options.go | 22 +- pkg/reviewer/direct/preload_test.go | 89 ++ pkg/reviewer/direct/provider.go | 8 +- pkg/reviewer/direct/provider_anthropic.go | 33 +- .../direct/provider_anthropic_test.go | 130 +++ pkg/reviewer/direct/provider_factory.go | 137 ++- pkg/reviewer/direct/provider_factory_test.go | 57 +- pkg/reviewer/direct/provider_openai.go | 122 +-- pkg/reviewer/direct/provider_openai_test.go | 93 +- pkg/reviewer/direct/provider_responses.go | 194 ++++ .../direct/provider_responses_test.go | 136 +++ pkg/reviewer/direct/registry.go | 23 +- pkg/reviewer/direct/retry.go | 53 + pkg/reviewer/direct/sink.go | 2 + pkg/reviewer/direct/tools_git.go | 39 +- pkg/reviewer/direct/tools_review.go | 3 +- pkg/reviewer/direct/types.go | 36 +- pkg/reviewer/project.go | 10 + pkg/reviewer/run_metrics.go | 62 ++ pkg/reviewer/run_metrics_test.go | 34 + pkg/reviewer/run_outcome.go | 122 +++ pkg/reviewer/run_outcome_test.go | 70 ++ pkg/reviewer/runner/claude.go | 44 +- pkg/reviewer/runner/claude_failure.go | 133 +++ pkg/reviewer/runner/claude_failure_test.go | 129 +++ pkg/reviewer/runner/claude_test.go | 23 +- pkg/reviewer/runner/codex.go | 177 ++-- pkg/reviewer/runner/codex_test.go | 107 +- pkg/reviewer/runner/direct.go | 73 +- pkg/reviewer/runner/opencode.go | 87 +- pkg/reviewer/runner/opencode_test.go | 68 ++ pkg/vt/runner_profile.go | 9 +- pkg/vt/runner_profile_model.go | 5 +- pkg/vt/vt_zenrpc.go | 27 +- .../sashabaranov/go-openai/.codecov.yml | 8 + .../sashabaranov/go-openai/.gitignore | 4 +- .../sashabaranov/go-openai/.golangci.yml | 29 +- .../sashabaranov/go-openai/README.md | 943 +++--------------- .../sashabaranov/go-openai/audio.go | 70 +- .../sashabaranov/go-openai/batch.go | 28 +- .../github.com/sashabaranov/go-openai/chat.go | 51 +- .../sashabaranov/go-openai/client.go | 4 +- .../sashabaranov/go-openai/completion.go | 65 +- .../sashabaranov/go-openai/config.go | 7 +- .../sashabaranov/go-openai/fine_tunes.go | 1 + .../sashabaranov/go-openai/image.go | 41 +- .../go-openai/internal/form_builder.go | 19 +- .../sashabaranov/go-openai/jsonschema/json.go | 235 ----- .../go-openai/jsonschema/validate.go | 140 --- .../sashabaranov/go-openai/response.go | 597 +++++++++++ .../sashabaranov/go-openai/response_stream.go | 131 +++ .../sashabaranov/go-openai/stream.go | 2 +- .../sashabaranov/go-openai/stream_reader.go | 10 +- vendor/modules.txt | 3 +- 94 files changed, 4338 insertions(+), 1775 deletions(-) create mode 100644 docs/patches/2026-09-23-retired-models.sql create mode 100644 pkg/app/handlers_test.go create mode 100644 pkg/reviewer/direct/errors.go create mode 100644 pkg/reviewer/direct/provider_responses.go create mode 100644 pkg/reviewer/direct/provider_responses_test.go create mode 100644 pkg/reviewer/direct/retry.go create mode 100644 pkg/reviewer/run_metrics.go create mode 100644 pkg/reviewer/run_metrics_test.go create mode 100644 pkg/reviewer/run_outcome.go create mode 100644 pkg/reviewer/run_outcome_test.go create mode 100644 pkg/reviewer/runner/claude_failure.go create mode 100644 pkg/reviewer/runner/claude_failure_test.go delete mode 100644 vendor/github.com/sashabaranov/go-openai/jsonschema/json.go delete mode 100644 vendor/github.com/sashabaranov/go-openai/jsonschema/validate.go create mode 100644 vendor/github.com/sashabaranov/go-openai/response.go create mode 100644 vendor/github.com/sashabaranov/go-openai/response_stream.go diff --git a/README.md b/README.md index 2bf1abc..949bccc 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,10 @@ PoolSize = 5 [Sentry] DSN = "" Environment = "" + +[Debug] # optional; defaults shown +Capacity = 200 # runs kept at /v1/debug/storage/ (metadata) +FilesCapacity = 20 # newest runs that keep their artifact files ``` ## API Endpoints @@ -152,14 +156,14 @@ reviewctl comment # Post MR comments for an existing review reviewctl version # Print version ``` -Key flags: `--key`, `--url`, `--runner` (`claude` | `opencode` | `codex` | `direct`), `--model`, `--session` (prompt cache reuse), `--continue` (resume last session), `--allow-dangerous-permissions` (opencode `--dangerously-skip-permissions`, default `true` for unattended CI). All flags have env variable equivalents for CI. See `reviewctl --help` for details. +Key flags: `--key`, `--url`, `--runner` (`claude` | `opencode` | `codex` | `direct`), `--model`, `--session` (prompt cache reuse), `--continue` (resume last session), `--allow-dangerous-permissions` (opencode `--auto`; default `true` for unattended CI). All flags have env variable equivalents for CI. See `reviewctl --help` for details. **Runners:** the runner and its model/effort/provider come from the project's **runner profile** (configured in the admin panel and fetched at run time); CI does not pass them. The `--runner`/`--model`/`--effort`/`--api-*` flags still override the profile for local runs. - `claude` (default) — Claude Code CLI, full agentic exploration. -- `opencode` — opencode CLI (any provider configured in opencode, incl. OpenRouter), `--model provider/model`. -- `codex` — `codex exec` CLI (OpenAI Codex), `--model gpt-5.1-codex`. -- `direct` — calls the LLM API itself (no CLI) with a narrow review tool set (read/grep/glob/git_diff/ast, plus `http_fetch` scoped to the project's task tracker). Prompt caching + diff preload make it the cheapest and fastest path. Adds `--api-provider` (`deepseek` | `openai-compat` | `anthropic`), `--api-base-url`, `--effort` (`low`..`max`); the API key comes from `REVIEW_API_KEY` (or `ANTHROPIC_API_KEY` / `DEEPSEEK_API_KEY` / `OPENAI_API_KEY`). The `ast_*` navigation tools shell out to the optional [`ast-index`](https://github.com/defendend/Claude-ast-index-search) binary — auto-detected on `PATH` (the generated CI image installs it), with the index rebuilt per run; when absent the tools are simply not offered. +- `opencode` — opencode CLI (any provider configured in opencode, incl. OpenRouter), `--model provider/model`. Runs on the operator's own opencode config only (project config, formatters and LSP off) and refuses a checkout that ships `.opencode/` or `opencode.json(c)`: opencode would execute their plugins with the job's secrets, and no opencode switch prevents it. +- `codex` — `codex exec` CLI (OpenAI Codex), `--model gpt-6-sol` by default; `--effort` maps to `model_reasoning_effort`. +- `direct` — calls the LLM API itself (no CLI) with a narrow review tool set (read/grep/glob/git_diff/ast, plus `http_fetch` scoped to the project's task tracker). Prompt caching + diff preload make it the cheapest and fastest path. Adds `--api-provider` (`deepseek` | `openai` (Responses API) | `openai-compat` | `anthropic`), `--api-base-url`, `--effort` (`low`..`max`); the API key comes from `REVIEW_API_KEY` (or `ANTHROPIC_API_KEY` / `DEEPSEEK_API_KEY` / `OPENAI_API_KEY`). The `ast_*` navigation tools shell out to the optional [`ast-index`](https://github.com/defendend/Claude-ast-index-search) binary — auto-detected on `PATH` (the generated CI image installs it), with the index rebuilt per run; when absent the tools are simply not offered. **Task tracker access:** the tracker token never appears in the prompt. reviewctl fetches it with the review config and hands it to runners out-of-band: CLI runners get it as the `REVIEW_TRACKER_TOKEN` env var (prompts reference `$REVIEW_TRACKER_TOKEN` in curl instructions), the `direct` runner offers the model an `http_fetch` tool locked to the tracker origin that injects the `Authorization` header itself. A `REVIEW_TRACKER_TOKEN` CI variable overrides the server-stored token. Backward compatible in both directions: a legacy reviewctl (which doesn't export the env var) receives the old-style prompt with the token substituted in, and a new reviewctl against an older server falls back to the same legacy prompt. @@ -219,6 +223,7 @@ When deploying behind a reverse proxy, URLs should be split by access level: | `/v1/upload/` | Deprecated upload aliases + debug bundle upload (older CI images) | | `/v1/debug/` | Debug bundle viewer — raw runner transcripts and artifacts | | `/debug/` | pprof and service metadata | +| `/metrics` | Prometheus metrics (incl. per-project run outcomes and spend) | | `/status` | Healthcheck (DB ping) | Example nginx configuration: @@ -237,9 +242,35 @@ location /v1/reviewctl/ { deny all; } location /v1/upload/ { deny all; } location /v1/debug/ { deny all; } location /debug/ { deny all; } +location /metrics { deny all; } location /status { deny all; } ``` +### Monitoring + +Besides HTTP and DB-pool metrics, `/metrics` (Prometheus) exposes the outcome of every reviewctl run: + +| Metric | Labels | Description | +|--------|--------|-------------| +| `reviewer_runs_total` | `project`, `runner`, `status`, `reason` | Runs by outcome: `ok` is counted when the review is uploaded, any other status when reviewctl uploads the run's debug bundle | +| `reviewer_run_cost_usd_total` | `project`, `runner`, `status` | Dollars spent by runs, failed ones included | + +- `status`: `ok` \| `failed` \| `cancelled` (the CI job was cancelled — not a failure) \| `timeout`. +- `reason` (for `failed`): `billing` (provider credit balance exhausted), `auth`, `rate_limit`, `api_error` (provider 5xx / overload / dropped stream, after retries), `max_rounds`, `truncated`, `not_submitted` (the last three from the `direct` runner), `other`. +- `project` is the project title (`unknown` for an unknown key); a runner outside `claude`/`opencode`/`codex`/`direct` is `other`. + +Alerting examples: + +```promql +# Credit balance exhausted — every review fails until it is topped up +increase(reviewer_runs_total{reason="billing"}[15m]) > 0 + +# A project keeps failing (cancelled jobs excluded) +sum by (project) (increase(reviewer_runs_total{status=~"failed|timeout"}[1h])) >= 3 +``` + +Counters live in memory and reset on restart (`increase()` handles that). For the details of a run, failed runs upload their artifacts (runner transcript, `review.json`, `R*.md`) to `/v1/debug/storage/`: the list shows status, reason, cost and error of the last 200 runs, the newest 20 keep their files (see `[Debug]`); the buffer is dropped on restart. reviewsrv logs each such upload as `debug bundle stored` — at WARN for `failed`/`timeout`, at INFO for a cancelled job. + ## Development ```bash diff --git a/cfg/local.toml.dist b/cfg/local.toml.dist index cd9a618..e65a5ee 100644 --- a/cfg/local.toml.dist +++ b/cfg/local.toml.dist @@ -15,3 +15,8 @@ ApplicationName = "reviewsrv" [Sentry] DSN = "" Environment = "" + +# In-memory ring of recent reviewctl runs at /v1/debug/storage/ (defaults shown). +[Debug] +Capacity = 200 # runs kept (metadata) +FilesCapacity = 20 # newest runs that keep their artifact files diff --git a/cmd/reviewctl/README.md b/cmd/reviewctl/README.md index d20a6da..0128d28 100644 --- a/cmd/reviewctl/README.md +++ b/cmd/reviewctl/README.md @@ -19,6 +19,13 @@ passes only the project key, server URL and credentials, so it stays thin. The runner flags below are **local overrides**: an explicit flag always wins over the profile. Multi-review is profile-driven too (see below). +The reviewed checkout is untrusted, so CLI runners ignore the agent config it +ships: `claude` loads user settings only and no MCP servers; `opencode` runs +with project config, formatters and LSP off (`OPENCODE_DISABLE_PROJECT_CONFIG`, +`OPENCODE_CONFIG_CONTENT`; set either yourself to override) and refuses to start +when the checkout contains `.opencode/` or `opencode.json(c)`, whose plugins it +would load regardless. + ## Flags & Environment Variables Most flags have an environment-variable default for CI. @@ -43,10 +50,12 @@ Most flags have an environment-variable default for CI. |------|-------------|---------|-------------| | `--runner` | `$REVIEW_RUNNER` | `claude` | Runner: `claude` \| `opencode` \| `codex` \| `direct` | | `--model` | `$REVIEW_MODEL` | *runner default* | Model name | -| `--effort` | `$REVIEW_EFFORT` | — | Reasoning effort `low`..`max` (honored by the `claude` and `direct` Anthropic runners; others ignore it) | -| `--api-provider` | `$REVIEW_API_PROVIDER` | `deepseek` | `direct` runner provider: `deepseek` \| `openai-compat` \| `anthropic` | +| `--effort` | `$REVIEW_EFFORT` | — | Reasoning effort `low`..`max` (honored by the `claude`, `codex` and `direct` runners; opencode and `direct` with `openai-compat` ignore it) | +| `--api-provider` | `$REVIEW_API_PROVIDER` | `deepseek` | `direct` runner provider: `deepseek` \| `openai` (Responses API) \| `openai-compat` \| `anthropic` | | `--api-base-url` | `$REVIEW_API_BASE_URL` | *provider default* | `direct` runner API base URL | -| `--allow-dangerous-permissions` | `$REVIEW_ALLOW_DANGEROUS_PERMISSIONS` | `true` | Pass `--dangerously-skip-permissions` to opencode (needed for unattended CI) | +| `--max-rounds` | `$REVIEW_MAX_ROUNDS` | `60` | `direct` runner round budget; the model is warned 10 and 3 rounds before it, then gets up to 5 review-only rounds to deliver | +| `--allow-dangerous-permissions` | `$REVIEW_ALLOW_DANGEROUS_PERMISSIONS` | `true` | Pass `--auto` to opencode (needed for unattended CI) | +| `--codex-sandbox` | `$REVIEW_CODEX_SANDBOX` | `workspace-write` | codex `--sandbox` mode; `danger-full-access` inside containers, where codex's bubblewrap sandbox can't start | ### MR & CI metadata (from GitLab CI) diff --git a/cmd/reviewctl/main.go b/cmd/reviewctl/main.go index ec0fdab..67f1887 100644 --- a/cmd/reviewctl/main.go +++ b/cmd/reviewctl/main.go @@ -1,10 +1,13 @@ package main import ( + "context" "fmt" "log/slog" "os" + "os/signal" "strings" + "syscall" "time" "reviewsrv/pkg/reviewer" @@ -60,12 +63,14 @@ func main() { pf.StringVar(&cfg.SessionID, "session", "", "Claude session ID for --resume (reuses prompt cache)") pf.BoolVar(&cfg.ContinueSession, "continue", false, "continue last Claude session (auto-detect)") pf.BoolVar(&cfg.DebugUpload, "debug-upload", ctl.EnvBool("REVIEW_DEBUG_UPLOAD", false), "always upload artifacts to /v1/upload/debug/ (failures upload regardless)") - pf.BoolVar(&cfg.AllowDangerousPermissions, "allow-dangerous-permissions", ctl.EnvBool("REVIEW_ALLOW_DANGEROUS_PERMISSIONS", true), "pass --dangerously-skip-permissions to opencode (default true; required for unattended CI)") - pf.StringVar(&cfg.APIProvider, "api-provider", ctl.EnvDefault("REVIEW_API_PROVIDER", "deepseek"), "direct runner provider: deepseek | openai-compat | anthropic (key from ANTHROPIC_API_KEY/DEEPSEEK_API_KEY env)") + pf.BoolVar(&cfg.AllowDangerousPermissions, "allow-dangerous-permissions", ctl.EnvBool("REVIEW_ALLOW_DANGEROUS_PERMISSIONS", true), "pass --auto to opencode, auto-approving permission prompts (default true; required for unattended CI)") + pf.StringVar(&cfg.CodexSandbox, "codex-sandbox", os.Getenv("REVIEW_CODEX_SANDBOX"), "codex --sandbox mode: workspace-write (default) | read-only | danger-full-access (for containers, where codex's bubblewrap sandbox can't start)") + pf.StringVar(&cfg.APIProvider, "api-provider", ctl.EnvDefault("REVIEW_API_PROVIDER", "deepseek"), "direct runner provider: deepseek | openai | openai-compat | anthropic (key from ANTHROPIC_API_KEY/OPENAI_API_KEY/DEEPSEEK_API_KEY env)") pf.StringVar(&cfg.APIBaseURL, "api-base-url", os.Getenv("REVIEW_API_BASE_URL"), "direct runner API base URL (defaults to provider's standard endpoint)") - pf.StringVar(&cfg.Effort, "effort", os.Getenv("REVIEW_EFFORT"), "direct runner reasoning effort for Anthropic: low|medium|high|xhigh|max") - pf.StringVar(&multiRaw, "multi", os.Getenv("REVIEW_MULTI"), "local multi-review panel: comma-separated runner:model members (e.g. codex:gpt-5.5,opencode:deepseek-v4); bypasses server config, uses ambient credentials") + pf.StringVar(&cfg.Effort, "effort", os.Getenv("REVIEW_EFFORT"), "reasoning effort for claude, codex and direct: low|medium|high|xhigh|max") + pf.StringVar(&multiRaw, "multi", os.Getenv("REVIEW_MULTI"), "local multi-review panel: comma-separated runner:model members (e.g. codex:gpt-6-sol,opencode:deepseek-v4); bypasses server config, uses ambient credentials") pf.StringVar(&judgeRaw, "judge", os.Getenv("REVIEW_JUDGE"), "multi-review judge runner:model (e.g. claude:opus); with >=2 --multi members, fuses them into one review") + pf.IntVar(&cfg.MaxRounds, "max-rounds", ctl.EnvInt("REVIEW_MAX_ROUNDS", 0), "direct runner round budget (0 = default 60); overrides the runner profile") pf.DurationVar(&cfg.Timeout, "timeout", ctl.EnvDuration("REVIEW_TIMEOUT", 30*time.Minute), "per-member/judge run timeout (e.g. 30m, 1h); 0 = no timeout") // --multi/--judge are local debug overrides (ambient creds, bypass the server // panel config). Keep them working but hidden so they don't become a stable CLI @@ -116,7 +121,13 @@ func main() { } rootCmd.AddCommand(reviewCmd, uploadCmd, commentCmd, versionCmd) - if err := rootCmd.Execute(); err != nil { + // A cancelled CI job sends SIGTERM: cancel the run context instead of dying + // mid-flight, so the runner stops and the debug bundle still ships, marked + // cancelled rather than failed. + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + err := rootCmd.ExecuteContext(ctx) + stop() + if err != nil { os.Exit(1) } } @@ -185,6 +196,9 @@ func applyReviewConfig(cmd *cobra.Command, cfg *ctl.Config, log *slog.Logger) er if !fl.Changed("allow-dangerous-permissions") { cfg.AllowDangerousPermissions = p.Params.AllowDangerousPermissions } + if cfg.MaxRounds == 0 { + cfg.MaxRounds = p.Params.MaxRounds + } // Task-tracker access: the URL comes from the server config; the CI env var // wins over the server-stored token (same precedence as the API keys). The @@ -263,11 +277,11 @@ func buildRunner(cfg *ctl.Config, log *slog.Logger) (runner.ReviewRunner, error) Log: log, }, nil case runner.RunnerCodex: - return &runner.ExecCodexRunner{Model: cfg.Model, Dir: cfg.Dir, SessionID: cfg.SessionID, ContinueSession: cfg.ContinueSession, Token: cfg.Token, TrackerToken: cfg.TrackerToken, Log: log}, nil + return &runner.ExecCodexRunner{Model: cfg.Model, Effort: cfg.Effort, Sandbox: cfg.CodexSandbox, Dir: cfg.Dir, SessionID: cfg.SessionID, ContinueSession: cfg.ContinueSession, Token: cfg.Token, TrackerToken: cfg.TrackerToken, Log: log}, nil case runner.RunnerDirect: return buildDirectRunner(cfg, log) default: - return nil, fmt.Errorf("unknown --runner %q (supported: %s, %s, %s, %s)", cfg.Runner, runner.RunnerClaude, runner.RunnerOpenCode, runner.RunnerCodex, runner.RunnerDirect) + return nil, fmt.Errorf("unknown --runner %q (supported: %s)", cfg.Runner, strings.Join(runner.Names, ", ")) } } @@ -295,14 +309,16 @@ func buildDirectRunner(cfg *ctl.Config, log *slog.Logger) (runner.ReviewRunner, tracker = &direct.TrackerConfig{URL: cfg.TrackerURL, Token: cfg.TrackerToken} } return &runner.DirectRunner{ - Provider: prov, - Dir: cfg.Dir, - DiffBase: cfg.TargetBranch, - DiffHead: cfg.SourceBranch, - Effort: cfg.Effort, - CompactAt: direct.DefaultCompactAt(cfg.APIProvider), - Tracker: tracker, - Log: log, + Provider: prov, + Dir: cfg.Dir, + DiffBase: cfg.TargetBranch, + DiffHead: cfg.SourceBranch, + DiffBaseSHA: cfg.DiffBaseSHA, + Effort: cfg.Effort, + MaxRounds: cfg.MaxRounds, + CompactAt: direct.DefaultCompactAt(cfg.APIProvider), + Tracker: tracker, + Log: log, }, nil } diff --git a/docs/patches/2026-09-23-retired-models.sql b/docs/patches/2026-09-23-retired-models.sql new file mode 100644 index 0000000..5f629c9 --- /dev/null +++ b/docs/patches/2026-09-23-retired-models.sql @@ -0,0 +1,32 @@ +-- Remap runner profiles that point at model ids their providers have retired (as of +-- 2026-09) to the current replacements. Data only — model/effort are free-form columns, +-- no DDL needed; profiles on live models are left as chosen. The ids are unambiguous, +-- so the mapping applies to every runner (prefixed ones only exist under opencode). +UPDATE "runnerProfiles" p SET "model" = m."new" +FROM (VALUES + -- OpenAI: the gpt-5-codex / gpt-5.1-codex* / gpt-5.2-codex family, shut down 2026-07-23. + ('gpt-5-codex', 'gpt-6-sol'), + ('gpt-5.1-codex', 'gpt-6-sol'), + ('gpt-5.1-codex-max', 'gpt-6-sol'), + ('gpt-5.1-codex-mini', 'gpt-6-sol'), + ('gpt-5.2-codex', 'gpt-6-sol'), + -- DeepSeek: V4 Flash and the chat/reasoner aliases are retired, served as deepseek-flash. + ('deepseek-v4-flash', 'deepseek-flash'), + ('deepseek-chat', 'deepseek-flash'), + ('deepseek-reasoner', 'deepseek-flash'), + ('deepseek/deepseek-v4-flash', 'deepseek/deepseek-flash'), + -- Anthropic: Opus 4.1 retired 2026-08-05, Opus 4 / Sonnet 4 retired 2026-06-15. + ('claude-opus-4-1', 'claude-opus-5-5'), + ('claude-opus-4-1-20250805', 'claude-opus-5-5'), + ('claude-opus-4-0', 'claude-opus-5-5'), + ('claude-opus-4-20250514', 'claude-opus-5-5'), + ('claude-sonnet-4-0', 'claude-sonnet-5'), + ('claude-sonnet-4-20250514', 'claude-sonnet-5'), + ('anthropic/claude-opus-4-1', 'anthropic/claude-opus-5-5'), + ('anthropic/claude-opus-4-1-20250805', 'anthropic/claude-opus-5-5'), + ('anthropic/claude-opus-4-0', 'anthropic/claude-opus-5-5'), + ('anthropic/claude-opus-4-20250514', 'anthropic/claude-opus-5-5'), + ('anthropic/claude-sonnet-4-0', 'anthropic/claude-sonnet-5'), + ('anthropic/claude-sonnet-4-20250514', 'anthropic/claude-sonnet-5') +) AS m("old", "new") +WHERE p."model" = m."old"; diff --git a/frontend/src/api/vt.generated.ts b/frontend/src/api/vt.generated.ts index d513ae5..46bda41 100644 --- a/frontend/src/api/vt.generated.ts +++ b/frontend/src/api/vt.generated.ts @@ -194,7 +194,8 @@ export interface IRunnerProfile { } export interface IRunnerProfileParams { - allowDangerousPermissions: boolean + allowDangerousPermissions: boolean, + maxRounds: number // direct runner round budget; 0 = default } export interface IRunnerProfileSearch { @@ -683,6 +684,7 @@ export class RunnerProfileParams implements IRunnerProfileParams { static entityName = "runnerprofileparams"; allowDangerousPermissions: boolean = false; + maxRounds: number = 0; } export class RunnerProfileSearch implements IRunnerProfileSearch { diff --git a/frontend/src/vt/pages/projects/ProjectsPage.vue b/frontend/src/vt/pages/projects/ProjectsPage.vue index c744ea6..3370fc5 100644 --- a/frontend/src/vt/pages/projects/ProjectsPage.vue +++ b/frontend/src/vt/pages/projects/ProjectsPage.vue @@ -186,13 +186,17 @@ const localRunVisible = ref(false) const localRunProject = ref(null) const localRunCopied = ref(false) -// Base image (Claude Code CLI + settings + HTML template) is built by -// vmkteam/docker-claude-ci. We layer ast-index (AST navigation for the direct -// runner) and the latest reviewctl release on top. +// Base image (Claude Code, Codex and opencode CLIs, Go + gopls, settings, HTML +// template) is built by vmkteam/docker-claude-ci. We layer ast-index (AST +// navigation for the direct runner) and the latest reviewctl release on top. // Build this, push to your registry, then point the template's `dockerimage` at it. -const dockerfileContent = `# Claude Code CLI + settings, built by vmkteam/docker-claude-ci. +const dockerfileContent = `# Claude Code / Codex / opencode CLIs + Go, built by vmkteam/docker-claude-ci. FROM vmkteam/claude-ci:latest +# Codex's bubblewrap sandbox can't start in an unprivileged container; the +# container itself is the sandbox. +ENV REVIEW_CODEX_SANDBOX=danger-full-access + # ast-index — AST navigation for the direct runner's ast_* tools. # Static-pie binary, works on musl/alpine as is; optional (skipped when absent). ARG AST_INDEX_VERSION=v3.49.2 diff --git a/frontend/src/vt/pages/runner-profiles/RunnerProfileFormPage.vue b/frontend/src/vt/pages/runner-profiles/RunnerProfileFormPage.vue index 9e37888..e9a5571 100644 --- a/frontend/src/vt/pages/runner-profiles/RunnerProfileFormPage.vue +++ b/frontend/src/vt/pages/runner-profiles/RunnerProfileFormPage.vue @@ -31,7 +31,7 @@ - +

Ignored by this runner/provider.

@@ -47,13 +47,18 @@ + + + +

Round budget of the review loop, 10–500. Empty or 0 = default (60).

+
@@ -115,19 +120,20 @@ const efforts = ['low', 'medium', 'high', 'xhigh', 'max'] const providers = ['anthropic', 'deepseek', 'openai', 'openai-compat'] // Runner-dependent model suggestions for the datalist (free text still allowed). -// Keep in sync with ResolveDefaults and the price tables in codex.go / -// provider_factory.go. As of 2026-07. +// Keep in sync with ResolveDefaults and the price table (PricingFor in +// pkg/reviewer/direct/provider_factory.go). As of 2026-09. const MODEL_SUGGESTIONS: Record = { - claude: ['opus', 'sonnet', 'haiku', 'claude-opus-5', 'claude-sonnet-5', 'claude-fable-5', 'claude-opus-4-8', 'claude-sonnet-4-6', 'claude-haiku-4-5'], - codex: ['gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5', 'gpt-5.4', 'gpt-5.4-mini', 'gpt-5.3-codex', 'gpt-5.2-codex', 'gpt-5.1-codex-max', 'gpt-5.1-codex'], - opencode: ['anthropic/claude-opus-5', 'anthropic/claude-opus-4-8', 'openai/gpt-5.6-sol', 'openai/gpt-5.5', 'deepseek/deepseek-v4-pro', 'deepseek/deepseek-v4-flash'], + claude: ['opus', 'sonnet', 'haiku', 'fable', 'claude-opus-5-5', 'claude-fable-5-1', 'claude-sonnet-5', 'claude-opus-5', 'claude-opus-4-8', 'claude-haiku-4-5'], + codex: ['gpt-6-sol', 'gpt-6-astra', 'gpt-6-luna', 'gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5'], + opencode: ['anthropic/claude-opus-5-5', 'anthropic/claude-sonnet-5', 'openai/gpt-6-sol', 'openai/gpt-6-astra', 'deepseek/deepseek-v4-pro', 'deepseek/deepseek-flash'], } // For the direct runner, suggestions depend on the selected API provider. -// deepseek-chat/reasoner are intentionally omitted — they retire 2026-07-24. +// Retired DeepSeek names (deepseek-v4-flash, deepseek-chat/reasoner) are omitted — +// they are served as deepseek-flash. const DIRECT_MODEL_SUGGESTIONS: Record = { - anthropic: ['claude-opus-5', 'claude-sonnet-5', 'claude-fable-5', 'claude-opus-4-8', 'claude-sonnet-4-6', 'claude-haiku-4-5'], - deepseek: ['deepseek-v4-pro', 'deepseek-v4-flash'], - openai: ['gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5', 'gpt-5.4', 'gpt-5.4-mini'], + anthropic: ['claude-opus-5-5', 'claude-fable-5-1', 'claude-sonnet-5', 'claude-opus-5', 'claude-opus-4-8', 'claude-haiku-4-5'], + deepseek: ['deepseek-v4-pro', 'deepseek-flash'], + openai: ['gpt-6-sol', 'gpt-6-astra', 'gpt-6-luna', 'gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5'], 'openai-compat': [], } // Default API base URL per direct provider, shown as the input placeholder. @@ -140,7 +146,7 @@ const PROVIDER_BASE_URL: Record = { const { entity, loading, saving, error, fieldError, load, save, remove } = useForm(vtApi.runnerprofile, 'runnerProfile', () => ({ id: 0, title: '', runner: 'claude', model: '', effort: '', apiProvider: '', apiBaseURL: '', - params: { allowDangerousPermissions: false }, isDefault: false, statusId: 1, tokenMasked: '', hasToken: false, + params: { allowDangerousPermissions: false, maxRounds: 0 }, isDefault: false, statusId: 1, tokenMasked: '', hasToken: false, })) // Token is write-only: bind a separate input so an untouched field stays "keep". @@ -161,16 +167,12 @@ const modelPlaceholder = computed(() => { const apiBaseURLPlaceholder = computed(() => PROVIDER_BASE_URL[entity.apiProvider ?? ''] || 'https://...') -// Effort is honoured by the claude/codex CLIs and, for the direct runner, only by -// the Anthropic provider; deepseek/openai-compat and opencode ignore it. codex has -// no "max" level. -const effortUsed = computed(() => { - if (entity.runner === 'claude' || entity.runner === 'codex') return true - if (entity.runner === 'direct') return entity.apiProvider === 'anthropic' - return false -}) -const effortOptions = computed(() => - entity.runner === 'codex' ? efforts.filter((e) => e !== 'max') : efforts, +// Effort is honoured by the claude/codex CLIs and by the direct runner's +// anthropic/openai/deepseek providers (DeepSeek maps medium/xhigh to high +// itself); opencode and openai-compat ignore it. Supported levels vary by model +// (e.g. gpt-5.5 stops at xhigh). +const effortUsed = computed( + () => entity.runner !== 'opencode' && !(entity.runner === 'direct' && entity.apiProvider === 'openai-compat'), ) onMounted(() => { @@ -180,7 +182,9 @@ onMounted(() => { async function handleSave() { // Empty token input means "keep existing" (update) or "none" (add): omit it. entity.token = tokenInput.value ? tokenInput.value : undefined - if (!entity.params) entity.params = { allowDangerousPermissions: false } + if (!entity.params) entity.params = { allowDangerousPermissions: false, maxRounds: 0 } + // A cleared number input yields ""; the API expects an integer (0 = default). + entity.params.maxRounds = Number(entity.params.maxRounds) || 0 if (await save()) router.push('/runner-profiles') } diff --git a/go.mod b/go.mod index 626ba73..fedbd18 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/labstack/echo/v4 v4.15.4 github.com/namsral/flag v1.7.4-pre github.com/prometheus/client_golang v1.24.1 - github.com/sashabaranov/go-openai v1.41.2 + github.com/sashabaranov/go-openai v1.42.1 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 github.com/vmkteam/appkit v0.1.2 diff --git a/go.sum b/go.sum index 244fd9d..6198e7a 100644 --- a/go.sum +++ b/go.sum @@ -226,8 +226,8 @@ github.com/prometheus/procfs v0.21.1/go.mod h1:aB55Cww9pdSJVHk0hUf0inxWyyjPogFIj github.com/rogpeppe/go-internal v1.8.0 h1:FCbCCtXNOY3UtUuHUYaghJg4y7Fd14rXifAYUAtL9R8= github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= -github.com/sashabaranov/go-openai v1.41.2 h1:vfPRBZNMpnqu8ELsclWcAvF19lDNgh1t6TVfFFOPiSM= -github.com/sashabaranov/go-openai v1.41.2/go.mod h1:lj5b/K+zjTSFxVLijLSTDZuP7adOgerWeFyZLUhAKRg= +github.com/sashabaranov/go-openai v1.42.1 h1:9nK2UgDVVSIyoEUNDeWqu3Ttj8EqCO6FT8HK0Cv8VEo= +github.com/sashabaranov/go-openai v1.42.1/go.mod h1:lj5b/K+zjTSFxVLijLSTDZuP7adOgerWeFyZLUhAKRg= github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo= github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE= github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88= diff --git a/pkg/app/app.go b/pkg/app/app.go index 944778f..4ae2f0d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1,12 +1,15 @@ package app import ( + "cmp" "context" "time" "reviewsrv/pkg/db" "reviewsrv/pkg/debug" "reviewsrv/pkg/reviewctl" + "reviewsrv/pkg/reviewer" + "reviewsrv/pkg/reviewer/runner" "reviewsrv/pkg/rpc" "reviewsrv/pkg/vt" @@ -34,11 +37,20 @@ type Config struct { Environment string DSN string } + // Debug sizes the in-memory ring of recent reviewctl runs; zero values + // take the defaults below. + Debug struct { + Capacity int // runs kept (metadata) + FilesCapacity int // newest runs that keep their artifact files + } } -// debugBufferCapacity caps the in-memory ring of recent reviewctl runs. -// Sized for a handful of CI failures — bigger values just waste RAM. -const debugBufferCapacity = 10 +// Defaults for the debug ring: run metadata is small, so keep days of CI +// failures; artifacts run to megabytes each, so only the newest keep them. +const ( + debugBufferCapacity = 200 + debugFilesCapacity = 20 +) type App struct { embedlog.Logger @@ -53,6 +65,7 @@ type App struct { srv *zenrpc.Server reviewctlsrv *zenrpc.Server debugStorage *debug.Storage + runMetrics *reviewer.RunMetrics } func New(appName, version string, sl embedlog.Logger, cfg Config, db db.DB, dbc *pg.DB) *App { @@ -64,7 +77,8 @@ func New(appName, version string, sl embedlog.Logger, cfg Config, db db.DB, dbc dbc: dbc, echo: appkit.NewEcho(), Logger: sl, - debugStorage: debug.New(debugBufferCapacity), + debugStorage: debug.New(cmp.Or(cfg.Debug.Capacity, debugBufferCapacity), cmp.Or(cfg.Debug.FilesCapacity, debugFilesCapacity)), + runMetrics: reviewer.NewRunMetrics(runner.Names...), } // add services diff --git a/pkg/app/handlers.go b/pkg/app/handlers.go index 3e9890f..433ca38 100644 --- a/pkg/app/handlers.go +++ b/pkg/app/handlers.go @@ -11,6 +11,7 @@ import ( "reviewsrv/frontend" "reviewsrv/pkg/debug" "reviewsrv/pkg/rest" + "reviewsrv/pkg/reviewer" "reviewsrv/pkg/slack" "github.com/labstack/echo/v4" @@ -30,13 +31,27 @@ func (a *App) runHTTPServer(ctx context.Context, host string, port int) error { return a.echo.Start(listenAddress) } +// debugUploadPath receives debug bundles, whose runner transcripts (opencode +// NDJSON streams) outgrow the 2MB API body cap; the route has its own 20MB cap. +const debugUploadPath = "/v1/upload/debug/:projectKey/" + +// apiBodyLimit caps request bodies at 2MB, skipping debugUploadPath: a +// route-level BodyLimit runs after this one, so it can lower the cap but never +// raise it. +func apiBodyLimit() echo.MiddlewareFunc { + return middleware.BodyLimitWithConfig(middleware.BodyLimitConfig{ + Limit: "2M", + Skipper: func(c echo.Context) bool { return c.Path() == debugUploadPath }, + }) +} + // registerHandlers register echo handlers. func (a *App) registerHandlers() { a.echo.Use(middleware.CORSWithConfig(middleware.CORSConfig{ AllowOrigins: []string{"*"}, AllowMethods: []string{echo.GET, echo.PUT, echo.POST, echo.DELETE}, AllowHeaders: []string{"Authorization", "Authorization2", "Origin", "X-Requested-With", "Content-Type", "Accept", "Platform", "Version"}, - }), middleware.BodyLimit("2M")) + }), apiBodyLimit()) lg := middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{ LogStatus: true, @@ -66,7 +81,7 @@ func (a *App) registerHandlers() { }, }) - h := rest.NewHandler(a.db, slack.NewNotifier(a.Logger), a.cfg.Server.BaseURL) + h := rest.NewHandler(a.db, slack.NewNotifier(a.Logger), a.cfg.Server.BaseURL, a.runMetrics) // Internal reviewctl API: config + prompt over JSON-RPC (registered as a // separate zenrpc server in registerReviewctlAPIHandlers), plus review upload. @@ -80,9 +95,12 @@ func (a *App) registerHandlers() { a.echo.GET("/v1/rpc/review-fix-:id", h.ReviewFixMarkdown, lg) a.echo.GET("/v1/rpc/project-instructions-:id", h.ProjectInstructionsMarkdown, lg) - dh := debug.NewHandler(a.debugStorage, a.Log()) - // Per-route 20MB limit overrides the global 2MB cap so opencode NDJSON streams fit. - a.echo.POST("/v1/upload/debug/:projectKey/", dh.Upload, lg, middleware.BodyLimit("20M")) + pm := reviewer.NewProjectManager(a.db) + dh := debug.NewHandler(a.debugStorage, a.Log(), func(ctx context.Context, projectKey string) string { + title, _ := pm.TitleByKey(ctx, projectKey) + return title + }, a.runMetrics) + a.echo.POST(debugUploadPath, dh.Upload, lg, middleware.BodyLimit("20M")) a.echo.GET(debug.StoragePathPrefix, dh.List, lg) a.echo.GET(debug.StoragePathPrefix+":id/", dh.Bundle, lg) a.echo.GET(debug.StoragePathPrefix+":id/:filename", dh.File, lg) diff --git a/pkg/app/handlers_test.go b/pkg/app/handlers_test.go new file mode 100644 index 0000000..0cab791 --- /dev/null +++ b/pkg/app/handlers_test.go @@ -0,0 +1,32 @@ +package app + +import ( + "bytes" + "net/http" + "net/http/httptest" + "testing" + + "github.com/labstack/echo/v4" + "github.com/labstack/echo/v4/middleware" + "github.com/stretchr/testify/assert" +) + +func TestAPIBodyLimit(t *testing.T) { + e := echo.New() + e.Use(apiBodyLimit()) + ok := func(c echo.Context) error { return c.NoContent(http.StatusOK) } + e.POST(debugUploadPath, ok, middleware.BodyLimit("20M")) + e.POST("/v1/reviewctl/upload/:projectKey/", ok) + + post := func(path string, size int) int { + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, path, bytes.NewReader(make([]byte, size)))) + return rec.Code + } + const mb = 1 << 20 + + assert.Equal(t, http.StatusOK, post("/v1/upload/debug/key/", 5*mb), "debug bundles get the 20MB route cap") + assert.Equal(t, http.StatusRequestEntityTooLarge, post("/v1/upload/debug/key/", 21*mb)) + assert.Equal(t, http.StatusOK, post("/v1/reviewctl/upload/key/", mb)) + assert.Equal(t, http.StatusRequestEntityTooLarge, post("/v1/reviewctl/upload/key/", 3*mb), "other routes keep the 2MB cap") +} diff --git a/pkg/app/metrics.go b/pkg/app/metrics.go index fb85854..3e39d6b 100644 --- a/pkg/app/metrics.go +++ b/pkg/app/metrics.go @@ -23,6 +23,8 @@ func (a *App) registerMetrics() { ) a.mon.Open() + a.runMetrics.Register(prometheus.DefaultRegisterer) + a.echo.Use(appkit.HTTPMetrics(appkit.DefaultServerName)) a.echo.Any("/metrics", echo.WrapHandler(promhttp.Handler())) } diff --git a/pkg/db/model_params.go b/pkg/db/model_params.go index f600d3d..10de686 100644 --- a/pkg/db/model_params.go +++ b/pkg/db/model_params.go @@ -58,8 +58,10 @@ type ModelUseStats struct { // profile (runnerProfiles.params jsonb). New runner options live here so they // need no schema migration. type RunnerProfileParams struct { - // AllowDangerousPermissions maps to opencode's --dangerously-skip-permissions. + // AllowDangerousPermissions maps to opencode's --auto. AllowDangerousPermissions bool `json:"allowDangerousPermissions,omitempty"` + // MaxRounds overrides the direct runner's round budget; 0 = its default. + MaxRounds int `json:"maxRounds,omitempty"` } // ReviewRunnerProfile is the snapshot of the resolved runner profile used to diff --git a/pkg/debug/handler.go b/pkg/debug/handler.go index 0bda430..0b1ed7e 100644 --- a/pkg/debug/handler.go +++ b/pkg/debug/handler.go @@ -1,26 +1,39 @@ package debug import ( + "bytes" "compress/gzip" + "context" "embed" "errors" "fmt" "html/template" "io" "log/slog" + "math" "mime" "mime/multipart" "net/http" "path/filepath" "sort" + "strconv" "strings" + "unicode/utf8" + + "reviewsrv/pkg/reviewer" "github.com/google/uuid" "github.com/labstack/echo/v4" ) -// maxFileBytes caps a single decompressed artifact (defends against gzip bombs). -const maxFileBytes = 32 * 1024 * 1024 +const ( + // maxFileBytes caps a single decompressed artifact (defends against gzip bombs). + maxFileBytes = 32 * 1024 * 1024 + // maxErrorMsgBytes caps a bundle's error message; real ones are a few KB. + maxErrorMsgBytes = 64 * 1024 + // maxFieldBytes caps the other metadata fields (ids, branches, model). + maxFieldBytes = 1024 +) // StoragePathPrefix is the URL prefix under which bundle pages and files are served. // Both clients (templates, reviewctl) and the upload handler reference it. @@ -37,6 +50,9 @@ const ( FieldSourceBranch = "sourceBranch" FieldTargetBranch = "targetBranch" FieldCommitHash = "commitHash" + FieldStatus = "status" // reviewer.RunStatus* + FieldReason = "reason" // reviewer.RunReason* + FieldCostUsd = "costUsd" // spent by the run, incl. a failed one ) //go:embed templates/*.html @@ -44,20 +60,25 @@ var templatesFS embed.FS // Handler serves debug bundle endpoints over echo. type Handler struct { - storage *Storage - log *slog.Logger - tmpl *template.Template + storage *Storage + log *slog.Logger + tmpl *template.Template + projectTitle func(ctx context.Context, projectKey string) string + metrics *reviewer.RunMetrics } -// NewHandler wires templates to the storage. Templates are embedded at compile time. -func NewHandler(storage *Storage, log *slog.Logger) *Handler { +// NewHandler wires templates to the storage. Templates are embedded at compile +// time. projectTitle resolves a project key to its title ("" when unknown) for +// the pages and the run metrics; metrics counts every uploaded run that did not +// complete (ok runs are counted on review upload). Either may be nil. +func NewHandler(storage *Storage, log *slog.Logger, projectTitle func(ctx context.Context, projectKey string) string, metrics *reviewer.RunMetrics) *Handler { tmpl := template.Must(template.New("").Funcs(template.FuncMap{ "shortKey": shortKey, "preview": preview, "storageURL": func() string { return StoragePathPrefix }, }).ParseFS(templatesFS, "templates/*.html")) - return &Handler{storage: storage, log: log, tmpl: tmpl} + return &Handler{storage: storage, log: log, tmpl: tmpl, projectTitle: projectTitle, metrics: metrics} } // Upload accepts a multipart bundle. Each file part is gzip-compressed by the @@ -73,34 +94,53 @@ func (h *Handler) Upload(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "parse multipart: "+err.Error()) } + // Metadata outlives the artifacts in the ring, so a client can't park + // megabytes in it: fields are clipped. + field := func(key string) string { return clip(formValue(form.Value, key), maxFieldBytes) } b := &Bundle{ ProjectKey: projectKey, - MRIid: formValue(form.Value, FieldMRIid), - ExternalID: formValue(form.Value, FieldExternalID), - Runner: formValue(form.Value, FieldRunner), - Model: formValue(form.Value, FieldModel), - ErrorMsg: formValue(form.Value, FieldErrorMsg), - SourceBranch: formValue(form.Value, FieldSourceBranch), - TargetBranch: formValue(form.Value, FieldTargetBranch), - CommitHash: formValue(form.Value, FieldCommitHash), - Files: make(map[string][]byte, len(form.File)), + MRIid: field(FieldMRIid), + ExternalID: field(FieldExternalID), + Runner: field(FieldRunner), + Model: field(FieldModel), + ErrorMsg: clip(formValue(form.Value, FieldErrorMsg), maxErrorMsgBytes), + SourceBranch: field(FieldSourceBranch), + TargetBranch: field(FieldTargetBranch), + CommitHash: field(FieldCommitHash), + CostUsd: parseCost(formValue(form.Value, FieldCostUsd)), + Files: make(map[string]File, len(form.File)), + } + b.Status, b.Reason = reviewer.NormalizeRunOutcome(formValue(form.Value, FieldStatus), formValue(form.Value, FieldReason), b.ErrorMsg != "") + ctx := c.Request().Context() + if h.projectTitle != nil { + b.ProjectTitle = h.projectTitle(ctx, projectKey) } for _, headers := range form.File { for _, fh := range headers { - data, err := readGzipped(fh) + f, err := readArtifact(fh) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("read %s: %v", fh.Filename, err)) } - name := strings.TrimSuffix(fh.Filename, ".gz") - b.Files[name] = data + b.Files[strings.TrimSuffix(fh.Filename, ".gz")] = f } } h.storage.Add(b) - h.log.InfoContext(c.Request().Context(), "debug bundle stored", - "id", b.ID, "projectKey", projectKey, "files", len(b.Files), "hasError", b.ErrorMsg != "", + // A failed or timed-out run is what an operator must notice; a cancelled + // job or a --debug-upload of a good run is routine. + lvl := slog.LevelInfo + if b.Status == reviewer.RunStatusFailed || b.Status == reviewer.RunStatusTimeout { + lvl = slog.LevelWarn + } + h.log.Log(ctx, lvl, "debug bundle stored", + "id", b.ID, "projectKey", projectKey, "project", b.ProjectTitle, "files", len(b.Files), "hasError", b.ErrorMsg != "", + "status", b.Status, "reason", b.Reason, "runner", b.Runner, "model", b.Model, "costUsd", b.CostUsd, + "errorMsg", preview(b.ErrorMsg, 300), ) + if b.Status != reviewer.RunStatusOK { + h.metrics.Observe(b.ProjectTitle, b.Runner, b.Status, b.Reason, b.CostUsd) + } return c.JSON(http.StatusOK, map[string]string{ "id": b.ID, @@ -126,8 +166,8 @@ func (h *Handler) Bundle(c echo.Context) error { Size int } files := make([]fileEntry, 0, len(b.Files)) - for name, data := range b.Files { - files = append(files, fileEntry{Name: name, Size: len(data)}) + for name, f := range b.Files { + files = append(files, fileEntry{Name: name, Size: f.Size}) } sort.Slice(files, func(i, j int) bool { return files[i].Name < files[j].Name }) @@ -139,13 +179,19 @@ func (h *Handler) Bundle(c echo.Context) error { return h.renderHTML(c, "bundle.html", data) } -// File serves a single artifact inline so the browser can render it. +// File serves a single artifact inline so the browser can render it, +// decompressing it on the fly. func (h *Handler) File(c echo.Context) error { - data, ok := h.storage.GetFile(c.Param("id"), c.Param("filename")) + f, ok := h.storage.GetFile(c.Param("id"), c.Param("filename")) if !ok { return echo.NewHTTPError(http.StatusNotFound, "file not found") } - return c.Blob(http.StatusOK, contentTypeFor(c.Param("filename")), data) + gr, err := gzip.NewReader(bytes.NewReader(f.Gzip)) + if err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, "gzip: "+err.Error()) + } + defer gr.Close() + return c.Stream(http.StatusOK, contentTypeFor(c.Param("filename")), gr) } func (h *Handler) renderHTML(c echo.Context, name string, data any) error { @@ -156,6 +202,15 @@ func (h *Handler) renderHTML(c echo.Context, name string, data any) error { return c.HTML(http.StatusOK, sb.String()) } +// parseCost reads a client-reported dollar cost; anything unusable is 0. +func parseCost(s string) float64 { + v, err := strconv.ParseFloat(s, 64) + if err != nil || v < 0 || math.IsNaN(v) || math.IsInf(v, 0) { + return 0 + } + return v +} + func formValue(m map[string][]string, key string) string { if v, ok := m[key]; ok && len(v) > 0 { return v[0] @@ -163,29 +218,44 @@ func formValue(m map[string][]string, key string) string { return "" } -// readGzipped opens the multipart file, gunzips it, and returns the raw bytes. -// Caps at maxFileBytes to prevent a malicious gzip from exhausting memory. -func readGzipped(fh *multipart.FileHeader) ([]byte, error) { +// readArtifact reads a gzip-compressed multipart file and checks that it +// decompresses within maxFileBytes, so a gzip bomb is rejected at upload. The +// compressed bytes are kept as they are. +func readArtifact(fh *multipart.FileHeader) (File, error) { f, err := fh.Open() if err != nil { - return nil, fmt.Errorf("open: %w", err) + return File{}, fmt.Errorf("open: %w", err) } defer f.Close() - gr, err := gzip.NewReader(f) + raw, err := io.ReadAll(f) + if err != nil { + return File{}, fmt.Errorf("read: %w", err) + } + gr, err := gzip.NewReader(bytes.NewReader(raw)) if err != nil { - return nil, fmt.Errorf("gzip: %w", err) + return File{}, fmt.Errorf("gzip: %w", err) } defer gr.Close() - - data, err := io.ReadAll(io.LimitReader(gr, maxFileBytes+1)) + n, err := io.Copy(io.Discard, io.LimitReader(gr, maxFileBytes+1)) if err != nil { - return nil, fmt.Errorf("read: %w", err) + return File{}, fmt.Errorf("gunzip: %w", err) + } + if n > maxFileBytes { + return File{}, errors.New("artifact exceeds size limit") + } + return File{Gzip: raw, Size: int(n)}, nil +} + +// clip cuts s to at most n bytes, on a rune boundary. +func clip(s string, n int) string { + if len(s) <= n { + return s } - if len(data) > maxFileBytes { - return nil, errors.New("artifact exceeds size limit") + for n > 0 && !utf8.RuneStart(s[n]) { + n-- } - return data, nil + return s[:n] } // contentTypeFor picks a browser-friendly Content-Type by extension. diff --git a/pkg/debug/handler_test.go b/pkg/debug/handler_test.go index 7149161..9988b0a 100644 --- a/pkg/debug/handler_test.go +++ b/pkg/debug/handler_test.go @@ -3,6 +3,7 @@ package debug import ( "bytes" "compress/gzip" + "context" "encoding/json" "io" "log/slog" @@ -18,8 +19,8 @@ import ( func newTestHandler(t *testing.T) (*Storage, *echo.Echo) { t.Helper() - storage := New(5) - h := NewHandler(storage, slog.Default()) + storage := New(5, 5) + h := NewHandler(storage, slog.Default(), nil, nil) e := echo.New() e.POST("/v1/upload/debug/:projectKey/", h.Upload) @@ -42,6 +43,26 @@ func gzipBytes(t *testing.T, data []byte) []byte { return buf.Bytes() } +// gzFile builds a stored artifact from its content. +func gzFile(t *testing.T, content string) File { + t.Helper() + return File{Gzip: gzipBytes(t, []byte(content)), Size: len(content)} +} + +// unzip returns a stored artifact's content. +func unzip(t *testing.T, f File) string { + t.Helper() + gr, err := gzip.NewReader(bytes.NewReader(f.Gzip)) + if err != nil { + t.Fatalf("gzip: %v", err) + } + data, err := io.ReadAll(gr) + if err != nil { + t.Fatalf("gunzip: %v", err) + } + return string(data) +} + func buildMultipart(t *testing.T, fields map[string]string, files map[string][]byte) (*bytes.Buffer, string) { t.Helper() var buf bytes.Buffer @@ -112,11 +133,11 @@ func TestHandler_UploadStoresBundle(t *testing.T) { if b.MRIid != "42" || b.Runner != "claude" || b.Model != "opus" { t.Errorf("metadata mismatch: %+v", b) } - if string(b.Files["review.json"]) != `{"files":[]}` { - t.Errorf("review.json content mismatch: %q", b.Files["review.json"]) + if got := unzip(t, b.Files["review.json"]); got != `{"files":[]}` { + t.Errorf("review.json content mismatch: %q", got) } - if string(b.Files["claude-output.json"]) != `{"type":"result"}` { - t.Errorf("claude-output.json content mismatch: %q", b.Files["claude-output.json"]) + if f := b.Files["claude-output.json"]; unzip(t, f) != `{"type":"result"}` || f.Size != len(`{"type":"result"}`) { + t.Errorf("claude-output.json mismatch: %q (size %d)", unzip(t, f), f.Size) } } @@ -165,7 +186,7 @@ func TestHandler_ListAndBundleHTML(t *testing.T) { ErrorMsg: "boom", SourceBranch: "feat/x", TargetBranch: "master", - Files: map[string][]byte{"review.json": []byte(`{"x":1}`)}, + Files: map[string]File{"review.json": gzFile(t, `{"x":1}`)}, }) req := httptest.NewRequest(http.MethodGet, "/v1/debug/storage/", nil) @@ -200,7 +221,7 @@ func TestHandler_FileServesArtifactWithContentType(t *testing.T) { storage, e := newTestHandler(t) storage.Add(&Bundle{ ID: "xyz", - Files: map[string][]byte{"review.json": []byte(`{"ok":true}`)}, + Files: map[string]File{"review.json": gzFile(t, `{"ok":true}`)}, }) req := httptest.NewRequest(http.MethodGet, "/v1/debug/storage/xyz/review.json", nil) @@ -228,3 +249,79 @@ func TestHandler_FileNotFound(t *testing.T) { t.Errorf("status = %d, want 404", rec.Code) } } + +// upload posts a bundle and returns the stored copy. +func upload(t *testing.T, storage *Storage, e *echo.Echo, projectKey string, fields map[string]string) *Bundle { + t.Helper() + body, ct := buildMultipart(t, fields, map[string][]byte{"review.json": []byte(`{}`)}) + req := httptest.NewRequest(http.MethodPost, "/v1/upload/debug/"+projectKey+"/", body) + req.Header.Set("Content-Type", ct) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + var resp map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + return storage.Get(resp["id"]) +} + +func TestHandler_UploadRecordsRunOutcome(t *testing.T) { + storage := New(5, 5) + knownKey := uuid.NewString() + h := NewHandler(storage, slog.Default(), func(_ context.Context, key string) string { + if key == knownKey { + return "demo" + } + return "" + }, nil) + e := echo.New() + e.POST("/v1/upload/debug/:projectKey/", h.Upload) + e.GET("/v1/debug/storage/", h.List) + + b := upload(t, storage, e, knownKey, map[string]string{ + "runner": "claude", "errorMsg": "run claude: billing_error: Credit balance is too low", + "status": "failed", "reason": "billing", "costUsd": "1.5", + }) + if b.Status != "failed" || b.Reason != "billing" || b.CostUsd != 1.5 || b.ProjectTitle != "demo" { + t.Errorf("outcome not recorded: %+v", b) + } + + // A legacy client sends no status: derived from errorMsg. Garbage is sanitized. + b = upload(t, storage, e, uuid.NewString(), map[string]string{"errorMsg": "boom", "reason": ""), + "x.svg": gzFile(t, ``), + }}) + + for _, name := range []string{"x.html", "x.svg"} { + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/debug/storage/xss/"+name, nil)) + if got := rec.Header().Get("Content-Type"); got != "text/plain; charset=utf-8" { + t.Errorf("%s: Content-Type = %q, want text/plain", name, got) + } + if rec.Header().Get("X-Content-Type-Options") != "nosniff" || rec.Header().Get("Content-Security-Policy") != "sandbox" { + t.Errorf("%s: missing nosniff/sandbox headers: %v", name, rec.Header()) + } + } +} + +func TestHandler_UploadRefusedWhenProjectLookupFails(t *testing.T) { + _, e := newTestHandlerWith(t, func(context.Context, string) (string, error) { + return "", errors.New("db down") + }, nil) + + body, ct := buildMultipart(t, map[string]string{"errorMsg": "boom"}, nil) + req := httptest.NewRequest(http.MethodPost, "/v1/upload/debug/"+uuid.NewString()+"/", body) + req.Header.Set("Content-Type", ct) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + if rec.Code != http.StatusServiceUnavailable { + t.Errorf("status = %d, want 503: an unverified key is not accepted", rec.Code) + } +} + +func TestHandler_UploadHidesProjectKey(t *testing.T) { + storage, e := newTestHandler(t) + key := uuid.NewString() + b := upload(t, storage, e, key, map[string]string{ + "errorMsg": `upload: Post "https://reviewer/v1/reviewctl/upload/` + key + `/": EOF`, + "reviewId": "+42", + }) + if strings.Contains(b.ErrorMsg, key) { + t.Errorf("errorMsg reveals the project key: %q", b.ErrorMsg) + } + if b.ReviewID != "42" { + t.Errorf("reviewId = %q, want the canonical 42", b.ReviewID) + } + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/debug/storage/"+b.ID+"/", nil)) + if strings.Contains(rec.Body.String(), key) { + t.Error("the bundle page reveals the project key") + } +} + func TestHandler_FileNotFound(t *testing.T) { _, e := newTestHandler(t) req := httptest.NewRequest(http.MethodGet, "/v1/debug/storage/missing/review.json", nil) @@ -273,20 +335,16 @@ func upload(t *testing.T, storage *Storage, e *echo.Echo, projectKey string, fie } func TestHandler_UploadRecordsRunOutcome(t *testing.T) { - storage := New(5, 5) knownKey := uuid.NewString() metrics := reviewer.NewRunMetrics("claude") reg := prometheus.NewRegistry() metrics.Register(reg) - h := NewHandler(storage, slog.Default(), func(_ context.Context, key string) (string, error) { + storage, e := newTestHandlerWith(t, func(_ context.Context, key string) (string, error) { if key != knownKey { return "", nil // no such project } return "demo", nil }, metrics) - e := echo.New() - e.POST("/v1/upload/debug/:projectKey/", h.Upload) - e.GET("/v1/debug/storage/", h.List) b := upload(t, storage, e, knownKey, map[string]string{ "runner": "claude", "errorMsg": "run claude: billing_error: Credit balance is too low", @@ -302,8 +360,15 @@ func TestHandler_UploadRecordsRunOutcome(t *testing.T) { if b.Status != "failed" || b.Reason != "other" { t.Errorf("legacy upload not normalized: %+v", b) } - // An ok run is counted on review upload, not here. + // An ok run is counted on review upload, not here — and so is a run whose + // review was created before it failed. upload(t, storage, e, knownKey, map[string]string{"runner": "claude", "status": "ok", "costUsd": "2"}) + b = upload(t, storage, e, knownKey, map[string]string{"runner": "claude", "status": "failed", "reason": "other", "costUsd": "3", "reviewId": "80"}) + if b.ReviewID != "80" { + t.Errorf("reviewId not stored: %+v", b) + } + // A garbage reviewId can't switch the metrics off: this one is counted. + upload(t, storage, e, knownKey, map[string]string{"runner": "claude", "status": "failed", "reason": "billing", "reviewId": "x"}) // A key that matches no project is refused. body, ct := buildMultipart(t, map[string]string{"errorMsg": "x", "costUsd": "999"}, nil) @@ -321,7 +386,7 @@ func TestHandler_UploadRecordsRunOutcome(t *testing.T) { labels labels want float64 }{ - {"reviewer_runs_total", labels{"project": "demo", "runner": "claude", "status": "failed", "reason": "billing"}, 1}, + {"reviewer_runs_total", labels{"project": "demo", "runner": "claude", "status": "failed", "reason": "billing"}, 2}, {"reviewer_runs_total", labels{"project": "demo", "runner": "other", "status": "failed", "reason": "other"}, 1}, {"reviewer_run_cost_usd_total", labels{"project": "demo", "runner": "claude", "status": "failed"}, 1.5}, {"reviewer_run_cost_usd_total", labels{"project": "demo", "runner": "other", "status": "failed"}, 0}, diff --git a/pkg/debug/storage.go b/pkg/debug/storage.go index b807f6a..28548f8 100644 --- a/pkg/debug/storage.go +++ b/pkg/debug/storage.go @@ -38,6 +38,7 @@ type Bundle struct { Status string // reviewer.RunStatus*, normalized on upload Reason string // reviewer.RunReason*; empty for an ok run CostUsd float64 // what the run spent, including a failed one + ReviewID string // the review created before the run failed, if any ProjectTitle string // resolved from ProjectKey; empty for an unknown key Files map[string]File FilesEvicted bool // artifacts dropped to cap memory; metadata kept diff --git a/pkg/debug/templates/bundle.html b/pkg/debug/templates/bundle.html index b472764..7478890 100644 --- a/pkg/debug/templates/bundle.html +++ b/pkg/debug/templates/bundle.html @@ -26,9 +26,10 @@

Bundle {{.Bundle.ID}}

Timestamp
{{.Bundle.Timestamp.Format "2006-01-02 15:04:05 MST"}}
-
Project
{{or .Bundle.ProjectTitle "—"}} {{.Bundle.ProjectKey}}
+
Project
{{or .Bundle.ProjectTitle "—"}} {{shortKey .Bundle.ProjectKey}}
Status
{{.Bundle.Status}}{{if .Bundle.Reason}} · {{.Bundle.Reason}}{{end}}
Cost
{{if .Bundle.CostUsd}}${{printf "%.4f" .Bundle.CostUsd}}{{else}}—{{end}}
+ {{if .Bundle.ReviewID}}
Review
#{{.Bundle.ReviewID}} created before the failure
{{end}}
MR IID
{{or .Bundle.MRIid "—"}}
External ID
{{or .Bundle.ExternalID "—"}}
Runner
{{.Bundle.Runner}}
diff --git a/pkg/rest/rest_test.go b/pkg/rest/rest_test.go index dee7c23..c16e791 100644 --- a/pkg/rest/rest_test.go +++ b/pkg/rest/rest_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "strings" "testing" "time" @@ -13,6 +14,7 @@ import ( "reviewsrv/pkg/reviewer" "github.com/labstack/echo/v4" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -169,3 +171,62 @@ func cleanupReview(t *testing.T, dbc db.DB, rv *reviewer.Review) { t.Logf("cleanup review %d: %v", rv.ID, err) } } + +func TestDBCreateReviewCountsOkRun(t *testing.T) { + dbc, _ := dbtest.Setup(t) + pr, prCl := dbtest.Project(t, dbc, nil, dbtest.WithProjectRelations, dbtest.WithFakeProject) + t.Cleanup(prCl) + metrics := reviewer.NewRunMetrics("claude") + reg := prometheus.NewRegistry() + metrics.Register(reg) + h := NewHandler(dbc, nil, "http://localhost", metrics) + + body := `{"review":{"title":"t","commitHash":"abc","sourceBranch":"f","targetBranch":"master","author":"dev", + "createdAt":"2026-09-24T12:00:00Z","modelInfo":{"model":"opus","costUsd":1.25},"runnerProfile":{"runner":"claude"}}, + "files":[{"reviewType":"code","summary":"ok","isAccepted":true}],"issues":[]}` + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + rec := httptest.NewRecorder() + c := echo.New().NewContext(req, rec) + c.SetParamNames("projectKey") + c.SetParamValues(pr.ProjectKey) + + require.NoError(t, h.CreateReview(c)) + id, err := strconv.Atoi(rec.Body.String()) + require.NoError(t, err) + t.Cleanup(func() { // hard delete: the project's cleanup needs the rows gone + ctx := context.Background() + _, _ = dbc.ModelContext(ctx, (*db.ReviewFile)(nil)).Where(`"reviewId" = ?`, id).Delete() + _, _ = dbc.ModelContext(ctx, &db.Review{ID: id}).WherePK().Delete() + }) + + // The profile's runner labels the run; the uploaded review is its ok outcome. + runs, cost := okRunCounters(t, reg, pr.Title, "claude") + assert.InDelta(t, 1, runs, 0) + assert.InDelta(t, 1.25, cost, 1e-9) +} + +// okRunCounters reads the ok-run and cost counters of project and runner. +func okRunCounters(t *testing.T, reg *prometheus.Registry, project, runner string) (runs, cost float64) { + t.Helper() + mfs, err := reg.Gather() + require.NoError(t, err) + for _, mf := range mfs { + for _, m := range mf.GetMetric() { + l := map[string]string{} + for _, lp := range m.GetLabel() { + l[lp.GetName()] = lp.GetValue() + } + if l["project"] != project || l["runner"] != runner || l["status"] != reviewer.RunStatusOK { + continue + } + switch mf.GetName() { + case "reviewer_runs_total": + runs = m.GetCounter().GetValue() + case "reviewer_run_cost_usd_total": + cost = m.GetCounter().GetValue() + } + } + } + return runs, cost +} diff --git a/pkg/reviewer/ctl/ctl.go b/pkg/reviewer/ctl/ctl.go index a0fd7b3..6208c5d 100644 --- a/pkg/reviewer/ctl/ctl.go +++ b/pkg/reviewer/ctl/ctl.go @@ -121,12 +121,6 @@ func NewController(cfg *Config, rr runner.ReviewRunner, log *slog.Logger, opts . return c } -func (c *Controller) addSpent(usd float64) { - c.spentMu.Lock() - c.spent += usd - c.spentMu.Unlock() -} - func (c *Controller) spentTotal() float64 { c.spentMu.Lock() defer c.spentMu.Unlock() @@ -215,7 +209,9 @@ func (c *Controller) Review(ctx context.Context) (retErr error) { // debug ring before a panel worktree goes. The upload gets a detached context: // a cancelled or timed-out run is precisely what the bundle should explain. func (c *Controller) settleRun(ctx context.Context, cfg *Config, rr *spendTracker, runErr error) { - c.addSpent(rr.total()) + c.spentMu.Lock() + c.spent += rr.total() + c.spentMu.Unlock() if runErr == nil && !cfg.DebugUpload { return } @@ -246,6 +242,10 @@ func (c *Controller) uploadDebugBundle(ctx context.Context, cfg *Config, runErr CostUsd: costUsd, } meta.Status, meta.Reason = reviewer.RunOutcome(runErr) + var created *reviewCreatedError + if errors.As(runErr, &created) { + meta.ReviewID = created.reviewID + } if runErr != nil { meta.ErrorMsg = runErr.Error() // A cancelled job is expected, not a failure worth a warning. @@ -395,7 +395,9 @@ func (c *Controller) fillMetadata(ctx context.Context, draft *rest.ReviewDraft) // gitMeta returns the trimmed output of a git command in dir, or "" on any // error — callers treat the value as optional metadata. func gitMeta(ctx context.Context, dir string, args ...string) string { - out, err := exec.CommandContext(ctx, "git", append([]string{"-C", dir}, args...)...).Output() + cmd := exec.CommandContext(ctx, "git", reviewer.GitArgs(append([]string{"-C", dir}, args...)...)...) + cmd.Env = reviewer.ChildEnv() + out, err := cmd.Output() if err != nil { return "" } diff --git a/pkg/reviewer/ctl/ctl_test.go b/pkg/reviewer/ctl/ctl_test.go index 9220bac..583fa9c 100644 --- a/pkg/reviewer/ctl/ctl_test.go +++ b/pkg/reviewer/ctl/ctl_test.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "testing" "reviewsrv/pkg/rest" @@ -178,11 +179,31 @@ func editReviewJSON(dir string, edit func(*rest.ReviewDraft)) error { return WriteReviewJSON(dir, draft) } -// newDebugCaptureServer serves the prompt RPC and records the form fields of -// the debug bundle upload; nothing else is expected. -func newDebugCaptureServer(t *testing.T) (*httptest.Server, map[string]string) { +// debugCapture records the debug bundle uploads a test server received. +type debugCapture struct { + mu sync.Mutex + last map[string]string // form fields of the latest bundle + bundles int +} + +func (d *debugCapture) field(name string) string { + d.mu.Lock() + defer d.mu.Unlock() + return d.last[name] +} + +func (d *debugCapture) count() int { + d.mu.Lock() + defer d.mu.Unlock() + return d.bundles +} + +// newDebugCaptureServer serves the prompt RPC and records the debug bundle +// uploads (concurrent ones too: panel members); other requests go to other +// (404 when nil). +func newDebugCaptureServer(t *testing.T, other http.HandlerFunc) (*httptest.Server, *debugCapture) { t.Helper() - fields := map[string]string{} + dc := &debugCapture{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/v1/reviewctl/rpc/" { w.Header().Set("Content-Type", "application/json") @@ -193,21 +214,57 @@ func newDebugCaptureServer(t *testing.T) (*httptest.Server, map[string]string) { if !assert.NoError(t, r.ParseMultipartForm(32<<20)) { return } + fields := make(map[string]string, len(r.MultipartForm.Value)) for k, v := range r.MultipartForm.Value { fields[k] = v[0] } + dc.mu.Lock() + dc.last, dc.bundles = fields, dc.bundles+1 + dc.mu.Unlock() w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"id":"x","url":"/v1/debug/storage/x/"}`)) return } + if other != nil { + other(w, r) + return + } w.WriteHeader(http.StatusNotFound) })) t.Cleanup(srv.Close) - return srv, fields + return srv, dc +} + +func TestController_Review_PartialUploadNamesTheReview(t *testing.T) { + // The review is created, then an R*.md upload fails. + srv, dc := newDebugCaptureServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v1/reviewctl/upload/test-key/" { // the review itself; its files fail + _, _ = w.Write([]byte("42")) + return + } + w.WriteHeader(http.StatusInternalServerError) + }) + dir := setupTestDir(t) + cfg := &Config{Key: "test-key", URL: srv.URL, Model: "opus", Dir: dir, Runner: runner.RunnerClaude} + rr := &testClaudeRunner{fixturePath: "testdata/claude_result.json", beforeRun: func() error { + if err := os.WriteFile(filepath.Join(dir, "R1.architecture.md"), []byte("# Architecture"), 0o644); err != nil { + return err + } + return editReviewJSON(dir, func(d *rest.ReviewDraft) { + for i := range d.Files { + d.Files[i].Summary = "reviewed" + } + }) + }} + + err := NewController(cfg, rr, slog.Default()).Review(context.Background()) + require.ErrorContains(t, err, "upload") + assert.Equal(t, reviewer.RunStatusFailed, dc.field("status")) + assert.Equal(t, "42", dc.field("reviewId"), "the server counted this run when the review was created") } func TestController_Review_FailsOnUnfilledReview(t *testing.T) { - srv, fields := newDebugCaptureServer(t) + srv, dc := newDebugCaptureServer(t, nil) cfg := &Config{Key: "test-key", URL: srv.URL, Model: "opus", Dir: t.TempDir(), Runner: runner.RunnerClaude} // The runner never touches review.json, neither on the run nor on the Step 2 retry. rr := &testClaudeRunner{fixturePath: "testdata/claude_result.json"} @@ -215,13 +272,13 @@ func TestController_Review_FailsOnUnfilledReview(t *testing.T) { err := NewController(cfg, rr, slog.Default()).Review(context.Background()) require.ErrorContains(t, err, "empty review") assert.NotEmpty(t, rr.sessionID, "the Step 2 retry resumed the session") - assert.Equal(t, reviewer.RunStatusFailed, fields["status"]) - assert.Equal(t, reviewer.RunReasonNotSubmitted, fields["reason"]) - assert.Equal(t, "4.175053", fields["costUsd"], "both runs are billed") + assert.Equal(t, reviewer.RunStatusFailed, dc.field("status")) + assert.Equal(t, reviewer.RunReasonNotSubmitted, dc.field("reason")) + assert.Equal(t, "4.175053", dc.field("costUsd"), "both runs are billed") } func TestController_Review_UploadsDebugBundleOnValidationFailure(t *testing.T) { - srv, fields := newDebugCaptureServer(t) + srv, dc := newDebugCaptureServer(t, nil) // Simulate Claude overwriting the skeleton with an invalid review.json // — reproduces the CI failure where the model picks empty reviewType. @@ -241,8 +298,8 @@ func TestController_Review_UploadsDebugBundleOnValidationFailure(t *testing.T) { err := c.Review(context.Background()) require.Error(t, err, "Review must fail on invalid review.json") assert.Contains(t, err.Error(), "invalid reviewType") - assert.Contains(t, fields, "errorMsg", "debug bundle must be uploaded on failure") - assert.Contains(t, fields["errorMsg"], "files[0]", "errorMsg must carry verbose validation detail") + assert.Equal(t, 1, dc.count(), "debug bundle must be uploaded on failure") + assert.Contains(t, dc.field("errorMsg"), "files[0]", "errorMsg must carry verbose validation detail") } // failingRunner fails like a real runner: a tagged error plus the partial result @@ -260,7 +317,7 @@ func (r failingRunner) Name() string { return r.name } func (r failingRunner) SetSession(string) {} func TestController_Review_ReportsFailureOutcome(t *testing.T) { - srv, fields := newDebugCaptureServer(t) + srv, dc := newDebugCaptureServer(t, nil) tests := []struct { name string @@ -293,17 +350,19 @@ func TestController_Review_ReportsFailureOutcome(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - clear(fields) + dc.mu.Lock() + dc.last = nil + dc.mu.Unlock() cfg := &Config{Key: "test-key", URL: srv.URL, Model: "m", Dir: t.TempDir(), Runner: tt.runner.name} err := NewController(cfg, tt.runner, slog.Default()).Review(context.Background()) require.Error(t, err) - assert.True(t, strings.HasPrefix(fields["errorMsg"], tt.wantMsg), fields["errorMsg"]) - assert.Equal(t, tt.wantStatus, fields["status"]) - assert.Equal(t, tt.wantReason, fields["reason"]) + assert.True(t, strings.HasPrefix(dc.field("errorMsg"), tt.wantMsg), dc.field("errorMsg")) + assert.Equal(t, tt.wantStatus, dc.field("status")) + assert.Equal(t, tt.wantReason, dc.field("reason")) if tt.runner.cost > 0 { - assert.Equal(t, strconv.FormatFloat(tt.runner.cost, 'f', -1, 64), fields["costUsd"]) + assert.Equal(t, strconv.FormatFloat(tt.runner.cost, 'f', -1, 64), dc.field("costUsd")) } else { - assert.NotContains(t, fields, "costUsd") + assert.Empty(t, dc.field("costUsd")) } }) } diff --git a/pkg/reviewer/ctl/panel.go b/pkg/reviewer/ctl/panel.go index e8ccda2..5eb7db8 100644 --- a/pkg/reviewer/ctl/panel.go +++ b/pkg/reviewer/ctl/panel.go @@ -29,13 +29,17 @@ type memberOutput struct { mdFiles map[string]string // reviewType → R*.md path in dir } +// errAllMembersFailed ends a panel whose members all failed, each of which has +// shipped its own debug bundle. +var errAllMembersFailed = errors.New("panel: every member failed to produce a review") + // reviewPanel fans out the --multi panel into per-member git worktrees, then — // when a judge is configured and ≥2 members succeed — runs the judge over the // members' outputs to produce one fused review. Members and the judge share the // project prompt and MR metadata; only runner/model/working dir differ. Members // run concurrently (bounded by panelConcurrency), and per-member failures are // tolerated as long as one member produces a review. -func (c *Controller) reviewPanel(ctx context.Context, start time.Time) error { +func (c *Controller) reviewPanel(ctx context.Context, start time.Time) (err error) { if c.runnerFactory == nil { return errors.New("panel review requires a runner factory") } @@ -44,6 +48,17 @@ func (c *Controller) reviewPanel(ctx context.Context, start time.Time) error { for i, m := range c.cfg.Multi { members[i] = m.String() } + // A failure of the panel itself (prompt fetch, worktree, upload) ships a + // bundle and counts in the metrics under the "panel" runner — metadata + // only, as the panel writes nothing to the base checkout, and without cost: + // the runs report their own. When every member failed, each already has. + defer func() { + if err != nil && !errors.Is(err, errAllMembersFailed) { + pc := *c.cfg + pc.Dir, pc.Runner, pc.Model = "", reviewer.PanelRunner, strings.Join(members, " ") + c.settleRun(ctx, &pc, nil, err) + } + }() judge := "" if c.cfg.Judge != nil { judge = c.cfg.Judge.String() @@ -87,7 +102,7 @@ func (c *Controller) reviewPanel(ctx context.Context, start time.Time) error { outputs := c.runMembers(ctx, dirs, labels, prompt) if len(outputs) == 0 { - return errors.New("panel: every member failed to produce a review") + return errAllMembersFailed } primaryID, err := c.finishPanel(ctx, base, commit, outputs) @@ -317,25 +332,25 @@ func (c *Controller) runJudge(ctx context.Context, judgeDir, fusionPrompt string for attempt := 1; attempt <= 2; attempt++ { // initial run + one retry // Wipe the previous attempt's root artifacts; the staged members/ subdirs // are untouched (CleanReviewArtifacts only looks at the dir root). - if err = CleanReviewArtifacts(judgeDir); err != nil { - c.log.WarnContext(ctx, "clean judge artifacts", "err", err) + if cerr := CleanReviewArtifacts(judgeDir); cerr != nil { + c.log.WarnContext(ctx, "clean judge artifacts", "err", cerr) } - if err = WriteReviewSkeleton(judgeDir, &jc); err != nil { - return nil, fmt.Errorf("write judge skeleton: %w", err) + if werr := WriteReviewSkeleton(judgeDir, &jc); werr != nil { + return nil, fmt.Errorf("write judge skeleton: %w", werr) } rctx, cancel := withTimeout(ctx, c.cfg.Timeout) - _, err := rr.Run(rctx, prompt) + _, runErr := rr.Run(rctx, prompt) cancel() - if err != nil { - lastErr = err - c.log.WarnContext(ctx, "judge run failed", "attempt", attempt, "err", err) + if runErr != nil { + lastErr = runErr + c.log.WarnContext(ctx, "judge run failed", "attempt", attempt, "err", runErr) continue } - draft, err := ReadReviewJSON(jc.Dir) - if err != nil { - lastErr = err - c.log.WarnContext(ctx, "judge review.json invalid", "attempt", attempt, "err", err) + draft, readErr := ReadReviewJSON(jc.Dir) + if readErr != nil { + lastErr = readErr + c.log.WarnContext(ctx, "judge review.json invalid", "attempt", attempt, "err", readErr) continue } if isReviewJSONUnfilled(draft) { @@ -346,9 +361,9 @@ func (c *Controller) runJudge(ctx context.Context, judgeDir, fusionPrompt string // Every attempt so far is in the record: the failed ones were billed too. c.applyRunResult(ctx, draft, &jc, rr) - mdFiles, err := FindMDFiles(jc.Dir) - if err != nil { - return nil, fmt.Errorf("find judge md files: %w", err) + mdFiles, findErr := FindMDFiles(jc.Dir) + if findErr != nil { + return nil, fmt.Errorf("find judge md files: %w", findErr) } return &memberOutput{label: "judge", dir: judgeDir, draft: draft, mdFiles: mdFiles}, nil } @@ -428,7 +443,8 @@ func (c *Controller) panelCommit() string { // gitWorktreeAdd creates a detached worktree at dir checked out to commit, run // from the working repo so git finds it. func (c *Controller) gitWorktreeAdd(ctx context.Context, dir, commit string) error { - cmd := exec.CommandContext(ctx, "git", "worktree", "add", "--detach", dir, commit) + cmd := exec.CommandContext(ctx, "git", reviewer.GitArgs("worktree", "add", "--detach", dir, commit)...) + cmd.Env = reviewer.ChildEnv() cmd.Dir = c.cfg.Dir if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("%w: %s", err, strings.TrimSpace(string(out))) @@ -439,7 +455,8 @@ func (c *Controller) gitWorktreeAdd(ctx context.Context, dir, commit string) err // gitWorktreeRemove tears down a worktree. Best-effort; uses a detached context // so cleanup still runs when the review context was cancelled. func (c *Controller) gitWorktreeRemove(ctx context.Context, dir string) { - cmd := exec.CommandContext(context.WithoutCancel(ctx), "git", "worktree", "remove", "--force", dir) + cmd := exec.CommandContext(context.WithoutCancel(ctx), "git", reviewer.GitArgs("worktree", "remove", "--force", dir)...) + cmd.Env = reviewer.ChildEnv() cmd.Dir = c.cfg.Dir if out, err := cmd.CombinedOutput(); err != nil { c.log.WarnContext(ctx, "worktree remove", "dir", dir, "err", err, "out", strings.TrimSpace(string(out))) diff --git a/pkg/reviewer/ctl/panel_test.go b/pkg/reviewer/ctl/panel_test.go index e3ae870..08e9718 100644 --- a/pkg/reviewer/ctl/panel_test.go +++ b/pkg/reviewer/ctl/panel_test.go @@ -104,6 +104,25 @@ func TestGitWorktreeAddRemove(t *testing.T) { assert.NoDirExists(t, wt, "worktree dir must be gone after remove") } +func TestGitWorktreeAddIgnoresPlantedHooks(t *testing.T) { + repo := t.TempDir() + runGit(t, repo, "init", "-q") + require.NoError(t, os.WriteFile(filepath.Join(repo, "file.txt"), []byte("hi"), 0o600)) + runGit(t, repo, "add", ".") + runGit(t, repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-q", "-m", "init") + // What an agent with write access to the checkout could plant: a hook in the + // .git that every panel worktree shares. + marker := filepath.Join(t.TempDir(), "hook-ran") + hook := filepath.Join(repo, ".git", "hooks", "post-checkout") + require.NoError(t, os.WriteFile(hook, []byte("#!/bin/sh\ntouch "+marker+"\n"), 0o755)) + + c := &Controller{cfg: &Config{Dir: repo}, log: slog.Default()} + wt := filepath.Join(t.TempDir(), "wt-judge") + require.NoError(t, c.gitWorktreeAdd(t.Context(), wt, "HEAD")) + t.Cleanup(func() { c.gitWorktreeRemove(context.Background(), wt) }) + assert.NoFileExists(t, marker, "reviewctl must not run hooks from the checkout") +} + func TestRunMembers_ToleratesFailures(t *testing.T) { // Failed members must ship their artifacts as a debug bundle BEFORE panel // cleanup wipes the worktree (its only copy) — count the uploads. @@ -256,7 +275,7 @@ func TestSourceLabels(t *testing.T) { } func TestRunJudge_FailureShipsDebugBundle(t *testing.T) { - srv, fields := newDebugCaptureServer(t) + srv, dc := newDebugCaptureServer(t, nil) mdir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(mdir, "review.json"), []byte(`{"issues":[]}`), 0o600)) outputs := []*memberOutput{{label: "opus", dir: mdir}} @@ -269,8 +288,45 @@ func TestRunJudge_FailureShipsDebugBundle(t *testing.T) { _, err := c.runJudge(t.Context(), t.TempDir(), "fuse the members", outputs) require.ErrorContains(t, err, "judge produced an empty review") - assert.Equal(t, reviewer.RunStatusFailed, fields["status"]) - assert.Equal(t, reviewer.RunReasonNotSubmitted, fields["reason"]) - assert.Equal(t, "1", fields["costUsd"], "both judge attempts are billed") - assert.Equal(t, runner.RunnerClaude, fields["runner"]) + assert.Equal(t, reviewer.RunStatusFailed, dc.field("status")) + assert.Equal(t, reviewer.RunReasonNotSubmitted, dc.field("reason")) + assert.Equal(t, "1", dc.field("costUsd"), "both judge attempts are billed") + assert.Equal(t, runner.RunnerClaude, dc.field("runner")) +} + +func TestReviewPanel_ShipsPanelLevelFailure(t *testing.T) { + srv, dc := newDebugCaptureServer(t, nil) + // A checkout that doesn't exist: no worktree, so no member ever runs — the + // panel itself must report the failure. + cfg := &Config{Key: "k", URL: srv.URL, Dir: filepath.Join(t.TempDir(), "missing"), Commit: "abc123", + Runner: runner.RunnerClaude, Multi: []MemberSpec{{Runner: runner.RunnerClaude, Model: "opus"}}} + c := NewController(cfg, nil, slog.Default(), WithRunnerFactory(func(*Config) (runner.ReviewRunner, error) { + return failingRunner{name: runner.RunnerClaude}, nil + })) + + err := c.Review(t.Context()) + require.ErrorContains(t, err, "worktree add") + assert.Equal(t, 1, dc.count()) + assert.Equal(t, reviewer.RunStatusFailed, dc.field("status")) + assert.Equal(t, reviewer.PanelRunner, dc.field("runner"), "not the primary profile's runner") + assert.Equal(t, "claude:opus", dc.field("model")) + assert.Contains(t, dc.field("errorMsg"), "worktree add") +} + +func TestReviewPanel_AllMembersFailedReportsEachOnce(t *testing.T) { + srv, dc := newDebugCaptureServer(t, nil) + repo := t.TempDir() + runGit(t, repo, "init", "-q") + require.NoError(t, os.WriteFile(filepath.Join(repo, "file.txt"), []byte("hi"), 0o600)) + runGit(t, repo, "add", ".") + runGit(t, repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-q", "-m", "init") + cfg := &Config{Key: "k", URL: srv.URL, Dir: repo, Commit: "HEAD", Runner: runner.RunnerClaude, + Multi: []MemberSpec{{Runner: runner.RunnerClaude, Model: "a"}, {Runner: runner.RunnerCodex, Model: "b"}}} + c := NewController(cfg, nil, slog.Default(), WithRunnerFactory(func(mc *Config) (runner.ReviewRunner, error) { + return failingRunner{name: mc.Runner, err: errors.New("boom")}, nil + })) + + err := c.Review(t.Context()) + require.ErrorIs(t, err, errAllMembersFailed) + assert.Equal(t, 2, dc.count(), "one bundle per failed member, none for the panel on top") } diff --git a/pkg/reviewer/ctl/upload.go b/pkg/reviewer/ctl/upload.go index bcc0e84..854a5f7 100644 --- a/pkg/reviewer/ctl/upload.go +++ b/pkg/reviewer/ctl/upload.go @@ -122,16 +122,26 @@ func (c *UploadClient) UploadAll(ctx context.Context, serverURL, projectKey stri for reviewType, filePath := range mdFiles { content, err := os.ReadFile(filePath) if err != nil { - return reviewID, fmt.Errorf("read %s: %w", filePath, err) + return reviewID, &reviewCreatedError{reviewID: reviewID, err: fmt.Errorf("read %s: %w", filePath, err)} } if err := c.UploadFile(ctx, serverURL, projectKey, reviewID, reviewType, content); err != nil { - return reviewID, err + return reviewID, &reviewCreatedError{reviewID: reviewID, err: err} } } return reviewID, nil } +// reviewCreatedError is a failure after the review itself was created, which +// already counted the run as ok on the server; its debug bundle says so. +type reviewCreatedError struct { + reviewID int + err error +} + +func (e *reviewCreatedError) Error() string { return e.err.Error() } +func (e *reviewCreatedError) Unwrap() error { return e.err } + // ReadReviewJSON reads and validates review.json from the given directory. // On validation failure, also returns the parsed draft so the caller can // surface diagnostic detail without re-reading the file. @@ -180,6 +190,7 @@ type DebugMeta struct { Status string // reviewer.RunStatus* Reason string // reviewer.RunReason*; empty for an ok run CostUsd float64 // what the run spent, including a failed run + ReviewID int // the review created before the run failed, if any } // UploadDebugBundle posts artifacts as a multipart form with each file @@ -242,6 +253,7 @@ func buildDebugMultipart(meta DebugMeta, files map[string][]byte) (io.Reader, st {debug.FieldStatus, meta.Status}, {debug.FieldReason, meta.Reason}, {debug.FieldCostUsd, formatCost(meta.CostUsd)}, + {debug.FieldReviewID, formatID(meta.ReviewID)}, } for _, f := range fields { if f.value == "" { @@ -285,6 +297,14 @@ func formatCost(usd float64) string { return strconv.FormatFloat(usd, 'f', -1, 64) } +// formatID renders an optional id for a form field; "" when unset. +func formatID(id int) string { + if id <= 0 { + return "" + } + return strconv.Itoa(id) +} + // reviewArtifactFiles are the fixed-name outputs a runner writes into the review // directory (the R*.md bodies are matched separately by FindMDFiles). The // canonical set lives in the reviewer package, shared with the direct runner's @@ -294,6 +314,9 @@ var reviewArtifactFiles = reviewer.ReviewArtifactFiles // CollectDebugArtifacts reads the artifacts that reviewctl writes during a run. // Missing files are silently skipped — the caller wants whatever is on disk. func CollectDebugArtifacts(dir string) map[string][]byte { + if dir == "" { // a run with no working dir of its own, e.g. a failed panel + return nil + } out := make(map[string][]byte, len(reviewArtifactFiles)+len(reviewTypeByPrefix)) for _, name := range reviewArtifactFiles { diff --git a/pkg/reviewer/ctl/upload_test.go b/pkg/reviewer/ctl/upload_test.go index 14d310e..4b80d92 100644 --- a/pkg/reviewer/ctl/upload_test.go +++ b/pkg/reviewer/ctl/upload_test.go @@ -298,3 +298,10 @@ func TestCleanReviewArtifacts(t *testing.T) { // Idempotent: clean again on an already-clean dir is a no-op, no error. require.NoError(t, CleanReviewArtifacts(dir)) } + +func TestCollectDebugArtifactsWithoutDir(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "review.json"), []byte("{}"), 0o600)) + t.Chdir(dir) + assert.Nil(t, CollectDebugArtifacts(""), "no dir means no artifacts, not the current one") +} diff --git a/pkg/reviewer/direct/errors.go b/pkg/reviewer/direct/errors.go index ba86f88..37835ac 100644 --- a/pkg/reviewer/direct/errors.go +++ b/pkg/reviewer/direct/errors.go @@ -2,11 +2,12 @@ package direct import ( "cmp" - "context" "errors" "fmt" + "io" + "net" "net/http" - "strings" + "net/url" "reviewsrv/pkg/reviewer" @@ -17,19 +18,24 @@ import ( // providerReason maps a failed LLMProvider.Complete (after the provider's own // retries) to a reviewer.RunReason*. func providerReason(err error) string { - var ae *anthropic.Error - if errors.As(err, &ae) { + var ( + ae *anthropic.Error + apiErr *openai.APIError + reqErr *openai.RequestError + rf *responseFailedError + ) + switch { + case errors.As(err, &ae): return anthropicReason(ae) - } - var oe *openai.APIError - if errors.As(err, &oe) { - return openaiReason(oe) - } - var rf *responseFailedError - if errors.As(err, &rf) { + case errors.As(err, &apiErr): + return openaiReason(apiErr) + case errors.As(err, &reqErr): + // An error body go-openai could not decode (a proxy's HTML page). + return reviewer.ReasonForHTTPStatus(reqErr.HTTPStatusCode) + case errors.As(err, &rf): return rf.reason() } - // Not an API error: a transport failure (reset stream, EOF, refused + // No API answer: a transport failure (reset stream, EOF, refused // connection) that outlived the retries. return reviewer.RunReasonAPIError } @@ -37,7 +43,7 @@ func providerReason(err error) string { // anthropicReason classifies an Anthropic API error by its body type, falling // back to the HTTP status. Errors inside the SSE stream carry the stream's 200. func anthropicReason(e *anthropic.Error) string { - if isCreditBalanceError(e.RawJSON()) { + if reviewer.IsBillingMessage(e.RawJSON()) { return reviewer.RunReasonBilling } switch e.Type() { @@ -54,12 +60,6 @@ func anthropicReason(e *anthropic.Error) string { return reviewer.ReasonForHTTPStatus(e.StatusCode) } -// isCreditBalanceError catches an exhausted balance reported as a plain -// invalid_request_error ("Your credit balance is too low ..."). -func isCreditBalanceError(body string) bool { - return strings.Contains(strings.ToLower(body), "credit balance") -} - // openaiReason classifies an OpenAI-protocol API error (OpenAI, DeepSeek, ...) // by its code or type, falling back to the HTTP status. func openaiReason(e *openai.APIError) string { @@ -112,20 +112,33 @@ func (e *responseFailedError) reason() string { } // isOpenAITransient reports whether an OpenAI-protocol call is worth retrying: -// a rate limit or server-side API error — as an HTTP status or inside a failed -// Responses reply — or a network-level request error. Billing -// (insufficient_quota also comes as a 429) and auth are definitive. +// a rate limit or server-side error — by HTTP status or inside a failed +// Responses reply — or a transport failure with no answer (go-openai returns it +// bare, e.g. a *url.Error). Billing (insufficient_quota also comes as a 429), +// auth and bad requests are definitive, and so is anything else: go-openai's +// own request validation fails the same way every time. A cancelled context is +// withRetry's call. func isOpenAITransient(err error) bool { - var apiErr *openai.APIError - if errors.As(err, &apiErr) { - return isTransientReason(openaiReason(apiErr)) + var ( + apiErr *openai.APIError + reqErr *openai.RequestError + rf *responseFailedError + ) + if errors.As(err, &apiErr) || errors.As(err, &reqErr) || errors.As(err, &rf) { + return isTransientReason(providerReason(err)) } - var rf *responseFailedError - if errors.As(err, &rf) { - return isTransientReason(rf.reason()) - } - var reqErr *openai.RequestError - return errors.As(err, &reqErr) + return isTransportError(err) +} + +// isTransportError reports a failure with no HTTP answer: a reset, refused or +// timed-out connection, or a body cut short. +func isTransportError(err error) bool { + var ( + urlErr *url.Error + netErr net.Error + ) + return errors.As(err, &urlErr) || errors.As(err, &netErr) || + errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, io.EOF) } // isTransientReason reports whether a failure of that reason may pass on retry. @@ -139,9 +152,6 @@ func isTransientReason(reason string) bool { // error event inside the SSE stream (200 + api_error/overloaded_error) and a // reset HTTP/2 stream — a bare transport error, hence retry-by-default. func isAnthropicTransient(err error) bool { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return false - } var ae *anthropic.Error if !errors.As(err, &ae) { return true @@ -149,7 +159,7 @@ func isAnthropicTransient(err error) bool { switch ae.Type() { case anthropic.ErrorTypeRateLimitError, anthropic.ErrorTypeOverloadedError, anthropic.ErrorTypeAPIError, anthropic.ErrorTypeTimeoutError: - return !isCreditBalanceError(ae.RawJSON()) + return !reviewer.IsBillingMessage(ae.RawJSON()) case anthropic.ErrorTypeInvalidRequestError, anthropic.ErrorTypeAuthenticationError, anthropic.ErrorTypePermissionError, anthropic.ErrorTypeNotFoundError, anthropic.ErrorTypeBillingError: return false diff --git a/pkg/reviewer/direct/provider_anthropic_test.go b/pkg/reviewer/direct/provider_anthropic_test.go index b4e3f89..62ecd13 100644 --- a/pkg/reviewer/direct/provider_anthropic_test.go +++ b/pkg/reviewer/direct/provider_anthropic_test.go @@ -1,9 +1,7 @@ package direct import ( - "context" "errors" - "fmt" "io" "net/http" "net/http/httptest" @@ -159,6 +157,4 @@ func TestAnthropicCompleteFailsFastOnBilling(t *testing.T) { func TestIsAnthropicTransient(t *testing.T) { require.True(t, isAnthropicTransient(errors.New("stream error: stream ID 41; INTERNAL_ERROR; received from peer"))) - require.False(t, isAnthropicTransient(context.Canceled)) - require.False(t, isAnthropicTransient(fmt.Errorf("post: %w", context.DeadlineExceeded))) } diff --git a/pkg/reviewer/direct/provider_openai_test.go b/pkg/reviewer/direct/provider_openai_test.go index 4e87f8f..7120b40 100644 --- a/pkg/reviewer/direct/provider_openai_test.go +++ b/pkg/reviewer/direct/provider_openai_test.go @@ -3,10 +3,13 @@ package direct import ( "context" "encoding/json" - "errors" + "io" + "net/url" "testing" "time" + "reviewsrv/pkg/reviewer" + openai "github.com/sashabaranov/go-openai" "github.com/stretchr/testify/require" ) @@ -46,8 +49,25 @@ func TestIsOpenAITransient(t *testing.T) { require.False(t, isOpenAITransient(&openai.APIError{HTTPStatusCode: 400})) require.False(t, isOpenAITransient(&openai.APIError{HTTPStatusCode: 401})) require.False(t, isOpenAITransient(&openai.APIError{HTTPStatusCode: 429, Code: "insufficient_quota"}), "an exhausted quota is billing, not a rate limit") - require.True(t, isOpenAITransient(&openai.RequestError{HTTPStatusCode: 500})) - require.False(t, isOpenAITransient(errors.New("plain error"))) + + // An error body go-openai can't decode (a proxy's HTML page) keeps its status. + require.True(t, isOpenAITransient(&openai.RequestError{HTTPStatusCode: 502})) + require.False(t, isOpenAITransient(&openai.RequestError{HTTPStatusCode: 401})) + require.False(t, isOpenAITransient(&openai.RequestError{HTTPStatusCode: 402})) + + // No HTTP answer at all: go-openai returns the transport error as is. + require.True(t, isOpenAITransient(&url.Error{Op: "Post", URL: "https://api.openai.com/v1/responses", Err: io.ErrUnexpectedEOF})) + require.True(t, isOpenAITransient(io.ErrUnexpectedEOF)) + // Anything else — e.g. go-openai rejecting the request before sending it — + // fails the same way on every attempt. + require.False(t, isOpenAITransient(openai.ErrReasoningModelMaxTokensDeprecated)) +} + +func TestProviderReasonOpenAIShapes(t *testing.T) { + require.Equal(t, reviewer.RunReasonBilling, providerReason(&openai.RequestError{HTTPStatusCode: 402})) + require.Equal(t, reviewer.RunReasonAuth, providerReason(&openai.RequestError{HTTPStatusCode: 401})) + require.Equal(t, reviewer.RunReasonBilling, providerReason(&openai.APIError{HTTPStatusCode: 429, Code: "insufficient_quota"})) + require.Equal(t, reviewer.RunReasonAPIError, providerReason(&url.Error{Op: "Post", URL: "x", Err: io.ErrUnexpectedEOF})) } func TestToOpenAIMessagesSkipsBareAssistant(t *testing.T) { diff --git a/pkg/reviewer/direct/tools_git.go b/pkg/reviewer/direct/tools_git.go index 4be2ccc..466d684 100644 --- a/pkg/reviewer/direct/tools_git.go +++ b/pkg/reviewer/direct/tools_git.go @@ -257,8 +257,9 @@ func gitDiff(ctx context.Context, root, base, head, path string) (string, error) // runGit runs git -C root with the given args. When allowExit1 is set, an exit // code of 1 (git diff "differences found") is treated as success. func runGit(ctx context.Context, root string, allowExit1 bool, args ...string) (string, error) { - full := append([]string{"-C", root}, args...) - out, err := exec.CommandContext(ctx, "git", full...).Output() + cmd := exec.CommandContext(ctx, "git", reviewer.GitArgs(append([]string{"-C", root}, args...)...)...) + cmd.Env = reviewer.ChildEnv() + out, err := cmd.Output() if err == nil { return string(out), nil } diff --git a/pkg/reviewer/project.go b/pkg/reviewer/project.go index 52acbf5..6456cd6 100644 --- a/pkg/reviewer/project.go +++ b/pkg/reviewer/project.go @@ -14,6 +14,14 @@ import ( // cmd/reviewctl alias it so the prompt reference and the injected env can't drift. const EnvTrackerToken = "REVIEW_TRACKER_TOKEN" +// reviewctl's own secrets from the CI env: canonical names, which cmd/reviewctl +// reads and pkg/reviewer/runner keeps out of the agents' env. +const ( + EnvProjectKey = "PROJECT_KEY" + EnvGitLabToken = "REVIEWER_GITLAB_TOKEN" + EnvAPIKey = "REVIEW_API_KEY" // the direct runner's provider-agnostic key +) + type ProjectManager struct { repo db.ProjectRepo reviewRepo db.ReviewRepo diff --git a/pkg/reviewer/run_metrics.go b/pkg/reviewer/run_metrics.go index 55080a7..3724afc 100644 --- a/pkg/reviewer/run_metrics.go +++ b/pkg/reviewer/run_metrics.go @@ -9,6 +9,10 @@ const ( metricOtherRunner = "other" ) +// PanelRunner is the runner label of a multi-review panel's own failure (prompt +// fetch, worktree, upload), outside any member or judge run. +const PanelRunner = "panel" + // maxRunCostUsd bounds a plausible run cost (a 500-round Opus run stays well // below it): a higher client-reported value is not added to the cost counter, // which only ever grows. diff --git a/pkg/reviewer/run_outcome.go b/pkg/reviewer/run_outcome.go index 2bf079b..5ec03aa 100644 --- a/pkg/reviewer/run_outcome.go +++ b/pkg/reviewer/run_outcome.go @@ -4,6 +4,9 @@ import ( "context" "errors" "net/http" + "regexp" + "strconv" + "strings" ) // Status of a reviewctl run, reported with its debug bundle. The status and @@ -57,6 +60,54 @@ func ReasonForHTTPStatus(code int) string { return RunReasonOther } +// httpStatusRe finds an HTTP status in CLI error text: "unexpected status 401", +// "last status: 429", "statusCode: 429", "HTTP 502". +var httpStatusRe = regexp.MustCompile(`\b(?:status(?:\s*code)?|http)[\s:=]*([45]\d\d)\b`) // on lowercased text + +// ReasonFromMessage guesses a RunReason* from a CLI's error text (codex and +// opencode report API failures only as text), or "" when nothing matches. +// Billing wording goes first: an exhausted quota often comes as a 429. +func ReasonFromMessage(msg string) string { + if IsBillingMessage(msg) { + return RunReasonBilling + } + m := strings.ToLower(msg) + if sm := httpStatusRe.FindStringSubmatch(m); sm != nil { + code, _ := strconv.Atoi(sm[1]) + if r := ReasonForHTTPStatus(code); r != RunReasonOther { + return r + } + } + switch { + case containsAny(m, "unauthorized", "invalid api key", "invalid_api_key", "authentication"): + return RunReasonAuth + case containsAny(m, "rate limit", "rate_limit", "too many requests"): + return RunReasonRateLimit + case containsAny(m, "overloaded", "internal server error", "server_error", "service unavailable", "bad gateway"): + return RunReasonAPIError + } + return "" +} + +// IsBillingMessage reports whether an error text says the account is out of +// credit or quota ("Your credit balance is too low", insufficient_quota, a +// plan's usage limit). Deliberately narrow: a per-minute "Quota exceeded ... +// check your plan and billing details" is a rate limit, not billing. +func IsBillingMessage(msg string) bool { + return containsAny(strings.ToLower(msg), "insufficient_quota", "credit balance", "usage limit") +} + +// containsAny reports whether s contains any of subs (whole substrings, unlike +// strings.ContainsAny). +func containsAny(s string, subs ...string) bool { + for _, sub := range subs { + if strings.Contains(s, sub) { + return true + } + } + return false +} + // RunError tags a runner failure with a RunReason* so the controller reports a // bounded reason without parsing the message. The message is the wrapped error's. type RunError struct { @@ -67,10 +118,13 @@ type RunError struct { func (e *RunError) Error() string { return e.Err.Error() } func (e *RunError) Unwrap() error { return e.Err } -// WithRunReason tags err with a failure reason. A nil err or an empty reason -// returns err unchanged. +// WithRunReason tags err with a failure reason. A nil err, an empty reason or +// an err already tagged (the tag closest to the cause wins — e.g. runExec's +// cancelled over a reason guessed from the runner's output) returns err +// unchanged. func WithRunReason(reason string, err error) error { - if err == nil || reason == "" { + var tagged *RunError + if err == nil || reason == "" || errors.As(err, &tagged) { return err } return &RunError{Reason: reason, Err: err} diff --git a/pkg/reviewer/run_outcome_test.go b/pkg/reviewer/run_outcome_test.go index 86e2f76..a3256b1 100644 --- a/pkg/reviewer/run_outcome_test.go +++ b/pkg/reviewer/run_outcome_test.go @@ -68,3 +68,33 @@ func TestNormalizeRunOutcome(t *testing.T) { }) } } + +func TestReasonFromMessage(t *testing.T) { + for _, tt := range []struct{ msg, want string }{ + {"unexpected status 401 Unauthorized: Incorrect API key provided", RunReasonAuth}, + {"exceeded retry limit, last status: 429 Too Many Requests", RunReasonRateLimit}, + {"You've hit your usage limit. Upgrade to Pro", RunReasonBilling}, + {"status 429: insufficient_quota", RunReasonBilling}, + {"HTTP 502 Bad Gateway", RunReasonAPIError}, + {"stream disconnected: model is overloaded", RunReasonAPIError}, + {"unexpected status 400 Bad Request: invalid tool schema", ""}, + {"APICallError: statusCode: 429", RunReasonRateLimit}, + {"Quota exceeded for quota metric per minute; check your plan and billing details. status 429", RunReasonRateLimit}, + {"401 Unauthorized", RunReasonAuth}, + {"Rate limit reached for requests", RunReasonRateLimit}, + {"HTTP 404 Not Found", ""}, + {"fetch https://example.com/v1/models failed", ""}, + {"panic: runtime error", ""}, + } { + assert.Equal(t, tt.want, ReasonFromMessage(tt.msg), tt.msg) + } +} + +func TestWithRunReasonKeepsExistingTag(t *testing.T) { + cancelled := WithRunReason(RunReasonCancelled, errors.New("signal: terminated")) + // A reason guessed from the runner's output must not mask runExec's tag. + err := WithRunReason(RunReasonBilling, fmt.Errorf("usage limit (codex %w)", cancelled)) + status, reason := RunOutcome(err) + assert.Equal(t, RunStatusCancelled, status) + assert.Equal(t, RunReasonCancelled, reason) +} diff --git a/pkg/reviewer/runner/claude.go b/pkg/reviewer/runner/claude.go index 5a3a2b7..b1e4be6 100644 --- a/pkg/reviewer/runner/claude.go +++ b/pkg/reviewer/runner/claude.go @@ -528,11 +528,6 @@ func (w *lineWriter) flush() { } } -// runExec spawns the runner CLI under the shared runnerTimeout and captures -// stdout/stderr. Centralising I/O wiring keeps the per-runner Run() bodies -// focused on argv and result parsing. When onLine is non-nil it receives each -// stdout line as it streams, so a runner can surface significant events live -// (e.g. tool calls from a JSONL agent stream) instead of only after completion. // Credential env var names CLI runners read their API key from, plus the // task-tracker token. envTrackerToken aliases the canonical reviewer const so // the $REVIEW_TRACKER_TOKEN reference in assembled prompts and the env injected @@ -555,6 +550,11 @@ func credEnv(varName, token string) []string { return []string{varName + "=" + token} } +// runExec spawns the runner CLI under the shared runnerTimeout and captures +// stdout/stderr. Centralising I/O wiring keeps the per-runner Run() bodies +// focused on argv and result parsing. When onLine is non-nil it receives each +// stdout line as it streams, so a runner can surface significant events live +// (e.g. tool calls from a JSONL agent stream) instead of only after completion. func runExec(ctx context.Context, log *slog.Logger, binary, dir string, args []string, prompt string, extraEnv []string, onLine func([]byte)) *runOutput { ctx, cancel := context.WithTimeout(ctx, runnerTimeout) defer cancel() @@ -562,12 +562,7 @@ func runExec(ctx context.Context, log *slog.Logger, binary, dir string, args []s cmd := exec.CommandContext(ctx, binary, args...) cmd.Dir = dir cmd.Stdin = strings.NewReader(prompt) - // extraEnv comes last: for a key also in the ambient env (a merged - // OPENCODE_CONFIG_CONTENT) os/exec keeps the last value. nil extraEnv leaves - // cmd.Env nil = inherit. - if len(extraEnv) > 0 { - cmd.Env = append(os.Environ(), extraEnv...) - } + cmd.Env = reviewer.ChildEnv(extraEnv...) out := &runOutput{} var lw *lineWriter diff --git a/pkg/reviewer/runner/claude_failure.go b/pkg/reviewer/runner/claude_failure.go index be6d392..46df4cb 100644 --- a/pkg/reviewer/runner/claude_failure.go +++ b/pkg/reviewer/runner/claude_failure.go @@ -62,9 +62,11 @@ func claudeRunError(err error, cr *ClaudeResult, stdout []byte, stderr string) e // claudeReason maps a Claude Code error kind (falling back to the HTTP status and // the result text) to a RunReason. func claudeReason(kind string, status int, text string) string { - switch kind { - case claudeErrBilling: + if kind == claudeErrBilling || reviewer.IsBillingMessage(text) { + // Before the status: an exhausted balance can come as a 400 or a 429. return reviewer.RunReasonBilling + } + switch kind { case claudeErrAuth: return reviewer.RunReasonAuth case claudeErrRateLimit: @@ -75,6 +77,9 @@ func claudeReason(kind string, status int, text string) string { if r := reviewer.ReasonForHTTPStatus(status); r != reviewer.RunReasonOther { return r } + if r := reviewer.ReasonFromMessage(text); r != "" { + return r + } if strings.HasPrefix(text, claudeAPIErrorPrefix) { return reviewer.RunReasonAPIError } @@ -114,6 +119,17 @@ func lastAssistantError(stdout []byte) (kind, text string) { return kind, text } +// errTail is the end of a CLI's error output, where its final error sits: the +// lines before it (stack traces, the checkout's paths) would only mislead +// ReasonFromMessage. +func errTail(s string) string { + const n = 2000 + if len(s) <= n { + return s + } + return s[len(s)-n:] +} + // isInterrupted reports whether a subprocess was stopped by SIGTERM/SIGINT — // killed outright ("signal: terminated") or exiting 128+signo after handling it // (143/130), as Claude Code does. In CI that is a cancelled job. diff --git a/pkg/reviewer/runner/claude_failure_test.go b/pkg/reviewer/runner/claude_failure_test.go index 5bf01f1..05ee616 100644 --- a/pkg/reviewer/runner/claude_failure_test.go +++ b/pkg/reviewer/runner/claude_failure_test.go @@ -121,6 +121,8 @@ func TestClaudeReason(t *testing.T) { {claudeErrServer, 0, "", reviewer.RunReasonAPIError}, {claudeErrUnknown, 529, "", reviewer.RunReasonAPIError}, {claudeErrUnknown, 0, "API Error: Internal server error", reviewer.RunReasonAPIError}, + {claudeErrUnknown, 0, "API Error: Connection error.", reviewer.RunReasonAPIError}, + {claudeErrUnknown, 429, "Your credit balance is too low", reviewer.RunReasonBilling}, {"invalid_request", 400, "prompt is too long", reviewer.RunReasonOther}, } for _, tt := range tests { diff --git a/pkg/reviewer/runner/cli_failure_test.go b/pkg/reviewer/runner/cli_failure_test.go new file mode 100644 index 0000000..af68644 --- /dev/null +++ b/pkg/reviewer/runner/cli_failure_test.go @@ -0,0 +1,69 @@ +package runner + +import ( + "log/slog" + "os" + "path/filepath" + "testing" + + "reviewsrv/pkg/reviewer" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeCLI puts an executable sh script called name first on PATH. +func fakeCLI(t *testing.T, name, script string) { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+script+"\n"), 0o755)) + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func TestExecCodexRunnerTagsAPIFailure(t *testing.T) { + for _, tt := range []struct{ name, script, want string }{ + {"billing from the error event", + `echo '{"type":"turn.failed","error":{"message":"You exceeded your current quota: insufficient_quota"}}'; exit 1`, + reviewer.RunReasonBilling}, + {"auth from stderr when nothing streamed", + `echo 'unexpected status 401 Unauthorized' >&2; exit 1`, + reviewer.RunReasonAuth}, + {"a cancelled job stays cancelled", + `echo '{"type":"turn.failed","error":{"message":"insufficient_quota"}}'; exit 143`, + reviewer.RunReasonCancelled}, + } { + t.Run(tt.name, func(t *testing.T) { + fakeCLI(t, "codex", tt.script) + r := &ExecCodexRunner{Dir: t.TempDir(), Log: slog.New(slog.DiscardHandler)} + _, err := r.Run(t.Context(), "review") + require.Error(t, err) + _, reason := reviewer.RunOutcome(err) + assert.Equal(t, tt.want, reason) + }) + } +} + +func TestExecOpenCodeRunnerTagsAPIFailure(t *testing.T) { + for _, tt := range []struct { + name, script, want string + partial bool + }{ + {"auth from stderr", `echo 'Error: 401 Unauthorized' >&2; exit 1`, reviewer.RunReasonAuth, false}, + {"rate limit after some steps", + `echo '{"type":"step_finish","sessionID":"s","part":{"reason":"tool-calls","tokens":{"input":1,"output":1}}}' +echo 'AI_APICallError: statusCode: 429' >&2; exit 1`, + reviewer.RunReasonRateLimit, true}, + } { + t.Run(tt.name, func(t *testing.T) { + fakeCLI(t, "opencode", tt.script) + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + r := &ExecOpenCodeRunner{Dir: dir, Log: slog.New(slog.DiscardHandler)} + cr, err := r.Run(t.Context(), "review") + require.Error(t, err) + assert.Equal(t, tt.partial, cr != nil, "a parsed partial result is kept") + _, reason := reviewer.RunOutcome(err) + assert.Equal(t, tt.want, reason) + }) + } +} diff --git a/pkg/reviewer/runner/codex.go b/pkg/reviewer/runner/codex.go index 67461c1..829b728 100644 --- a/pkg/reviewer/runner/codex.go +++ b/pkg/reviewer/runner/codex.go @@ -14,6 +14,7 @@ import ( "strings" "time" + "reviewsrv/pkg/reviewer" "reviewsrv/pkg/reviewer/direct" ) @@ -116,12 +117,15 @@ func (r *ExecCodexRunner) Run(ctx context.Context, prompt string) (*ClaudeResult if out.stdout.Len() == 0 { r.Log.WarnContext(ctx, "codex produced empty stdout", "stderr", truncate(out.stderr.String(), 2000)) if out.err != nil { - return nil, fmt.Errorf("codex exited with error: %w (stderr: %s)", out.err, truncate(out.stderr.String(), 500)) + stderr := out.stderr.String() + return nil, reviewer.WithRunReason(reviewer.ReasonFromMessage(errTail(stderr)), + fmt.Errorf("codex exited with error: %w (stderr: %s)", out.err, truncate(stderr, 500))) } return nil, errors.New("codex produced empty output") } - cr := r.result(parseCodexStream(out.stdout.Bytes())) + agg := parseCodexStream(out.stdout.Bytes()) + cr := r.result(agg) // codex events report no duration: take the subprocess wall-clock time. cr.DurationMs = int(out.elapsed.Milliseconds()) // codex can report a structured failure with a zero exit code; conversely a @@ -133,7 +137,14 @@ func (r *ExecCodexRunner) Run(ctx context.Context, prompt string) (*ClaudeResult if out.err != nil { r.Log.WarnContext(ctx, "codex error", "stderr", truncate(out.stderr.String(), 2000)) - return cr, fmt.Errorf("codex exited with error: %w", out.err) + if agg.errMsg == "" { + return cr, fmt.Errorf("codex exited with error: %w", out.err) + } + // codex reports an API failure only as text: lead with it, and read the + // run's reason off it so a billing or auth failure is alertable (a + // cancelled tag from runExec stays). + return cr, reviewer.WithRunReason(reviewer.ReasonFromMessage(agg.errMsg), + fmt.Errorf("%s (codex %w)", agg.errMsg, out.err)) } return cr, nil } diff --git a/pkg/reviewer/runner/env_test.go b/pkg/reviewer/runner/env_test.go index 8370603..619756d 100644 --- a/pkg/reviewer/runner/env_test.go +++ b/pkg/reviewer/runner/env_test.go @@ -1,9 +1,11 @@ package runner import ( + "log/slog" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCredEnv(t *testing.T) { @@ -32,3 +34,14 @@ func TestClaudeIsolationEnv(t *testing.T) { t.Setenv("CLAUDE_CODE_DISABLE_AUTO_MEMORY", "0") assert.Nil(t, claudeIsolationEnv(), "the operator's env wins") } + +func TestRunExecScrubsCISecrets(t *testing.T) { + t.Setenv("REVIEWER_GITLAB_TOKEN", "glpat-secret") + t.Setenv("ANTHROPIC_API_KEY", "sk-ambient") + out := runExec(t.Context(), slog.New(slog.DiscardHandler), "sh", t.TempDir(), + []string{"-c", `echo "gitlab=${REVIEWER_GITLAB_TOKEN-unset} key=$ANTHROPIC_API_KEY tracker=$REVIEW_TRACKER_TOKEN"`}, + "", []string{"REVIEW_TRACKER_TOKEN=tracker", "ANTHROPIC_API_KEY=sk-extra"}, nil) + require.NoError(t, out.err) + assert.Equal(t, "gitlab=unset key=sk-extra tracker=tracker\n", out.stdout.String(), + "CI secrets are dropped; an extra var wins over the ambient one") +} diff --git a/pkg/reviewer/runner/opencode.go b/pkg/reviewer/runner/opencode.go index 67bc282..95d9192 100644 --- a/pkg/reviewer/runner/opencode.go +++ b/pkg/reviewer/runner/opencode.go @@ -13,6 +13,8 @@ import ( "path/filepath" "strings" "time" + + "reviewsrv/pkg/reviewer" ) // exportTimeout caps `opencode export` — it should return in seconds, @@ -101,27 +103,37 @@ func (r *ExecOpenCodeRunner) Run(ctx context.Context, prompt string) (*ClaudeRes return nil, fmt.Errorf("refusing to run opencode: the checkout ships %s, and opencode would execute its plugins and commands with this job's secrets; review this repository with the claude or codex runner", cfgPath) } + isolation, merged := opencodeIsolationEnv() + if !merged { + r.Log.WarnContext(ctx, "OPENCODE_CONFIG_CONTENT is not a JSON object (JSONC?): formatters and LSP are not forced off, whatever it enables runs on the checkout") + } + args := r.buildArgs() // opencode reads its own stored provider credentials, so no API token is // injected — only the tracker token the prompt references. - env := append(credEnv(envTrackerToken, r.TrackerToken), opencodeIsolationEnv()...) + env := append(credEnv(envTrackerToken, r.TrackerToken), isolation...) // Surface significant events (tool calls, per-step usage) live as opencode streams. out := runExec(ctx, r.Log, RunnerOpenCode, r.Dir, args, prompt, env, func(line []byte) { r.logEvent(ctx, line) }) r.saveOutput(ctx, out.stdout.Bytes()) if out.err != nil { + stderr := out.stderr.String() r.Log.WarnContext(ctx, "opencode error", - "stderr", truncate(out.stderr.String(), 2000), + "stderr", truncate(stderr, 2000), "stdout", truncate(out.stdout.String(), 2000), ) // Try to parse whatever arrived before the error — matches Claude runner behaviour. + // opencode reports an API failure only as text on stderr (a cancelled + // tag from runExec stays). + runErr := reviewer.WithRunReason(reviewer.ReasonFromMessage(errTail(stderr)), + fmt.Errorf("opencode exited with error: %w (stderr: %s)", out.err, truncate(stderr, 500))) if out.stdout.Len() > 0 { if cr, parseErr := ParseOpenCodeResult(out.stdout.Bytes(), r.Model); parseErr == nil { - return cr, fmt.Errorf("opencode exited with error: %w", out.err) + return cr, runErr } } - return nil, fmt.Errorf("opencode exited with error: %w (stderr: %s)", out.err, truncate(out.stderr.String(), 500)) + return nil, runErr } if out.stdout.Len() == 0 { @@ -139,7 +151,7 @@ func (r *ExecOpenCodeRunner) Run(ctx context.Context, prompt string) (*ClaudeRes return nil, parseErr } - r.resolveSessionModel(ctx, cr) + r.resolveSessionModel(ctx, cr, isolation) r.logResult(ctx, cr) return cr, nil @@ -195,11 +207,11 @@ func (r *ExecOpenCodeRunner) logEvent(ctx context.Context, line []byte) { // resolveSessionModel enriches cr.ModelUsage when neither -m nor the stream named // the model — opencode's step_finish events carry none. Falls back to // `opencode export `, which reliably returns messages[*].info.model. -func (r *ExecOpenCodeRunner) resolveSessionModel(ctx context.Context, cr *ClaudeResult) { +func (r *ExecOpenCodeRunner) resolveSessionModel(ctx context.Context, cr *ClaudeResult, isolation []string) { if len(cr.ModelUsage) > 0 || cr.SessionID == "" { return } - name := fetchOpenCodeSessionModel(ctx, r.Dir, cr.SessionID) + name := fetchOpenCodeSessionModel(ctx, r.Dir, cr.SessionID, isolation) if name == "" { r.Log.WarnContext(ctx, "opencode session model not resolved", "sessionId", cr.SessionID) return @@ -219,7 +231,7 @@ func (r *ExecOpenCodeRunner) resolveSessionModel(ctx context.Context, cr *Claude // fetchOpenCodeSessionModel queries `opencode export ` to extract // the model used in the session. Returns "" on any failure — caller must // treat the result as optional (it's a best-effort enrichment). -func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string) string { +func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string, isolation []string) string { if sessionID == "" { return "" } @@ -232,9 +244,7 @@ func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string) strin // export boots an instance in its working dir and loads that project's // plugins, so run it in the dir Run vetted, under the same isolation. cmd.Dir = dir - if env := opencodeIsolationEnv(); len(env) > 0 { - cmd.Env = append(os.Environ(), env...) - } + cmd.Env = reviewer.ChildEnv(isolation...) out, err := cmd.Output() if err != nil { return "" @@ -272,13 +282,15 @@ func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string) strin // config (the legacy loader, plus AGENTS.md auto-loading) is off unless the // operator's env says otherwise. Formatters and LSP servers are forced off even // then — prettier and eslint run the repo's JS configs, rust-analyzer its -// build.rs — by merging into the operator's inline config. -func opencodeIsolationEnv() []string { +// build.rs — by merging into the operator's inline config; merged is false when +// that config can't be merged (see opencodeConfigContent). +func opencodeIsolationEnv() ([]string, bool) { env := credEnv("OPENCODE_DISABLE_PROJECT_CONFIG", "1") - if content, ok := opencodeConfigContent(os.Getenv("OPENCODE_CONFIG_CONTENT")); ok { + content, merged := opencodeConfigContent(os.Getenv("OPENCODE_CONFIG_CONTENT")) + if merged { env = append(env, "OPENCODE_CONFIG_CONTENT="+content) } - return env + return env, merged } // opencodeConfigContent returns the operator's inline opencode config with @@ -286,7 +298,8 @@ func opencodeIsolationEnv() []string { // comments) cannot be merged and is left alone: ok is false. func opencodeConfigContent(operator string) (string, bool) { cfg := map[string]any{} - if operator != "" && json.Unmarshal([]byte(operator), &cfg) != nil { + // "null" decodes into a nil map: not an object either. + if operator != "" && (json.Unmarshal([]byte(operator), &cfg) != nil || cfg == nil) { return "", false } cfg["formatter"], cfg["lsp"] = false, false diff --git a/pkg/reviewer/runner/opencode_test.go b/pkg/reviewer/runner/opencode_test.go index d024b0b..274c0ac 100644 --- a/pkg/reviewer/runner/opencode_test.go +++ b/pkg/reviewer/runner/opencode_test.go @@ -151,17 +151,27 @@ func TestExecOpenCodeRunnerRefusesProjectConfig(t *testing.T) { func TestOpenCodeIsolationEnv(t *testing.T) { t.Setenv("OPENCODE_DISABLE_PROJECT_CONFIG", "") t.Setenv("OPENCODE_CONFIG_CONTENT", "") + env, merged := opencodeIsolationEnv() + assert.True(t, merged) assert.Equal(t, []string{ "OPENCODE_DISABLE_PROJECT_CONFIG=1", `OPENCODE_CONFIG_CONTENT={"formatter":false,"lsp":false}`, - }, opencodeIsolationEnv()) + }, env) t.Setenv("OPENCODE_DISABLE_PROJECT_CONFIG", "0") t.Setenv("OPENCODE_CONFIG_CONTENT", `{"share":"disabled","lsp":true}`) + env, merged = opencodeIsolationEnv() + assert.True(t, merged) assert.Equal(t, []string{ `OPENCODE_CONFIG_CONTENT={"formatter":false,"lsp":false,"share":"disabled"}`, - }, opencodeIsolationEnv(), "the operator's config is kept, formatter and lsp forced off") + }, env, "the operator's config is kept, formatter and lsp forced off") t.Setenv("OPENCODE_CONFIG_CONTENT", "{ // jsonc\n}") - assert.Empty(t, opencodeIsolationEnv(), "an unparsable operator config is left alone") + env, merged = opencodeIsolationEnv() + assert.False(t, merged) + assert.Empty(t, env, "an unparsable operator config is left alone") + + t.Setenv("OPENCODE_CONFIG_CONTENT", "null") + _, merged = opencodeIsolationEnv() + assert.False(t, merged, "null is not an object either") } diff --git a/pkg/reviewer/subprocess.go b/pkg/reviewer/subprocess.go new file mode 100644 index 0000000..1b75d31 --- /dev/null +++ b/pkg/reviewer/subprocess.go @@ -0,0 +1,39 @@ +package reviewer + +import ( + "os" + "slices" + "strings" +) + +// ciSecretEnv lists variables no subprocess on the checkout needs but a +// prompt-injected agent could exfiltrate: reviewctl's own credentials and +// GitLab's job secrets. LLM provider keys stay — the runner CLIs read them. +// Secrets under names the operator picks (CI/CD variables, id_tokens) are not +// covered, and the job token also sits in the checkout's git remote URL. +var ciSecretEnv = []string{ + EnvGitLabToken, EnvProjectKey, EnvAPIKey, + "CI_JOB_TOKEN", "CI_REPOSITORY_URL", + "CI_REGISTRY_PASSWORD", "CI_DEPLOY_PASSWORD", "CI_DEPENDENCY_PROXY_PASSWORD", +} + +// ChildEnv is the environment of a subprocess reviewctl starts on the untrusted +// checkout — a runner CLI, git: the ambient one without ciSecretEnv, plus extra. +// extra comes last, and for a key also in the ambient env (a merged +// OPENCODE_CONFIG_CONTENT) os/exec keeps the last value. Defense in depth, not a +// boundary: a process under the same uid can still read reviewctl's own +// /proc//environ; only a separate uid or PID namespace closes that. +func ChildEnv(extra ...string) []string { + env := slices.DeleteFunc(os.Environ(), func(kv string) bool { + name, _, _ := strings.Cut(kv, "=") + return slices.Contains(ciSecretEnv, name) + }) + return append(env, extra...) +} + +// GitArgs prefixes the arguments of a git command reviewctl runs on the +// checkout: an agent can write hooks and an fsmonitor command into its .git +// (shared by every panel worktree), which git would otherwise execute. +func GitArgs(args ...string) []string { + return append([]string{"-c", "core.hooksPath=/dev/null", "-c", "core.fsmonitor=false"}, args...) +} diff --git a/pkg/reviewer/subprocess_test.go b/pkg/reviewer/subprocess_test.go new file mode 100644 index 0000000..d81f4a7 --- /dev/null +++ b/pkg/reviewer/subprocess_test.go @@ -0,0 +1,28 @@ +package reviewer + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestChildEnv(t *testing.T) { + t.Setenv(EnvGitLabToken, "glpat-secret") + t.Setenv(EnvProjectKey, "key") + t.Setenv(EnvAPIKey, "sk-review") + t.Setenv("CI_JOB_TOKEN", "job-secret") + t.Setenv("ANTHROPIC_API_KEY", "sk-ant") + + vars := map[string]string{} + for _, kv := range ChildEnv("REVIEW_TRACKER_TOKEN=tracker") { + name, value, _ := strings.Cut(kv, "=") + vars[name] = value + } + for _, name := range []string{EnvGitLabToken, EnvProjectKey, EnvAPIKey, "CI_JOB_TOKEN"} { + assert.NotContains(t, vars, name) + } + assert.Equal(t, "sk-ant", vars["ANTHROPIC_API_KEY"], "provider keys stay: the CLIs read them") + assert.Equal(t, "tracker", vars["REVIEW_TRACKER_TOKEN"]) + assert.Contains(t, vars, "PATH") +} From 454cbf6808153ae16ffd5827b82b9ae2562bfe86 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 24 Sep 2026 18:51:14 +0300 Subject: [PATCH 6/7] Bump golangci-lint to v2.13.2 - Pin v2.13.2 in the lint workflow and Makefile: CI's v2.12.2 flagged goconst repeats and the //go:fix directive that the local v2.13.2 passes cleanly --- .github/workflows/golangci-lint.yml | 2 +- Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 32ee706..4ca7a81 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -25,4 +25,4 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v8 with: - version: v2.12.2 + version: v2.13.2 diff --git a/Makefile b/Makefile index b64be0b..6abfe5c 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ ifeq ($(RACE),1) GOFLAGS+=-race endif -LINT_VERSION := v2.12.2 +LINT_VERSION := v2.13.2 VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) From cf720d1014cf38c0f4c838b4062ab106e127248b Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 24 Sep 2026 20:56:54 +0300 Subject: [PATCH 7/7] Fix panel metrics, key leaks and CLI shutdown - Upload every member when the judge fails or can't start, so each completed run is counted; report a judge that never started - Keep --max-rounds / REVIEW_MAX_ROUNDS over panel member and judge profiles - Mask the project key in the access log, RPC param logs, Sentry events, reviewctl logs and upload transport errors - Keep a failed Step 2 retry's own reason (cancelled, timeout, billing) instead of reporting not_submitted - Run runner CLIs in their own process group: SIGTERM it on cancel, kill leftovers after exit, bound pipe waits with WaitDelay; handle SIGHUP in reviewctl - Skip Slack notifications for member reviews - Test the decompressed artifact size limit at its boundary --- cmd/reviewctl/main.go | 7 +- cmd/reviewctl/main_test.go | 1 + cmd/reviewsrv/main.go | 1 + pkg/app/handlers.go | 30 +++-- pkg/app/handlers_test.go | 15 +++ pkg/app/sentry.go | 22 ++++ pkg/app/sentry_test.go | 22 ++++ pkg/debug/handler.go | 19 +-- pkg/debug/handler_test.go | 30 +++++ pkg/rest/rest.go | 4 +- pkg/rest/rest_test.go | 17 +++ pkg/reviewctl/server.go | 26 +++- pkg/reviewctl/server_test.go | 37 ++++++ pkg/reviewer/ctl/config.go | 3 + pkg/reviewer/ctl/ctl.go | 11 +- pkg/reviewer/ctl/ctl_test.go | 31 +++++ pkg/reviewer/ctl/panel.go | 61 ++++++++-- pkg/reviewer/ctl/panel_test.go | 131 +++++++++++++++++++++ pkg/reviewer/ctl/prompt.go | 7 +- pkg/reviewer/ctl/recovery.go | 50 ++++---- pkg/reviewer/ctl/skip_detect_test.go | 21 ++-- pkg/reviewer/ctl/upload.go | 30 +++-- pkg/reviewer/ctl/upload_test.go | 20 ++++ pkg/reviewer/project.go | 30 +++++ pkg/reviewer/project_test.go | 9 ++ pkg/reviewer/runner/claude.go | 8 +- pkg/reviewer/runner/claude_failure_test.go | 32 +++++ pkg/reviewer/runner/cli_failure_test.go | 11 ++ pkg/reviewer/runner/opencode.go | 7 +- pkg/reviewer/runner/procgroup.go | 39 ++++++ pkg/reviewer/runner/procgroup_other.go | 14 +++ pkg/reviewer/runner/procgroup_unix.go | 36 ++++++ pkg/reviewer/runner/procgroup_unix_test.go | 33 ++++++ 33 files changed, 716 insertions(+), 99 deletions(-) create mode 100644 pkg/app/sentry.go create mode 100644 pkg/app/sentry_test.go create mode 100644 pkg/reviewctl/server_test.go create mode 100644 pkg/reviewer/runner/procgroup.go create mode 100644 pkg/reviewer/runner/procgroup_other.go create mode 100644 pkg/reviewer/runner/procgroup_unix.go create mode 100644 pkg/reviewer/runner/procgroup_unix_test.go diff --git a/cmd/reviewctl/main.go b/cmd/reviewctl/main.go index d6714bc..f73d171 100644 --- a/cmd/reviewctl/main.go +++ b/cmd/reviewctl/main.go @@ -123,8 +123,10 @@ func main() { rootCmd.AddCommand(reviewCmd, uploadCmd, commentCmd, versionCmd) // A cancelled CI job sends SIGTERM: cancel the run context instead of dying // mid-flight, so the runner stops and the debug bundle still ships, marked - // cancelled rather than failed. - ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + // cancelled rather than failed. The runner CLIs sit in process groups of + // their own, beyond the terminal's signals: a closed terminal (SIGHUP) must + // stop them the same way. + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM, syscall.SIGHUP) // After the first signal, restore the default handling: a second Ctrl+C or // SIGTERM ends reviewctl even while it still ships bundles and cleans up. context.AfterFunc(ctx, stop) @@ -243,6 +245,7 @@ func applyProfile(changed func(string) bool, cfg *ctl.Config, p *ctl.ResolvedPro if !changed("allow-dangerous-permissions") { cfg.AllowDangerousPermissions = p.Params.AllowDangerousPermissions } + cfg.MaxRoundsSet = changed("max-rounds") serverDefault(changed, "max-rounds", p.Params.MaxRounds, &cfg.MaxRounds) } diff --git a/cmd/reviewctl/main_test.go b/cmd/reviewctl/main_test.go index 22a5620..18315d5 100644 --- a/cmd/reviewctl/main_test.go +++ b/cmd/reviewctl/main_test.go @@ -83,6 +83,7 @@ func TestApplyProfile(t *testing.T) { applyProfile(passed("runner", "max-rounds"), cfg, p) assert.Equal(t, "codex", cfg.Runner, "an explicit flag wins") assert.Zero(t, cfg.MaxRounds, "an explicit --max-rounds 0 (the default) wins too") + assert.True(t, cfg.MaxRoundsSet, "and overrides the panel members' profiles") cfg = &ctl.Config{MaxRounds: 200} applyProfile(passed("max-rounds"), cfg, &ctl.ResolvedProfile{}) diff --git a/cmd/reviewsrv/main.go b/cmd/reviewsrv/main.go index d36c11e..cc6b5ef 100644 --- a/cmd/reviewsrv/main.go +++ b/cmd/reviewsrv/main.go @@ -70,6 +70,7 @@ func main() { Dsn: cfg.Sentry.DSN, Environment: cfg.Sentry.Environment, Release: version, + BeforeSend: app.MaskSentryKeys, })) } diff --git a/pkg/app/handlers.go b/pkg/app/handlers.go index f1e86ba..5916f32 100644 --- a/pkg/app/handlers.go +++ b/pkg/app/handlers.go @@ -45,15 +45,9 @@ func apiBodyLimit() echo.MiddlewareFunc { }) } -// registerHandlers register echo handlers. -func (a *App) registerHandlers() { - a.echo.Use(middleware.CORSWithConfig(middleware.CORSConfig{ - AllowOrigins: []string{"*"}, - AllowMethods: []string{echo.GET, echo.PUT, echo.POST, echo.DELETE}, - AllowHeaders: []string{"Authorization", "Authorization2", "Origin", "X-Requested-With", "Content-Type", "Accept", "Platform", "Version"}, - }), apiBodyLimit()) - - lg := middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{ +// requestLogger logs every request of the routes it is attached to. +func requestLogger(log *slog.Logger) echo.MiddlewareFunc { + return middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{ LogStatus: true, LogURI: true, LogError: true, @@ -65,7 +59,8 @@ func (a *App) registerHandlers() { LogValuesFunc: func(c echo.Context, v middleware.RequestLoggerValues) error { attrs := []slog.Attr{ slog.String("ip", v.RemoteIP), - slog.String("uri", v.URI), + // The upload routes carry the project key. + slog.String("uri", reviewer.MaskKey(v.URI, c.Param("projectKey"))), slog.Int("status", v.Status), slog.String("userAgent", v.UserAgent), slog.String("duration", v.Latency.String()), @@ -73,13 +68,24 @@ func (a *App) registerHandlers() { } if v.Error == nil { - a.Log().LogAttrs(context.Background(), slog.LevelInfo, "http request", attrs...) + log.LogAttrs(context.Background(), slog.LevelInfo, "http request", attrs...) } else { - a.Log().LogAttrs(context.Background(), slog.LevelError, "http request error", append(attrs, slog.String("err", v.Error.Error()))...) + log.LogAttrs(context.Background(), slog.LevelError, "http request error", append(attrs, slog.String("err", v.Error.Error()))...) } return nil }, }) +} + +// registerHandlers register echo handlers. +func (a *App) registerHandlers() { + a.echo.Use(middleware.CORSWithConfig(middleware.CORSConfig{ + AllowOrigins: []string{"*"}, + AllowMethods: []string{echo.GET, echo.PUT, echo.POST, echo.DELETE}, + AllowHeaders: []string{"Authorization", "Authorization2", "Origin", "X-Requested-With", "Content-Type", "Accept", "Platform", "Version"}, + }), apiBodyLimit()) + + lg := requestLogger(a.Log()) h := rest.NewHandler(a.db, slack.NewNotifier(a.Logger), a.cfg.Server.BaseURL, a.runMetrics) diff --git a/pkg/app/handlers_test.go b/pkg/app/handlers_test.go index 0cab791..71baa3c 100644 --- a/pkg/app/handlers_test.go +++ b/pkg/app/handlers_test.go @@ -2,6 +2,7 @@ package app import ( "bytes" + "log/slog" "net/http" "net/http/httptest" "testing" @@ -30,3 +31,17 @@ func TestAPIBodyLimit(t *testing.T) { assert.Equal(t, http.StatusOK, post("/v1/reviewctl/upload/key/", mb)) assert.Equal(t, http.StatusRequestEntityTooLarge, post("/v1/reviewctl/upload/key/", 3*mb), "other routes keep the 2MB cap") } + +func TestRequestLoggerMasksProjectKey(t *testing.T) { + var buf bytes.Buffer + e := echo.New() + e.POST("/v1/reviewctl/upload/:projectKey/", func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "bad draft") + }, requestLogger(slog.New(slog.NewTextHandler(&buf, nil)))) + + const key = "11111111-2222-3333-4444-555555555555" + e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/v1/reviewctl/upload/"+key+"/", nil)) + + assert.NotContains(t, buf.String(), key) + assert.Contains(t, buf.String(), "/v1/reviewctl/upload/11111111…/") +} diff --git a/pkg/app/sentry.go b/pkg/app/sentry.go new file mode 100644 index 0000000..e27f38d --- /dev/null +++ b/pkg/app/sentry.go @@ -0,0 +1,22 @@ +package app + +import ( + "encoding/json" + + "reviewsrv/pkg/reviewer" + + "github.com/getsentry/sentry-go" +) + +// MaskSentryKeys is a sentry BeforeSend hook that keeps project keys out of +// events: zenrpc attaches each call's raw params, the echo integration the +// request URL, and both can carry a key. +func MaskSentryKeys(e *sentry.Event, _ *sentry.EventHint) *sentry.Event { + if p, ok := e.Extra["params"].(json.RawMessage); ok { + e.Extra["params"] = json.RawMessage(reviewer.MaskKeys(string(p))) + } + if e.Request != nil { + e.Request.URL = reviewer.MaskKeys(e.Request.URL) + } + return e +} diff --git a/pkg/app/sentry_test.go b/pkg/app/sentry_test.go new file mode 100644 index 0000000..256e022 --- /dev/null +++ b/pkg/app/sentry_test.go @@ -0,0 +1,22 @@ +package app + +import ( + "encoding/json" + "testing" + + "github.com/getsentry/sentry-go" + "github.com/stretchr/testify/assert" +) + +func TestMaskSentryKeys(t *testing.T) { + const key = "11111111-2222-3333-4444-555555555555" + e := MaskSentryKeys(&sentry.Event{ + Extra: map[string]any{"params": json.RawMessage(`{"projectKey":"` + key + `"}`), "ip": "10.0.0.1"}, + Request: &sentry.Request{URL: "https://srv/v1/reviewctl/upload/" + key + "/"}, + }, nil) + + assert.JSONEq(t, `{"projectKey":"11111111…"}`, string(e.Extra["params"].(json.RawMessage))) + assert.Equal(t, "https://srv/v1/reviewctl/upload/11111111…/", e.Request.URL) + assert.Equal(t, "10.0.0.1", e.Extra["ip"]) + assert.NotNil(t, MaskSentryKeys(&sentry.Event{}, nil), "an event without params or request") +} diff --git a/pkg/debug/handler.go b/pkg/debug/handler.go index ee4b6b0..479a265 100644 --- a/pkg/debug/handler.go +++ b/pkg/debug/handler.go @@ -73,7 +73,7 @@ type Handler struct { // upload). Either may be nil. func NewHandler(storage *Storage, log *slog.Logger, projectTitle func(ctx context.Context, projectKey string) (string, error), metrics *reviewer.RunMetrics) *Handler { tmpl := template.Must(template.New("").Funcs(template.FuncMap{ - "shortKey": shortKey, + "shortKey": reviewer.ShortKey, "preview": preview, "storageURL": func() string { return StoragePathPrefix }, }).ParseFS(templatesFS, "templates/*.html")) @@ -96,7 +96,7 @@ func (h *Handler) Upload(c echo.Context) error { title, err := h.projectTitle(ctx, projectKey) switch { case err != nil: - h.log.ErrorContext(ctx, "debug upload: resolve project", "projectKey", shortKey(projectKey), "err", err) + h.log.ErrorContext(ctx, "debug upload: resolve project", "projectKey", reviewer.ShortKey(projectKey), "err", err) return echo.NewHTTPError(http.StatusServiceUnavailable, "resolve project") case title == "": return echo.NewHTTPError(http.StatusNotFound, "unknown project") @@ -122,9 +122,8 @@ func (h *Handler) Upload(c echo.Context) error { ExternalID: field(FieldExternalID, maxFieldBytes), Runner: field(FieldRunner, maxFieldBytes), Model: field(FieldModel, maxFieldBytes), - // The key authorizes project config (runner-profile and tracker - // tokens): an upload URL quoted in an error must not reveal it. - ErrorMsg: strings.ReplaceAll(field(FieldErrorMsg, maxErrorMsgBytes), projectKey, shortKey(projectKey)+"…"), + // An upload URL quoted in an error must not reveal the key. + ErrorMsg: reviewer.MaskKey(field(FieldErrorMsg, maxErrorMsgBytes), projectKey), SourceBranch: field(FieldSourceBranch, maxFieldBytes), TargetBranch: field(FieldTargetBranch, maxFieldBytes), CommitHash: field(FieldCommitHash, maxFieldBytes), @@ -155,7 +154,7 @@ func (h *Handler) Upload(c echo.Context) error { // on that upload: counting its later failure too would double both. counted := b.Status != reviewer.RunStatusOK && b.ReviewID == "" h.log.Log(ctx, lvl, "debug bundle stored", - "id", b.ID, "projectKey", shortKey(projectKey), "project", b.ProjectTitle, "files", len(b.Files), "hasError", b.ErrorMsg != "", + "id", b.ID, "projectKey", reviewer.ShortKey(projectKey), "project", b.ProjectTitle, "files", len(b.Files), "hasError", b.ErrorMsg != "", "status", b.Status, "reason", b.Reason, "runner", b.Runner, "model", b.Model, "costUsd", b.CostUsd, "reviewId", b.ReviewID, "countedInMetrics", counted, "errorMsg", preview(b.ErrorMsg, 300), ) @@ -294,14 +293,6 @@ func contentTypeFor(name string) string { return "text/plain; charset=utf-8" } -// shortKey returns the first 8 chars of a UUID for compact display. -func shortKey(s string) string { - if len(s) <= 8 { - return s - } - return s[:8] -} - // preview truncates a string to n bytes, on a rune boundary, for the index // error column and the log. func preview(s string, n int) string { diff --git a/pkg/debug/handler_test.go b/pkg/debug/handler_test.go index adf5ae2..2aa69f7 100644 --- a/pkg/debug/handler_test.go +++ b/pkg/debug/handler_test.go @@ -186,6 +186,36 @@ func TestHandler_UploadRejectsMalformedGzip(t *testing.T) { } } +// The decompressed size is checked once, at upload: File streams a stored +// artifact without a limit of its own. +func TestHandler_UploadLimitsDecompressedSize(t *testing.T) { + for _, tt := range []struct { + name string + size int + want int + bundles int + }{ + {"at the limit", maxFileBytes, http.StatusOK, 1}, + {"over the limit", maxFileBytes + 1, http.StatusBadRequest, 0}, + } { + t.Run(tt.name, func(t *testing.T) { + storage, e := newTestHandler(t) + body, ct := buildMultipart(t, nil, map[string][]byte{"runner.log": bytes.Repeat([]byte("x"), tt.size)}) + req := httptest.NewRequest(http.MethodPost, "/v1/upload/debug/"+uuid.NewString()+"/", body) + req.Header.Set("Content-Type", ct) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + if rec.Code != tt.want { + t.Errorf("status = %d, want %d", rec.Code, tt.want) + } + if got := len(storage.List()); got != tt.bundles { + t.Errorf("bundles = %d, want %d", got, tt.bundles) + } + }) + } +} + func TestHandler_ListAndBundleHTML(t *testing.T) { storage, e := newTestHandler(t) storage.Add(&Bundle{ diff --git a/pkg/rest/rest.go b/pkg/rest/rest.go index b8b9b8e..896f656 100644 --- a/pkg/rest/rest.go +++ b/pkg/rest/rest.go @@ -88,8 +88,10 @@ func (h *Handler) CreateReview(c echo.Context) error { return c.String(http.StatusOK, strconv.Itoa(rv.ID)) } +// notifySlack announces a review. A member review is link-only, hidden from +// the lists: its fusion (or the promoted single review) is announced instead. func (h *Handler) notifySlack(project *reviewer.Project, rv *reviewer.Review) { - if h.notifier == nil || !project.HasSlackWebhook() { + if h.notifier == nil || !project.HasSlackWebhook() || rv.ReviewRole == reviewer.ReviewRoleMember { return } diff --git a/pkg/rest/rest_test.go b/pkg/rest/rest_test.go index c16e791..acd8539 100644 --- a/pkg/rest/rest_test.go +++ b/pkg/rest/rest_test.go @@ -6,17 +6,20 @@ import ( "net/http/httptest" "strconv" "strings" + "sync/atomic" "testing" "time" "reviewsrv/pkg/db" dbtest "reviewsrv/pkg/db/test" "reviewsrv/pkg/reviewer" + "reviewsrv/pkg/slack" "github.com/labstack/echo/v4" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/vmkteam/embedlog" ) // newTestHandler bootstraps the test DB and a Handler wired without a Slack @@ -230,3 +233,17 @@ func okRunCounters(t *testing.T, reg *prometheus.Registry, project, runner strin } return runs, cost } + +func TestNotifySlackSkipsMemberReviews(t *testing.T) { + var posts atomic.Int32 + webhook := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { posts.Add(1) })) + t.Cleanup(webhook.Close) + h := &Handler{notifier: slack.NewNotifier(embedlog.NewDevLogger()), baseURL: "http://localhost"} + project := &reviewer.Project{Project: db.Project{Title: "p", SlackChannel: &db.SlackChannel{WebhookURL: webhook.URL}}} + + h.notifySlack(project, &reviewer.Review{Review: db.Review{ID: 1, ReviewRole: reviewer.ReviewRoleMember}}) + h.notifySlack(project, &reviewer.Review{Review: db.Review{ID: 2, ReviewRole: reviewer.ReviewRoleSingle}}) + + require.Eventually(t, func() bool { return posts.Load() == 1 }, 5*time.Second, 10*time.Millisecond) + assert.Never(t, func() bool { return posts.Load() > 1 }, 200*time.Millisecond, 10*time.Millisecond, "a member review is not announced") +} diff --git a/pkg/reviewctl/server.go b/pkg/reviewctl/server.go index d081e42..f940d23 100644 --- a/pkg/reviewctl/server.go +++ b/pkg/reviewctl/server.go @@ -1,9 +1,13 @@ package reviewctl import ( + "context" + "encoding/json" "net/http" + "slices" "reviewsrv/pkg/db" + "reviewsrv/pkg/reviewer" "github.com/vmkteam/embedlog" zm "github.com/vmkteam/zenrpc-middleware" @@ -46,9 +50,10 @@ func New(dbo db.DB, logger embedlog.Logger, isDevel bool) *zenrpc.Server { zm.WithSQLLogger(dbo.DB, isDevel, allowDebugFn(), allowDebugFn()), ) + pf := maskKeys(logger.Print) rpc.Use( - zm.WithSLog(logger.Print, zm.DefaultServerName, nil), - zm.WithErrorSLog(logger.Print, zm.DefaultServerName, nil), + zm.WithSLog(pf, zm.DefaultServerName, nil), + zm.WithErrorSLog(pf, zm.DefaultServerName, nil), ) // services @@ -58,3 +63,20 @@ func New(dbo db.DB, logger embedlog.Logger, isDevel bool) *zenrpc.Server { return rpc } + +// maskKeys wraps the printer of the RPC log middlewares, which log each call's +// raw params: every method here takes the project key. +func maskKeys(pf zm.Print) zm.Print { + return func(ctx context.Context, msg string, args ...any) { + args = slices.Clone(args) + for i := 1; i < len(args); i += 2 { + if args[i-1] != "params" { + continue + } + if p, ok := args[i].(json.RawMessage); ok { + args[i] = json.RawMessage(reviewer.MaskKeys(string(p))) + } + } + pf(ctx, msg, args...) + } +} diff --git a/pkg/reviewctl/server_test.go b/pkg/reviewctl/server_test.go new file mode 100644 index 0000000..06dd32d --- /dev/null +++ b/pkg/reviewctl/server_test.go @@ -0,0 +1,37 @@ +package reviewctl + +import ( + "bytes" + "context" + "encoding/json" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + zm "github.com/vmkteam/zenrpc-middleware" + "github.com/vmkteam/zenrpc/v2" +) + +func TestMaskKeysHidesProjectKeyInLoggedParams(t *testing.T) { + const key = "11111111-2222-3333-4444-555555555555" + for _, params := range []string{ + `{"projectKey":"` + key + `","tokenEnv":true}`, + `["` + key + `",true]`, // positional + } { + var buf bytes.Buffer + pf := maskKeys(slog.New(slog.NewJSONHandler(&buf, nil)).InfoContext) + var handled json.RawMessage + h := zm.WithSLog(pf, zm.DefaultServerName, nil)(func(_ context.Context, _ string, p json.RawMessage) zenrpc.Response { + handled = p + return zenrpc.Response{} + }) + + h(t.Context(), "ReviewConfig", json.RawMessage(params)) + logged := buf.String() + require.NotEmpty(t, logged) + assert.NotContains(t, logged, key) + assert.Contains(t, logged, "11111111…") + assert.JSONEq(t, params, string(handled), "the method gets the real key") + } +} diff --git a/pkg/reviewer/ctl/config.go b/pkg/reviewer/ctl/config.go index 8f6ac7d..a877b46 100644 --- a/pkg/reviewer/ctl/config.go +++ b/pkg/reviewer/ctl/config.go @@ -48,6 +48,9 @@ type Config struct { APIBaseURL string Effort string MaxRounds int // direct runner round budget (--max-rounds / profile); 0 = default + // MaxRoundsSet marks MaxRounds as passed (--max-rounds / REVIEW_MAX_ROUNDS): + // it then overrides every profile, the panel members' and the judge's too. + MaxRoundsSet bool // Resolved runner profile (fetched from the server over /v1/reviewctl/rpc/). // Token is the optional API-key fallback used only when the matching env var diff --git a/pkg/reviewer/ctl/ctl.go b/pkg/reviewer/ctl/ctl.go index 6208c5d..208b9c3 100644 --- a/pkg/reviewer/ctl/ctl.go +++ b/pkg/reviewer/ctl/ctl.go @@ -135,7 +135,7 @@ func (c *Controller) Review(ctx context.Context) (retErr error) { if len(c.cfg.Multi) > 0 { return c.reviewPanel(ctx, start) } - c.log.InfoContext(ctx, "starting review", "projectKey", c.cfg.Key, "runner", c.cfg.Runner, "model", c.cfg.Model) + c.log.InfoContext(ctx, "starting review", "projectKey", reviewer.ShortKey(c.cfg.Key), "runner", c.cfg.Runner, "model", c.cfg.Model) retried := false @@ -176,14 +176,9 @@ func (c *Controller) Review(ctx context.Context) (retErr error) { if isReviewJSONUnfilled(draft) { retried = true - d2 := c.runStep2Recovery(ctx, draft, result) - if d2 == nil { - // Like an empty panel member: an untouched skeleton is no review, so - // the run fails (and ships its debug bundle) instead of uploading it. - return reviewer.WithRunReason(reviewer.RunReasonNotSubmitted, - errors.New("empty review: the runner left review.json unfilled, and the Step 2 retry did not fill it")) + if draft, err = c.runStep2Recovery(ctx, draft, result); err != nil { + return err } - draft = d2 } mdFiles, err := FindMDFiles(c.cfg.Dir) diff --git a/pkg/reviewer/ctl/ctl_test.go b/pkg/reviewer/ctl/ctl_test.go index 583fa9c..da2cc70 100644 --- a/pkg/reviewer/ctl/ctl_test.go +++ b/pkg/reviewer/ctl/ctl_test.go @@ -277,6 +277,37 @@ func TestController_Review_FailsOnUnfilledReview(t *testing.T) { assert.Equal(t, "4.175053", dc.field("costUsd"), "both runs are billed") } +func TestController_Review_KeepsStep2RetryFailureReason(t *testing.T) { + for _, tt := range []struct { + name string + err error + status, reason string + }{ + {"cancelled", context.Canceled, reviewer.RunStatusCancelled, reviewer.RunReasonCancelled}, + {"billing", reviewer.WithRunReason(reviewer.RunReasonBilling, errors.New("credit balance is too low")), reviewer.RunStatusFailed, reviewer.RunReasonBilling}, + } { + t.Run(tt.name, func(t *testing.T) { + srv, dc := newDebugCaptureServer(t, nil) + cfg := &Config{Key: "test-key", URL: srv.URL, Model: "opus", Dir: t.TempDir(), Runner: runner.RunnerClaude} + // The run leaves review.json untouched; the Step 2 retry fails. + var runs int + rr := &testClaudeRunner{fixturePath: "testdata/claude_result.json", beforeRun: func() error { + runs++ + if runs == 2 { + return tt.err + } + return nil + }} + + err := NewController(cfg, rr, slog.Default()).Review(context.Background()) + require.ErrorIs(t, err, tt.err) + assert.Equal(t, tt.status, dc.field("status")) + assert.Equal(t, tt.reason, dc.field("reason")) + assert.Equal(t, "2.0875265", dc.field("costUsd"), "the first run stays billed") + }) + } +} + func TestController_Review_UploadsDebugBundleOnValidationFailure(t *testing.T) { srv, dc := newDebugCaptureServer(t, nil) diff --git a/pkg/reviewer/ctl/panel.go b/pkg/reviewer/ctl/panel.go index 5eb7db8..afc500f 100644 --- a/pkg/reviewer/ctl/panel.go +++ b/pkg/reviewer/ctl/panel.go @@ -63,7 +63,7 @@ func (c *Controller) reviewPanel(ctx context.Context, start time.Time) (err erro if c.cfg.Judge != nil { judge = c.cfg.Judge.String() } - c.log.InfoContext(ctx, "starting panel review", "projectKey", c.cfg.Key, "members", members, "judge", judge) + c.log.InfoContext(ctx, "starting panel review", "projectKey", reviewer.ShortKey(c.cfg.Key), "members", members, "judge", judge) prompt, err := c.prompt.FetchPrompt(ctx, c.cfg.URL, c.cfg.Key) if err != nil { @@ -196,27 +196,26 @@ func (c *Controller) finishPanel(ctx context.Context, base, commit string, outpu } // fuse runs the judge over the staged member outputs and uploads one fused -// review that links the members as children. If the judge fails after a retry, -// it degrades to a single review from the primary member so CI never goes red on -// a judge flap. +// review that links the members as children. If the judge can't run or fails +// after a retry, it degrades to uploadUnfused so CI never goes red on a judge +// flap. func (c *Controller) fuse(ctx context.Context, base, commit string, outputs []*memberOutput) (int, error) { // The judge prompt is a server built-in (served via the reviewctl RPC) so a // future per-project override needs no client change — same as the member prompt. fusionPrompt, err := c.prompt.FetchFusionPrompt(ctx, c.cfg.URL, c.cfg.Key) if err != nil { - return 0, fmt.Errorf("fetch fusion prompt: %w", err) + return c.judgeNotRun(ctx, outputs, fmt.Errorf("fetch fusion prompt: %w", err)) } judgeDir := filepath.Join(base, "wt-judge") if err = c.gitWorktreeAdd(ctx, judgeDir, commit); err != nil { - return 0, fmt.Errorf("judge worktree add: %w", err) + return c.judgeNotRun(ctx, outputs, fmt.Errorf("judge worktree add: %w", err)) } defer c.gitWorktreeRemove(ctx, judgeDir) fusion, err := c.runJudge(ctx, judgeDir, fusionPrompt, outputs) if err != nil { - c.log.ErrorContext(ctx, "judge failed, promoting primary member to single", "err", err) - return c.uploadMember(ctx, outputs[0], reviewer.ReviewRoleSingle) + return c.uploadUnfused(ctx, outputs, err) } // Upload members first to get their ids, then the fusion that links them. @@ -239,6 +238,45 @@ func (c *Controller) fuse(ctx context.Context, base, commit string, outputs []*m return id, nil } +// judgeNotRun reports a judge that could not start as a failed judge run +// (runJudge settles only the runs it starts), then degrades to uploadUnfused. +func (c *Controller) judgeNotRun(ctx context.Context, outputs []*memberOutput, err error) (int, error) { + jc := c.judgeConfig("") // no worktree: the bundle carries the error only + c.settleRun(ctx, &jc, nil, err) + return c.uploadUnfused(ctx, outputs, err) +} + +// uploadUnfused uploads the primary member as a single review and the others +// as member reviews: only an uploaded review reports an ok run, with its cost, +// to the server's run metrics. A cancelled job uploads nothing more. +func (c *Controller) uploadUnfused(ctx context.Context, outputs []*memberOutput, cause error) (int, error) { + if ctx.Err() != nil { + return 0, cause + } + c.log.ErrorContext(ctx, "no fused review, promoting primary member to single", "err", cause) + id, err := c.uploadMember(ctx, outputs[0], reviewer.ReviewRoleSingle) + if err != nil { + return 0, err + } + for _, o := range outputs[1:] { + // The review is out: a lost member upload costs its metrics, not the run. + mid, err := c.uploadMember(ctx, o, reviewer.ReviewRoleMember) + if err != nil { + c.log.WarnContext(ctx, "upload unfused member", "member", o.label, "err", err) + continue + } + c.log.InfoContext(ctx, "unfused member uploaded", "member", o.label, "reviewId", mid) + } + return id, nil +} + +// judgeConfig is the judge's run config, working in dir. +func (c *Controller) judgeConfig(dir string) Config { + jc := c.memberConfig(dir, *c.cfg.Judge) + jc.Label = "judge" + return jc +} + // memberConfig clones the base config for one panel member (or the judge): its own // working dir + runner/model, the panel fields cleared, and — for a server-driven // member — its resolved profile's credentials/settings overlaid (nil profile = the @@ -256,7 +294,9 @@ func (c *Controller) memberConfig(dir string, m MemberSpec) Config { mc.APIProvider = p.APIProvider mc.APIBaseURL = p.APIBaseURL mc.AllowDangerousPermissions = p.Params.AllowDangerousPermissions - mc.MaxRounds = p.Params.MaxRounds + if !mc.MaxRoundsSet { + mc.MaxRounds = p.Params.MaxRounds + } mc.RunnerProfileID = p.RunnerProfileID mc.RunnerProfileTitle = p.Title } @@ -311,8 +351,7 @@ func (c *Controller) produceMember(ctx context.Context, dir, label string, m Mem // worktree, then runs the judge with the fusion prompt (one retry) and returns // the fused draft + R*.md. func (c *Controller) runJudge(ctx context.Context, judgeDir, fusionPrompt string, outputs []*memberOutput) (_ *memberOutput, err error) { - jc := c.memberConfig(judgeDir, *c.cfg.Judge) - jc.Label = "judge" + jc := c.judgeConfig(judgeDir) var rr *spendTracker // The panel survives a failed judge (the primary member is promoted), but // the judge's run and spend must not vanish with its worktree. diff --git a/pkg/reviewer/ctl/panel_test.go b/pkg/reviewer/ctl/panel_test.go index 08e9718..dcab59e 100644 --- a/pkg/reviewer/ctl/panel_test.go +++ b/pkg/reviewer/ctl/panel_test.go @@ -2,6 +2,7 @@ package ctl import ( "context" + "encoding/json" "errors" "log/slog" "net/http" @@ -9,7 +10,9 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" + "sync" "sync/atomic" "testing" "time" @@ -248,6 +251,19 @@ func TestMemberConfigInheritsTracker(t *testing.T) { assert.True(t, mc.AllowDangerousPermissions) } +func TestMemberConfigKeepsPassedMaxRounds(t *testing.T) { + member := MemberSpec{Runner: "direct", Model: "m", Profile: &ResolvedProfile{Params: RunnerProfileParams{MaxRounds: 120}}} + + c := NewController(&Config{MaxRounds: 10, MaxRoundsSet: true}, nil, slog.Default()) + assert.Equal(t, 10, c.memberConfig("/tmp/wt", member).MaxRounds, "--max-rounds overrides the member's profile") + + c = NewController(&Config{MaxRoundsSet: true}, nil, slog.Default()) + assert.Zero(t, c.memberConfig("/tmp/wt", member).MaxRounds, "an explicit 0 (the default) too") + + c = NewController(&Config{MaxRounds: 10}, nil, slog.Default()) + assert.Equal(t, 120, c.memberConfig("/tmp/wt", member).MaxRounds, "the primary profile's budget does not") +} + func TestMemberLabel(t *testing.T) { assert.Equal(t, "1-codex-gpt-5.5", memberLabel(0, MemberSpec{Runner: "codex", Model: "gpt-5.5"})) // duplicates stay distinct by index @@ -294,6 +310,121 @@ func TestRunJudge_FailureShipsDebugBundle(t *testing.T) { assert.Equal(t, runner.RunnerClaude, dc.field("runner")) } +// uploadCapture answers review uploads with sequential ids and records each +// one's role; failAt fails that upload (1-based) with a 500. +type uploadCapture struct { + mu sync.Mutex + roles []string + failAt int +} + +func (u *uploadCapture) handle(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if strings.Count(strings.Trim(r.URL.Path, "/"), "/") != 3 { // an R*.md upload + return + } + var d rest.ReviewDraft + if !assert.NoError(t, json.NewDecoder(r.Body).Decode(&d)) { + return + } + u.mu.Lock() + defer u.mu.Unlock() + if len(u.roles)+1 == u.failAt { + u.failAt = 0 + w.WriteHeader(http.StatusInternalServerError) + return + } + u.roles = append(u.roles, d.Review.ReviewRole) + _, _ = w.Write([]byte(strconv.Itoa(len(u.roles)))) + } +} + +func testOutputs() []*memberOutput { + return []*memberOutput{{label: "a", draft: &rest.ReviewDraft{}}, {label: "b", draft: &rest.ReviewDraft{}}} +} + +func TestFuse_JudgeNotStartedUploadsEveryMember(t *testing.T) { + uc := &uploadCapture{} + srv, dc := newDebugCaptureServer(t, uc.handle(t)) + // Not a repository: the judge worktree can't be created. + cfg := &Config{Key: "k", URL: srv.URL, Dir: t.TempDir(), Judge: &MemberSpec{Runner: runner.RunnerClaude, Model: "opus"}} + + id, err := NewController(cfg, nil, slog.Default()).fuse(t.Context(), t.TempDir(), "HEAD", testOutputs()) + require.NoError(t, err, "a judge failure never fails the panel") + assert.Equal(t, 1, id, "the primary member is the review") + assert.Equal(t, []string{reviewer.ReviewRoleSingle, reviewer.ReviewRoleMember}, uc.roles, "every member run is reported") + assert.Equal(t, 1, dc.count(), "the judge that never ran is reported too") + assert.Equal(t, runner.RunnerClaude, dc.field("runner")) + assert.Contains(t, dc.field("errorMsg"), "judge worktree add") +} + +func TestUploadUnfused(t *testing.T) { + cause := errors.New("judge failed") + for _, tt := range []struct { + name string + failAt int + wantErr bool + roles []string + }{ + {"all uploaded", 0, false, []string{reviewer.ReviewRoleSingle, reviewer.ReviewRoleMember}}, + {"a lost member upload is tolerated", 2, false, []string{reviewer.ReviewRoleSingle}}, + {"a lost primary upload fails the panel", 1, true, nil}, + } { + t.Run(tt.name, func(t *testing.T) { + uc := &uploadCapture{failAt: tt.failAt} + srv := httptest.NewServer(uc.handle(t)) + t.Cleanup(srv.Close) + + _, err := NewController(&Config{Key: "k", URL: srv.URL}, nil, slog.Default()).uploadUnfused(t.Context(), testOutputs(), cause) + assert.Equal(t, tt.wantErr, err != nil, "err = %v", err) + assert.Equal(t, tt.roles, uc.roles) + }) + } + + t.Run("a cancelled job uploads nothing", func(t *testing.T) { + uc := &uploadCapture{} + srv := httptest.NewServer(uc.handle(t)) + t.Cleanup(srv.Close) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + _, err := NewController(&Config{Key: "k", URL: srv.URL}, nil, slog.Default()).uploadUnfused(ctx, testOutputs(), cause) + require.ErrorIs(t, err, cause) + assert.Empty(t, uc.roles) + }) +} + +func TestReviewPanel_JudgeFailureUploadsEveryMember(t *testing.T) { + uc := &uploadCapture{} + srv, dc := newDebugCaptureServer(t, uc.handle(t)) + repo := t.TempDir() + runGit(t, repo, "init", "-q") + require.NoError(t, os.WriteFile(filepath.Join(repo, "file.txt"), []byte("hi"), 0o600)) + runGit(t, repo, "add", ".") + runGit(t, repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-q", "-m", "init") + profile := &ResolvedProfile{Params: RunnerProfileParams{MaxRounds: 120}} + cfg := &Config{Key: "k", URL: srv.URL, Dir: repo, Commit: "HEAD", Runner: runner.RunnerClaude, MaxRounds: 10, MaxRoundsSet: true, + Multi: []MemberSpec{{Runner: runner.RunnerClaude, Model: "a", Profile: profile}, {Runner: runner.RunnerCodex, Model: "b", Profile: profile}}, + Judge: &MemberSpec{Runner: runner.RunnerClaude, Model: "j", Profile: profile}} + var mu sync.Mutex + var maxRounds []int + c := NewController(cfg, nil, slog.Default(), WithRunnerFactory(func(mc *Config) (runner.ReviewRunner, error) { + mu.Lock() + maxRounds = append(maxRounds, mc.MaxRounds) + mu.Unlock() + if mc.Label == "judge" { // leaves the skeleton untouched, on both attempts + return failingRunner{name: runner.RunnerClaude}, nil + } + return &fakeRunner{name: mc.Runner, dir: mc.Dir}, nil + })) + + require.NoError(t, c.Review(t.Context())) + assert.Equal(t, []string{reviewer.ReviewRoleSingle, reviewer.ReviewRoleMember}, uc.roles, "every member run is reported") + assert.Equal(t, 1, dc.count(), "one bundle, the judge's") + assert.Equal(t, reviewer.RunReasonNotSubmitted, dc.field("reason")) + assert.Equal(t, []int{10, 10, 10}, maxRounds, "--max-rounds overrides every member's and the judge's profile") +} + func TestReviewPanel_ShipsPanelLevelFailure(t *testing.T) { srv, dc := newDebugCaptureServer(t, nil) // A checkout that doesn't exist: no worktree, so no member ever runs — the diff --git a/pkg/reviewer/ctl/prompt.go b/pkg/reviewer/ctl/prompt.go index 89499c0..c2a060b 100644 --- a/pkg/reviewer/ctl/prompt.go +++ b/pkg/reviewer/ctl/prompt.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "reviewsrv/pkg/reviewer" "reviewsrv/pkg/reviewer/ctl/reviewctlclient" ) @@ -134,7 +135,7 @@ func (c *PromptClient) FetchConfig(ctx context.Context, serverURL, projectKey st } judging := rc.Judge != nil - c.log.InfoContext(ctx, "fetched review config", "projectKey", projectKey, + c.log.InfoContext(ctx, "fetched review config", "projectKey", reviewer.ShortKey(projectKey), "panelMembers", 1+len(rc.Panel), "judging", judging) return rc, nil } @@ -146,7 +147,7 @@ func (c *PromptClient) FetchFusionPrompt(ctx context.Context, serverURL, project if err != nil { return "", err } - c.log.InfoContext(ctx, "fetched fusion prompt", "projectKey", projectKey, "length", len(prompt)) + c.log.InfoContext(ctx, "fetched fusion prompt", "projectKey", reviewer.ShortKey(projectKey), "length", len(prompt)) return prompt, nil } @@ -161,7 +162,7 @@ func (c *PromptClient) FetchPrompt(ctx context.Context, serverURL, projectKey st if err != nil { return "", err } - c.log.InfoContext(ctx, "fetched prompt", "projectKey", projectKey, "length", len(prompt)) + c.log.InfoContext(ctx, "fetched prompt", "projectKey", reviewer.ShortKey(projectKey), "length", len(prompt)) return prompt, nil } diff --git a/pkg/reviewer/ctl/recovery.go b/pkg/reviewer/ctl/recovery.go index 749cdff..c4b3bc8 100644 --- a/pkg/reviewer/ctl/recovery.go +++ b/pkg/reviewer/ctl/recovery.go @@ -2,6 +2,8 @@ package ctl import ( "context" + "errors" + "fmt" "strings" "reviewsrv/pkg/rest" @@ -27,50 +29,50 @@ func isReviewJSONUnfilled(draft *rest.ReviewDraft) bool { } // runStep2Recovery wraps the retry path: logs the skip, invokes retryStep2, -// records both passes on the recovered draft, and returns it (or nil if the -// retry didn't help, which fails the run). -func (c *Controller) runStep2Recovery(ctx context.Context, draft *rest.ReviewDraft, first *runner.ClaudeResult) *rest.ReviewDraft { +// records both passes on the recovered draft, and returns it. A retry that +// could not fill review.json fails the run as not submitted — unless the retry +// run failed for a reason of its own (cancelled, timeout, billing). +func (c *Controller) runStep2Recovery(ctx context.Context, draft *rest.ReviewDraft, first *runner.ClaudeResult) (*rest.ReviewDraft, error) { c.log.WarnContext(ctx, "review.json appears unfilled — attempting Step 2 retry with session continuation", "files", len(draft.Files), "issues", len(draft.Issues), "sessionId", first.SessionID) - d2 := c.retryStep2(ctx, first.SessionID) - if d2 == nil { - return nil + d2, err := c.retryStep2(ctx, first.SessionID) + if err != nil { + return nil, reviewer.WithRunReason(reviewer.RunReasonNotSubmitted, err) } // The tracker holds both passes, so the record covers the retry too. c.applyRunResult(ctx, d2, c.cfg, c.runner) - if isReviewJSONUnfilled(d2) { - c.log.WarnContext(ctx, "Step 2 retry did not fill review.json") - return nil + return nil, errEmptyReview("the Step 2 retry did not fill it") } c.log.InfoContext(ctx, "Step 2 retry filled review.json", "issues", len(d2.Issues)) - return d2 + return d2, nil } // retryStep2 invokes the runner a second time with a focused "fill review.json" // prompt, resuming the previous session so the cached original prompt isn't -// re-billed. Returns the re-read draft; nil signals "retry could not happen or -// runner failed". -func (c *Controller) retryStep2(ctx context.Context, lastSessionID string) *rest.ReviewDraft { - if lastSessionID == "" { - c.log.WarnContext(ctx, "Step 2 retry skipped: no sessionId from previous run") - return nil - } - if c.runner == nil { - return nil +// re-billed. Returns the re-read draft, or why there is none. +func (c *Controller) retryStep2(ctx context.Context, lastSessionID string) (*rest.ReviewDraft, error) { + if lastSessionID == "" || c.runner == nil { + return nil, errEmptyReview("there is no session to resume for a Step 2 retry") } c.runner.SetSession(lastSessionID) if _, err := c.runner.Run(ctx, reviewer.PromptStep2Retry); err != nil { - c.log.WarnContext(ctx, "Step 2 retry runner failed", "err", err) - return nil + return nil, fmt.Errorf("run %s: step 2 retry: %w", c.runner.Name(), err) } draft, err := ReadReviewJSON(c.cfg.Dir) if err != nil { - c.log.WarnContext(ctx, "Step 2 retry: review.json still unparseable", "err", err) - return nil + return nil, errEmptyReview("the Step 2 retry left it unparseable: " + err.Error()) } - return draft + return draft, nil +} + +// errEmptyReview fails a run whose review.json stayed an untouched skeleton, +// like an empty panel member: that is no review, so the run fails (and ships +// its debug bundle) instead of uploading it. +func errEmptyReview(why string) error { + return reviewer.WithRunReason(reviewer.RunReasonNotSubmitted, + errors.New("empty review: the runner left review.json unfilled, and "+why)) } diff --git a/pkg/reviewer/ctl/skip_detect_test.go b/pkg/reviewer/ctl/skip_detect_test.go index dd52ee5..7cded28 100644 --- a/pkg/reviewer/ctl/skip_detect_test.go +++ b/pkg/reviewer/ctl/skip_detect_test.go @@ -10,6 +10,7 @@ import ( "testing" "reviewsrv/pkg/rest" + "reviewsrv/pkg/reviewer" "reviewsrv/pkg/reviewer/runner" "github.com/stretchr/testify/assert" @@ -120,36 +121,41 @@ func (r *retryStep2RunnerStub) Run(_ context.Context, _ string) (*runner.ClaudeR } func TestRetryStep2(t *testing.T) { - t.Run("empty sessionId returns nil draft and skips runner", func(t *testing.T) { + t.Run("empty sessionId fails as not submitted and skips runner", func(t *testing.T) { stub := &retryStep2RunnerStub{} c := &Controller{ cfg: &Config{Dir: t.TempDir()}, log: slog.Default(), runner: trackSpend(stub), } - draft := c.retryStep2(context.Background(), "") + draft, err := c.retryStep2(context.Background(), "") + require.ErrorContains(t, err, "no session to resume") + _, reason := reviewer.RunOutcome(err) + assert.Equal(t, reviewer.RunReasonNotSubmitted, reason) assert.Nil(t, draft) assert.False(t, stub.runCalled, "runner.Run must not be called when sessionId empty") assert.Empty(t, stub.sessionSet, "SetSession must not be called when sessionId empty") }) - t.Run("nil runner returns nil without panic", func(t *testing.T) { + t.Run("nil runner fails without panic", func(t *testing.T) { c := &Controller{ cfg: &Config{Dir: t.TempDir()}, log: slog.Default(), } - draft := c.retryStep2(context.Background(), "ses_123") + draft, err := c.retryStep2(context.Background(), "ses_123") + require.Error(t, err) assert.Nil(t, draft) }) - t.Run("runner error returns nil draft", func(t *testing.T) { + t.Run("runner error is returned", func(t *testing.T) { stub := &retryStep2RunnerStub{runErr: errors.New("boom")} c := &Controller{ cfg: &Config{Dir: t.TempDir()}, log: slog.Default(), runner: trackSpend(stub), } - draft := c.retryStep2(context.Background(), "ses_123") + draft, err := c.retryStep2(context.Background(), "ses_123") + require.ErrorContains(t, err, "boom") assert.Nil(t, draft) assert.Equal(t, "ses_123", stub.sessionSet, "SetSession must be called before Run") }) @@ -172,7 +178,8 @@ func TestRetryStep2(t *testing.T) { log: slog.Default(), runner: trackSpend(stub), } - draft := c.retryStep2(context.Background(), "ses_abc") + draft, err := c.retryStep2(context.Background(), "ses_abc") + require.NoError(t, err) require.NotNil(t, draft, "expected non-nil draft after successful retry") assert.Equal(t, "ses_abc", stub.sessionSet) require.Len(t, draft.Files, 1) diff --git a/pkg/reviewer/ctl/upload.go b/pkg/reviewer/ctl/upload.go index 854a5f7..a03dc67 100644 --- a/pkg/reviewer/ctl/upload.go +++ b/pkg/reviewer/ctl/upload.go @@ -11,6 +11,7 @@ import ( "log/slog" "mime/multipart" "net/http" + "net/url" "os" "path/filepath" "sort" @@ -46,22 +47,33 @@ func NewUploadClient(log *slog.Logger) *UploadClient { } } +// do sends an upload request, masking the project key in the URL that +// http.Client quotes in a transport error. +func (c *UploadClient) do(req *http.Request, projectKey string) (*http.Response, error) { + resp, err := c.httpClient.Do(req) + var ue *url.Error + if errors.As(err, &ue) { + ue.URL = reviewer.MaskKey(ue.URL, projectKey) + } + return resp, err +} + // UploadReview uploads review.json and returns the reviewId. func (c *UploadClient) UploadReview(ctx context.Context, serverURL, projectKey string, draft *rest.ReviewDraft) (int, error) { - url := fmt.Sprintf("%s/v1/reviewctl/upload/%s/", strings.TrimRight(serverURL, "/"), projectKey) + endpoint := fmt.Sprintf("%s/v1/reviewctl/upload/%s/", strings.TrimRight(serverURL, "/"), projectKey) body, err := json.Marshal(draft) if err != nil { return 0, fmt.Errorf("marshal review draft: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) if err != nil { return 0, fmt.Errorf("create upload request: %w", err) } req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) + resp, err := c.do(req, projectKey) if err != nil { return 0, fmt.Errorf("upload review: %w", err) } @@ -88,15 +100,15 @@ func (c *UploadClient) UploadReview(ctx context.Context, serverURL, projectKey s // UploadFile uploads a single review file (markdown content). func (c *UploadClient) UploadFile(ctx context.Context, serverURL, projectKey string, reviewID int, reviewType string, content []byte) error { - url := fmt.Sprintf("%s/v1/reviewctl/upload/%s/%d/%s/", strings.TrimRight(serverURL, "/"), projectKey, reviewID, reviewType) + endpoint := fmt.Sprintf("%s/v1/reviewctl/upload/%s/%d/%s/", strings.TrimRight(serverURL, "/"), projectKey, reviewID, reviewType) - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(content)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(content)) if err != nil { return fmt.Errorf("create file upload request: %w", err) } req.Header.Set("Content-Type", "application/octet-stream") - resp, err := c.httpClient.Do(req) + resp, err := c.do(req, projectKey) if err != nil { return fmt.Errorf("upload file %s: %w", reviewType, err) } @@ -205,14 +217,14 @@ func (c *UploadClient) UploadDebugBundle(ctx context.Context, serverURL, project return "", fmt.Errorf("build multipart: %w", err) } - url := fmt.Sprintf("%s/v1/upload/debug/%s/", strings.TrimRight(serverURL, "/"), projectKey) - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, body) + endpoint := fmt.Sprintf("%s/v1/upload/debug/%s/", strings.TrimRight(serverURL, "/"), projectKey) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, body) if err != nil { return "", fmt.Errorf("create debug upload request: %w", err) } req.Header.Set("Content-Type", contentType) - resp, err := c.httpClient.Do(req) + resp, err := c.do(req, projectKey) if err != nil { return "", fmt.Errorf("post debug bundle: %w", err) } diff --git a/pkg/reviewer/ctl/upload_test.go b/pkg/reviewer/ctl/upload_test.go index 4b80d92..e7d65e4 100644 --- a/pkg/reviewer/ctl/upload_test.go +++ b/pkg/reviewer/ctl/upload_test.go @@ -87,6 +87,26 @@ func TestUploadReview_ServerError(t *testing.T) { } } +func TestUploadClient_TransportErrorMasksProjectKey(t *testing.T) { + srv := httptest.NewServer(http.NotFoundHandler()) + srv.Close() // connection refused: http.Client quotes the URL in its error + + const key = "11111111-2222-3333-4444-555555555555" + c := NewUploadClient(slog.Default()) + _, err := c.UploadReview(t.Context(), srv.URL, key, testDraft(t)) + require.Error(t, err) + assert.NotContains(t, err.Error(), key) + assert.Contains(t, err.Error(), "/v1/reviewctl/upload/11111111…/") + + err = c.UploadFile(t.Context(), srv.URL, key, 42, "architecture", []byte("x")) + require.Error(t, err) + assert.NotContains(t, err.Error(), key) + + _, err = c.UploadDebugBundle(t.Context(), srv.URL, key, DebugMeta{ErrorMsg: "boom"}, nil) + require.Error(t, err) + assert.NotContains(t, err.Error(), key) +} + func TestReadReviewJSON(t *testing.T) { draft, err := ReadReviewJSON("testdata") require.NoError(t, err) diff --git a/pkg/reviewer/project.go b/pkg/reviewer/project.go index 6456cd6..f87c954 100644 --- a/pkg/reviewer/project.go +++ b/pkg/reviewer/project.go @@ -3,6 +3,7 @@ package reviewer import ( "context" "fmt" + "regexp" "strings" "reviewsrv/pkg/db" @@ -22,6 +23,35 @@ const ( EnvAPIKey = "REVIEW_API_KEY" // the direct runner's provider-agnostic key ) +// ShortKey returns the first 8 chars of a project key, for logs and pages: the +// full key authorizes the project's config (runner-profile and tracker tokens). +func ShortKey(key string) string { + if len(key) <= 8 { + return key + } + return key[:8] +} + +// MaskKey replaces every occurrence of key in s with its ShortKey, e.g. in a +// URL or an error message that quotes one. +func MaskKey(s, key string) string { + if key == "" { + return s + } + return strings.ReplaceAll(s, key, maskedKey(key)) +} + +// keyRe matches a project key (a UUID) in text that may quote one. +var keyRe = regexp.MustCompile(`[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}`) + +// MaskKeys masks every project key in s, for text where the key is not known +// up front (logged RPC params, a Sentry event). +func MaskKeys(s string) string { + return keyRe.ReplaceAllStringFunc(s, maskedKey) +} + +func maskedKey(key string) string { return ShortKey(key) + "…" } + type ProjectManager struct { repo db.ProjectRepo reviewRepo db.ReviewRepo diff --git a/pkg/reviewer/project_test.go b/pkg/reviewer/project_test.go index 0b91199..9d775ef 100644 --- a/pkg/reviewer/project_test.go +++ b/pkg/reviewer/project_test.go @@ -277,3 +277,12 @@ func TestDBProjectManager_Prompt(t *testing.T) { assert.Empty(t, result) }) } + +func TestMaskKey(t *testing.T) { + const key = "11111111-2222-3333-4444-555555555555" + assert.Equal(t, "POST /v1/upload/11111111…/ → /v1/upload/11111111…/42/", MaskKey("POST /v1/upload/"+key+"/ → /v1/upload/"+key+"/42/", key)) + assert.Equal(t, "no key here", MaskKey("no key here", ""), "an empty key masks nothing") + assert.JSONEq(t, `{"projectKey":"11111111…","other":"AAAAAAAA…"}`, + MaskKeys(`{"projectKey":"`+key+`","other":"AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE"}`), "every key, the case aside") + assert.Equal(t, "short", ShortKey("short")) +} diff --git a/pkg/reviewer/runner/claude.go b/pkg/reviewer/runner/claude.go index b1e4be6..512ff62 100644 --- a/pkg/reviewer/runner/claude.go +++ b/pkg/reviewer/runner/claude.go @@ -563,6 +563,7 @@ func runExec(ctx context.Context, log *slog.Logger, binary, dir string, args []s cmd.Dir = dir cmd.Stdin = strings.NewReader(prompt) cmd.Env = reviewer.ChildEnv(extraEnv...) + prepareCLI(cmd) out := &runOutput{} var lw *lineWriter @@ -577,7 +578,7 @@ func runExec(ctx context.Context, log *slog.Logger, binary, dir string, args []s log.InfoContext(ctx, "running "+binary, "dir", dir, "promptLen", len(prompt), "args", args) start := time.Now() - out.err = cmd.Run() + out.err = finishCLI(ctx, log, cmd, cmd.Run()) out.elapsed = time.Since(start) // Killed by the context (runnerTimeout, the caller's deadline or a cancelled // job): keep the cause in the chain so a timeout/cancel is not reported as a @@ -585,8 +586,9 @@ func runExec(ctx context.Context, log *slog.Logger, binary, dir string, args []s if out.err != nil && ctx.Err() != nil { out.err = fmt.Errorf("%w: %w", ctx.Err(), out.err) } - // Stopped by SIGTERM/SIGINT from outside (a cancelled CI job reaches the - // runner before, or instead of, reviewctl itself): not a runner failure. + // Stopped by SIGTERM/SIGINT from outside — the CLI runs in a process group + // of its own, so from whatever signals every process, e.g. a container + // stop on a cancelled CI job: not a runner failure. if isInterrupted(out.err) { out.err = reviewer.WithRunReason(reviewer.RunReasonCancelled, out.err) } diff --git a/pkg/reviewer/runner/claude_failure_test.go b/pkg/reviewer/runner/claude_failure_test.go index 05ee616..8a1cfa9 100644 --- a/pkg/reviewer/runner/claude_failure_test.go +++ b/pkg/reviewer/runner/claude_failure_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "strings" "testing" + "time" "reviewsrv/pkg/reviewer" @@ -107,6 +108,37 @@ func TestRunExecTagsContextCause(t *testing.T) { assert.Equal(t, reviewer.RunStatusCancelled, status) } +// setWaitDelay overrides cliWaitDelay for one test. +func setWaitDelay(t *testing.T, d time.Duration) { + t.Helper() + old := cliWaitDelay + cliWaitDelay = d + t.Cleanup(func() { cliWaitDelay = old }) +} + +func TestRunExecCancelKillsLeftoverChildren(t *testing.T) { + setWaitDelay(t, time.Minute) // only the process-group kill ends the run in time + ctx, cancel := context.WithTimeout(t.Context(), 300*time.Millisecond) + defer cancel() + + start := time.Now() + // The background sleep inherits the CLI's stdout and would hold it open. + out := runExec(ctx, slog.New(slog.DiscardHandler), "sh", t.TempDir(), []string{"-c", "sleep 60 & sleep 60"}, "", nil, nil) + assert.Less(t, time.Since(start), 10*time.Second) + status, _ := reviewer.RunOutcome(out.err) + assert.Equal(t, reviewer.RunStatusTimeout, status) +} + +func TestRunExecDoesNotWaitForLeftoverChild(t *testing.T) { + setWaitDelay(t, 100*time.Millisecond) + + start := time.Now() + out := runExec(t.Context(), slog.New(slog.DiscardHandler), "sh", t.TempDir(), []string{"-c", "sleep 60 & echo done"}, "", nil, nil) + require.NoError(t, out.err, "the CLI itself succeeded") + assert.Equal(t, "done\n", out.stdout.String()) + assert.Less(t, time.Since(start), 10*time.Second) +} + func TestClaudeReason(t *testing.T) { tests := []struct { kind string diff --git a/pkg/reviewer/runner/cli_failure_test.go b/pkg/reviewer/runner/cli_failure_test.go index af68644..2a72126 100644 --- a/pkg/reviewer/runner/cli_failure_test.go +++ b/pkg/reviewer/runner/cli_failure_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "testing" + "time" "reviewsrv/pkg/reviewer" @@ -67,3 +68,13 @@ echo 'AI_APICallError: statusCode: 429' >&2; exit 1`, }) } } + +func TestFetchOpenCodeSessionModelIgnoresLeftoverChild(t *testing.T) { + setWaitDelay(t, 100*time.Millisecond) + // A plugin's background process keeps export's stdout open after it exits. + fakeCLI(t, "opencode", `sleep 60 & printf '{"messages":[{"info":{"model":{"providerID":"p","modelID":"m"}}}]}'`) + + start := time.Now() + assert.Equal(t, "p/m", fetchOpenCodeSessionModel(t.Context(), slog.New(slog.DiscardHandler), t.TempDir(), "ses_1", nil)) + assert.Less(t, time.Since(start), 10*time.Second) +} diff --git a/pkg/reviewer/runner/opencode.go b/pkg/reviewer/runner/opencode.go index 95d9192..bc96b31 100644 --- a/pkg/reviewer/runner/opencode.go +++ b/pkg/reviewer/runner/opencode.go @@ -211,7 +211,7 @@ func (r *ExecOpenCodeRunner) resolveSessionModel(ctx context.Context, cr *Claude if len(cr.ModelUsage) > 0 || cr.SessionID == "" { return } - name := fetchOpenCodeSessionModel(ctx, r.Dir, cr.SessionID, isolation) + name := fetchOpenCodeSessionModel(ctx, r.Log, r.Dir, cr.SessionID, isolation) if name == "" { r.Log.WarnContext(ctx, "opencode session model not resolved", "sessionId", cr.SessionID) return @@ -231,7 +231,7 @@ func (r *ExecOpenCodeRunner) resolveSessionModel(ctx context.Context, cr *Claude // fetchOpenCodeSessionModel queries `opencode export ` to extract // the model used in the session. Returns "" on any failure — caller must // treat the result as optional (it's a best-effort enrichment). -func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string, isolation []string) string { +func fetchOpenCodeSessionModel(ctx context.Context, log *slog.Logger, dir, sessionID string, isolation []string) string { if sessionID == "" { return "" } @@ -245,8 +245,9 @@ func fetchOpenCodeSessionModel(ctx context.Context, dir, sessionID string, isola // plugins, so run it in the dir Run vetted, under the same isolation. cmd.Dir = dir cmd.Env = reviewer.ChildEnv(isolation...) + prepareCLI(cmd) out, err := cmd.Output() - if err != nil { + if err = finishCLI(ctx, log, cmd, err); err != nil { return "" } // `opencode export` prints "Exporting session: \n" before the JSON body. diff --git a/pkg/reviewer/runner/procgroup.go b/pkg/reviewer/runner/procgroup.go new file mode 100644 index 0000000..943987a --- /dev/null +++ b/pkg/reviewer/runner/procgroup.go @@ -0,0 +1,39 @@ +package runner + +import ( + "context" + "errors" + "log/slog" + "os/exec" + "path/filepath" + "time" +) + +// cliWaitDelay bounds how long a runner CLI is awaited once its context is done +// (then it is killed) and how long its output pipes are once it exited. +var cliWaitDelay = 5 * time.Second + +// prepareCLI makes a runner CLI command stoppable as a whole: a cancelled +// context stops its process group — the CLI together with what it spawned (a +// shell tool, a test run) — and a leftover child that still holds the output +// pipes is not awaited past cliWaitDelay. +func prepareCLI(cmd *exec.Cmd) { + setProcessGroup(cmd) + cmd.WaitDelay = cliWaitDelay +} + +// finishCLI kills whatever the CLI left running in its process group (a +// background shell, a dev server) and returns the run's error — nil for +// exec.ErrWaitDelay: the CLI itself succeeded, only a leftover child held its +// pipes open. A process that left the group (setsid) is out of reach. +func finishCLI(ctx context.Context, log *slog.Logger, cmd *exec.Cmd, err error) error { + name := filepath.Base(cmd.Path) + if killProcessGroup(cmd) == nil { + log.WarnContext(ctx, "killed processes "+name+" left running") + } + if errors.Is(err, exec.ErrWaitDelay) { + log.WarnContext(ctx, name+" exited, but a process it left running held its output open") + return nil + } + return err +} diff --git a/pkg/reviewer/runner/procgroup_other.go b/pkg/reviewer/runner/procgroup_other.go new file mode 100644 index 0000000..ace37bf --- /dev/null +++ b/pkg/reviewer/runner/procgroup_other.go @@ -0,0 +1,14 @@ +//go:build !unix + +package runner + +import ( + "os" + "os/exec" +) + +// setProcessGroup is a no-op without Unix process groups: a cancelled context +// kills the runner CLI alone. +func setProcessGroup(*exec.Cmd) {} + +func killProcessGroup(*exec.Cmd) error { return os.ErrProcessDone } diff --git a/pkg/reviewer/runner/procgroup_unix.go b/pkg/reviewer/runner/procgroup_unix.go new file mode 100644 index 0000000..ac36240 --- /dev/null +++ b/pkg/reviewer/runner/procgroup_unix.go @@ -0,0 +1,36 @@ +//go:build unix + +package runner + +import ( + "errors" + "os" + "os/exec" + "syscall" +) + +// setProcessGroup starts cmd in a process group of its own and makes a +// cancelled context stop the whole group with SIGTERM, so the CLI can still +// flush its transcript; finishCLI kills whatever outlives it. +func setProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { return signalGroup(cmd, syscall.SIGTERM) } +} + +// killProcessGroup kills the process group of a started cmd. +func killProcessGroup(cmd *exec.Cmd) error { + return signalGroup(cmd, syscall.SIGKILL) +} + +// signalGroup signals the process group of a started cmd. A group already gone +// is os.ErrProcessDone, which exec.Cmd.Cancel takes for "exited on its own". +func signalGroup(cmd *exec.Cmd, sig syscall.Signal) error { + if cmd.Process == nil { + return os.ErrProcessDone + } + err := syscall.Kill(-cmd.Process.Pid, sig) + if errors.Is(err, syscall.ESRCH) { + return os.ErrProcessDone + } + return err +} diff --git a/pkg/reviewer/runner/procgroup_unix_test.go b/pkg/reviewer/runner/procgroup_unix_test.go new file mode 100644 index 0000000..43fcbcd --- /dev/null +++ b/pkg/reviewer/runner/procgroup_unix_test.go @@ -0,0 +1,33 @@ +//go:build unix + +package runner + +import ( + "log/slog" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRunExecKillsWhatTheCLILeftRunning(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "pid") + // The background sleep lets go of the pipes: only the group kill ends it. + script := "sleep 60 >/dev/null 2>&1 & echo $! >" + pidFile + "; echo done" + out := runExec(t.Context(), slog.New(slog.DiscardHandler), "sh", t.TempDir(), []string{"-c", script}, "", nil, nil) + require.NoError(t, out.err) + + data, err := os.ReadFile(pidFile) + require.NoError(t, err) + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + require.NoError(t, err) + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) }) + assert.Eventually(t, func() bool { return syscall.Kill(pid, 0) != nil }, 5*time.Second, 20*time.Millisecond, + "the leftover process is killed with the run") +}