From c3addd91fc579677a8bb664e6d86fa9cdc89e5b8 Mon Sep 17 00:00:00 2001 From: Binay Date: Tue, 18 Aug 2026 17:05:31 -0400 Subject: [PATCH] refactor(executors): reimplement the Aider adapter independently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the provenance question flagged in #99. The version merged in #96 was a port of an unsigned contribution (#55). Without a signed CLA the copyright assignment never happened, so keeping derived code — especially after removing the attribution — was the least defensible combination. This rewrites the adapter from the sibling adapters (codex.py / claude_code.py, both owner-authored) so nothing in the tree derives from an unassigned contribution, and drops the attribution from the module docstring and the v0.7.0 changelog entry. Behaviour is unchanged: all 33 executor tests written against the previous implementation pass without modification, because they assert behaviour rather than structure. Two things the rewrite adds, both from house patterns the port lacked: * --model is validated before use, like the Codex adapter's -m value, so a caller-supplied string cannot smuggle shell metacharacters or extra arguments into the command. The first character must be alphanumeric — my own test caught that a naive character class accepts "--dangerously-x", since "-" is legal inside a model name, and an argument parser may read a leading dash as a new option rather than as --model's value. * The withheld-authority flags are a named constant, so removing one shows up in review instead of disappearing into the command construction. A test asserts the set, not just individual flags. Also logs a non-zero exit like claude_code.py does, which the port did not. --- CHANGELOG.md | 6 +- signetry_core/executors/aider.py | 161 +++++++++++++++---------------- tests/test_executors.py | 27 ++++++ 3 files changed, 110 insertions(+), 84 deletions(-) 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)