diff --git a/README.md b/README.md index 51e3a7e..f3d088a 100644 --- a/README.md +++ b/README.md @@ -262,6 +262,14 @@ claim, reruns the test command, pushes the branch, and opens a PR containing `Closes #19` plus test evidence. Replace `19` with the selected issue number; do not run without `--issue` when processing a preselected ticket. +Agent and test commands use POSIX argument quoting and run as argument vectors, +not through a shell. Issue context is available to the agent only through +`RELEASE_ISSUE_NUMBER`, `RELEASE_ISSUE_TITLE`, `RELEASE_ISSUE_BODY`, +`RELEASE_REPO`, and `RELEASE_BRANCH`; do not put issue placeholders in command +text. Shell substitutions, redirects, and pipelines are intentionally not +interpreted. Put any trusted shell workflow in a reviewed wrapper script and +configure that script as `RELEASE_AGENT_COMMAND` instead. + Durable state defaults to `.release-engine/state.json`. Full behavior and safety gates are documented in [`docs/release-engine-spec.md`](docs/release-engine-spec.md). diff --git a/docs/release-engine-spec.md b/docs/release-engine-spec.md index feb4f51..3ae4c13 100644 --- a/docs/release-engine-spec.md +++ b/docs/release-engine-spec.md @@ -12,8 +12,8 @@ Convert one eligible Gitea issue into a tested, traceable pull request without d - Repository key (`owner/repo`). - Agent username. - Optional explicit issue number; otherwise deterministic queue selection. -- Coding-agent command template. -- Test command. +- Coding-agent command, parsed into an argument vector. +- Test command, parsed into an argument vector. - Local repository path and durable state-file path. ## State machine @@ -39,7 +39,10 @@ Terminal failure states are `claim_failed`, `agent_failed`, `tests_failed`, and ## Execution - Branch format: `/-`. -- Coding command receives issue number, title, body, repo, and branch through template fields and environment variables. +- Coding and test commands are parsed with POSIX argument quoting and executed directly without a shell. +- Issue number, title, body, repo, and branch are supplied to the coding agent only through `RELEASE_ISSUE_NUMBER`, `RELEASE_ISSUE_TITLE`, `RELEASE_ISSUE_BODY`, `RELEASE_REPO`, and `RELEASE_BRANCH` environment variables. Gitea-controlled content is never interpolated into executable syntax. +- Shell operators, substitutions, and pipelines are not interpreted. Operators that are intentionally required must live in a separately reviewed wrapper script configured as the command. +- Empty or malformed commands abort before the issue claim. - Non-zero coding-agent exit blocks tests and PR creation. - Tests run using the configured command; stdout/stderr and exit code become evidence. diff --git a/src/release_engine.py b/src/release_engine.py index e86a74b..ae4305e 100644 --- a/src/release_engine.py +++ b/src/release_engine.py @@ -63,7 +63,7 @@ class Client(Protocol): class Runner(Protocol): - def run(self, command: str, cwd: Path, env: dict[str, str] | None = None) -> CommandResult: ... + def run(self, command: list[str], cwd: Path, env: dict[str, str] | None = None) -> CommandResult: ... class StateStore: @@ -113,8 +113,18 @@ def branch_name(agent: str, issue: Issue) -> str: return f"{actor}/{issue.number}-{slug or 'issue'}" +def parse_command(command: str, name: str) -> list[str]: + try: + arguments = shlex.split(command) + except ValueError as exc: + raise ValueError(f"invalid {name}: {exc}") from exc + if not arguments: + raise ValueError(f"invalid {name}: command is empty") + return arguments + + class ShellRunner: - def run(self, command: str, cwd: Path, env: dict[str, str] | None = None) -> CommandResult: + def run(self, command: list[str], cwd: Path, env: dict[str, str] | None = None) -> CommandResult: merged_env = os.environ.copy() if env: merged_env.update(env) @@ -122,7 +132,7 @@ class ShellRunner: command, cwd=str(cwd), env=merged_env, - shell=True, + shell=False, text=True, capture_output=True, ) @@ -220,6 +230,9 @@ class ReleaseEngine: "agent": self.config.agent, } + agent_command = parse_command(self.config.agent_command, "agent command") + test_command = parse_command(self.config.test_command, "test command") + self.client.claim_issue(self.config.repo, issue.number, self.config.agent) verified = self.client.get_issue(self.config.repo, issue.number) if verified.assignee != self.config.agent: @@ -228,19 +241,11 @@ class ReleaseEngine: ) self.store.save(RunState(issue.number, branch, "claimed")) - branch_result = self.runner.run(f"git checkout -B {shlex.quote(branch)}", self.config.repo_path) + branch_result = self.runner.run(["git", "checkout", "-B", branch], self.config.repo_path) if branch_result.returncode != 0: raise RuntimeError(f"branch creation failed: {branch_result.stderr.strip()}") - context = { - "issue_number": issue.number, - "issue_title": issue.title, - "issue_body": issue.body, - "repo": self.config.repo, - "branch": branch, - } - command = self.config.agent_command.format(**context) - agent_result = self.runner.run(command, self.config.repo_path, { + agent_result = self.runner.run(agent_command, self.config.repo_path, { "RELEASE_ISSUE_NUMBER": str(issue.number), "RELEASE_ISSUE_TITLE": issue.title, "RELEASE_ISSUE_BODY": issue.body, @@ -252,14 +257,14 @@ class ReleaseEngine: raise RuntimeError(f"coding agent failed: {agent_result.stderr.strip()}") self.store.save(RunState(issue.number, branch, "agent_complete")) - test_result = self.runner.run(self.config.test_command, self.config.repo_path) + test_result = self.runner.run(test_command, self.config.repo_path) evidence = (test_result.stdout + "\n" + test_result.stderr).strip()[-4000:] if test_result.returncode != 0: self.store.save(RunState(issue.number, branch, "tests_failed", evidence)) raise RuntimeError(f"tests failed: {evidence}") self.store.save(RunState(issue.number, branch, "tests_passed", evidence)) - push_result = self.runner.run(f"git push -u origin {shlex.quote(branch)}", self.config.repo_path) + push_result = self.runner.run(["git", "push", "-u", "origin", branch], self.config.repo_path) if push_result.returncode != 0: self.store.save(RunState(issue.number, branch, "push_failed", push_result.stderr[-2000:])) raise RuntimeError(f"push failed: {push_result.stderr.strip()}") diff --git a/tests/test_release_engine.py b/tests/test_release_engine.py index 3286f40..1926725 100644 --- a/tests/test_release_engine.py +++ b/tests/test_release_engine.py @@ -1,4 +1,5 @@ import json +import sys import threading from http.server import BaseHTTPRequestHandler, HTTPServer from pathlib import Path @@ -12,12 +13,34 @@ from src.release_engine import ( Issue, ReleaseEngine, RunState, + ShellRunner, StateStore, branch_name, select_issue, ) +def test_shell_runner_treats_metacharacters_as_literal_arguments(tmp_path): + output = tmp_path / "argument.txt" + injected = tmp_path / "injected.txt" + payload = f"release title; touch {injected}" + + result = ShellRunner().run( + [ + sys.executable, + "-c", + "from pathlib import Path; import sys; Path(sys.argv[1]).write_text(sys.argv[2])", + str(output), + payload, + ], + tmp_path, + ) + + assert result.returncode == 0 + assert output.read_text() == payload + assert not injected.exists() + + def test_stale_token_error_explains_how_to_restore_cron_authentication(): class UnauthorizedHandler(BaseHTTPRequestHandler): def do_GET(self): @@ -70,9 +93,11 @@ class FakeRunner: def __init__(self, results): self.results = list(results) self.commands = [] + self.environments = [] def run(self, command, cwd, env=None): self.commands.append(command) + self.environments.append(env) return self.results.pop(0) @@ -91,7 +116,7 @@ def config(tmp_path): agent="timmy", repo_path=tmp_path, state_path=tmp_path / "state.json", - agent_command="agent --issue {issue_number}", + agent_command="agent --issue-from-env", test_command="pytest -q", ) @@ -121,6 +146,24 @@ def test_claim_verification_blocks_execution(tmp_path): assert runner.commands == [] +def test_empty_agent_command_fails_before_claim(tmp_path): + issue = Issue(14, "Engine", "body", "open", [], None) + client = FakeClient([issue]) + cfg = EngineConfig( + repo="stackchain/stackchain-dashboard", + agent="timmy", + repo_path=tmp_path, + state_path=tmp_path / "state.json", + agent_command=" ", + test_command="pytest -q", + ) + + with pytest.raises(ValueError, match="invalid agent command: command is empty"): + ReleaseEngine(cfg, client, FakeRunner([])).execute(issue_number=14) + + assert client.claimed == [] + + def test_agent_failure_blocks_tests_and_pr(tmp_path): issue = Issue(14, "Engine", "body", "open", [], None) client = FakeClient([issue]) @@ -134,12 +177,41 @@ def test_agent_failure_blocks_tests_and_pr(tmp_path): engine.execute(issue_number=14) assert runner.commands == [ - "git checkout -B timmy/14-engine", - "agent --issue 14", + ["git", "checkout", "-B", "timmy/14-engine"], + ["agent", "--issue-from-env"], ] assert client.created_prs == [] +def test_issue_content_reaches_agent_only_through_environment(tmp_path): + title = "$(touch /tmp/title-injected); `id`" + body = "line one\n&& touch /tmp/body-injected > stolen" + issue = Issue(14, title, body, "open", [], None) + client = FakeClient([issue]) + runner = FakeRunner([ + CommandResult(0, "", ""), + CommandResult(1, "", "agent stopped"), + ]) + cfg = EngineConfig( + repo="stackchain/stackchain-dashboard", + agent="timmy", + repo_path=tmp_path, + state_path=tmp_path / "state.json", + agent_command="agent --work", + test_command="pytest -q", + ) + + with pytest.raises(RuntimeError, match="coding agent failed"): + ReleaseEngine(cfg, client, runner).execute(issue_number=14) + + assert runner.commands == [ + ["git", "checkout", "-B", "timmy/14-touch-tmp-title-injected-id"], + ["agent", "--work"], + ] + assert runner.environments[1]["RELEASE_ISSUE_TITLE"] == title + assert runner.environments[1]["RELEASE_ISSUE_BODY"] == body + + def test_failed_tests_block_push_and_pr(tmp_path): issue = Issue(14, "Engine", "body", "open", [], None) client = FakeClient([issue]) @@ -153,7 +225,7 @@ def test_failed_tests_block_push_and_pr(tmp_path): with pytest.raises(RuntimeError, match="tests failed"): engine.execute(issue_number=14) - assert "git push" not in " ".join(runner.commands) + assert not any(command[:2] == ["git", "push"] for command in runner.commands) assert client.created_prs == [] @@ -171,7 +243,7 @@ def test_passing_run_pushes_and_opens_linked_pr(tmp_path): result = engine.execute(issue_number=14) assert result["pr_number"] == 44 - assert runner.commands[-1].startswith("git push -u origin") + assert runner.commands[-1] == ["git", "push", "-u", "origin", "timmy/14-engine-mvp"] assert len(client.created_prs) == 1 body = client.created_prs[0][4] assert "Closes #14" in body