refactor(executors): reimplement the Aider adapter independently

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
 

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] + ["<agent prompt redacted from command replay>"] + command[3:]
 
-        # The prompt can contain the mission verbatim; keep it out of the replay
-        # command recorded on the receipt.
-        redacted_command = (
-            command[:2] + ["<agent prompt redacted from command replay>"] + 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(),

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)