diff --git a/src/mergai/agents/claude_cli.py b/src/mergai/agents/claude_cli.py index 7614876..7860c10 100644 --- a/src/mergai/agents/claude_cli.py +++ b/src/mergai/agents/claude_cli.py @@ -5,9 +5,70 @@ 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 +# 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. @@ -22,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) @@ -296,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: @@ -344,7 +414,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..fd8802a 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 @@ -66,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", ] @@ -97,7 +103,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/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/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_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" 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 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) 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