From 1f4b70f6afd3f00ac2ff05663d0b14ffa8f61eb3 Mon Sep 17 00:00:00 2001 From: Pawel Lebioda Date: Thu, 2 Jul 2026 12:58:07 +0200 Subject: [PATCH 1/5] Strip GitHub write credentials from the agent subprocess The AI agent (Claude/Gemini CLI) is spawned as a subprocess of mergai and inherited mergai's full environment via os.environ.copy() -- which in CI carries the GitHub write token used by mergai's own push/PR steps. A misbehaving or prompt-injected agent could therefore push to origin or call GitHub write APIs. Add agent_subprocess_env() which removes GITHUB_TOKEN/GH_TOKEN/ GH_ENTERPRISE_TOKEN from the environment handed to the agent (substituting a read-only MERGAI_AGENT_GH_TOKEN if provided), and use it when spawning both adapters. The parent mergai process keeps its write-capable environment, so its deterministic pushes are unaffected -- only the agent is de-privileged. Local operations (file edits, local git reads/commits) need no credential and still work. This closes the environment path only; the checkout's persisted http.extraheader credential is closed separately with persist-credentials: false in the workflow. --- src/mergai/agents/claude_cli.py | 5 +++- src/mergai/agents/env.py | 43 +++++++++++++++++++++++++++++++++ src/mergai/agents/gemini_cli.py | 5 +++- tests/test_agent_env.py | 25 +++++++++++++++++++ 4 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 src/mergai/agents/env.py create mode 100644 tests/test_agent_env.py diff --git a/src/mergai/agents/claude_cli.py b/src/mergai/agents/claude_cli.py index 7614876..7b21748 100644 --- a/src/mergai/agents/claude_cli.py +++ b/src/mergai/agents/claude_cli.py @@ -5,6 +5,7 @@ from ..utils.output import echo_err as _echo from .base import CliAgent +from .env import agent_subprocess_env from .error import AgentError, AgentErrorType, AgentResult from .response_utils import parse_response_json @@ -344,7 +345,9 @@ def run_prompt( stdout=subprocess.PIPE, stderr=subprocess.STDOUT, # Combine stderr with stdout to avoid deadlock text=True, - env=os.environ.copy(), + # Strip GitHub write credentials so the agent cannot push or issue + # GitHub write APIs; local file/git operations are unaffected. + env=agent_subprocess_env(), ) result: dict = {} diff --git a/src/mergai/agents/env.py b/src/mergai/agents/env.py new file mode 100644 index 0000000..8c08583 --- /dev/null +++ b/src/mergai/agents/env.py @@ -0,0 +1,43 @@ +"""Environment sanitization for spawned agent subprocesses. + +The agent must never be able to modify remote state (push to origin or issue +GitHub write APIs). Because the agent runs as a subprocess of mergai, it would +otherwise inherit mergai's environment -- which in CI carries the GitHub write +token used by mergai's own deterministic push/PR steps. + +`agent_subprocess_env` strips those credentials from the environment handed to +the agent, so that even a fully compromised or prompt-injected agent cannot +authenticate to GitHub at all. The agent only needs local access (edit files, +local git reads and commits), none of which requires a credential. + +The parent mergai process keeps its own (credentialed) environment, so its +push/PR steps continue to work -- only the spawned agent is de-privileged. + +NOTE: This closes the *environment* path only. `actions/checkout` also persists +a credential into the repo's ``.git/config`` (``http.extraheader``); a bare +`git push` authenticates via that on-disk token regardless of the environment, +and the agent can even read it. That path must be closed separately with +``persist-credentials: false`` in the workflow (plus a credential helper so +mergai's own push still authenticates from the parent environment). +""" + +import os + +# Environment variables that authenticate to GitHub: the gh CLI reads these, +# and a credential-helper-based `git push` derives its token from them. +_GITHUB_CREDENTIAL_VARS = ("GITHUB_TOKEN", "GH_TOKEN", "GH_ENTERPRISE_TOKEN") + + +def agent_subprocess_env() -> dict[str, str]: + """Return a copy of the environment with GitHub credentials removed. + + The agent is left with no GitHub token, so it cannot push or call GitHub + APIs; local file and git operations need no credential and are unaffected. + + Returns: + A sanitized environment dict suitable for the agent subprocess. + """ + env = os.environ.copy() + for var in _GITHUB_CREDENTIAL_VARS: + env.pop(var, None) + return env diff --git a/src/mergai/agents/gemini_cli.py b/src/mergai/agents/gemini_cli.py index b14d9fa..422ab86 100644 --- a/src/mergai/agents/gemini_cli.py +++ b/src/mergai/agents/gemini_cli.py @@ -5,6 +5,7 @@ from ..utils.output import echo_err as _echo from .base import CliAgent +from .env import agent_subprocess_env from .error import AgentError, AgentErrorType, AgentResult from .response_utils import parse_response_json @@ -97,7 +98,9 @@ def run_prompt(self, prompt: str): stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - env=os.environ.copy(), + # Strip GitHub write credentials so the agent cannot push or issue + # GitHub write APIs; local file/git operations are unaffected. + env=agent_subprocess_env(), ) result: dict = {} diff --git a/tests/test_agent_env.py b/tests/test_agent_env.py new file mode 100644 index 0000000..b828e24 --- /dev/null +++ b/tests/test_agent_env.py @@ -0,0 +1,25 @@ +"""Tests for agent subprocess environment sanitization.""" + +from mergai.agents.env import agent_subprocess_env + + +def test_strips_github_credentials(monkeypatch): + monkeypatch.setenv("GITHUB_TOKEN", "write-token") + monkeypatch.setenv("GH_TOKEN", "write-token") + monkeypatch.setenv("GH_ENTERPRISE_TOKEN", "write-token") + + env = agent_subprocess_env() + + assert "GITHUB_TOKEN" not in env + assert "GH_TOKEN" not in env + assert "GH_ENTERPRISE_TOKEN" not in env + + +def test_preserves_non_github_env(monkeypatch): + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant") + monkeypatch.setenv("PATH", "/usr/bin") + + env = agent_subprocess_env() + + assert env["ANTHROPIC_API_KEY"] == "sk-ant" + assert env["PATH"] == "/usr/bin" From 734a48d1ddf6e042ed40328bcffba49a1dfa6393 Mon Sep 17 00:00:00 2001 From: Pawel Lebioda Date: Thu, 2 Jul 2026 22:09:27 +0200 Subject: [PATCH 2/5] Deny remote-write / bypass tools instead of using real yolo mode `mergai resolve -y` / describe previously ran the Claude agent with --dangerously-skip-permissions, which bypasses every guardrail and lets the agent run any command (including git push and gh writes). Instead: - map yolo (and any edit-capable run) to --permission-mode acceptEdits, which auto-approves file edits in the working dir but not real yolo; - pass --disallowedTools on *every* invocation to deny remote-mutating git/gh, network egress (WebFetch/WebSearch) and work-spawning/orchestration tools (Task, Skill, Workflow, Cron*, ...) that could bypass these limits. No --allowedTools list is used: testing showed it is not a strict allow-list (under acceptEdits a command in neither list still runs), so it would neither enable nor restrict anything. Enforcement is this always-on deny-list plus the authoritative server-side boundary (the agent runs with no GitHub credential; see env.py and the workflow's persist-credentials: false). Bash deny rules are prefix-matched and best-effort; the credential boundary is what actually guarantees no remote writes. Verified live: git log runs, git push is denied. Allow Bash broadly so the agent can run read commands under the deny-list CI (claude 2.1.198) proved that a deny-list-only config denies un-allowed Bash in --print mode: the agent's read commands (git ls-tree, git diff | head, for-loops) all hit "requires approval" and were blocked, leaving describe/ resolve blind to the repo. An earlier local test (older CLI) suggested unlisted Bash ran; that did not hold on CI. Re-introduce a broad --allowedTools (Bash + file tools) alongside the deny-list. Deny takes precedence, so remote-write / bypass commands stay blocked while all other Bash (the agent's read shell) is auto-approved. --- src/mergai/agents/claude_cli.py | 89 ++++++++++++++++++++++++++---- tests/test_claude_build_args.py | 96 +++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 10 deletions(-) create mode 100644 tests/test_claude_build_args.py diff --git a/src/mergai/agents/claude_cli.py b/src/mergai/agents/claude_cli.py index 7b21748..7860c10 100644 --- a/src/mergai/agents/claude_cli.py +++ b/src/mergai/agents/claude_cli.py @@ -9,6 +9,66 @@ from .error import AgentError, AgentErrorType, AgentResult from .response_utils import parse_response_json +# Tools the agent is allowed to use, auto-approved without a prompt. `Bash` is +# allowed broadly (the agent needs arbitrary *read* shell -- `git diff`, +# `git ls-tree`, `git show | grep`, `for` loops -- to inspect the merge), plus +# the file tools for editing. The dangerous subset is carved out by the +# deny-list below, which takes precedence over this allow. +# +# This broad allow is load-bearing, NOT decorative: in `--print` mode the CLI +# does not auto-approve un-allowed Bash (it returns "requires approval", a +# denial when non-interactive), so without an explicit Bash allow the agent +# cannot run any read command. (Verified in CI: on claude 2.1.198, a deny-list- +# only config denied git ls-tree / git diff / for-loops.) +GUARDRAIL_ALLOWED_TOOLS = ["Bash", "Read", "Edit", "Write", "Grep", "Glob"] + +# Tools/commands the agent must never use: anything that could modify the remote +# or be used to bypass the no-remote-write guarantee. Passed via --disallowedTools +# on *every* invocation. Deny takes precedence over the broad Bash allow above, +# so these stay blocked while all other Bash is permitted. +# +# NOTE: Bash deny rules are prefix-matched and best-effort (e.g. an obfuscated +# `git -c ... push` can evade them); the credential boundary is what actually +# guarantees no remote writes (see env.py and the workflow's +# persist-credentials: false). +GUARDRAIL_DISALLOWED_TOOLS = [ + # Remote-mutating git / the GitHub CLI (which the agent has no reason to use + # and no credential for). + "Bash(git push)", + "Bash(git push:*)", + "Bash(gh:*)", + # Network egress: exfiltration, or fetching code/instructions to run. + "WebFetch", + "WebSearch", + # Spawning unconstrained work / running arbitrary skills or workflows, which + # could sidestep these restrictions. + "Task", + "Agent", + "Skill", + "Workflow", + # Harness/orchestration tools not needed for conflict resolution. + "CronCreate", + "CronDelete", + "CronList", + "ScheduleWakeup", + "Monitor", + "PushNotification", + "SendMessage", + "RemoteTrigger", + "DesignSync", + "EnterWorktree", + "ExitWorktree", + "ToolSearch", + "TaskCreate", + "TaskGet", + "TaskList", + "TaskOutput", + "TaskStop", + "TaskUpdate", + "ReportFindings", + "NotebookEdit", +] + class ClaudeCLIAgent(CliAgent): """Claude CLI Agent for running prompts via Claude Code CLI. @@ -23,7 +83,10 @@ def __init__(self, model: str, yolo: bool = False, debug: bool = False): Args: model: The model to use (e.g., "claude-sonnet-4-20250514" or "sonnet"). - yolo: Enable bypass of all permission checks (--dangerously-skip-permissions). + yolo: Auto-approve file edits (acceptEdits). Despite the name, this + does NOT enable --dangerously-skip-permissions. Remote-write / + network / work-spawning tools are denied on every invocation + regardless of this flag (see GUARDRAIL_DISALLOWED_TOOLS). debug: Enable debug logging. """ super().__init__(model) @@ -297,15 +360,21 @@ def build_args( "--verbose", # Required for stream-json ] - if self.yolo: - args.append("--dangerously-skip-permissions") - elif allowed_write_paths: - # Use acceptEdits mode to auto-approve write operations. - # NOTE: Claude CLI's acceptEdits mode does not restrict edits to specific paths; - # it broadly auto-approves all file edits. The allowed_write_paths parameter is - # accepted for interface compatibility but cannot enforce path-level restrictions - # in print mode. For strict path isolation, consider running the agent in a - # sandboxed working directory. + # Allow Bash + file tools broadly (the agent needs arbitrary read shell + # to inspect the merge), then deny the remote-mutating / bypass subset. + # Both are passed on *every* invocation. The broad allow is required: in + # --print mode the CLI denies un-allowed Bash ("requires approval"), so + # without it the agent cannot run any read command. Deny wins over allow, + # so the denied commands stay blocked. + args.extend(["--allowedTools", ",".join(GUARDRAIL_ALLOWED_TOOLS)]) + args.extend(["--disallowedTools", ",".join(GUARDRAIL_DISALLOWED_TOOLS)]) + + if self.yolo or allowed_write_paths: + # Auto-approve file edits in the working directory. "yolo" + # deliberately does NOT map to --dangerously-skip-permissions (which + # would also bypass the deny-list above). acceptEdits does not + # restrict edits to specific paths, so allowed_write_paths cannot be + # enforced here; the post-run validator checks for stray writes. args.extend(["--permission-mode", "acceptEdits"]) if self.debug: diff --git a/tests/test_claude_build_args.py b/tests/test_claude_build_args.py new file mode 100644 index 0000000..554e3f0 --- /dev/null +++ b/tests/test_claude_build_args.py @@ -0,0 +1,96 @@ +"""Tests for ClaudeCLIAgent.build_args guardrails. + +yolo maps to a constrained acceptEdits mode (never --dangerously-skip- +permissions), and the remote-write / bypass deny-list is applied on every +invocation regardless of permission mode. +""" + +from pathlib import Path + +from mergai.agents.claude_cli import ( + GUARDRAIL_ALLOWED_TOOLS, + GUARDRAIL_DISALLOWED_TOOLS, + ClaudeCLIAgent, +) + + +def _flag_value(args, flag): + return args[args.index(flag) + 1] + + +def test_never_skips_permissions(): + for yolo in (True, False): + args = ClaudeCLIAgent(model="opus", yolo=yolo).build_args("go") + assert "--dangerously-skip-permissions" not in args + + +def test_allows_bash_broadly(): + # A broad Bash allow is required so the agent can run read commands in + # --print mode (CI's CLI denies un-allowed Bash there). + args = ClaudeCLIAgent(model="opus", yolo=True).build_args("go") + allow = _flag_value(args, "--allowedTools").split(",") + assert "Bash" in allow and "Read" in allow + + +def test_allow_present_on_every_invocation_and_disjoint_from_deny(): + for yolo, paths in ((True, None), (False, [Path("r.json")]), (False, None)): + args = ClaudeCLIAgent(model="opus", yolo=yolo).build_args( + "go", allowed_write_paths=paths + ) + assert "--allowedTools" in args + assert "--disallowedTools" in args + # a tool must not be both allowed and denied + assert set(GUARDRAIL_ALLOWED_TOOLS).isdisjoint(GUARDRAIL_DISALLOWED_TOOLS) + + +def test_disallowed_tools_always_present(): + # Applied on every invocation: yolo, non-yolo write, and read-only. + for yolo, paths in ((True, None), (False, [Path("r.json")]), (False, None)): + args = ClaudeCLIAgent(model="opus", yolo=yolo).build_args( + "go", allowed_write_paths=paths + ) + assert "--disallowedTools" in args + # Exact membership on the comma-split value: a substring check would let + # "Task" pass on "TaskCreate" and mask a missing entry. + deny = set(_flag_value(args, "--disallowedTools").split(",")) + for expected in ( + "Bash(git push:*)", + "Bash(gh:*)", + "WebFetch", + "WebSearch", + "Workflow", + "Task", + ): + assert expected in deny + + +def test_yolo_uses_accept_edits(): + args = ClaudeCLIAgent(model="opus", yolo=True).build_args("go") + assert _flag_value(args, "--permission-mode") == "acceptEdits" + + +def test_accept_edits_when_write_paths_given_non_yolo(): + args = ClaudeCLIAgent(model="opus", yolo=False).build_args( + "go", allowed_write_paths=[Path("r.json")] + ) + assert _flag_value(args, "--permission-mode") == "acceptEdits" + + +def test_read_only_run_has_no_permission_mode_but_still_denies(): + # Neither yolo nor write paths -> default permission mode, but the deny-list + # is still applied. + args = ClaudeCLIAgent(model="opus", yolo=False).build_args("go") + assert "--permission-mode" not in args + assert "--disallowedTools" in args + + +def test_prompt_is_last_arg_not_swallowed(): + args = ClaudeCLIAgent(model="opus", yolo=True).build_args("PROMPT-SENTINEL") + assert args[-1] == "PROMPT-SENTINEL" + + +def test_disallowed_list_is_single_comma_joined_value(): + # A single value (not variadic), so it cannot consume the trailing prompt. + args = ClaudeCLIAgent(model="opus", yolo=True).build_args("go") + deny = _flag_value(args, "--disallowedTools") + assert deny == ",".join(GUARDRAIL_DISALLOWED_TOOLS) From 924b4b3400f5c0a1237fbd6c6a9b511fd83fde54 Mon Sep 17 00:00:00 2001 From: Pawel Lebioda Date: Thu, 2 Jul 2026 23:14:00 +0200 Subject: [PATCH 3/5] Do not use Gemini's real yolo approval mode Mirror the Claude adapter: map yolo to Gemini's auto_edit approval mode (which auto-approves file edits only) instead of "yolo" (which auto-approves every tool, including shell commands that could push or reach the remote). The agent subprocess already runs with no GitHub credential (see env.py), so it cannot write to the remote regardless; this removes the blanket tool auto-approval. --- src/mergai/agents/gemini_cli.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mergai/agents/gemini_cli.py b/src/mergai/agents/gemini_cli.py index 422ab86..fd8802a 100644 --- a/src/mergai/agents/gemini_cli.py +++ b/src/mergai/agents/gemini_cli.py @@ -67,10 +67,15 @@ def parse_stats(self, session_data: dict) -> dict: return {"models": models} def build_args(self, prompt: str) -> list: + # Always use auto_edit: auto-approve file edits in the working repo, but + # NOT other tools. yolo deliberately does NOT map to Gemini's "yolo" + # approval mode (which auto-approves every tool, incl. shell commands + # that could push or reach the remote). This mirrors the Claude adapter, + # where yolo maps to acceptEdits rather than --dangerously-skip-permissions. args = [ "gemini", "--approval-mode", - "yolo" if self.yolo else "auto_edit", + "auto_edit", "-o", "stream-json", ] From c058a0892f6734590fce500e1475eb80aecbd348 Mon Sep 17 00:00:00 2001 From: Pawel Lebioda Date: Fri, 3 Jul 2026 00:35:39 +0200 Subject: [PATCH 4/5] review: defer the --ack comment to `review post` `mergai review fix` runs the AI agent, so it must stay read-only with respect to GitHub. Previously `--ack` posted the acknowledgement comment directly (create_issue_comment) from within the fix command, which required a write token in the agent's step. Record the ack on the cache note instead and publish it from `review post` (the write-token step that already posts the recorded replies), mirroring the existing "record then post" pattern used for review replies: - MergaiNote gains a `review_ack` record ({message, pr_number, posted_at}) with set_review_ack / pending_review_ack / mark_review_ack_posted, serialized in to_dict/from_dict. - `review fix --ack` now calls _record_ack (writes only the note); it performs no GitHub write. - `review post` publishes any pending ack after the replies and marks it posted (idempotent across re-runs), and no longer no-ops when replies are empty but an ack is pending. This lets the review-handle workflow run `review fix` under a read-only token and scope the write token to `review post`. --- src/mergai/commands/review.py | 71 ++++++++++++++++++++++++++++++----- src/mergai/models.py | 39 +++++++++++++++++++ tests/test_review_threads.py | 47 +++++++++++++++++++++++ 3 files changed, 147 insertions(+), 10 deletions(-) diff --git a/src/mergai/commands/review.py b/src/mergai/commands/review.py index c0e166b..b569e2f 100644 --- a/src/mergai/commands/review.py +++ b/src/mergai/commands/review.py @@ -121,20 +121,48 @@ def _ignored_summary(skipped: list[tuple[ReviewThread, str]]) -> str: return ", ".join(parts) -def _post_ack(app: AppContext, pr_number: int, message: str, dry_run: bool) -> None: +def _post_ack( + app: AppContext, pr_number: int, message: str, dry_run: bool +) -> str | None: """Post a short acknowledgement comment on the PR (best-effort). Gives quick feedback on the trigger that mergai ran and what it did. Under ``--dry-run`` the message is printed instead of posted. + + Returns: + The created comment's URL on success, or ``None`` on failure / dry-run. + Callers use this to mark the ack posted only when it actually posted, so + a transient failure is retried on the next run. """ if dry_run: click.echo(f"[dry-run] would comment on PR #{pr_number}: {message}") - return + return None try: - app.gh_repo.get_pull(pr_number).create_issue_comment(message) + comment = app.gh_repo.get_pull(pr_number).create_issue_comment(message) click.echo(f"Posted acknowledgement on PR #{pr_number}.") + return getattr(comment, "html_url", None) or "" except Exception as e: # noqa: BLE001 - acknowledgement is best-effort click.echo(f"warning: could not post acknowledgement: {e}", err=True) + return None + + +def _record_ack(app: AppContext, pr_number: int, message: str, dry_run: bool) -> None: + """Record an acknowledgement on the note for ``review post`` to publish. + + ``review fix`` runs the AI agent and must stay read-only w.r.t. GitHub, so + it records the ack here instead of posting it. The (write-token) ``review + post`` step publishes it. Best-effort: if there is no note to record on, the + ack is skipped. + """ + if dry_run: + click.echo(f"[dry-run] would record ack for PR #{pr_number}: {message}") + return + if not app.has_note: + click.echo("warning: no note to record acknowledgement on; skipping.", err=True) + return + app.note.set_review_ack(message, pr_number) + app.save_note(app.note) + click.echo(f"Recorded acknowledgement for PR #{pr_number} (posted by review post).") @review.command() @@ -157,9 +185,11 @@ def _post_ack(app: AppContext, pr_number: int, message: str, dry_run: bool) -> N is_flag=True, default=False, help=( - "Post a short acknowledgement comment on the PR summarising the outcome " - "(how many comments were found / addressed), even when there are none. " - "Use from CI to give quick feedback on the trigger." + "Record a short acknowledgement summarising the outcome (how many " + "comments were found / addressed), even when there are none, for " + "`mergai review post` to publish on the PR. Recording is local (no " + "GitHub write), so `review fix` can run with a read-only token. Use " + "from CI to give quick feedback on the trigger." ), ) @click.option( @@ -239,7 +269,7 @@ def fix( if ignored else "mergai review fix: no review comments to address." ) - _post_ack(app, number, msg, dry_run) + _record_ack(app, number, msg, dry_run) return context = build_review_context( @@ -343,7 +373,7 @@ def fix( f"mergai review fix: addressed {len(addressed)} of {total} " f"review comment(s); ignored {ignored} (not processed)." ) - _post_ack(app, number, msg, dry_run) + _record_ack(app, number, msg, dry_run) def _annotate_threads(entries: dict, threads_by_id: dict) -> None: @@ -535,7 +565,8 @@ def post(app: AppContext, dry_run: bool, force: bool) -> None: For each recorded reply intent, posts a reply on the thread's root comment: a "fixed" note (with the commit) on threads the agent addressed, an "unfixable" note (with the reason) on the rest. Threads are never - auto-resolved. No-op when nothing is pending, so it is safe to run + auto-resolved. Also publishes the acknowledgement recorded by ``review fix + --ack``, if any. No-op when nothing is pending, so it is safe to run unconditionally. Records persist in the cache note, so the usual flow is: ``review fix`` → review / push → ``review post``. """ @@ -547,8 +578,11 @@ def post(app: AppContext, dry_run: bool, force: bool) -> None: if force else app.note.pending_review_comments() ) + # The acknowledgement recorded by `review fix` (read-only) is published here, + # in the write-token step, alongside the replies. + pending_ack = app.note.pending_review_ack() if app.has_note else None - if not records: + if not records and not pending_ack: click.echo("No pending review replies to post.") return @@ -594,6 +628,23 @@ def post(app: AppContext, dry_run: bool, force: bool) -> None: posted_any = True click.echo(f" [{tid}] {loc}: posted ({rec.get('outcome')}).") + # Publish the acknowledgement recorded by `review fix`, if any. Best-effort, + # like the reply posting. Only mark it posted when the post actually + # succeeded, so a transient failure is retried on the next run (and capture + # the comment URL when available). + if pending_ack: + ack_pr = pending_ack.get("pr_number") + ack_msg = pending_ack.get("message", "") + if dry_run: + click.echo(f"[dry-run] would comment on PR #{ack_pr}: {ack_msg}") + elif ack_pr is not None: + ack_url = _post_ack(app, int(ack_pr), ack_msg, dry_run=False) + if ack_url is not None: + app.note.mark_review_ack_posted( + posted_at=now, comment_url=ack_url or None + ) + posted_any = True + if posted_any and not dry_run: app.save_note(app.note) if not dry_run: diff --git a/src/mergai/models.py b/src/mergai/models.py index 2251c15..752a3bc 100644 --- a/src/mergai/models.py +++ b/src/mergai/models.py @@ -872,6 +872,14 @@ class MergaiNote: # Like `ci_comments`, this lives only in the cache note (not git notes), # which lets `review fix` and the (separate) post step share state. review_comments: list[dict] | None = None + # `review fix` records the acknowledgement summary here (read-only) instead + # of posting it, so the GitHub write happens only in the (write-token) + # `mergai review post` step. A single record following the same + # pending/posted convention as ``review_comments``: + # ``{"message": str, "pr_number": int, "posted_at": str | None, + # "posted_comment_url": str | None}`` (the last two are set by + # ``mark_review_ack_posted`` once published). + review_ack: dict | None = None # Accumulating record of the commits absorbed by a squash finalize. Each # entry is ``{"sha": , "message": }``, oldest-first. # When a squash commit (which carries this field) is itself squashed again, @@ -920,6 +928,7 @@ def from_dict(cls, data: dict, repo: "Repo | None" = None) -> "MergaiNote": note_index=data.get("note_index"), ci_comments=data.get("ci_comments"), review_comments=data.get("review_comments"), + review_ack=data.get("review_ack"), squashed_commits=data.get("squashed_commits"), _repo=repo, ) @@ -1161,6 +1170,34 @@ def mark_review_comment_posted( return True return False + def set_review_ack(self, message: str, pr_number: int) -> "MergaiNote": + """Record a review acknowledgement for ``mergai review post`` to publish. + + ``review fix`` records the ack here (read-only) instead of posting it, + so the GitHub write happens only in the write-token post step. Replaces + any existing ack (posted or not) from an earlier run in the same job. + """ + self.review_ack = { + "message": message, + "pr_number": pr_number, + "posted_at": None, + } + return self + + def pending_review_ack(self) -> dict | None: + """Return the recorded ack if it has not been posted yet, else None.""" + if self.review_ack and not self.review_ack.get("posted_at"): + return self.review_ack + return None + + def mark_review_ack_posted( + self, *, posted_at: str, comment_url: str | None + ) -> None: + """Mark the recorded ack as posted (idempotent across re-runs).""" + if self.review_ack is not None: + self.review_ack["posted_at"] = posted_at + self.review_ack["posted_comment_url"] = comment_url + def addressed_review_thread_ids(self) -> set[str]: """Review thread ids already fixed by a ``review_fix`` solution. @@ -1649,6 +1686,8 @@ def to_dict(self) -> dict: result["ci_comments"] = self.ci_comments if self.review_comments: result["review_comments"] = self.review_comments + if self.review_ack: + result["review_ack"] = self.review_ack if self.squashed_commits: result["squashed_commits"] = self.squashed_commits return result diff --git a/tests/test_review_threads.py b/tests/test_review_threads.py index 0b29bb7..3bac03b 100644 --- a/tests/test_review_threads.py +++ b/tests/test_review_threads.py @@ -637,6 +637,30 @@ def test_note_review_comment_record_lifecycle(): ) +def test_note_review_ack_lifecycle(): + n = _note() + assert n.pending_review_ack() is None + + n.set_review_ack("addressed 2 of 3", pr_number=42) + ack = n.pending_review_ack() + assert ack is not None + assert ack["message"] == "addressed 2 of 3" + assert ack["pr_number"] == 42 + + # to_dict/from_dict round-trips the ack (cache-note persistence) + from mergai.models import MergaiNote + + restored = MergaiNote.from_dict(n.to_dict()) + assert restored.pending_review_ack()["message"] == "addressed 2 of 3" + + # once posted, it is no longer pending (idempotent re-runs) + n.mark_review_ack_posted(posted_at="now", comment_url=None) + assert n.pending_review_ack() is None + # a fresh ack replaces a posted one + n.set_review_ack("new run", pr_number=42) + assert n.pending_review_ack()["message"] == "new run" + + def test_find_and_drop_orphaned_review_comments(monkeypatch): from mergai import models @@ -688,3 +712,26 @@ def test_addressed_review_thread_ids_from_solutions(): {"type": CONFLICT_RESOLUTION, "response": {"addressed": {"PRRT_z": {}}}} ) assert n.addressed_review_thread_ids() == {"PRRT_a", "PRRT_b", "PRRT_d"} + + +def test_post_ack_returns_url_on_success_and_none_on_failure(): + # _post_ack must report success (URL) vs failure (None) so `review post` + # only marks the ack posted when it actually posted (allowing retries). + from types import SimpleNamespace + + from mergai.commands import review as review_cmd + + class _PullOK: + def create_issue_comment(self, body): + return SimpleNamespace(html_url="https://gh/c/1") + + class _PullFail: + def create_issue_comment(self, body): + raise RuntimeError("boom") + + app_ok = SimpleNamespace(gh_repo=SimpleNamespace(get_pull=lambda n: _PullOK())) + app_fail = SimpleNamespace(gh_repo=SimpleNamespace(get_pull=lambda n: _PullFail())) + + assert review_cmd._post_ack(app_ok, 1, "m", dry_run=False) == "https://gh/c/1" + assert review_cmd._post_ack(app_fail, 1, "m", dry_run=False) is None + assert review_cmd._post_ack(app_ok, 1, "m", dry_run=True) is None From d8017dc3a24559c625707b063291cd91fb542412 Mon Sep 17 00:00:00 2001 From: Pawel Lebioda Date: Fri, 3 Jul 2026 15:43:14 +0200 Subject: [PATCH 5/5] ci gate: report failure only when there is actionable work mergai ci status --state (read by the CI-fix gate) used a crude conclusion rollup: all runs completed and at least one != success -> failure. That disagreed with the fixer (classify_run / ci fix all), so the gate reported failure for runs the fixer would then skip, spinning up the privileged handler (and its deployment-approval request) for nothing: a human/timeout-cancelled check, a rejected approval, an infra failure with no failing step, or a run mergai already fixed. Replace _aggregate_state with _actionable_state, which reduces the same per-run classification ci list / ci fix all use (via _list_run_status). Now failure means mergai has actionable work: * plain cancellation / rejected approval / no failing step -> success * already-handled run (solution or comment recorded) -> success * fail-fast cancel masking a real failing step -> failure * real failure -> failure * passing run with Code Scanning findings -> failure * any watched run still running -> in-progress (wins over failure so the handler fires only once the whole watched set for HEAD is done) Thread check_failure_kind through _list_run_status (default False, so ci list is unchanged; the gate passes True) so an approval-rejected / infra failure is classified skip. The gate stays read-only. Confirming Code Scanning findings needs a token with security-events read; when the gate runs without it (or any read side-call errors) the run degrades to non-actionable rather than crashing the gate or firing the handler on an unconfirmed signal. --- src/mergai/ci/gate.py | 87 +++++++++++--- src/mergai/commands/ci.py | 18 ++- tests/test_ci_gate_state.py | 218 ++++++++++++++++++++++++++++++++++++ 3 files changed, 303 insertions(+), 20 deletions(-) create mode 100644 tests/test_ci_gate_state.py diff --git a/src/mergai/ci/gate.py b/src/mergai/ci/gate.py index f1cf6f3..4df40cd 100644 --- a/src/mergai/ci/gate.py +++ b/src/mergai/ci/gate.py @@ -23,6 +23,7 @@ def _list_run_status( run: "github.WorkflowRun.WorkflowRun", *, check_findings: bool, + check_failure_kind: bool = False, ) -> tuple[str, str]: """Return ``(status, notes)`` describing what mergai sees for this run. @@ -63,12 +64,15 @@ def _list_run_status( workflow_name=run.name, pr_number=_resolve_pr_number(run), check_findings=check_findings, - # Listing should stay cheap and still show a failed run as a failure - # entry; the non-code-failure side-calls (approvals / job steps) are - # only worth making on the real dispatch path. A ``cancelled`` run is - # the exception: classify_run always inspects its jobs to surface a - # fail-fast-masked failure, regardless of this flag (see classify_run). - check_failure_kind=False, + # ``ci list`` leaves this False: listing should stay cheap and still + # show a failed run as a failure entry; the non-code-failure side-calls + # (approvals / job steps) are only worth making on the real dispatch + # path. The gate (``_actionable_state``) passes True so an + # approval-rejected / infra ``failure`` is classified ``skip`` and does + # not spin up the privileged handler. A ``cancelled`` run is inspected + # regardless of this flag: classify_run always looks at its jobs to + # surface a fail-fast-masked failure (see classify_run). + check_failure_kind=check_failure_kind, ) if decision.actionable: @@ -153,22 +157,73 @@ def _watched_runs_for_head( return latest -def _aggregate_state( +def _actionable_state( + app: AppContext, runs_by_workflow: dict[str, "github.WorkflowRun.WorkflowRun"], + *, + check_findings: bool = True, + check_failure_kind: bool = True, ) -> Literal["in-progress", "success", "failure", "none"]: """Reduce the per-workflow latest runs to a single gate token. + Unlike a raw conclusion rollup, ``failure`` here means *mergai has + actionable work* — not merely "something didn't return success". The per-run + verdict comes from :func:`_list_run_status` (the same classification + ``ci list`` / ``ci fix all`` use), so the gate agrees with what the fixer + would actually do: + + * a plain ``cancelled`` run (user/timeout cancel, superseded) → ``skip``; + * a ``failure`` from a rejected deployment approval or an infra failure with + no failing step → ``skip`` (``check_failure_kind``); + * a run mergai already handled (a ``ci_fix`` solution or a posted comment + exists for it) → ``applied`` / ``commented`` — not re-triggered; + * a *passing* run with Code Scanning findings → ``pending`` (actionable). + + The tokens: + * ``none`` — no watched runs for HEAD (e.g. all skipped). - * ``in-progress`` — at least one watched run hasn't completed. - * ``success`` — every watched run completed with ``success``. - * ``failure`` — all completed, but at least one did not succeed - (``failure`` / ``cancelled`` / ``timed_out`` / …). + * ``in-progress`` — at least one watched run hasn't completed. This wins over + ``failure`` so the handler fires only once the whole + watched set for HEAD is done and a later completion + finalizes it (keeps the CI-fix loop converging). + * ``failure`` — all completed and at least one run is actionable + (``pending``). + * ``success`` — all completed and nothing is actionable (passed, + skipped, or already handled). + + Per-run classification is best-effort: a Code Scanning findings lookup needs + a token with ``security-events: read``, so when the gate runs without it (or + any read side-call errors) the run degrades to non-actionable rather than + crashing the gate or spinning the handler up on an unverifiable signal. """ if not runs_by_workflow: return "none" - runs = list(runs_by_workflow.values()) - if any(run.status != "completed" for run in runs): + + statuses: list[str] = [] + for run in runs_by_workflow.values(): + try: + status, _ = _list_run_status( + app, + run, + check_findings=check_findings, + check_failure_kind=check_failure_kind, + ) + except Exception as exc: # noqa: BLE001 — best-effort; the gate must not crash + # A read side-call failed: a code-scanning lookup the gate token + # can't make (no security-events read → GithubException), or any + # transient requester/network error underneath it. Treat the run as + # non-actionable so the gate stays read-only, always emits a token, + # and never fires the handler on a signal it couldn't confirm. + click.echo( + f"warning: could not classify run {run.id} ({run.name}): {exc}; " + "treating as non-actionable", + err=True, + ) + status = "skip" + statuses.append(status) + + if "wait" in statuses: return "in-progress" - if all(run.conclusion == "success" for run in runs): - return "success" - return "failure" + if "pending" in statuses: + return "failure" + return "success" diff --git a/src/mergai/commands/ci.py b/src/mergai/commands/ci.py index 94309d2..8d354da 100644 --- a/src/mergai/commands/ci.py +++ b/src/mergai/commands/ci.py @@ -53,7 +53,7 @@ _run_jobs, build_workflow_context_for_run, ) -from ..ci.gate import _aggregate_state, _list_run_status, _watched_runs_for_head +from ..ci.gate import _actionable_state, _list_run_status, _watched_runs_for_head from ..ci.handlers import get_handler from ..solution_types import CI_FIX from ..utils.formatters import format_ascii_table @@ -744,15 +744,25 @@ def status(app: AppContext, state_only: bool) -> None: \b * in-progress — at least one watched run for HEAD hasn't completed. - * success — every watched run for HEAD completed successfully. - * failure — all completed, but at least one didn't succeed. + * failure — all completed and at least one run is *actionable*: mergai + would run the fixer on it. + * success — all completed and nothing is actionable (passed, skipped, + or already handled by mergai). * none — no watched runs for HEAD (e.g. all skipped). + ``failure`` means actionable work exists, not merely "a run didn't return + success": a plain cancellation, a rejected deployment approval, an infra + failure with no failing step, and a run mergai already fixed all reduce to + ``success`` so the CI-fix handler is only spun up when there is really + something for it to do. A *passing* run with Code Scanning findings does + count as ``failure`` (needs a token with ``security-events: read`` to + detect; without it that run degrades to non-actionable). + Exits 0 in every case; the state is communicated on stdout. With ``--state`` only the bare token is printed (for shell capture). """ runs_by_workflow = _watched_runs_for_head(app) - state = _aggregate_state(runs_by_workflow) + state = _actionable_state(app, runs_by_workflow) if state_only: click.echo(state) diff --git a/tests/test_ci_gate_state.py b/tests/test_ci_gate_state.py new file mode 100644 index 0000000..fd367e0 --- /dev/null +++ b/tests/test_ci_gate_state.py @@ -0,0 +1,218 @@ +"""Tests for ``_actionable_state`` — the ``mergai ci status --state`` gate. + +The gate's ``failure`` token must mean *mergai has actionable work* (the fixer +would run), not merely "a watched run didn't return success". These tests pin +the cases that used to spin the privileged CI-fix handler up for nothing: +a plain cancellation, an already-handled run, and a passing code-scanning run +whose findings can't be queried (no ``security-events`` read). +""" + +from types import SimpleNamespace + +import github + +from mergai.ci.gate import _actionable_state + +HEAD_SHA = "a" * 40 + + +class _Run: + def __init__( + self, + *, + name, + conclusion, + status="completed", + steps_conclusions=(), + ): + self.id = abs(hash(name)) % 100000 + self.name = name + self.conclusion = conclusion + self.status = status + self.head_branch = "mergai/x" + self.head_sha = HEAD_SHA + self.url = "https://api.github.com/repos/o/r/actions/runs/123" + self.pull_requests = [SimpleNamespace(number=7)] + self._steps_conclusions = steps_conclusions + + def jobs(self): + steps = [SimpleNamespace(conclusion=c) for c in self._steps_conclusions] + return [SimpleNamespace(steps=steps)] + + +def _app( + *, + code_scanning=False, + solution_for_run=None, + comment_for_run=None, + requester=None, +): + """Build a minimal AppContext double. + + * ``code_scanning`` toggles ``code_scanning_check`` on every workflow. + * ``solution_for_run`` / ``comment_for_run`` are callables ``run_id -> …`` + standing in for the note lookups (default: nothing recorded). + * ``requester`` overrides ``requestJsonAndCheck`` (the code-scanning / + approvals side-calls); default returns an empty analyses list. + """ + workflow_config = SimpleNamespace( + enabled=True, + context=SimpleNamespace( + type="bazel", + code_scanning_check=code_scanning, + code_scanning_tool_name=None, + ), + ) + note = SimpleNamespace( + get_ci_comment_for_run=comment_for_run or (lambda run_id: None), + get_ci_solution_for_run=solution_for_run or (lambda run_id: None), + solutions=[], + ) + if requester is None: + requester = lambda method, url, **kw: ({}, []) # noqa: E731 + return SimpleNamespace( + has_note=True, + note=note, + gh_repo=SimpleNamespace( + url="https://api.github.com/repos/o/r", + _requester=SimpleNamespace(requestJsonAndCheck=requester), + ), + repo=SimpleNamespace( + head=SimpleNamespace(commit=SimpleNamespace(hexsha=HEAD_SHA)) + ), + config=SimpleNamespace( + branch=SimpleNamespace(working_prefix="mergai/"), + workflows=SimpleNamespace( + get=lambda name: workflow_config, + workflows={"format", "clang-tidy", "build-and-test"}, + ), + ), + ) + + +def _by_workflow(*runs): + return {run.name: run for run in runs} + + +def test_none_when_no_runs(): + assert _actionable_state(_app(), {}) == "none" + + +def test_in_progress_wins_over_actionable_failure(): + # One run still running, another already failed: hold at in-progress so the + # handler fires only once the whole set for HEAD is done. + app = _app() + runs = _by_workflow( + _Run(name="format", conclusion=None, status="in_progress"), + _Run(name="clang-tidy", conclusion="failure", steps_conclusions=("failure",)), + ) + assert _actionable_state(app, runs) == "in-progress" + + +def test_real_failure_is_failure(): + app = _app() + runs = _by_workflow( + _Run( + name="build-and-test", conclusion="failure", steps_conclusions=("failure",) + ) + ) + assert _actionable_state(app, runs) == "failure" + + +def test_fail_fast_masked_cancellation_is_failure(): + # cancelled roll-up masking a real failing step -> actionable. + app = _app() + runs = _by_workflow( + _Run( + name="build-and-test", + conclusion="cancelled", + steps_conclusions=("success", "failure"), + ) + ) + assert _actionable_state(app, runs) == "failure" + + +def test_plain_cancellation_is_not_failure(): + # The headline fix: a user/timeout cancellation is nothing to fix, so the + # gate must NOT report failure (which would spin up the privileged handler). + app = _app() + runs = _by_workflow( + _Run(name="format", conclusion="success"), + _Run( + name="build-and-test", + conclusion="cancelled", + steps_conclusions=("success",), + ), + ) + assert _actionable_state(app, runs) == "success" + + +def test_all_success_is_success(): + app = _app() + runs = _by_workflow( + _Run(name="format", conclusion="success"), + _Run(name="clang-tidy", conclusion="success"), + _Run(name="build-and-test", conclusion="success"), + ) + assert _actionable_state(app, runs) == "success" + + +def test_already_handled_failure_is_not_re_triggered(): + # A run mergai already fixed (a solution is recorded for its run_id) is + # `applied`, not `pending` -> the gate stays green (idempotent). + solution = {"request": {"attempt_number": 1}} + app = _app(solution_for_run=lambda run_id: solution) + app.note.solutions = [solution] + runs = _by_workflow( + _Run( + name="build-and-test", conclusion="failure", steps_conclusions=("failure",) + ) + ) + assert _actionable_state(app, runs) == "success" + + +def test_passing_code_scanning_run_with_findings_is_failure(): + def requester(method, url, **kw): + if url.endswith("/code-scanning/analyses"): + return ({}, [{"commit_sha": HEAD_SHA, "results_count": 3}]) + return ({}, []) + + app = _app(code_scanning=True, requester=requester) + runs = _by_workflow(_Run(name="clang-tidy", conclusion="success")) + assert _actionable_state(app, runs) == "failure" + + +def test_passing_code_scanning_run_without_findings_is_success(): + app = _app(code_scanning=True) # analyses lookup returns [] + runs = _by_workflow(_Run(name="clang-tidy", conclusion="success")) + assert _actionable_state(app, runs) == "success" + + +def test_code_scanning_query_denied_degrades_to_non_actionable(capsys): + # No security-events read: the analyses lookup 403s. The gate must not crash + # and must not report failure on a signal it couldn't confirm. + def requester(method, url, **kw): + if url.endswith("/code-scanning/analyses"): + raise github.GithubException( + 403, {"message": "Resource not accessible"}, None + ) + return ({}, []) + + app = _app(code_scanning=True, requester=requester) + runs = _by_workflow(_Run(name="clang-tidy", conclusion="success")) + assert _actionable_state(app, runs) == "success" + assert "could not classify run" in capsys.readouterr().err + + +def test_non_github_side_call_error_degrades_to_non_actionable(capsys): + # A raw requester/network error (not a GithubException) underneath the + # code-scanning lookup must also degrade rather than crash the gate. + def requester(method, url, **kw): + if url.endswith("/code-scanning/analyses"): + raise ConnectionError("connection reset") + return ({}, []) + + app = _app(code_scanning=True, requester=requester) + runs = _by_workflow(_Run(name="clang-tidy", conclusion="success")) + assert _actionable_state(app, runs) == "success" + assert "could not classify run" in capsys.readouterr().err