From 207b0949546700ca95eb7144b88aab3b52835843 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 10:39:00 -0400 Subject: [PATCH 1/7] feat: drive act model from DOTBOT_ACT_MODEL org/repo variable Review already takes its model from the DOTBOT_REVIEW_MODELS variable (first entry = primary reviewer, the rest = the "fight" roster), rendered into .openrouter-review.yml for the run. Act had no equivalent: the act model could only be changed by editing act.model in the consuming repo's committed config. Add the act counterpart: * dotbot-act.yml renders `act:\n model: ` from vars.DOTBOT_ACT_MODEL and passes it through the `config_path` action input. The variable is a REPLACEMENT, not an addition - when set it is authoritative, when unset the in-repo file (or action default) stands. * The generated file is written to $RUNNER_TEMP, NOT the checkout: act mode pushes commits to the PR branch, so a generated file in the worktree could be swept into the agent's commit and overwrite the repo's own config. Review can render in place because review never commits. * Slugs are validated (`[A-Za-z0-9._:/-]+`) so a malformed variable fails the run instead of producing a broken config file. Tests: workflow wiring assertions for both variables, plus model-config tests proving an act-only generated file outside GITHUB_WORKSPACE is loaded and that it wins over the repo's committed act.model. Docs: new "Org/Repo Variables (vars.DOTBOT_*)" README section covering both variables, their precedence, and why act renders outside the checkout. --- .github/workflows/dotbot-act.yml | 41 +++++++++++++++++++++++++++- README.md | 22 +++++++++++++++ tests/test_model_config.py | 47 ++++++++++++++++++++++++++++++++ tests/test_module_coverage.py | 26 ++++++++++++++++++ 4 files changed, 135 insertions(+), 1 deletion(-) diff --git a/.github/workflows/dotbot-act.yml b/.github/workflows/dotbot-act.yml index 70982f8..6c67876 100644 --- a/.github/workflows/dotbot-act.yml +++ b/.github/workflows/dotbot-act.yml @@ -42,11 +42,50 @@ jobs: # .git/config. Pin the action to a trusted ref below instead. ref: ${{ github.event.pull_request.head.sha || format('refs/pull/{0}/head', github.event.issue.number) }} token: ${{ secrets.REPO_ACCESS_TOKEN }} + # DOTBOT_ACT_MODEL (org or repo VARIABLE), when set, OVERRIDES the act + # model that would otherwise come from `act.model` in the in-repo + # .openrouter-review.yml. Like DOTBOT_REVIEW_MODELS on the review side, + # this is a REPLACEMENT of the act block, not an addition: the variable is + # authoritative whenever it is set, so one place (org/repo variables) + # drives the act model for every consuming repo. When it is unset, nothing + # is generated and the in-repo file (or the action default) stands. + # + # Unlike the review workflow, which re-renders .openrouter-review.yml in + # place (review never commits), the generated act config is written + # OUTSIDE the checkout: act mode pushes commits to the PR branch, so a + # generated file sitting in the worktree could be staged by the agent's + # commit and overwrite the repo's own config. Writing it to RUNNER_TEMP + # and passing it via `config_path` leaves the committed config untouched. + - name: Override act model from DOTBOT_ACT_MODEL (when set) + id: act_model + if: ${{ vars.DOTBOT_ACT_MODEL != '' }} + env: + ACT_MODEL: ${{ vars.DOTBOT_ACT_MODEL }} + run: | + set -euo pipefail + model="${ACT_MODEL:-}" + model="${model//[[:space:]]/}" + if [ -z "$model" ]; then + echo "::warning::DOTBOT_ACT_MODEL set but empty — using in-repo act model" + exit 0 + fi + # Model slugs only (letters, digits and . _ : / -). Anything else + # would produce a malformed config file, so fail loudly instead. + if [[ ! "$model" =~ ^[A-Za-z0-9._:/-]+$ ]]; then + echo "::error::DOTBOT_ACT_MODEL '$model' is not a valid OpenRouter model slug" + exit 2 + fi + printf 'act:\n model: %s\n' "$model" > "$RUNNER_TEMP/dotbot-act-model.yml" + echo "config_path=$RUNNER_TEMP/dotbot-act-model.yml" >> "$GITHUB_OUTPUT" + echo "Rendered act model config from DOTBOT_ACT_MODEL:" + cat "$RUNNER_TEMP/dotbot-act-model.yml" - name: dotbot autonomous edits # Trusted main ref, NOT the PR-head checkout. Update this SHA on each # release (matches the v1/latest tag). uses: wezell/openrouter-code-review-action@d68edbb67a3365b53b41d9f05406c084bf338295 with: mode: act + # DOTBOT_ACT_MODEL wins when set; otherwise the in-repo config file. + config_path: ${{ steps.act_model.outputs.config_path || '.openrouter-review.yml' }} openrouter_api_key: ${{ secrets.OPENROUTER_API_KEY }} - debug_level: 1 \ No newline at end of file + debug_level: 1 diff --git a/README.md b/README.md index a8e819b..5a2dc8e 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,28 @@ Each reviewer runs the full pipeline in sequence: Review state (resume threads, SHA-delta scope, cache keys) is isolated per model, so changing the roster won't reuse the wrong review. +### Org/Repo Variables (`vars.DOTBOT_*`) + +The self-hosted workflows read two GitHub **Variables** (org- or repo-level), +so one place can pin the models for every consuming repo without editing its +config file: + +| Variable | Mode | Effect | +|----------|------|--------| +| `DOTBOT_REVIEW_MODELS` | review | Comma-separated roster. First entry becomes the primary `review.model`, the rest become `review.models` ("the fight"). Replaces the in-repo `review:` block for that run. | +| `DOTBOT_ACT_MODEL` | act | Model slug for `/dotbot` edits. Replaces the in-repo `act:` block for that run. | + +Both are optional. With a variable unset, nothing is generated and the in-repo +`.openrouter-review.yml` (or the action default) applies. When set, the variable +is authoritative — a variable set to a malformed slug fails the run instead of +silently falling back. + +The act override is rendered to `$RUNNER_TEMP/dotbot-act-model.yml` and passed +through the `config_path` input rather than written into the checkout: act mode +pushes commits to the PR branch, so a generated file inside the worktree could +otherwise be swept into the agent's commit. Review renders in place, since +review never commits. + Override the file path with the `config_path` action input or `OPENROUTER_REVIEW_CONFIG` env var (e.g. `ci/openrouter-models.yml`). Per-call action inputs (`model:`, `reasoning_effort:`) still win over the file when diff --git a/tests/test_model_config.py b/tests/test_model_config.py index 294c4b6..f25429e 100644 --- a/tests/test_model_config.py +++ b/tests/test_model_config.py @@ -306,3 +306,50 @@ def test_review_models_rejects_empty_list(base_env: Path) -> None: with pytest.raises(ConfigurationError, match="non-empty list"): ReviewConfig.from_args(pr_number=1, mode="review") + + +# --- DOTBOT_ACT_MODEL: generated act config outside the checkout ------------- + + +def test_act_model_from_generated_config_outside_repo( + base_env: Path, + tmp_path_factory: pytest.TempPathFactory, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Mirror the dotbot-act workflow's DOTBOT_ACT_MODEL override. + + The workflow renders ``act:\n model: \n`` to a file in RUNNER_TEMP + (outside GITHUB_WORKSPACE) and passes it as the ``config_path`` action + input, which the action forwards as ``OPENROUTER_REVIEW_CONFIG``. The + variable must win over the repo's committed ``act.model``, and the + generated file must be usable from outside the checkout. + """ + + _write_config(base_env, "act:\n model: anthropic/claude-opus-4.7\n") + + runner_temp = tmp_path_factory.mktemp("runner-temp") + generated = runner_temp / "dotbot-act-model.yml" + generated.write_text("act:\n model: openai/gpt-5.4\n", encoding="utf-8") + monkeypatch.setenv("OPENROUTER_REVIEW_CONFIG", str(generated)) + + config = ReviewConfig.from_args(pr_number=1, mode="act") + + assert config.act_model == "openai/gpt-5.4" + assert config.selected_model == "openai/gpt-5.4" + # Act-only file: review fields fall back to defaults. Act runs never read + # them, and the absolute out-of-checkout path must not break loading. + assert config.review_model == DEFAULT_REVIEW_MODEL + + +def test_act_model_generated_config_without_review_block_is_valid( + tmp_path: Path, +) -> None: + """The generated act-only file is a complete, valid config on its own.""" + + generated = tmp_path / "dotbot-act-model.yml" + generated.write_text("act:\n model: google/gemini-2.5-pro\n", encoding="utf-8") + + cfg = load_model_config(config_path=str(generated)) + + assert cfg.act_model == "google/gemini-2.5-pro" + assert cfg.review_model == DEFAULT_REVIEW_MODEL diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index 03d0976..3e22a27 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -787,6 +787,32 @@ def test_review_action_and_workflow_use_expected_resume_guard_and_model() -> Non assert "wezell/openrouter-code-review-action@" in act_workflow +def test_self_hosted_workflows_drive_models_from_org_repo_variables() -> None: + """Review/act models come from the DOTBOT_* org/repo variables. + + DOTBOT_REVIEW_MODELS is rendered into .openrouter-review.yml for the run + (first entry = primary reviewer). DOTBOT_ACT_MODEL is rendered to a file in + RUNNER_TEMP and passed via `config_path` — never written into the checkout, + because act mode commits to the PR branch. + """ + + review_workflow = Path(".github/workflows/dotbot-review.yml").read_text(encoding="utf-8") + act_workflow = Path(".github/workflows/dotbot-act.yml").read_text(encoding="utf-8") + + assert "if: ${{ vars.DOTBOT_REVIEW_MODELS != '' }}" in review_workflow + assert "REVIEW_MODELS: ${{ vars.DOTBOT_REVIEW_MODELS }}" in review_workflow + + assert "if: ${{ vars.DOTBOT_ACT_MODEL != '' }}" in act_workflow + assert "ACT_MODEL: ${{ vars.DOTBOT_ACT_MODEL }}" in act_workflow + assert 'model="${model//[[:space:]]/}"' in act_workflow + assert "$RUNNER_TEMP/dotbot-act-model.yml" in act_workflow + assert "config_path=$RUNNER_TEMP/dotbot-act-model.yml" in act_workflow + assert ( + "config_path: ${{ steps.act_model.outputs.config_path || '.openrouter-review.yml' }}" + in act_workflow + ) + + def test_edit_workflow_helpers_cover_reply_formatting_and_context_normalization() -> None: truncated = _format_edit_reply("x" * 3605, pushed=False, dry_run=False, changed=True) assert "not pushed" in truncated From 4fadf9f406b14cb458d8cf998a25b93d5a1ff1a2 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:00:22 -0400 Subject: [PATCH 2/7] fix: accept OpenRouter ~latest aliases in DOTBOT_ACT_MODEL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dotCMS org variable carries ~deepseek/deepseek-flash-latest, and the `~` "latest" alias prefix is a normal part of OpenRouter slugs — the first revision of the guard rejected it and would have failed every act run. * Widen the slug guard to [A-Za-z0-9._:/~@+-]. * Emit the rendered value as a single-quoted YAML scalar, so unusual-but-legal slugs survive the round trip. * Add tests/test_dotbot_workflow_model_vars.py: executes the workflow step's real `run:` script under bash and asserts the generated config (parsed back through load_model_config) for ~latest aliases, :free variants, whitespace stripping, the blank-variable fallback, and the non-slug failure path. Assertions on the script's internals in test_module_coverage.py are dropped in favour of this. --- .github/workflows/dotbot-act.yml | 12 ++- README.md | 2 +- tests/test_dotbot_workflow_model_vars.py | 128 +++++++++++++++++++++++ tests/test_module_coverage.py | 3 +- 4 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 tests/test_dotbot_workflow_model_vars.py diff --git a/.github/workflows/dotbot-act.yml b/.github/workflows/dotbot-act.yml index 6c67876..bf96548 100644 --- a/.github/workflows/dotbot-act.yml +++ b/.github/workflows/dotbot-act.yml @@ -69,13 +69,17 @@ jobs: echo "::warning::DOTBOT_ACT_MODEL set but empty — using in-repo act model" exit 0 fi - # Model slugs only (letters, digits and . _ : / -). Anything else - # would produce a malformed config file, so fail loudly instead. - if [[ ! "$model" =~ ^[A-Za-z0-9._:/-]+$ ]]; then + # OpenRouter slugs: letters/digits plus . _ : / - @ + and the `~` + # "latest" alias prefix (e.g. ~deepseek/deepseek-flash-latest, the + # value this repo's org variable actually carries). Anything else is + # not a slug, so fail loudly instead of writing a config the action + # would choke on. + if [[ ! "$model" =~ ^[A-Za-z0-9._:/~@+-]+$ ]]; then echo "::error::DOTBOT_ACT_MODEL '$model' is not a valid OpenRouter model slug" exit 2 fi - printf 'act:\n model: %s\n' "$model" > "$RUNNER_TEMP/dotbot-act-model.yml" + # Single-quoted YAML scalar keeps unusual-but-legal slugs intact. + printf "act:\n model: '%s'\n" "${model//\'/\'\'}" > "$RUNNER_TEMP/dotbot-act-model.yml" echo "config_path=$RUNNER_TEMP/dotbot-act-model.yml" >> "$GITHUB_OUTPUT" echo "Rendered act model config from DOTBOT_ACT_MODEL:" cat "$RUNNER_TEMP/dotbot-act-model.yml" diff --git a/README.md b/README.md index 5a2dc8e..32595e5 100644 --- a/README.md +++ b/README.md @@ -171,7 +171,7 @@ config file: | Variable | Mode | Effect | |----------|------|--------| | `DOTBOT_REVIEW_MODELS` | review | Comma-separated roster. First entry becomes the primary `review.model`, the rest become `review.models` ("the fight"). Replaces the in-repo `review:` block for that run. | -| `DOTBOT_ACT_MODEL` | act | Model slug for `/dotbot` edits. Replaces the in-repo `act:` block for that run. | +| `DOTBOT_ACT_MODEL` | act | Model slug for `/dotbot` edits, including OpenRouter's `~` "latest" aliases (e.g. `~deepseek/deepseek-flash-latest`). Replaces the in-repo `act:` block for that run. | Both are optional. With a variable unset, nothing is generated and the in-repo `.openrouter-review.yml` (or the action default) applies. When set, the variable diff --git a/tests/test_dotbot_workflow_model_vars.py b/tests/test_dotbot_workflow_model_vars.py new file mode 100644 index 0000000..19472d6 --- /dev/null +++ b/tests/test_dotbot_workflow_model_vars.py @@ -0,0 +1,128 @@ +"""Workflow-level tests for the DOTBOT_* org/repo model variables. + +`DOTBOT_ACT_MODEL` is resolved by a shell step in dotbot-act.yml, not by Python +code, so these tests execute that step's real `run:` script with bash and assert +the generated config file the action is then pointed at via `config_path`. + +Covered: + +* a `~`-prefixed OpenRouter "latest" alias slug (the value the dotCMS org + variable actually carries, e.g. ``~deepseek/deepseek-flash-latest``) is + accepted and round-trips through ``load_model_config``; +* surrounding/embedded whitespace is stripped; +* a blank variable falls back to the in-repo file (no `config_path` output); +* a non-slug value fails the run instead of emitting an unparseable config. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml + +from cli.core.model_config import load_model_config + +ACT_WORKFLOW = Path(".github/workflows/dotbot-act.yml") +RENDER_STEP_PREFIX = "Override act model from DOTBOT_ACT_MODEL" + +pytestmark = pytest.mark.skipif(shutil.which("bash") is None, reason="bash not available") + + +def _render_step_script() -> str: + workflow = yaml.safe_load(ACT_WORKFLOW.read_text(encoding="utf-8")) + steps = workflow["jobs"]["act"]["steps"] + for step in steps: + if str(step.get("name", "")).startswith(RENDER_STEP_PREFIX): + assert step["id"] == "act_model" + assert step["if"] == "${{ vars.DOTBOT_ACT_MODEL != '' }}" + assert step["env"]["ACT_MODEL"] == "${{ vars.DOTBOT_ACT_MODEL }}" + return str(step["run"]) + raise AssertionError(f"no step named '{RENDER_STEP_PREFIX} ...' in {ACT_WORKFLOW}") + + +def _run_step( + tmp_path: Path, + act_model: str | None, +) -> tuple[subprocess.CompletedProcess[str], Path, dict[str, str]]: + runner_temp = tmp_path / "runner-temp" + runner_temp.mkdir() + + env = { + "PATH": os.environ.get("PATH", "/usr/bin:/bin"), + "HOME": str(tmp_path), + "RUNNER_TEMP": str(runner_temp), + "GITHUB_OUTPUT": str(tmp_path / "github_output"), + } + if act_model is not None: + env["ACT_MODEL"] = act_model + + result = subprocess.run( # noqa: S603 - fixed, repo-owned script under test + ["bash", "-c", _render_step_script()], + capture_output=True, + text=True, + env=env, + cwd=tmp_path, + check=False, + ) + + generated = runner_temp / "dotbot-act-model.yml" + outputs: dict[str, str] = {} + output_file = tmp_path / "github_output" + if output_file.exists(): + for line in output_file.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + outputs[key] = value + + return result, generated, outputs + + +@pytest.mark.parametrize( + "slug", + [ + "~deepseek/deepseek-flash-latest", + "~z-ai/glm-latest", + "anthropic/claude-opus-4.7", + "google/gemini-2.5-pro:free", + ], +) +def test_render_step_accepts_openrouter_slugs(tmp_path: Path, slug: str) -> None: + result, generated, outputs = _run_step(tmp_path, slug) + + assert result.returncode == 0, result.stderr + cfg = load_model_config(config_path=str(generated)) + assert cfg.act_model == slug + # The action reads this path from the config_path input. + assert outputs["config_path"] == str(generated) + + +def test_render_step_strips_whitespace(tmp_path: Path) -> None: + result, generated, _ = _run_step(tmp_path, " ~deepseek/deepseek-flash-latest \n") + + assert result.returncode == 0, result.stderr + assert load_model_config(config_path=str(generated)).act_model == ( + "~deepseek/deepseek-flash-latest" + ) + + +def test_render_step_falls_back_when_variable_blank(tmp_path: Path) -> None: + """Variable set but whitespace-only: warn, emit no config_path, keep the file.""" + result, generated, outputs = _run_step(tmp_path, " ") + + assert result.returncode == 0 + assert "::warning::" in result.stdout + assert "config_path" not in outputs + assert not generated.exists() + + +def test_render_step_fails_loudly_on_non_slug(tmp_path: Path) -> None: + """A malformed variable must not silently produce a broken config file.""" + result, generated, outputs = _run_step(tmp_path, "evil: [1,2]") + + assert result.returncode == 2 + assert "::error::" in result.stdout + assert "config_path" not in outputs + assert not generated.exists() diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index 3e22a27..d088986 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -804,13 +804,14 @@ def test_self_hosted_workflows_drive_models_from_org_repo_variables() -> None: assert "if: ${{ vars.DOTBOT_ACT_MODEL != '' }}" in act_workflow assert "ACT_MODEL: ${{ vars.DOTBOT_ACT_MODEL }}" in act_workflow - assert 'model="${model//[[:space:]]/}"' in act_workflow assert "$RUNNER_TEMP/dotbot-act-model.yml" in act_workflow assert "config_path=$RUNNER_TEMP/dotbot-act-model.yml" in act_workflow assert ( "config_path: ${{ steps.act_model.outputs.config_path || '.openrouter-review.yml' }}" in act_workflow ) + # The render step's behaviour (slug validation, YAML shape) is exercised + # end-to-end by tests/test_dotbot_workflow_model_vars.py. def test_edit_workflow_helpers_cover_reply_formatting_and_context_normalization() -> None: From 1427f3d066cc1182144c69fc328c679fd370a356 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:29:48 -0400 Subject: [PATCH 3/7] ci: pass github.token as github_approval_token so dotbot can approve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-review workflow passed no `github_approval_token`, so every run ended with "[debug1] All reviewers agree the patch is correct; no approval token configured" and dotbot never approved: PRs needing 1 approving review stalled. This repo has no DOTBOT_GITHUB_USER_PAT (0 repo secrets; the org secret does not exist either), and REPO_ACCESS_TOKEN belongs to the PR author, whose approvals GitHub rejects — which is why the earlier machine-user wiring was dropped in a26dc15. The repo does allow Actions to approve PRs (can_approve_pull_request_reviews: true), so pass `github.token` instead: the approval lands as github-actions[bot] and satisfies the ruleset's 1-approval / last-push-approval requirements. Tradeoff documented in the workflow comment and README: a bot identity cannot approve a PR the bot authored, and a machine-user PAT remains the better option where one exists. --- .github/workflows/dotbot-review.yml | 9 +++++++++ README.md | 6 ++++++ tests/test_module_coverage.py | 3 +++ 3 files changed, 18 insertions(+) diff --git a/.github/workflows/dotbot-review.yml b/.github/workflows/dotbot-review.yml index cf74a2d..e2bb4b4 100644 --- a/.github/workflows/dotbot-review.yml +++ b/.github/workflows/dotbot-review.yml @@ -92,6 +92,15 @@ jobs: mode: review openrouter_api_key: ${{ secrets.OPENROUTER_API_KEY }} model: deepseek/deepseek-v4-pro-0813 + # Auto-approval when every reviewer reports "patch is correct". This + # repo has no DOTBOT_GITHUB_USER_PAT machine-user PAT (see the note + # above), so the workflow token is used: the approval lands as + # github-actions[bot], which the repo permits + # (Settings → Actions → "Allow GitHub Actions to create and approve + # pull requests"). A PAT is still the better choice where one exists + # — it attributes the approval to a named user, and a bot cannot + # approve a PR the bot itself authored. + github_approval_token: ${{ github.token }} reasoning_effort: medium web_search_mode: cached debug_level: 1 diff --git a/README.md b/README.md index 32595e5..c8b7ce0 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,12 @@ submission failures are logged as warnings and never fail the review run. > The PAT needs `pull-requests: write` scope, and its user must differ from > the PR author (GitHub rejects approvals from the PR author). +> +> Where no machine-user PAT is available, `github_approval_token: ${{ github.token }}` +> works too, provided the repo allows it (Settings → Actions → "Allow GitHub +> Actions to create and approve pull requests"). The approval is then attributed +> to `github-actions[bot]` instead of a named user, and the bot cannot approve a +> PR that the bot itself authored. ## Act on `/dotbot` Comments diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index d088986..5181e59 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -785,6 +785,9 @@ def test_review_action_and_workflow_use_expected_resume_guard_and_model() -> Non assert " uses: ./\n" not in act_workflow assert "wezell/openrouter-code-review-action@" in review_workflow assert "wezell/openrouter-code-review-action@" in act_workflow + # Auto-approval when every reviewer agrees: this repo has no machine-user + # PAT, so the workflow token is passed explicitly. + assert "github_approval_token: ${{ github.token }}" in review_workflow def test_self_hosted_workflows_drive_models_from_org_repo_variables() -> None: From 3665e575eaea5afacb84f73836e1e20354a38c9e Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:34:39 -0400 Subject: [PATCH 4/7] fix: bump self-review action pin to v1.2.0 so inputs are not dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinning to d68edbb (the split-review-act era) meant GitHub silently rejected inputs that revision did not declare: ##[warning]Unexpected input(s) 'github_approval_token', valid inputs are [...] So `github_approval_token: ${{ github.token }}` never reached the action and auto-approval still did not fire after the previous commit. Bump both workflows to bbe2345 (v1.2.0 / v1 / latest), which declares the inputs the workflows pass. Also assert in tests that both workflows pin the same released commit — drift between them is how the review side ended up on a pre-approval revision. --- .github/workflows/dotbot-act.yml | 5 +++-- .github/workflows/dotbot-review.yml | 7 ++++++- tests/test_module_coverage.py | 11 +++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/.github/workflows/dotbot-act.yml b/.github/workflows/dotbot-act.yml index bf96548..ff53ed1 100644 --- a/.github/workflows/dotbot-act.yml +++ b/.github/workflows/dotbot-act.yml @@ -85,8 +85,9 @@ jobs: cat "$RUNNER_TEMP/dotbot-act-model.yml" - name: dotbot autonomous edits # Trusted main ref, NOT the PR-head checkout. Update this SHA on each - # release (matches the v1/latest tag). - uses: wezell/openrouter-code-review-action@d68edbb67a3365b53b41d9f05406c084bf338295 + # release (matches the v1/latest tag; kept in lockstep with the review + # workflow). + uses: wezell/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda with: mode: act # DOTBOT_ACT_MODEL wins when set; otherwise the in-repo config file. diff --git a/.github/workflows/dotbot-review.yml b/.github/workflows/dotbot-review.yml index e2bb4b4..f5e18e9 100644 --- a/.github/workflows/dotbot-review.yml +++ b/.github/workflows/dotbot-review.yml @@ -87,7 +87,12 @@ jobs: # Trusted ref, NOT the PR-head checkout. Update this SHA on each # release (matches the v1/latest tag) — enforced by # tests/test_module_coverage.py. - uses: wezell/openrouter-code-review-action@d68edbb67a3365b53b41d9f05406c084bf338295 + # + # bbe2345 = v1.2.0 / v1 / latest. The previous pin (d68edbb, the + # split-review-act era) predated the github_approval_token input, so + # GitHub silently dropped it ("Unexpected input(s)") and auto-approval + # never fired. + uses: wezell/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda with: mode: review openrouter_api_key: ${{ secrets.OPENROUTER_API_KEY }} diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index 5181e59..df032d5 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -785,6 +785,17 @@ def test_review_action_and_workflow_use_expected_resume_guard_and_model() -> Non assert " uses: ./\n" not in act_workflow assert "wezell/openrouter-code-review-action@" in review_workflow assert "wezell/openrouter-code-review-action@" in act_workflow + # Both workflows must pin the SAME released commit, and it must be a release + # that carries every input the workflows pass: a pin predating an input makes + # GitHub silently drop it (that is how github_approval_token was ignored and + # dotbot never approved a PR). Bump this constant with the pins on release. + expected_action_pin = "bbe2345626d658ba630921faa6be8166cd421bda" + pins = { + line.split("wezell/openrouter-code-review-action@", 1)[1].strip() + for line in (review_workflow + act_workflow).splitlines() + if "wezell/openrouter-code-review-action@" in line + } + assert pins == {expected_action_pin}, f"unexpected action pins: {sorted(pins)}" # Auto-approval when every reviewer agrees: this repo has no machine-user # PAT, so the workflow token is passed explicitly. assert "github_approval_token: ${{ github.token }}" in review_workflow From 863820f5083dc04bb3f7ea6d8ca1a3aec26c8fff Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:36:42 -0400 Subject: [PATCH 5/7] fix: pin self-review workflows to the canonical repo's v1.2.0 wezell/openrouter-code-review-action is no longer the source of releases: its main/v1/latest sit at 34ee169 (v1.0.0-era, no github_approval_token input), and it has no release carrying that input. So after the previous two commits the run died earlier still: ##[error]Unable to resolve action `wezell/openrouter-code-review-action@bbe2345...` This repo is canonical and cuts its own releases (auto-release.yml), so point both workflows at dotCMS/openrouter-code-review-action@bbe2345... (v1.2.0 / v1 / latest), which declares every input the workflows pass, and require the canonical owner in tests. Supersedes the still-open PR #4, which pinned the review workflow to v1.1.0 and left act on wezell. --- .github/workflows/dotbot-act.yml | 8 ++++---- .github/workflows/dotbot-review.yml | 12 +++++++----- tests/test_module_coverage.py | 11 +++++++---- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/.github/workflows/dotbot-act.yml b/.github/workflows/dotbot-act.yml index ff53ed1..6074663 100644 --- a/.github/workflows/dotbot-act.yml +++ b/.github/workflows/dotbot-act.yml @@ -84,10 +84,10 @@ jobs: echo "Rendered act model config from DOTBOT_ACT_MODEL:" cat "$RUNNER_TEMP/dotbot-act-model.yml" - name: dotbot autonomous edits - # Trusted main ref, NOT the PR-head checkout. Update this SHA on each - # release (matches the v1/latest tag; kept in lockstep with the review - # workflow). - uses: wezell/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda + # Trusted main ref, NOT the PR-head checkout. This repo is the canonical + # home, so the pin points at itself (kept in lockstep with the review + # workflow). bbe2345 = v1.2.0 / v1 / latest. + uses: dotCMS/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda with: mode: act # DOTBOT_ACT_MODEL wins when set; otherwise the in-repo config file. diff --git a/.github/workflows/dotbot-review.yml b/.github/workflows/dotbot-review.yml index f5e18e9..0d6d03e 100644 --- a/.github/workflows/dotbot-review.yml +++ b/.github/workflows/dotbot-review.yml @@ -88,11 +88,13 @@ jobs: # release (matches the v1/latest tag) — enforced by # tests/test_module_coverage.py. # - # bbe2345 = v1.2.0 / v1 / latest. The previous pin (d68edbb, the - # split-review-act era) predated the github_approval_token input, so - # GitHub silently dropped it ("Unexpected input(s)") and auto-approval - # never fired. - uses: wezell/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda + # This repo is the canonical home (releases are cut here by + # auto-release.yml), so the pin points at itself: the upstream + # wezell/openrouter-code-review-action has no release carrying the + # github_approval_token input, and the old wezell@d68edbb pin predated it, + # so GitHub silently dropped the input and auto-approval never fired. + # bbe2345 = v1.2.0 / v1 / latest. + uses: dotCMS/openrouter-code-review-action@bbe2345626d658ba630921faa6be8166cd421bda with: mode: review openrouter_api_key: ${{ secrets.OPENROUTER_API_KEY }} diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index df032d5..534b8ad 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -783,17 +783,20 @@ def test_review_action_and_workflow_use_expected_resume_guard_and_model() -> Non # finding on dotbot-act.yml). assert " uses: ./\n" not in review_workflow assert " uses: ./\n" not in act_workflow - assert "wezell/openrouter-code-review-action@" in review_workflow - assert "wezell/openrouter-code-review-action@" in act_workflow + # This repo is the canonical home for the action, so the trusted pin points + # at itself — not at the upstream wezell/ mirror, whose newest release + # predates the github_approval_token input. + assert "dotCMS/openrouter-code-review-action@" in review_workflow + assert "dotCMS/openrouter-code-review-action@" in act_workflow # Both workflows must pin the SAME released commit, and it must be a release # that carries every input the workflows pass: a pin predating an input makes # GitHub silently drop it (that is how github_approval_token was ignored and # dotbot never approved a PR). Bump this constant with the pins on release. expected_action_pin = "bbe2345626d658ba630921faa6be8166cd421bda" pins = { - line.split("wezell/openrouter-code-review-action@", 1)[1].strip() + line.split("dotCMS/openrouter-code-review-action@", 1)[1].strip() for line in (review_workflow + act_workflow).splitlines() - if "wezell/openrouter-code-review-action@" in line + if "dotCMS/openrouter-code-review-action@" in line } assert pins == {expected_action_pin}, f"unexpected action pins: {sorted(pins)}" # Auto-approval when every reviewer agrees: this repo has no machine-user From b6ed73831785611603fbfb96fab3fa67c57796e5 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:40:53 -0400 Subject: [PATCH 6/7] fix: let an installation token submit the approval review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the input now wired, the run got as far as: Warning: failed to submit PR approval: failed to resolve approval token user: Resource not accessible by integration: 403 `submit_pr_approval` resolved the token's login via `GET /user` before doing anything else. Installation tokens (`${{ github.token }}`) cannot call that endpoint, so the approval was abandoned — the input reached the action and was still useless. - `_resolve_login` is now best-effort: an unresolvable token logs a debug line and continues with an empty login instead of raising. - Idempotency is keyed off APPROVAL_MARKER ("approved automatically by dotbot") in the review body in addition to the login, so re-runs skip duplicate approvals even when the approver identity cannot be read. Extracted the APPROVAL_MARKER constant so the body template and the check cannot drift. - Failure to load the PR still raises (unchanged); the approval POST failure message names the token as an installation token when the login is unknown, and the workflow's log lines omit "as " rather than printing an empty name. Also documents the choice in action.yml, README, and the `_maybe_approve_pr` docstring. --- README.md | 26 +++--- action.yml | 11 ++- cli/review/approval.py | 96 +++++++++++++++------- cli/workflows/review_workflow.py | 23 +++--- tests/test_approval.py | 136 +++++++++++++++++++++++++++++-- 5 files changed, 229 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index c8b7ce0..3af536b 100644 --- a/README.md +++ b/README.md @@ -39,14 +39,14 @@ jobs: ### Automatic PR Approval -If the `DOTBOT_GITHUB_USER_PAT` secret is set (passed via the -`github_approval_token` input), the action approves the PR **as the PAT's -user** — e.g. `dotCMS-Machine-User` — whenever *every* reviewer model in the -roster concludes `Overall: patch is correct`. The approval is idempotent per -head commit: re-running the workflow on the same SHA will not spam duplicate -approvals. - -If the secret is not set, or any reviewer model reports a finding, the action +If a token is passed via the `github_approval_token` input — normally the +`DOTBOT_GITHUB_USER_PAT` secret owned by a machine user such as +`dotCMS-Machine-User` — the action approves the PR **as that user** whenever +*every* reviewer model in the roster concludes `Overall: patch is correct`. The +approval is idempotent per head commit: re-running the workflow on the same SHA +will not spam duplicate approvals. + +If the input is unset, or any reviewer model reports a finding, the action simply posts its review comments and skips the approval step. Approval submission failures are logged as warnings and never fail the review run. @@ -55,9 +55,11 @@ submission failures are logged as warnings and never fail the review run. > > Where no machine-user PAT is available, `github_approval_token: ${{ github.token }}` > works too, provided the repo allows it (Settings → Actions → "Allow GitHub -> Actions to create and approve pull requests"). The approval is then attributed -> to `github-actions[bot]` instead of a named user, and the bot cannot approve a -> PR that the bot itself authored. +> Actions to create and approve pull requests"). Installation tokens cannot read +> `GET /user`, so the action does not try to resolve an identity for them: the +> approval is submitted as `github-actions[bot]` and idempotency falls back to a +> marker in the review body. A bot cannot approve a PR the bot itself authored, +> so a PAT remains the better choice where one exists. ## Act on `/dotbot` Comments @@ -212,7 +214,7 @@ call time; `cached` and `disabled` skip the live web fetch. | `model_timeout_seconds` | Wall-clock budget per reviewer model pass; 0 disables | `900` | | `web_search_mode` | `disabled` / `cached` / `live` | `live` | | **Review-only** | | | -| `github_approval_token` | GitHub user PAT (e.g. `secrets.DOTBOT_GITHUB_USER_PAT`) owned by a machine user such as `dotCMS-Machine-User`. When every reviewer model reports `Overall: patch is correct`, dotbot approves the PR as that user; when unset (or any model dissents) reviews post as normal comments with no approval | *(unset)* | +| `github_approval_token` | Token used to approve the PR when every reviewer model reports `Overall: patch is correct`. Normally a machine-user PAT (e.g. `secrets.DOTBOT_GITHUB_USER_PAT` for `dotCMS-Machine-User`); `${{ github.token }}` also works and approves as `github-actions[bot]`. When unset (or any model dissents) reviews post as normal comments with no approval | *(unset)* | | `additional_prompt` | Extra reviewer instructions (verbatim) | | | `resolve_stale_threads` | `0` or `1` — resolve prior unresolved dotbot threads the reviewer model saw but declined to carry forward (judged stale/fixed); threads never shown to the model stay open | `0` | | **Act-only** | | | diff --git a/action.yml b/action.yml index 2bab1a3..0235d1e 100644 --- a/action.yml +++ b/action.yml @@ -18,10 +18,13 @@ inputs: required: false github_approval_token: description: >- - GitHub user PAT owned by the machine user (e.g. dotCMS-Machine-User). - When every reviewer model reports "Overall: patch is correct", dotbot - approves the PR as that user. Leave empty (secret DOTBOT_GITHUB_USER_PAT - unset) to disable auto-approval and just post review comments normally. + Token used to submit the approval review when every reviewer model reports + "Overall: patch is correct". Usually a GitHub user PAT owned by a machine + user (e.g. dotCMS-Machine-User, secret DOTBOT_GITHUB_USER_PAT) so the + approval is attributed to a named user; `${{ github.token }}` also works, + in which case the approval shows as github-actions[bot] (the repo must + allow Actions to approve pull requests). Leave empty to disable + auto-approval and just post review comments normally. required: false default: "" config_path: diff --git a/cli/review/approval.py b/cli/review/approval.py index aae9bbe..7412820 100644 --- a/cli/review/approval.py +++ b/cli/review/approval.py @@ -2,27 +2,35 @@ The reviewer workflow runs a roster of LLM reviewers over a PR. When every reviewer in the roster concludes ``Overall: patch is correct``, the workflow -approves the PR on their behalf using a dedicated GitHub user token — so the -approval shows up as coming from the machine user, not from the action's -default ``GITHUB_TOKEN`` identity. - -The approval is idempotent per head SHA: if the machine user has already -approved the current head commit, we skip re-submitting so re-triggered runs -do not spam duplicate approvals. +approves the PR on their behalf. The token that submits the approval decides the +identity it appears under: a machine-user PAT shows a named user (e.g. +dotCMS-Machine-User), while ``${{ github.token }}`` shows ``github-actions[bot]``. + +Installation tokens cannot call ``GET /user`` (GitHub answers 403 "Resource not +accessible by integration"), so the approver identity is resolved best-effort: +when it is unknown, idempotency is keyed off the marker in the review body +instead of the login. + +The approval is idempotent per head SHA: if this action has already approved the +current head commit, we skip re-submitting so re-triggered runs do not spam +duplicate approvals. """ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass from github import Github +from github.PullRequestReview import PullRequestReview from ..core.exceptions import GitHubAPIError +APPROVAL_MARKER = "approved automatically by dotbot" + APPROVAL_BODY_TEMPLATE = ( "✅ dotbot review: all reviewer models ({models}) agree — **patch is correct**.\n\n" - "approved automatically by dotbot" + f"{APPROVAL_MARKER}" ) @@ -40,6 +48,42 @@ def build_approval_body(models: list[str]) -> str: return APPROVAL_BODY_TEMPLATE.format(models=", ".join(models)) +def _resolve_login(gh: Github, debug: Callable[[int, str], None]) -> str: + """Return the approval token's user login, or ``""`` when it cannot be read. + + Installation tokens (``${{ github.token }}``) get a 403 from ``GET /user``. + The login is only used for logging and for the login-based idempotency + shortcut, so this is best-effort: an unresolvable token still submits the + approval, deduplicated by :data:`APPROVAL_MARKER` in the review body. + """ + try: + return gh.get_user().login + except Exception: # noqa: BLE001 — an installation token cannot read /user + debug( + 1, + "Approval token cannot resolve its own user (installation token); " + "falling back to the review body marker for idempotency", + ) + return "" + + +def _is_our_approval(review: PullRequestReview, *, login: str, head_sha: str) -> bool: + """True when ``review`` is an existing approval of ``head_sha`` that we made. + + Matches the resolved login, or — when the token cannot report a login (the + installation-token case) — our own :data:`APPROVAL_MARKER` in the body. + """ + if review.state != "APPROVED" or (review.commit_id or "") != head_sha: + return False + author = review.user.login if review.user is not None else None + return bool(login and author == login) or APPROVAL_MARKER in (review.body or "") + + +def _approval_exists(reviews: Iterable[PullRequestReview], *, login: str, head_sha: str) -> bool: + """True when an approval for ``head_sha`` came from us already.""" + return any(_is_our_approval(review, login=login, head_sha=head_sha) for review in reviews) + + def submit_pr_approval( *, repository: str, @@ -51,20 +95,18 @@ def submit_pr_approval( ) -> ApprovalOutcome: """Submit an ``APPROVE`` review on the PR as the token's user. - Uses a dedicated ``Github`` client authenticated with ``token`` so the - review is attributed to the machine user (e.g. dotCMS-Machine-User) - rather than the default action token. Skips submission when that user - has already approved this exact head commit. + Uses a dedicated ``Github`` client authenticated with ``token`` so the review + is attributed to that token rather than the action's default credentials — + a machine-user PAT, or ``github.token`` for a ``github-actions[bot]`` + approval. Skips submission when this token has already approved this exact + head commit. Raises: GitHubAPIError: if any GitHub API call fails. """ gh = Github(login_or_token=token, per_page=100) - try: - login = gh.get_user().login - except Exception as exc: - raise GitHubAPIError(f"failed to resolve approval token user: {exc}") from exc + login = _resolve_login(gh, debug) try: pr = gh.get_repo(repository).get_pull(pr_number) @@ -74,15 +116,9 @@ def submit_pr_approval( ) from exc try: - for review in pr.get_reviews(): - review_author = review.user.login if review.user is not None else None - if ( - review_author == login - and review.state == "APPROVED" - and (review.commit_id or "") == head_sha - ): - debug(1, f"Approval already submitted by {login} for {head_sha}; skipping") - return ApprovalOutcome(submitted=False, login=login, reason="already_approved") + if _approval_exists(pr.get_reviews(), login=login, head_sha=head_sha): + debug(1, f"Approval already submitted for {head_sha}; skipping") + return ApprovalOutcome(submitted=False, login=login, reason="already_approved") except Exception as exc: raise GitHubAPIError(f"failed to list reviews on {repository}#{pr_number}: {exc}") from exc @@ -95,8 +131,12 @@ def submit_pr_approval( pr._requester.requestJsonAndCheck("POST", f"{pr.url}/reviews", input=payload) except Exception as exc: raise GitHubAPIError( - f"failed to submit approval review on {repository}#{pr_number} as {login}: {exc}" + f"failed to submit approval review on {repository}#{pr_number} as " + f"{login or 'installation token'}: {exc}" ) from exc - debug(1, f"Submitted APPROVE review on {repository}#{pr_number} as {login}") + debug( + 1, + f"Submitted APPROVE review on {repository}#{pr_number} as {login or 'installation token'}", + ) return ApprovalOutcome(submitted=True, login=login) diff --git a/cli/workflows/review_workflow.py b/cli/workflows/review_workflow.py index ce683f4..1c36f83 100644 --- a/cli/workflows/review_workflow.py +++ b/cli/workflows/review_workflow.py @@ -1042,13 +1042,14 @@ def _maybe_approve_pr( roster: tuple[str, ...], head_sha: str, ) -> None: - """Approve the PR as the machine user when every reviewer agrees it is correct. - - Requires ``github_approval_token`` (a PAT owned by the machine user, - e.g. dotCMS-Machine-User). Only fires when *every* model in the - roster reported ``Overall: patch is correct`` — a single dissent - (or any active finding) withholds the approval. Failures to submit - are logged as warnings; they never fail the review run itself. + """Approve the PR as the approving token's user when reviewers agree it is correct. + + Requires ``github_approval_token`` — a machine-user PAT, or + ``${{ github.token }}`` for a github-actions[bot] approval. Only fires + when *every* model in the roster reported ``Overall: patch is + correct`` — a single dissent (or any active finding) withholds the + approval. Failures to submit are logged as warnings; they never fail + the review run itself. """ unanimous = bool(results) and all( result.summary.overall_correctness.strip().casefold() @@ -1092,11 +1093,11 @@ def _maybe_approve_pr( print(f"Warning: failed to submit PR approval: {exc}", file=sys.stderr) return if outcome.submitted: - print( - f"Approved PR #{pr.number} as {outcome.login}: all reviewers agree the patch is correct" - ) + approver = f" as {outcome.login}" if outcome.login else "" + print(f"Approved PR #{pr.number}{approver}: all reviewers agree the patch is correct") elif outcome.reason == "already_approved": - print(f"PR #{pr.number} already approved by {outcome.login} for {head_sha}") + approver = f" by {outcome.login}" if outcome.login else "" + print(f"PR #{pr.number} already approved{approver} for {head_sha}") def _refresh_context_artifacts( self, diff --git a/tests/test_approval.py b/tests/test_approval.py index dc9d57d..a3eee7c 100644 --- a/tests/test_approval.py +++ b/tests/test_approval.py @@ -6,7 +6,13 @@ import pytest from cli.core.exceptions import GitHubAPIError -from cli.review.approval import ApprovalOutcome, build_approval_body, submit_pr_approval +from cli.review.approval import ( + APPROVAL_BODY_TEMPLATE, + APPROVAL_MARKER, + ApprovalOutcome, + build_approval_body, + submit_pr_approval, +) def _debug_spy() -> tuple[list[tuple[int, str]], Any]: @@ -33,10 +39,18 @@ def requestJsonAndCheck( class _FakeReview: - def __init__(self, *, login: str, state: str, commit_id: str) -> None: - self.user = SimpleNamespace(login=login) + def __init__( + self, + *, + login: str | None, + state: str, + commit_id: str, + body: str | None = None, + ) -> None: + self.user = SimpleNamespace(login=login) if login is not None else None self.state = state self.commit_id = commit_id + self.body = body class _FakeApprovalPR: @@ -179,15 +193,15 @@ def test_submit_pr_approval_submits_when_prior_approval_is_for_older_sha( assert len(pr._requester.requests) == 1 -def test_submit_pr_approval_wraps_api_failure(monkeypatch: pytest.MonkeyPatch) -> None: - class _BrokenGithub(_FakeGithub): - def get_user(self) -> _FakeApprovalUser: +def test_submit_pr_approval_wraps_pr_load_failure(monkeypatch: pytest.MonkeyPatch) -> None: + class _NoPRGithub(_FakeGithub): + def get_repo(self, repository: str) -> _FakeApprovalRepo: raise RuntimeError("boom") - _patch_github(monkeypatch, _BrokenGithub()) + _patch_github(monkeypatch, _NoPRGithub()) _, debug = _debug_spy() - with pytest.raises(GitHubAPIError, match="boom"): + with pytest.raises(GitHubAPIError, match="failed to load owner/repo#7 for approval: boom"): submit_pr_approval( repository="owner/repo", pr_number=7, @@ -198,8 +212,114 @@ def get_user(self) -> _FakeApprovalUser: ) +def test_submit_pr_approval_tolerates_token_that_cannot_read_its_user( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Installation tokens (${{ github.token }}) get 403 from GET /user. + + The approval must still be submitted: the identity is unknowable by design + for the workflow token, not a configuration error. + """ + + class _InstallationTokenGithub(_FakeGithub): + def get_user(self) -> _FakeApprovalUser: + raise RuntimeError("Resource not accessible by integration: 403") + + pr = _FakeApprovalPR(reviews=[]) + gh = _InstallationTokenGithub(repo=_FakeApprovalRepo(pr)) + _patch_github(monkeypatch, gh) + debug_messages, debug = _debug_spy() + + outcome = submit_pr_approval( + repository="owner/repo", + pr_number=7, + head_sha="abc123", + token="ghs_installation_token", + body="LGTM", + debug=debug, + ) + + assert outcome == ApprovalOutcome(submitted=True, login="") + assert len(pr._requester.requests) == 1 + assert any("installation token" in message for _, message in debug_messages) + + +def test_submit_pr_approval_dedupes_by_body_marker_when_login_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Without a login, our own marker in an approval body keeps re-runs idempotent.""" + + class _InstallationTokenGithub(_FakeGithub): + def get_user(self) -> _FakeApprovalUser: + raise RuntimeError("Resource not accessible by integration: 403") + + pr = _FakeApprovalPR( + reviews=[ + _FakeReview( + login="github-actions[bot]", + state="APPROVED", + commit_id="abc123", + body=APPROVAL_BODY_TEMPLATE.format(models="openai/gpt-5"), + ) + ] + ) + gh = _InstallationTokenGithub(repo=_FakeApprovalRepo(pr)) + _patch_github(monkeypatch, gh) + _, debug = _debug_spy() + + outcome = submit_pr_approval( + repository="owner/repo", + pr_number=7, + head_sha="abc123", + token="ghs_installation_token", + body="LGTM", + debug=debug, + ) + + assert outcome == ApprovalOutcome(submitted=False, login="", reason="already_approved") + assert pr._requester.requests == [] + + +def test_submit_pr_approval_ignores_unmarked_approval_when_login_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Another actor's approval must not suppress ours when we can't match logins.""" + + class _InstallationTokenGithub(_FakeGithub): + def get_user(self) -> _FakeApprovalUser: + raise RuntimeError("Resource not accessible by integration: 403") + + pr = _FakeApprovalPR( + reviews=[ + _FakeReview( + login="some-human", + state="APPROVED", + commit_id="abc123", + body="looks good to me", + ) + ] + ) + gh = _InstallationTokenGithub(repo=_FakeApprovalRepo(pr)) + _patch_github(monkeypatch, gh) + _, debug = _debug_spy() + + outcome = submit_pr_approval( + repository="owner/repo", + pr_number=7, + head_sha="abc123", + token="ghs_installation_token", + body="LGTM", + debug=debug, + ) + + assert outcome.submitted is True + assert len(pr._requester.requests) == 1 + + def test_build_approval_body_lists_models() -> None: body = build_approval_body(["openai/gpt-5", "google/gemini-2.5-pro"]) assert "patch is correct" in body assert "openai/gpt-5" in body assert "google/gemini-2.5-pro" in body + # The body marker is the idempotency key for tokens whose login is unknown. + assert APPROVAL_MARKER in body From 5bd98e4f2052b2087cd96bd0deb02b4d91205a73 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 29 Sep 2026 11:45:30 -0400 Subject: [PATCH 7/7] fix!: stop the checkout from shadowing the pinned action's code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Composite `run` steps execute in the caller's workspace, and `python -m` puts the current directory ahead of PYTHONPATH on sys.path. So `python3 -u -m cli.main` loaded `cli/` from the *checkout* whenever the consuming repo shipped one — for this repo's own self-review, the PR head's code ran with the workflow token instead of the pinned action. Demonstrated on PR #7: the run reported "Approved PR #7: all reviewers agree the patch is correct" (new message format from that very PR), and it submitted an approval the pinned revision cannot submit (its approval.py aborts on GET /user with 403). For the act workflow this is the escalation the trusted-ref pin exists to prevent: a /dotbot comment on an untrusted PR head would execute that PR's `cli/` package with `contents: write` and REPO_ACCESS_TOKEN. Set PYTHONSAFEPATH=1 on both CLI-invoking steps (3.11+: do not prepend cwd to sys.path), and add a test that every CLI step in action.yml carries it. Repro: mkdir -p action/cli work/cli; ... # both define cli.main cd work && PYTHONPATH=…/action python3 -m cli.main # -> ran from: work cd work && PYTHONSAFEPATH=1 PYTHONPATH=…/action python3 -m cli.main # -> action --- action.yml | 11 +++++++++++ tests/test_module_coverage.py | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/action.yml b/action.yml index 0235d1e..209383e 100644 --- a/action.yml +++ b/action.yml @@ -127,6 +127,11 @@ runs: DOTBOT_MODEL_INPUT: ${{ inputs.model }} OPENROUTER_REVIEW_CONFIG: ${{ inputs.config_path }} GITHUB_ACTION_PATH: ${{ github.action_path }} + # Composite steps run in the *caller's* workspace, and `python -m` + # puts the current directory ahead of PYTHONPATH on sys.path — so a + # checkout containing a `cli/` package would execute that code + # instead of this pinned action. PYTHONSAFEPATH stops the injection. + PYTHONSAFEPATH: "1" run: | set -euo pipefail PYTHONPATH="${GITHUB_ACTION_PATH}:${PYTHONPATH:-}" \ @@ -183,6 +188,12 @@ runs: # deltas reach the GitHub Actions live log as soon as the # OpenRouter SSE stream emits them. PYTHONUNBUFFERED: "1" + # See the note on the resume-state step: without this, a checkout + # containing its own `cli/` package shadows the pinned action's code + # (`python -m` prefers the cwd). For act mode that means running + # PR-head code with a write token — the exact escalation the trusted + # ref pin exists to prevent. + PYTHONSAFEPATH: "1" # Conversation threads for the OpenRouter agent loop live here so # the cache steps can persist them across runs for resume. OPENROUTER_THREAD_DIR: ${{ runner.temp }}/openrouter-review-threads diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py index 534b8ad..5eb807b 100644 --- a/tests/test_module_coverage.py +++ b/tests/test_module_coverage.py @@ -5,6 +5,7 @@ from typing import Any, cast import pytest +import yaml from cli.clients.github_client import GitHubClient, _extract_review_threads_page, _normalize_comment from cli.core.config import ReviewConfig @@ -831,6 +832,32 @@ def test_self_hosted_workflows_drive_models_from_org_repo_variables() -> None: # end-to-end by tests/test_dotbot_workflow_model_vars.py. +def test_action_cli_steps_cannot_be_shadowed_by_the_checkout() -> None: + """The pinned action's code must run, not the caller's `cli/` package. + + Composite steps execute in the caller's workspace and `python -m` puts the + current directory ahead of PYTHONPATH, so a checkout that ships a `cli/` + package would run *its* code instead of the pinned action's — with the + workflow's token. PYTHONSAFEPATH on every CLI-invoking step prevents it. + """ + + action_yaml = yaml.safe_load(Path("action.yml").read_text(encoding="utf-8")) + cli_steps = [ + step + for step in action_yaml["runs"]["steps"] + if "cli.main" in str(step.get("run", "")) + or "cli.review.prepare_resume_state" in str(step.get("run", "")) + ] + + assert cli_steps, "expected at least one step invoking the CLI" + for step in cli_steps: + assert step["env"].get("PYTHONSAFEPATH") == "1", ( + f"step {step.get('name')!r} can be shadowed by the checkout" + ) + run = str(step["run"]) + assert "github.action_path" in run or "GITHUB_ACTION_PATH" in run + + def test_edit_workflow_helpers_cover_reply_formatting_and_context_normalization() -> None: truncated = _format_edit_reply("x" * 3605, pushed=False, dry_run=False, changed=True) assert "not pushed" in truncated