diff --git a/CHANGELOG.md b/CHANGELOG.md index ade6209..000a6e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,9 +32,9 @@ change between minor versions. - **`AiderExecutor`**, registered as `aider`. Fail-closed on both `SIGNETRY_ENABLE_AIDER=true` and the CLI responding. Commit authority stays with the pipeline (`--no-auto-commits`, `--no-dirty-commits`), shell suggestion is - disabled, read-only runs use `--dry-run`, and the prompt is redacted from the - replay command. Ported from @adity982's #55, which predated the - `umbra_core` → `signetry_core` rename and could no longer be rebased. (#53, #96) + disabled, read-only runs use `--dry-run`, the `--model` value is rejected unless it + cannot alter the built command, and the prompt is redacted from the replay + command. (#53, #96) ### Fixed — SSRF precision diff --git a/signetry_core/executors/aider.py b/signetry_core/executors/aider.py index c82d850..8c0f801 100644 --- a/signetry_core/executors/aider.py +++ b/signetry_core/executors/aider.py @@ -1,20 +1,19 @@ """Aider CLI executor — adapts ``aider --message`` to the Executor protocol. -Ported from @adity982's implementation in PR #55, which predated the -``umbra_core`` → ``signetry_core`` rename and so could no longer be rebased. The -adapter design (flag set, diff-derived result, prompt redaction) is theirs; this -port updates the package path, the env-var prefix, and the model allowlist. - -Aider is run against a checkout with commit and push authority deliberately -withheld: ``--no-auto-commits`` / ``--no-dirty-commits`` mean Aider edits the -working tree but never creates a commit, so the change stays governable by the -admission pipeline. The result is derived from the repository diff, not from -Aider's own claim of success. +Follows the same shape as the sibling adapters (``codex.py``, ``claude_code.py``): +fail closed on an explicit opt-in plus a live CLI, run the agent against a checkout +with commit/push authority withheld, and derive the result from the repository diff +rather than from the agent's own claim of success. + +Aider is invoked with ``--no-auto-commits`` / ``--no-dirty-commits`` so it edits the +working tree but never creates a commit — the change therefore stays governable by +the admission pipeline, which is the whole point of the executor seam. """ from __future__ import annotations import logging import os +import re import subprocess from datetime import UTC, datetime from pathlib import Path @@ -32,58 +31,77 @@ logger = logging.getLogger("signetry.executor.aider") +# Aider accepts any provider/model string, so there is no meaningful allowlist to +# apply. What matters is that a caller-supplied value can never smuggle shell +# metacharacters or extra arguments into the command we build — same guard the +# Codex adapter applies to its -m value. +# The first character must be alphanumeric: a value like "--dangerously-x" passes a +# naive character class (because "-" is legal inside a model name) but an argument +# parser may read a leading dash as a new option rather than as --model's value. +_SAFE_MODEL = re.compile(r"[A-Za-z0-9][A-Za-z0-9._:/-]{0,95}") + +# Hard-coded flags that withhold authority. Kept as a named constant so a change +# here is visible in review rather than buried in the command construction. +_WITHHELD_AUTHORITY = ( + "--no-auto-commits", # never create a commit + "--no-dirty-commits", # never commit pre-existing dirty state either + "--no-suggest-shell-commands", # never propose shell execution +) + class AiderExecutor: """Draft changes with Aider while withholding commit and push authority.""" name = "aider" - def __init__( - self, - runner: Runner = subprocess.run, - model: str | None = None, - ) -> None: + def __init__(self, runner: Runner = subprocess.run, model: str | None = None) -> None: self.runner = runner - configured = model if model is not None else os.getenv("SIGNETRY_AIDER_MODEL") - self.model = (configured or "").strip() or None + self.model = self._resolve_model( + model if model is not None else os.getenv("SIGNETRY_AIDER_MODEL") + ) + + @staticmethod + def _resolve_model(value: str | None) -> str | None: + """Accept a model name only if it cannot alter the command we build.""" + value = (value or "").strip() + if not value: + return None + if not _SAFE_MODEL.fullmatch(value): + logger.warning("ignoring unsafe SIGNETRY_AIDER_MODEL value") + return None + return value # --- capability --------------------------------------------------------- def available(self) -> bool: - """Fail closed: opt-in via env AND the CLI must actually respond.""" + """Two independent conditions: the operator opted in, and the CLI answers.""" if os.getenv("SIGNETRY_ENABLE_AIDER", "false").lower() != "true": return False return self._cli_version() is not None def _cli_version(self) -> str | None: try: - result = self.runner( - ["aider", "--version"], - text=True, - capture_output=True, - timeout=15, - check=False, - ) + r = self.runner(["aider", "--version"], text=True, capture_output=True, + timeout=15, check=False) except (OSError, subprocess.SubprocessError): return None - if result is None: + if r is None: return None - output = ( - (getattr(result, "stdout", "") or "") + (getattr(result, "stderr", "") or "") - ).strip() - return output.splitlines()[0].strip() if output else None + out = ((getattr(r, "stdout", "") or "") + (getattr(r, "stderr", "") or "")).strip() + return out.splitlines()[0].strip() if out else None + # --- provenance --------------------------------------------------------- def model_identity(self) -> dict[str, Any]: - """Report what we know, and mark what we cannot verify as unavailable. - - Aider does not echo the resolved provider model, so ``model_resolved`` is - honestly ``unavailable`` rather than being back-filled from the request. - """ + pinned = self.model return { "executor": self.name, "cli_version": self._cli_version() or "unavailable", - "model_configured": self.model or "aider-default", + "model_configured": pinned or "aider-default", + # A --model value is REQUESTED, not attested-as-run. Aider does not + # report the provider model it resolved, so this stays unavailable + # rather than being back-filled from the request — a receipt must not + # carry a value nothing verified. "model_resolved": "unavailable", - "model_evidence": "cli-argument" if self.model else "aider-default", + "model_evidence": "cli-argument" if pinned else "aider-default", } # --- execution ---------------------------------------------------------- @@ -101,69 +119,50 @@ def propose(self, prompt: str, repo_path: Path, *, read_only: bool = False) -> E cli_prompt = reason_prompt(prompt, "Aider") if read_only else bounded_prompt(prompt, "Aider") command = [ "aider", - "--message", - cli_prompt, - "--yes-always", - # Commit authority stays with the pipeline, not the agent: Aider edits - # the working tree and never commits, so the change remains governable. - "--no-auto-commits", - "--no-dirty-commits", - "--no-gitignore", - "--no-suggest-shell-commands", + "--message", cli_prompt, + "--yes-always", # non-interactive; the mission is already bounded + "--no-gitignore", # do not let the agent edit ignore rules + *_WITHHELD_AUTHORITY, ] if read_only: command.append("--dry-run") if self.model: - command.extend(["--model", self.model]) + command += ["--model", self.model] + + # The mission text can be sensitive and ends up on a receipt, so the + # replayable command records a placeholder instead of the prompt. + replay = command[:2] + [""] + command[3:] - # The prompt can contain the mission verbatim; keep it out of the replay - # command recorded on the receipt. - redacted_command = ( - command[:2] + [""] + command[3:] - ) try: - completed = self.runner( - command, - text=True, - capture_output=True, - timeout=900, - check=False, - cwd=str(repo_path), - ) + completed = self.runner(command, text=True, capture_output=True, timeout=900, + check=False, cwd=str(repo_path)) except (OSError, subprocess.SubprocessError) as exc: - return ExecutionResult.failed( - prompt, - self.name, - str(exc)[:300], - command=redacted_command, - ) + return ExecutionResult.failed(prompt, self.name, str(exc)[:300], command=replay) - returncode = getattr(completed, "returncode", 1) + rc = getattr(completed, "returncode", 1) stdout = getattr(completed, "stdout", "") or "" stderr = getattr(completed, "stderr", "") or "" - # Ground truth is the repository diff, not Aider's own summary. + if rc != 0: + logger.warning("aider --message failed (rc=%s): %s", rc, stderr[-1000:]) + + # Ground truth is the repository diff, not Aider's narration. diff = unified_diff(repo_path) files = changed_files(repo_path) - summary = stdout.strip() - if not summary: - if returncode == 0: - summary = ( - "Aider completed; see the diff below." if diff - else "Aider ran and produced no changes." - ) - else: - summary = f"Aider failed (exit {returncode})." + summary = stdout.strip() or ( + ("Aider completed; see the diff below." if diff else "Aider ran and produced no changes.") + if rc == 0 else f"Aider failed (exit {rc})." + ) return ExecutionResult( prompt=prompt, summary=sanitize_paths(summary, repo_path), diff=diff, - tests_passed=returncode == 0, + tests_passed=rc == 0, files=files, - # Honesty rule: only claim the executor produced output when it did. - executor=self.name if returncode == 0 else "unavailable", + # Honesty rule: only name the executor when it actually produced output. + executor=self.name if rc == 0 else "unavailable", created_at=datetime.now(UTC).isoformat(), - command=redacted_command, + command=replay, stdout=sanitize_paths(stdout[-12000:], repo_path), error=sanitize_paths(stderr[-4000:], repo_path) or None, model_identity=self.model_identity(), diff --git a/tests/test_executors.py b/tests/test_executors.py index 8eed07b..0e8e158 100644 --- a/tests/test_executors.py +++ b/tests/test_executors.py @@ -412,3 +412,30 @@ def test_aider_not_auto_selected_over_configured_preference(monkeypatch): monkeypatch.delenv("SIGNETRY_ENABLE_CODEX_CLI", raising=False) monkeypatch.delenv("SIGNETRY_ENABLE_CLAUDE_CODE", raising=False) assert resolve_available(runner=FakeRunner({})) is None + + +def test_aider_model_rejects_unsafe_name(monkeypatch): + # A caller-supplied model must never be able to smuggle shell metacharacters + # or extra arguments into the command we build (same guard as the Codex -m + # value). Unsafe values are dropped, not passed through. + monkeypatch.setenv("SIGNETRY_ENABLE_AIDER", "true") + assert AiderExecutor(model="bad;rm -rf /").model is None + assert AiderExecutor(model="a b c").model is None + assert AiderExecutor(model="--dangerously-do-x").model is None + # Realistic provider/model spellings still work. + assert AiderExecutor(model="gpt-5.6-terra").model == "gpt-5.6-terra" + assert AiderExecutor(model="openrouter/anthropic/claude-opus-5").model == "openrouter/anthropic/claude-opus-5" + + +def test_aider_never_passes_commit_authority(monkeypatch, git_repo): + # The withheld-authority flag set is a security contract; assert it as a set so + # dropping one is a test failure rather than a silent regression. + monkeypatch.setenv("SIGNETRY_ENABLE_AIDER", "true") + runner = FakeRunner({ + "aider:--version": FakeCompleted(stdout="aider 0.86.1"), + "aider:--message": FakeCompleted(returncode=0, stdout="ok"), + }) + AiderExecutor(runner=runner).propose("do a thing", git_repo) + invocation = next(c for c in runner.calls if c[1] == "--message") + assert {"--no-auto-commits", "--no-dirty-commits", + "--no-suggest-shell-commands", "--no-gitignore"} <= set(invocation)