diff --git a/changelog.d/tsk-2z6kr6-agent-versions-findings.md b/changelog.d/tsk-2z6kr6-agent-versions-findings.md index c396db3bf..08993dafe 100644 --- a/changelog.d/tsk-2z6kr6-agent-versions-findings.md +++ b/changelog.d/tsk-2z6kr6-agent-versions-findings.md @@ -1,8 +1,7 @@ ### Fixed -- `InvalidRemoteError` is now caught in all agent version routes, returning 400 instead of 500 for malformed remotes. +- `InvalidContainerTargetError` (malformed agent name or remote) is now caught in all agent version routes, returning 400 instead of 500. - Agent state revert now uses dedicated `DirtyTreeError` and `NotAncestorError` exceptions instead of string matching. - `git rev-parse HEAD` return code is now checked in `git_revert`. - Agent version routes now enforce owner-or-admin authorization on list, diff, and revert operations. - Cross-process lock serializes state writers (committer and revert) to prevent lost commits. - Committer startup failures are now reported as `committer_failed` steps. -- `.aws/`, `credentials`, and `*.credentials` patterns added to agent state `.gitignore`. diff --git a/changelog.d/tsk-xa76qz-agent-versioning-allowlist.md b/changelog.d/tsk-xa76qz-agent-versioning-allowlist.md new file mode 100644 index 000000000..ac3c6e7a8 --- /dev/null +++ b/changelog.d/tsk-xa76qz-agent-versioning-allowlist.md @@ -0,0 +1,8 @@ +### Security +- Agent state versioning now versions an explicit allowlist of state paths (workspace, memory, per-framework AGENTS.md) instead of denying a list of secret patterns, so framework config carrying API keys and bridge tokens (`.hermes/config.yaml`, `.openclaw/env`), shell history, credential files and cache trees can no longer enter the agent's git history. + +### Fixed +- Agent state revert decides "noop" versus "reverted" inside the state lock, so a commit landing between resolving the requested version and the reset can no longer make the revert a silent no-op. +- Unknown revisions are reported as 404 whatever wording the installed git uses ("bad revision", "unknown revision", "ambiguous argument", "bad object") instead of 409 container_unreachable. +- A deployment whose auto-committer never starts now reports `versioning: false` with the reason, instead of claiming versioning is on while no commits will ever happen. +- The auto-committer now computes its changed-file summary from the staged index after `git add -A`, so a new untracked file (the common agent change) is named in the commit subject instead of falling back to a bare "auto-commit". diff --git a/changelog.d/tsk-yn5gze-agent-versions-fixes.md b/changelog.d/tsk-yn5gze-agent-versions-fixes.md index e6721448b..e026347a1 100644 --- a/changelog.d/tsk-yn5gze-agent-versions-fixes.md +++ b/changelog.d/tsk-yn5gze-agent-versions-fixes.md @@ -3,4 +3,4 @@ - Agent state version revert now restores the full snapshot at the target commit instead of inverting a single commit. `git_revert` runs `git revert --no-edit ..HEAD` so the tree matches the requested commit's state, and the revert endpoint asserts `README.md` remains with `notes.txt` absent after the operation. - `.taos/trace/` is now excluded from the agent state gitignore before the initial commit, preventing trace directory contents from being staged into git history. - Remote agent container targets are persisted in the agent record and used for all version operations, so remote-deployed agents resolve to `:taos-agent-{name}` instead of the unqualified local name. -- The `sha` path parameter on version diff and revert routes is validated against `^[0-9a-f]{4,40}$` before it reaches any git argv, preventing argument injection such as `--output=.bashrc`. +- The `sha` path parameter on version diff and revert routes is validated against `^[0-9a-fA-F]{7,40}$` (hex is case-insensitive) before it reaches any git argv, preventing argument injection such as `--output=.bashrc`. diff --git a/tests/test_agent_committer.py b/tests/test_agent_committer.py index 93c7dbfdc..b8137e19f 100644 --- a/tests/test_agent_committer.py +++ b/tests/test_agent_committer.py @@ -2,44 +2,53 @@ from __future__ import annotations import importlib.util -import os import subprocess import sys +from pathlib import Path import pytest -_COMMITTER_PATH = ( - os.path.dirname(__file__).replace("tests", "tinyagentos") + "/scripts/agent_committer.py" +_COMMITTER_PATH = str( + Path(__file__).resolve().parent.parent + / "tinyagentos" + / "scripts" + / "agent_committer.py" ) -def _load_committer(repo_path: str, interval: int = 1): +def _load_committer(repo_path, tmp_path, interval: int = 1): spec = importlib.util.spec_from_file_location("agent_committer", _COMMITTER_PATH) mod = importlib.util.module_from_spec(spec) spec.loader.exec_module(mod) - mod.REPO_PATH = repo_path + mod.REPO_PATH = str(repo_path) mod.INTERVAL = interval + # The lock file must live outside the repo under test: inside it, it + # would show up as an untracked file and would either get committed by + # `git add -A` or keep the tree permanently "dirty" for `_is_dirty()`. + mod._STATE_LOCK_PATH = str(tmp_path / "agent_state.lock") return mod -def _init_repo(tmp_path): - subprocess.run(["git", "init", "-b", "main"], cwd=tmp_path, check=True, capture_output=True) - subprocess.run(["git", "config", "user.email", "test@test"], cwd=tmp_path, check=True, capture_output=True) - subprocess.run(["git", "config", "user.name", "test"], cwd=tmp_path, check=True, capture_output=True) +def _init_repo(repo_path: Path): + subprocess.run(["git", "init", "-b", "main"], cwd=repo_path, check=True, capture_output=True) + subprocess.run(["git", "config", "user.email", "test@test"], cwd=repo_path, check=True, capture_output=True) + subprocess.run(["git", "config", "user.name", "test"], cwd=repo_path, check=True, capture_output=True) class TestAgentCommitter: def test_commit_creates_commit_for_new_file(self, tmp_path): - _init_repo(tmp_path) - (tmp_path / ".gitignore").write_text("*.secret\n.env\n*token*\n") - subprocess.run(["git", "add", ".gitignore"], cwd=tmp_path, check=True, capture_output=True) + repo = tmp_path / "repo" + repo.mkdir() + _init_repo(repo) + (repo / ".gitignore").write_text("*.secret\n.env\n*token*\n") + subprocess.run(["git", "add", ".gitignore"], cwd=repo, check=True, capture_output=True) subprocess.run( - ["git", "commit", "-m", "initial"], cwd=tmp_path, check=True, capture_output=True + ["git", "commit", "-m", "initial"], cwd=repo, check=True, capture_output=True ) - committer = _load_committer(str(tmp_path)) - (tmp_path / "hello.txt").write_text("hello") + committer = _load_committer(repo, tmp_path) + (repo / "hello.txt").write_text("hello") committer._commit() rc, out, _ = committer._git("log", "--oneline") @@ -49,16 +58,18 @@ def test_commit_creates_commit_for_new_file(self, tmp_path): assert "hello.txt" in stat def test_gitignored_secret_not_committed(self, tmp_path): - _init_repo(tmp_path) - (tmp_path / ".gitignore").write_text("*.secret\n.env\n*token*\n") - subprocess.run(["git", "add", ".gitignore"], cwd=tmp_path, check=True, capture_output=True) + repo = tmp_path / "repo" + repo.mkdir() + _init_repo(repo) + (repo / ".gitignore").write_text("*.secret\n.env\n*token*\n") + subprocess.run(["git", "add", ".gitignore"], cwd=repo, check=True, capture_output=True) subprocess.run( - ["git", "commit", "-m", "initial"], cwd=tmp_path, check=True, capture_output=True + ["git", "commit", "-m", "initial"], cwd=repo, check=True, capture_output=True ) - committer = _load_committer(str(tmp_path)) - (tmp_path / ".env").write_text("SECRET=abc") - (tmp_path / "token.rsa").write_text("key") + committer = _load_committer(repo, tmp_path) + (repo / ".env").write_text("SECRET=abc") + (repo / "token.rsa").write_text("key") committer._commit() _, log_out, _ = committer._git("log", "--all", "--stat") @@ -66,14 +77,16 @@ def test_gitignored_secret_not_committed(self, tmp_path): assert "token.rsa" not in log_out def test_no_commit_when_clean(self, tmp_path): - _init_repo(tmp_path) - (tmp_path / ".gitignore").write_text("") - subprocess.run(["git", "add", ".gitignore"], cwd=tmp_path, check=True, capture_output=True) + repo = tmp_path / "repo" + repo.mkdir() + _init_repo(repo) + (repo / ".gitignore").write_text("") + subprocess.run(["git", "add", ".gitignore"], cwd=repo, check=True, capture_output=True) subprocess.run( - ["git", "commit", "-m", "initial"], cwd=tmp_path, check=True, capture_output=True + ["git", "commit", "-m", "initial"], cwd=repo, check=True, capture_output=True ) - committer = _load_committer(str(tmp_path)) + committer = _load_committer(repo, tmp_path) committer._commit() rc, out, _ = committer._git("log", "--oneline") @@ -83,18 +96,47 @@ def test_no_commit_when_clean(self, tmp_path): assert lines[0].endswith("initial") def test_gitignored_ssh_key_not_committed(self, tmp_path): - _init_repo(tmp_path) - (tmp_path / ".gitignore").write_text("*.secret\n.env\n*token*\n.ssh/\n") - subprocess.run(["git", "add", ".gitignore"], cwd=tmp_path, check=True, capture_output=True) + repo = tmp_path / "repo" + repo.mkdir() + _init_repo(repo) + (repo / ".gitignore").write_text("*.secret\n.env\n*token*\n.ssh/\n") + subprocess.run(["git", "add", ".gitignore"], cwd=repo, check=True, capture_output=True) subprocess.run( - ["git", "commit", "-m", "initial"], cwd=tmp_path, check=True, capture_output=True + ["git", "commit", "-m", "initial"], cwd=repo, check=True, capture_output=True ) - committer = _load_committer(str(tmp_path)) - (tmp_path / ".ssh").mkdir() - (tmp_path / ".ssh" / "id_rsa").write_text("fake-key") + committer = _load_committer(repo, tmp_path) + (repo / ".ssh").mkdir() + (repo / ".ssh" / "id_rsa").write_text("fake-key") committer._commit() _, log_out, _ = committer._git("log", "--all", "--stat") assert ".ssh" not in log_out assert "id_rsa" not in log_out + + def test_commit_message_names_a_new_untracked_file(self, tmp_path): + """`_changed_summary` must be computed from what `git add -A` staged, + not from what was staged before it ran: an untracked file (the + common agent change — a new workspace/ file) shows up in neither + `git diff --cached --name-only` nor `git diff --name-only` before + staging, so a pre-add summary always falls back to "auto-commit" and + the commit message loses the file name for exactly this case.""" + repo = tmp_path / "repo" + repo.mkdir() + _init_repo(repo) + (repo / ".gitignore").write_text("") + subprocess.run(["git", "add", ".gitignore"], cwd=repo, check=True, capture_output=True) + subprocess.run( + ["git", "commit", "-m", "initial"], cwd=repo, check=True, capture_output=True + ) + + committer = _load_committer(repo, tmp_path) + (repo / "workspace_notes.md").write_text("new untracked file") + committer._commit() + + rc, out, _ = committer._git("log", "-1", "--format=%s") + assert rc == 0 + subject = out.strip() + assert "workspace_notes.md" in subject, ( + f"commit subject does not name the new file: {subject!r}" + ) diff --git a/tests/test_agent_git.py b/tests/test_agent_git.py index 0e924fbec..5b79d9b98 100644 --- a/tests/test_agent_git.py +++ b/tests/test_agent_git.py @@ -1,20 +1,211 @@ -"""Unit tests for the agent state versioning git helpers.""" +"""Tests for the agent state repo helpers in ``tinyagentos.agent_git``. + +The repo root *is* the agent home (``/root``), so anything ``.gitignore`` +fails to exclude enters git history inside the container the moment the +deployer or the auto-committer runs ``git add -A``. The versioning-scope +tests below therefore build a throwaway repo from the module's real +``_GITIGNORE_CONTENTS`` and assert, one path at a time, which paths are +tracked — a coarser assertion (e.g. "no file named .env.local") cannot +catch the next framework's config file. +""" from __future__ import annotations +import shutil import subprocess +from pathlib import Path +from unittest.mock import AsyncMock, patch import pytest -from unittest.mock import AsyncMock, patch -from tinyagentos.agent_git import ( - ContainerUnreachableError, - DirtyTreeError, - NotAncestorError, - _GITIGNORE_CONTENTS, - git_diff, - git_rev_parse, - git_revert, -) +from tinyagentos import agent_git +# Imported from the deployer (its public name) on purpose: the point of the +# test is that the versioned scope is derived from this map, not duplicated. +from tinyagentos.deployer import AGENTS_MD_PATHS + + +# Paths the deployer, the frameworks or the shell write into the agent home +# that must never be versioned: credentials on the left, bulk trees and +# machine-local noise on the right. +SECRET_AND_BULK_PATHS = [ + ".hermes/config.yaml", # model.api_key — install_hermes.sh patches it in + ".hermes/.env", # OPENAI_API_KEY / API_SERVER_KEY + ".openclaw/env", # TAOS_BRIDGE_TOKEN + OPENAI_API_KEY + ".openclaw/openclaw.json", # bridge connection info + ".env", + ".env.local", + ".netrc", + ".git-credentials", + ".npmrc", + ".config/gh/hosts.yml", # GitHub OAuth token (Secrets app) + ".kube/config", + ".aws/credentials", + ".ssh/id_ed25519", + ".bash_history", # every command the agent ran, inline tokens included + ".cache/pip/wheel.whl", + ".local/share/uv/tool.bin", + ".venv/lib/site.py", + ".npm/_cacache/index", + ".taos/committer.log", # the committer's own log — would dirty the tree it commits + ".taos/trace/events.jsonl", # bind mount from the host +] + +# Agent state the feature exists to version. +STATE_PATHS = [ + ".gitignore", + "AGENTS.md", + "workspace/notes.md", + "workspace/nested/deep/file.txt", + "memory/facts.md", + "memory/nested/deep/file.txt", + *sorted(p.removeprefix("/root/") for p in AGENTS_MD_PATHS.values()), +] + + +def _write(repo: Path, rel: str, content: str = "x") -> None: + target = repo / rel + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content) + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True + ) + + +@pytest.fixture(scope="module") +def tracked_paths(tmp_path_factory) -> set[str]: + """Paths ``git add -A`` tracks in a repo carrying the real .gitignore.""" + repo = tmp_path_factory.mktemp("agent_home") + (repo / ".gitignore").write_text(agent_git._GITIGNORE_CONTENTS) + for rel in SECRET_AND_BULK_PATHS + STATE_PATHS: + if rel != ".gitignore": + _write(repo, rel) + assert _git(repo, "init", "-b", "main").returncode == 0 + _git(repo, "config", "user.email", "agent@taos.local") + _git(repo, "config", "user.name", "test-agent") + add = _git(repo, "add", "-A") + assert add.returncode == 0, add.stderr + listed = _git(repo, "ls-files") + assert listed.returncode == 0, listed.stderr + return {line.strip() for line in listed.stdout.splitlines() if line.strip()} + + +@pytest.mark.parametrize("rel", SECRET_AND_BULK_PATHS) +def test_secret_and_bulk_paths_are_not_versioned(rel, tracked_paths): + assert rel not in tracked_paths, f"{rel} would be committed into agent state history" + + +@pytest.mark.parametrize("rel", STATE_PATHS) +def test_state_paths_are_versioned(rel, tracked_paths): + assert rel in tracked_paths, f"{rel} is agent state but is not versioned" + + +def test_unknown_framework_config_is_excluded_by_default(tmp_path): + """The scope is an allowlist: a framework nobody has written yet drops a + config file with an api_key in it, and it must be out of scope without + anyone adding a pattern for it.""" + repo = tmp_path / "home" + repo.mkdir() + (repo / ".gitignore").write_text(agent_git._GITIGNORE_CONTENTS) + _write(repo, ".futureframework/settings.json", '{"api_key": "sk-live-abc"}') + _write(repo, "future-agent-cli/credentials.toml", "token = 'abc'") + assert _git(repo, "init", "-b", "main").returncode == 0 + assert _git(repo, "add", "-A").returncode == 0 + tracked = _git(repo, "ls-files").stdout + assert ".futureframework/settings.json" not in tracked + assert "future-agent-cli/credentials.toml" not in tracked + + +def test_versioned_scope_is_a_single_constant(): + """A new framework adds a state path, never a secret pattern.""" + assert agent_git._STATE_PATHS + for framework_path in AGENTS_MD_PATHS.values(): + assert framework_path.removeprefix("/root/") in agent_git._STATE_PATHS + + +class TestUnknownRevisionDiagnostics: + """`git show` and `git rev-parse` word a missing object differently across + git versions; both must map to 404 (unknown revision), not to 409.""" + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "stderr", + [ + "fatal: bad revision 'deadbeef'", + "fatal: ambiguous argument 'deadbeef': unknown revision or path " + "not in the working tree.", + "fatal: bad object deadbeef", + ], + ) + async def test_git_diff_reports_unknown_revision(self, stderr, monkeypatch): + async def fake_exec(container, cmd, timeout=60): + return 128, stderr + + monkeypatch.setattr(agent_git, "exec_in_container", fake_exec) + with pytest.raises(RuntimeError) as excinfo: + await agent_git.git_diff("taos-agent-test", "deadbeef") + assert not isinstance(excinfo.value, agent_git.ContainerUnreachableError) + assert "unknown revision" in str(excinfo.value) + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "stderr", + [ + "fatal: bad revision 'deadbeef'", + "fatal: ambiguous argument 'deadbeef': unknown revision or path " + "not in the working tree.", + ], + ) + async def test_git_rev_parse_reports_unknown_revision(self, stderr, monkeypatch): + async def fake_exec(container, cmd, timeout=60): + return 128, stderr + + monkeypatch.setattr(agent_git, "exec_in_container", fake_exec) + with pytest.raises(RuntimeError) as excinfo: + await agent_git.git_rev_parse("taos-agent-test", "deadbeef") + assert not isinstance(excinfo.value, agent_git.ContainerUnreachableError) + assert "unknown revision" in str(excinfo.value) + + @pytest.mark.asyncio + async def test_exec_failure_is_still_container_unreachable(self, monkeypatch): + async def fake_exec(container, cmd, timeout=60): + return 1, "Error: Instance is not running" + + monkeypatch.setattr(agent_git, "exec_in_container", fake_exec) + with pytest.raises(agent_git.ContainerUnreachableError): + await agent_git.git_rev_parse("taos-agent-test", "deadbeef") + + + @pytest.mark.asyncio + async def test_marker_outside_a_git_diagnostic_is_still_unreachable(self, monkeypatch): + """The markers are git's words. A container-side failure that merely + quotes one of them is not a missing object.""" + async def fake_exec(container, cmd, timeout=60): + return 1, "Error: Instance is not running (ambiguous argument)" + + monkeypatch.setattr(agent_git, "exec_in_container", fake_exec) + with pytest.raises(agent_git.ContainerUnreachableError): + await agent_git.git_rev_parse("taos-agent-test", "deadbeef") + + +class TestRevertFailureClassification: + @pytest.mark.asyncio + async def test_failed_reset_raises_git_operation_error(self, monkeypatch): + """A reset that fails on repo state is not an unreachable container: + the container answered, git could not do the work.""" + async def fake_exec(container, cmd, timeout=60): + if cmd[0] == "bash": + return 1, "fatal: Unable to write new index file" + if "merge-base" in cmd: + return 0, "" + return 0, "a" * 40 + + monkeypatch.setattr(agent_git, "exec_in_container", fake_exec) + with pytest.raises(agent_git.GitOperationError) as excinfo: + await agent_git.git_revert("taos-agent-test", "a" * 40) + assert not isinstance(excinfo.value, agent_git.ContainerUnreachableError) + assert "Unable to write new index file" in str(excinfo.value) # Real git diagnostics observed against the actual binary (not guessed text): @@ -42,8 +233,8 @@ async def test_git_rev_parse_classifies_real_messages_as_unknown_revision(self): new=AsyncMock(return_value=(128, message)), ): with pytest.raises(RuntimeError) as exc_info: - await git_rev_parse("some-container", "deadbeef") - assert not isinstance(exc_info.value, ContainerUnreachableError) + await agent_git.git_rev_parse("some-container", "deadbeef") + assert not isinstance(exc_info.value, agent_git.ContainerUnreachableError) assert "unknown revision" in str(exc_info.value).lower() async def test_git_rev_parse_other_failures_still_raise_container_unreachable(self): @@ -51,8 +242,8 @@ async def test_git_rev_parse_other_failures_still_raise_container_unreachable(se "tinyagentos.agent_git.exec_in_container", new=AsyncMock(return_value=(255, "ssh: connect to host: Connection refused\n")), ): - with pytest.raises(ContainerUnreachableError): - await git_rev_parse("some-container", "deadbeef") + with pytest.raises(agent_git.ContainerUnreachableError): + await agent_git.git_rev_parse("some-container", "deadbeef") @pytest.mark.asyncio @@ -64,8 +255,8 @@ async def test_git_diff_classifies_real_messages_as_unknown_revision(self): new=AsyncMock(return_value=(128, message)), ): with pytest.raises(RuntimeError) as exc_info: - await git_diff("some-container", "deadbeef") - assert not isinstance(exc_info.value, ContainerUnreachableError) + await agent_git.git_diff("some-container", "deadbeef") + assert not isinstance(exc_info.value, agent_git.ContainerUnreachableError) assert "unknown revision" in str(exc_info.value).lower() async def test_git_diff_other_failures_still_raise_container_unreachable(self): @@ -73,27 +264,31 @@ async def test_git_diff_other_failures_still_raise_container_unreachable(self): "tinyagentos.agent_git.exec_in_container", new=AsyncMock(return_value=(255, "ssh: connect to host: Connection refused\n")), ): - with pytest.raises(ContainerUnreachableError): - await git_diff("some-container", "deadbeef") + with pytest.raises(agent_git.ContainerUnreachableError): + await agent_git.git_diff("some-container", "deadbeef") class TestGitignoreCoversEnvVariants: - def test_gitignore_ignores_env_dotfile_variants(self, tmp_path): - (tmp_path / ".gitignore").write_text(_GITIGNORE_CONTENTS) + def test_gitignore_is_an_allowlist_that_excludes_env_variants(self, tmp_path): + """The repo scope is an allowlist (_STATE_PATHS), not a denylist of + secret patterns — a `.env.*` denylist line would be redundant with the + leading `*` and is not how the shipped .gitignore is built.""" + lines = [ + line for line in agent_git._GITIGNORE_CONTENTS.splitlines() + if line and not line.startswith("#") + ] + assert lines[0] == "*" + assert not any(line.startswith("!/.env") for line in lines) + + if shutil.which("git") is None: + pytest.skip("git not on PATH") + (tmp_path / ".gitignore").write_text(agent_git._GITIGNORE_CONTENTS) subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) - subprocess.run(["git", "config", "user.email", "agent@taos.local"], cwd=tmp_path, check=True) - subprocess.run(["git", "config", "user.name", "test-agent"], cwd=tmp_path, check=True) - (tmp_path / ".env.local").write_text("SECRET=1") - (tmp_path / ".env.production").write_text("SECRET=2") - (tmp_path / "keep.txt").write_text("fine") - subprocess.run(["git", "add", "-A"], cwd=tmp_path, check=True) - staged = subprocess.run( - ["git", "diff", "--cached", "--name-only"], - cwd=tmp_path, capture_output=True, text=True, check=True, - ).stdout.splitlines() - assert ".env.local" not in staged - assert ".env.production" not in staged - assert "keep.txt" in staged + result = subprocess.run( + ["git", "check-ignore", "-q", ".env.production"], + cwd=tmp_path, + ) + assert result.returncode == 0 @pytest.mark.asyncio @@ -108,11 +303,11 @@ async def fake_exec(container, cmd, timeout=60): if cmd[:2] == ["git", "-C"] and cmd[3] == "merge-base": return (0, "") if cmd[0] == "bash": - return (3, "") + return (agent_git._REVERT_NOOP, "") return (0, "ok") with patch("tinyagentos.agent_git.exec_in_container", new=fake_exec): - status = await git_revert("some-container", "deadbeef") + status = await agent_git.git_revert("some-container", "deadbeef") assert status == "noop" # The noop decision must be made inside the locked script, not by a # separate pre-lock "rev-parse HEAD" call. @@ -133,10 +328,29 @@ async def fake_exec(container, cmd, timeout=60): return (0, "ok") with patch("tinyagentos.agent_git.exec_in_container", new=fake_exec): - status = await git_revert("some-container", "deadbeef") + status = await agent_git.git_revert("some-container", "deadbeef") assert status == "reverted" - async def test_locked_script_other_rc_raises_dirty_tree(self): + async def test_locked_script_rc2_raises_dirty_tree(self): + async def fake_exec(container, cmd, timeout=60): + if cmd[:2] == ["git", "-C"] and cmd[3:5] == ["rev-parse", "--verify"]: + return (0, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") + if cmd[:2] == ["git", "-C"] and cmd[3] == "merge-base": + return (0, "") + if cmd[0] == "bash": + return (agent_git._REVERT_DIRTY, "") + return (0, "ok") + + with patch("tinyagentos.agent_git.exec_in_container", new=fake_exec): + with pytest.raises(agent_git.DirtyTreeError): + await agent_git.git_revert("some-container", "deadbeef") + + async def test_locked_script_other_rc_raises_git_operation_error(self): + # rc 1 is neither the reserved noop (3) nor dirty (2) exit code — + # under this branch's stricter classification it is a repo-state + # failure (GitOperationError), not the DirtyTreeError dev's looser + # script (which only distinguished rc 3 from everything else) used + # to report here. async def fake_exec(container, cmd, timeout=60): if cmd[:2] == ["git", "-C"] and cmd[3:5] == ["rev-parse", "--verify"]: return (0, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") @@ -147,8 +361,8 @@ async def fake_exec(container, cmd, timeout=60): return (0, "ok") with patch("tinyagentos.agent_git.exec_in_container", new=fake_exec): - with pytest.raises(DirtyTreeError): - await git_revert("some-container", "deadbeef") + with pytest.raises(agent_git.GitOperationError): + await agent_git.git_revert("some-container", "deadbeef") async def test_non_ancestor_raises_before_lock(self): async def fake_exec(container, cmd, timeout=60): @@ -161,5 +375,5 @@ async def fake_exec(container, cmd, timeout=60): return (0, "ok") with patch("tinyagentos.agent_git.exec_in_container", new=fake_exec): - with pytest.raises(NotAncestorError): - await git_revert("some-container", "deadbeef") + with pytest.raises(agent_git.NotAncestorError): + await agent_git.git_revert("some-container", "deadbeef") diff --git a/tests/test_deployer.py b/tests/test_deployer.py index 6c3dd59de..7c23ffb4a 100644 --- a/tests/test_deployer.py +++ b/tests/test_deployer.py @@ -1510,6 +1510,92 @@ async def mock_exec(name, cmd, **kwargs): assert result["versioning"] is False assert result["versioning_error"] is not None + @pytest.mark.asyncio + async def test_missing_committer_script_disables_versioning(self, tmp_path): + """No committer means no commits will ever happen, so the deploy result + must not claim versioning is on.""" + req = _req(data_dir=tmp_path) + + async def mock_exec(name, cmd, **kwargs): + if "hostname -I" in " ".join(cmd): + return (0, "10.0.0.5") + return (0, "ok") + + with patch("tinyagentos.deployer.create_container", new_callable=AsyncMock) as mock_create, \ + patch("tinyagentos.deployer.exec_in_container", side_effect=mock_exec), \ + patch("tinyagentos.deployer.push_file", new_callable=AsyncMock, return_value=(0, "")), \ + patch("tinyagentos.deployer.add_proxy_device", new_callable=AsyncMock, return_value={"success": True, "output": ""}), \ + patch("tinyagentos.deployer._COMMITTER_SCRIPT", tmp_path / "absent" / "agent_committer.py"), \ + patch("tinyagentos.agent_git.git_init", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.write_gitignore", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_config_user", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_add_commit", new_callable=AsyncMock): + mock_create.return_value = {"success": True, "name": "taos-agent-test"} + result = await deploy_agent(req) + assert result["success"] is True + assert "committer_failed" in result["steps"] + assert result["versioning"] is False + assert result["versioning_error"] is not None + + @pytest.mark.asyncio + async def test_committer_push_failure_disables_versioning(self, tmp_path): + req = _req(data_dir=tmp_path) + + async def mock_exec(name, cmd, **kwargs): + if "hostname -I" in " ".join(cmd): + return (0, "10.0.0.5") + return (0, "ok") + + async def mock_push(name, local_path, remote_path): + if remote_path == "/root/.taos/agent_committer.py": + return (1, "permission denied") + return (0, "") + + with patch("tinyagentos.deployer.create_container", new_callable=AsyncMock) as mock_create, \ + patch("tinyagentos.deployer.exec_in_container", side_effect=mock_exec), \ + patch("tinyagentos.deployer.push_file", side_effect=mock_push), \ + patch("tinyagentos.deployer.add_proxy_device", new_callable=AsyncMock, return_value={"success": True, "output": ""}), \ + patch("tinyagentos.agent_git.git_init", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.write_gitignore", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_config_user", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_add_commit", new_callable=AsyncMock): + mock_create.return_value = {"success": True, "name": "taos-agent-test"} + result = await deploy_agent(req) + assert result["success"] is True + assert "committer_failed" in result["steps"] + assert result["versioning"] is False + assert result["versioning_error"] is not None + + @pytest.mark.asyncio + async def test_nohup_committer_failure_disables_versioning(self, tmp_path): + req = _req(data_dir=tmp_path) + + async def mock_exec(name, cmd, **kwargs): + cmd_str = " ".join(cmd) + if "hostname -I" in cmd_str: + return (0, "10.0.0.5") + if "command -v systemctl" in cmd_str: + return (0, "no") + if "nohup python3 /root/.taos/agent_committer.py" in cmd_str: + return (1, "bash: python3: command not found") + return (0, "ok") + + with patch("tinyagentos.deployer.create_container", new_callable=AsyncMock) as mock_create, \ + patch("tinyagentos.deployer.exec_in_container", side_effect=mock_exec), \ + patch("tinyagentos.deployer.push_file", new_callable=AsyncMock, return_value=(0, "")), \ + patch("tinyagentos.deployer.add_proxy_device", new_callable=AsyncMock, return_value={"success": True, "output": ""}), \ + patch("tinyagentos.agent_git.git_init", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.write_gitignore", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_config_user", new_callable=AsyncMock), \ + patch("tinyagentos.agent_git.git_add_commit", new_callable=AsyncMock): + mock_create.return_value = {"success": True, "name": "taos-agent-test"} + result = await deploy_agent(req) + assert result["success"] is True + assert "committer_failed" in result["steps"] + assert result["versioning"] is False + assert result["versioning_error"] is not None + + @pytest.mark.asyncio async def test_committer_script_push_failure_reports_versioning_false(self, tmp_path): req = _req(data_dir=tmp_path) diff --git a/tests/test_routes_agent_versions.py b/tests/test_routes_agent_versions.py index 92f9408c8..9fd3e510e 100644 --- a/tests/test_routes_agent_versions.py +++ b/tests/test_routes_agent_versions.py @@ -2,6 +2,7 @@ from __future__ import annotations import os +import shutil import subprocess import pytest @@ -13,6 +14,14 @@ import yaml +# Revert tests exercise the real locked-script path in agent_git.git_revert, +# which shells out to the system `flock`. Skip rather than fail on a host +# that lacks it. +requires_flock = pytest.mark.skipif( + shutil.which("flock") is None, reason="flock not available" +) + + def _init_fixture_repo(path): subprocess.run(["git", "init", "-b", "main"], cwd=path, check=True, capture_output=True) subprocess.run(["git", "config", "user.email", "agent@taos.local"], cwd=path, check=True, capture_output=True) @@ -26,6 +35,12 @@ def _init_fixture_repo(path): def _fake_exec_for_repo(fixture_repo): + # The real flock target (agent_git._STATE_LOCK_PATH) is a fixed host path + # shared by every test process and every developer running the suite at + # once; redirect it into this fixture's own tmp dir so revert tests never + # serialize on, or fight over ownership of, one file on the real host. + lock_path = str(fixture_repo.parent / "agent_state.lock") + async def _fake(container, cmd, timeout=60): if cmd[0] == "git" and cmd[1] == "-C" and cmd[2] == "/root": git_args = cmd[3:] @@ -36,7 +51,11 @@ async def _fake(container, cmd, timeout=60): ) return result.returncode, result.stdout if cmd[0] == "bash" and cmd[1] == "-c": - script = cmd[2].replace("/root", str(fixture_repo)) + script = ( + cmd[2] + .replace("/root", str(fixture_repo)) + .replace("/tmp/agent_state.lock", lock_path) + ) result = subprocess.run( [cmd[0], cmd[1], script], capture_output=True, @@ -110,6 +129,7 @@ async def test_diff_injection_sha_returns_400(self, client): resp = await client.get("/api/agents/test-agent/versions/--output=.bashrc/diff") assert resp.status_code == 400 + @requires_flock async def test_revert_restores_content(self, tmp_path, client): fixture = tmp_path / "repo" fixture.mkdir() @@ -127,7 +147,6 @@ async def test_revert_restores_content(self, tmp_path, client): new=_fake_exec_for_repo(fixture), ): resp = await client.post(f"/api/agents/test-agent/versions/{first_sha}/revert") - print("RESP:", resp.status_code, resp.text) assert resp.status_code == 200 assert resp.json()["status"] == "reverted" assert (fixture / "README.md").exists() @@ -203,6 +222,7 @@ async def test_list_versions_message_with_separator_parses_correctly(self, clien assert data["versions"][0]["author_email"] == "agent@taos.local" assert data["versions"][0]["date"] == "2026-01-01 00:00:00 +0000" + @requires_flock async def test_revert_to_head_returns_noop(self, tmp_path, client): fixture = tmp_path / "repo" fixture.mkdir() @@ -219,6 +239,49 @@ async def test_revert_to_head_returns_noop(self, tmp_path, client): assert resp.status_code == 200 assert resp.json()["status"] == "noop" + @requires_flock + async def test_revert_wins_a_commit_racing_the_sha_resolution(self, tmp_path, client): + """The auto-committer can commit between resolving the requested sha and + the reset. The noop decision therefore belongs inside the flock: a sha + that was HEAD a moment ago must still be restored, not reported `noop` + while the tree sits on the committer's new commit.""" + fixture = tmp_path / "repo" + fixture.mkdir() + _init_fixture_repo(fixture) + head_sha = subprocess.run( + ["git", "-C", str(fixture), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + passthrough = _fake_exec_for_repo(fixture) + raced = [] + + async def racing_exec(container, cmd, timeout=60): + result = await passthrough(container, cmd, timeout) + if not raced and "rev-parse" in cmd: + raced.append(cmd) + (fixture / "racy.txt").write_text("written between resolve and revert") + subprocess.run( + ["git", "-C", str(fixture), "add", "racy.txt"], + check=True, capture_output=True, + ) + subprocess.run( + ["git", "-C", str(fixture), "commit", "-m", "auto: racy"], + check=True, capture_output=True, + ) + return result + + with patch("tinyagentos.agent_git.exec_in_container", new=racing_exec): + resp = await client.post(f"/api/agents/test-agent/versions/{head_sha}/revert") + assert resp.status_code == 200 + assert resp.json()["status"] == "reverted" + final_sha = subprocess.run( + ["git", "-C", str(fixture), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + assert final_sha == head_sha + assert not (fixture / "racy.txt").exists() + + @requires_flock async def test_revert_racing_commit_before_lock_is_not_falsely_noop(self, tmp_path, client): # Regression for the noop decision being made outside the lock: a # committer can create a new commit after the route resolves the @@ -277,6 +340,7 @@ async def test_revert_non_ancestor_returns_409(self, tmp_path, client): resp = await client.post(f"/api/agents/test-agent/versions/{orphan}/revert") assert resp.status_code == 409 + @requires_flock async def test_revert_dirty_tree_returns_409(self, tmp_path, client): fixture = tmp_path / "repo" fixture.mkdir() @@ -297,6 +361,57 @@ async def test_short_sha_rejected_by_versions_route(self, client): resp = await client.get("/api/agents/test-agent/versions/abc1/diff") assert resp.status_code == 400 + @requires_flock + async def test_uppercase_sha_is_accepted(self, tmp_path, client): + """Hex object names are case-insensitive to git, and every route a + user copies a sha from can hand one over uppercase.""" + fixture = tmp_path / "repo" + fixture.mkdir() + _init_fixture_repo(fixture) + head_sha = subprocess.run( + ["git", "-C", str(fixture), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + with patch( + "tinyagentos.agent_git.exec_in_container", + new=_fake_exec_for_repo(fixture), + ): + resp = await client.post( + f"/api/agents/test-agent/versions/{head_sha.upper()}/revert" + ) + assert resp.status_code == 200 + assert resp.json()["status"] == "noop" + + async def test_agent_name_that_breaks_the_container_target_returns_400(self, tmp_path): + """"remote:container" is the qualified form, so a name carrying a + colon would silently address a different remote.""" + app, token = _make_app_with_remote(tmp_path, None) + app.state.config.agents[0]["name"] = "bad:name" + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + cookies={"taos_session": token}, + event_hooks=csrf_event_hooks(), + ) as c: + resp = await c.get("/api/agents/bad:name/versions") + assert resp.status_code == 400 + assert "invalid agent name" in resp.json()["error"] + + async def test_failed_reset_returns_409_with_the_git_error(self, client): + """A repo-state failure is 409 and says what git said — not a 404 + "unknown revision" and not a bare "container_unreachable".""" + async def _fake(container, cmd, timeout=60): + if cmd[0] == "bash": + return 1, "fatal: Unable to write new index file" + if "merge-base" in cmd: + return 0, "" + return 0, "b" * 40 + + with patch("tinyagentos.agent_git.exec_in_container", new=_fake): + resp = await client.post(f"/api/agents/test-agent/versions/{'b' * 40}/revert") + assert resp.status_code == 409 + assert "Unable to write new index file" in resp.json()["error"] + async def test_list_versions_403_for_unauthorized_user(self, tmp_path): config = { "server": {"host": "0.0.0.0", "port": 6969}, diff --git a/tinyagentos/agent_git.py b/tinyagentos/agent_git.py index 2bb2ebaa3..ffc2ef365 100644 --- a/tinyagentos/agent_git.py +++ b/tinyagentos/agent_git.py @@ -2,13 +2,17 @@ All container interactions go through ``exec_in_container`` and ``push_file`` so the same helpers work for both LXC and Docker backends. + +The repo root is the agent's whole home directory and every commit is made +with ``git add -A``, so the versioned scope is an ALLOWLIST (``_STATE_PATHS``) +rather than a list of secret patterns to deny — see ``_build_gitignore``. """ from __future__ import annotations import logging import os import tempfile -from typing import List +from typing import Iterable, List, NoReturn from tinyagentos.containers import exec_in_container, push_file @@ -18,6 +22,12 @@ _STATE_LOCK_PATH = "/tmp/agent_state.lock" +# The first `git add -A` only descends into the allowlisted paths below, so it +# never walks .cache/, .local/ or .venv/ — but an image that ships a populated +# workspace/ can still outlast an ordinary git call on Pi-class storage, and a +# timeout there disables versioning for the whole deployment. +_INITIAL_COMMIT_TIMEOUT = 300 + class DirtyTreeError(RuntimeError): pass @@ -30,52 +40,132 @@ class NotAncestorError(RuntimeError): class ContainerUnreachableError(RuntimeError): pass -_GITIGNORE_CONTENTS = """\ -.env -.env.* -*.cred -*token* -*.pem -*.p12 -*.key -*.secret -.ssh/ -caches/ -venv/ -node_modules/ -.browser_profiles/ -__pycache__/ -*.pyc -.taos/trace/ -.aws/ -credentials -*.credentials -""" - -async def _git(container: str, args: List[str], timeout: int = 60) -> tuple[int, str]: - rc, out = await exec_in_container( - container, ["git", "-C", _REPO_PATH, *args], timeout=timeout - ) - return rc, out +class GitOperationError(RuntimeError): + """A git command reached the container and failed on repo state. + + Distinct from ``ContainerUnreachableError``: the container answered, but + git could not do the work (corrupt index, unwritable .git, missing + object). Both map to 409 — the class is what tells the two apart in a log. + """ + + +# Per-framework AGENTS.md path inside the agent's container. Frameworks read +# this file on every turn to pick up agent rules (per the taosmd contract — +# see issue #378). It lives here because the versioned scope below derives +# from it: adding a framework must not also mean remembering to version its +# rules file. ``deployer`` re-exports the name it has always exposed. +AGENTS_MD_PATHS: dict[str, str] = { + "openclaw": "/root/.openclaw/AGENTS.md", + "hermes": "/root/.hermes/AGENTS.md", +} + + +def _home_relative(path: str) -> str: + """Return *path* relative to the agent home, for a .gitignore entry. + + Deliberately loud at import time rather than skipping the entry: a path + outside the home cannot be versioned by a repo rooted at the home, and a + silently dropped one would leave that framework's rules unversioned with + nothing to notice it. + """ + prefix = _REPO_PATH.rstrip("/") + "/" + if not path.startswith(prefix): + raise ValueError( + f"{path} is not inside the agent home {_REPO_PATH}, so the agent " + f"state repo cannot version it — put the file under {_REPO_PATH} " + f"or drop it from AGENTS_MD_PATHS" + ) + return path[len(prefix):] + + +# Everything the agent state repo versions. The repo root IS the agent home, +# so the scope has to be an allowlist: a denylist over a home directory can +# never be complete — every framework install drops another config file +# carrying an API key (.hermes/config.yaml), a bridge token (.openclaw/env) +# or a multi-gigabyte cache tree. A new framework adds a state path here, +# never a new secret pattern. +# +# Deliberately out of scope, and covered by the leading "*": .ssh/, .taos/ +# (the trace bind mount and the committer's own log), the framework config +# files that sit next to these AGENTS.md files, shell history, and every +# cache/venv tree. +_STATE_PATHS: tuple[str, ...] = ( + ".gitignore", + "AGENTS.md", + "workspace/", + "memory/", + *sorted(_home_relative(p) for p in AGENTS_MD_PATHS.values()), +) -# Real diagnostics git prints for an unknown/missing revision, observed against -# the actual binary (not guessed): `rev-parse --verify X^{commit}` prints -# "Needed a single revision" or "unknown revision or path not in the working -# tree", while `git show X` prints "bad object" (and sometimes "bad -# revision"). Matched case-insensitively since git's casing varies by command. -_UNKNOWN_REVISION_PHRASES = ( - "needed a single revision", - "unknown revision", +def _build_gitignore(state_paths: Iterable[str]) -> str: + """Render the allowlist .gitignore: ignore everything, re-include state. + + git refuses to re-include a file whose parent directory is excluded, so + every parent of a re-included file is re-included too. That is also what + keeps the scan cheap: git descends into the allowlisted directories only, + and the excluded ones (.cache/, .local/, .venv/) are never walked. + """ + lines = [ + "# taOS agent state repo — ALLOWLIST: everything under the agent home", + "# is ignored and only the paths re-included below are versioned.", + "# Generated from _STATE_PATHS in tinyagentos/agent_git.py — edit there,", + "# a deploy overwrites this file.", + "*", + ] + for path in state_paths: + if path.endswith("/"): + lines.append(f"!/{path}") + lines.append(f"!/{path}**") + continue + parents = path.split("/")[:-1] + for depth in range(1, len(parents) + 1): + entry = "!/" + "/".join(parents[:depth]) + "/" + if entry not in lines: + lines.append(entry) + lines.append(f"!/{path}") + return "\n".join(lines) + "\n" + + +_GITIGNORE_CONTENTS = _build_gitignore(_STATE_PATHS) + +# git words a missing object differently per subcommand and version: +# `rev-parse` says "bad revision", `show` says "ambiguous argument ...: +# unknown revision or path not in the working tree", older builds say "bad +# object". Every one of them is a 404, not an unreachable container. +_UNKNOWN_REV_MARKERS = ( "bad revision", + "unknown revision", + "ambiguous argument", "bad object", + "needed a single revision", ) -def _is_unknown_revision(out: str) -> bool: - lowered = out.lower() - return any(phrase in lowered for phrase in _UNKNOWN_REVISION_PHRASES) +# git reports a missing object on a line of its own prefixed "fatal:", and only +# those lines are searched for the markers. Matching anywhere in the container's +# combined output would let an unrelated failure that happens to quote one of +# these phrases — an incus "Error: Instance is not running (ambiguous +# argument)" — turn an unreachable container into a 404 "unknown revision". +_GIT_FATAL_PREFIX = "fatal:" + + +def _raise_unknown_revision_or_unreachable(sha: str, out: str) -> NoReturn: + for line in out.lower().splitlines(): + line = line.strip() + if not line.startswith(_GIT_FATAL_PREFIX): + continue + if any(marker in line for marker in _UNKNOWN_REV_MARKERS): + raise RuntimeError(f"unknown revision {sha}") + raise ContainerUnreachableError(out.strip() or "container unreachable") + + +async def _git(container: str, args: List[str], timeout: int = 60) -> tuple[int, str]: + rc, out = await exec_in_container( + container, ["git", "-C", _REPO_PATH, *args], timeout=timeout + ) + return rc, out async def git_init(container: str) -> None: @@ -102,10 +192,14 @@ async def git_config_user(container: str, name: str, email: str) -> None: async def git_add_commit(container: str, message: str) -> None: - rc, out = await _git(container, ["add", "-A"]) + rc, out = await _git(container, ["add", "-A"], timeout=_INITIAL_COMMIT_TIMEOUT) if rc != 0: raise RuntimeError(f"git add failed: {out}") - rc, out = await _git(container, ["commit", "-m", message, "--allow-empty"]) + rc, out = await _git( + container, + ["commit", "-m", message, "--allow-empty"], + timeout=_INITIAL_COMMIT_TIMEOUT, + ) if rc != 0: raise RuntimeError(f"git commit failed: {out}") @@ -118,9 +212,7 @@ async def git_is_dirty(container: str) -> bool: async def git_rev_parse(container: str, sha: str) -> str: rc, out = await _git(container, ["rev-parse", "--verify", f"{sha}^{{commit}}"]) if rc != 0: - if _is_unknown_revision(out): - raise RuntimeError(f"unknown revision {sha}") - raise ContainerUnreachableError(out.strip() or "container unreachable") + _raise_unknown_revision_or_unreachable(sha, out) return out.strip() @@ -154,35 +246,46 @@ async def git_log(container: str) -> List[dict]: async def git_diff(container: str, sha: str) -> str: rc, out = await _git(container, ["show", "--format=", "--patch", sha]) if rc != 0: - if _is_unknown_revision(out): - raise RuntimeError(f"unknown revision {sha}") - raise ContainerUnreachableError(out.strip() or "container unreachable") + _raise_unknown_revision_or_unreachable(sha, out) return out +# Exit codes the locked revert script reports back through `flock`. +_REVERT_DIRTY = 2 +_REVERT_NOOP = 3 + + async def git_revert(container: str, sha: str) -> str: - await git_rev_parse(container, sha) - if not await git_merge_base_is_ancestor(container, sha): + """Reset the state repo to *sha*, returning "reverted" or "noop". + + The whole decision — is *sha* already HEAD, is the tree clean, reset — + runs inside the flock the auto-committer also takes. Reading HEAD outside + the lock let the committer commit in the gap, so a caller that asked to + restore what was HEAD a moment ago got "noop" while the tree sat on the + committer's new commit. + """ + resolved = await git_rev_parse(container, sha) + if not await git_merge_base_is_ancestor(container, resolved): raise NotAncestorError(f"{sha} is not an ancestor of HEAD") - # The no-op decision (sha already == HEAD) must be made atomically with - # the reset, under the same lock: a committer can create a new commit - # after any pre-lock HEAD read and before this returns, which would make - # a pre-lock comparison stale and falsely report "noop" without actually - # restoring the requested sha. rc 3 is reserved to signal "noop" from - # inside the locked script; rc 0 is a successful reset; anything else is - # a dirty working tree. script = ( - "head=$(git -C /root rev-parse HEAD); " - f'[ "$head" = {sha} ] && exit 3; ' - "dirty=$(git -C /root status --porcelain); " - 'test -z "$dirty" && git -C /root reset --hard ' + sha + f"head=$(git -C {_REPO_PATH} rev-parse HEAD) || exit 1; " + f'test "$head" = {resolved} && exit {_REVERT_NOOP}; ' + f"dirty=$(git -C {_REPO_PATH} status --porcelain) || exit 1; " + f'test -n "$dirty" && exit {_REVERT_DIRTY}; ' + f"git -C {_REPO_PATH} reset --hard {resolved}" ) - rc, _out = await exec_in_container( + rc, out = await exec_in_container( container, ["bash", "-c", f"flock {_STATE_LOCK_PATH} -c {script!r}"], ) - if rc == 3: + if rc == _REVERT_NOOP: return "noop" - if rc != 0: + if rc == _REVERT_DIRTY: raise DirtyTreeError("dirty_tree: working tree has uncommitted changes") + if rc != 0: + # The reset itself failed: a corrupt index, an unwritable .git, a + # missing object. Its own class, because calling that "container + # unreachable" would misdescribe a repo-state problem, and a bare + # RuntimeError would surface it as 404 "unknown revision". + raise GitOperationError(f"git revert failed: {out.strip() or f'rc={rc}'}") return "reverted" diff --git a/tinyagentos/deployer.py b/tinyagentos/deployer.py index 0f5f95f8b..376cd8a5e 100644 --- a/tinyagentos/deployer.py +++ b/tinyagentos/deployer.py @@ -25,6 +25,12 @@ if TYPE_CHECKING: from tinyagentos.secrets import SecretsStore +# Per-framework AGENTS.md path inside the agent's container. Frameworks read +# this file on every turn to pick up agent rules (per the taosmd contract — +# see issue #378). It is defined in ``agent_git`` because the versioned scope +# of the agent state repo derives from it; re-exported here under the name +# callers have always used. +from tinyagentos.agent_git import AGENTS_MD_PATHS from tinyagentos.agent_image import ( GENERIC_BASE_ALIAS, base_image_alias, @@ -102,13 +108,8 @@ def _secret_env_name(name: str) -> str: # that do not match are skipped rather than written outside ~/.ssh. _SAFE_SSH_KEY_NAME = re.compile(r"^[A-Za-z0-9._-]+$") -# Per-framework AGENTS.md path inside the agent's container. -# Frameworks read this file on every turn to pick up agent rules -# (per the taosmd contract — see issue #378). -AGENTS_MD_PATHS: dict[str, str] = { - "openclaw": "/root/.openclaw/AGENTS.md", - "hermes": "/root/.hermes/AGENTS.md", -} +# Auto-committer pushed into every agent container (deploy step 4c). +_COMMITTER_SCRIPT = Path(__file__).parent / "scripts" / "agent_committer.py" def _splice_taosmd_block(existing: str, new_rules: str) -> str: @@ -717,9 +718,11 @@ async def deploy_agent(req: DeployRequest) -> dict: logger.exception("%s: AGENTS.md injection failed", req.framework) # Step 4b: Initialise a git repo inside the container for agent state - # versioning. The repo lives at /root and covers the agent's text - # state (workspace, memory, framework config). A .gitignore excludes - # secrets and bulk artefacts so they never enter history. + # versioning. The repo root is the agent home (/root), so its + # .gitignore is an ALLOWLIST (agent_git._STATE_PATHS): workspace, + # memory and the per-framework AGENTS.md are versioned and everything + # else — framework config carrying api keys, caches, shell history — + # stays out of history entirely. versioning = True versioning_error = None try: @@ -742,40 +745,47 @@ async def deploy_agent(req: DeployRequest) -> dict: # Step 4c: Install the auto-committer script and start it as a # background loop inside the container. Prefer a systemd unit so it # survives reboots; fall back to nohup when systemctl is absent. + # Every terminal failure below raises: the enclosing handler is what + # turns versioning off. A deploy result that says versioning=True while + # no committer ever starts is a lie no caller can detect. if versioning: try: - from pathlib import Path as _P - _committer = _P(__file__).parent / "scripts" / "agent_committer.py" - if _committer.exists(): - _mkdir_rc, _mkdir_out = await exec_in_container( - container_name, ["mkdir", "-p", "/root/.taos"], + if not _COMMITTER_SCRIPT.exists(): + raise RuntimeError( + f"committer script missing at {_COMMITTER_SCRIPT}" ) - if _mkdir_rc != 0: - raise RuntimeError( - f"failed to create /root/.taos (rc={_mkdir_rc}): {_mkdir_out[-300:]}" - ) - _push_rc, _push_out = await push_file( - container_name, - str(_committer), - "/root/.taos/agent_committer.py", + _mkdir_rc, _mkdir_out = await exec_in_container( + container_name, ["mkdir", "-p", "/root/.taos"], + ) + if _mkdir_rc != 0: + raise RuntimeError( + f"failed to create /root/.taos (rc={_mkdir_rc}): {_mkdir_out[-300:]}" ) - if _push_rc == 0: - await exec_in_container( - container_name, ["chmod", "+x", "/root/.taos/agent_committer.py"] - ) - _has_systemd = await exec_in_container( - container_name, ["bash", "-c", "command -v systemctl >/dev/null 2>&1 && echo yes || echo no"] - ) - _installed = False - if _has_systemd[0] == 0 and _has_systemd[1].strip() == "yes": - _unit = """\ + _push_rc, _push_out = await push_file( + container_name, + str(_COMMITTER_SCRIPT), + "/root/.taos/agent_committer.py", + ) + if _push_rc != 0: + raise RuntimeError( + f"failed to push committer script (rc={_push_rc}): {_push_out[-300:]}" + ) + await exec_in_container( + container_name, ["chmod", "+x", "/root/.taos/agent_committer.py"] + ) + _has_systemd = await exec_in_container( + container_name, ["bash", "-c", "command -v systemctl >/dev/null 2>&1 && echo yes || echo no"] + ) + _installed = False + if _has_systemd[0] == 0 and _has_systemd[1].strip() == "yes": + _unit = """\ [Unit] Description=taOS Agent Auto-Committer After=network.target [Service] Type=simple -ExecStart=/usr/bin/python3 /root/.taos/agent_committer.py +ExecStart=/usr/bin/env python3 /root/.taos/agent_committer.py Restart=always RestartSec=5 Environment=AGENT_STATE_REPO=/root @@ -784,75 +794,55 @@ async def deploy_agent(req: DeployRequest) -> dict: [Install] WantedBy=multi-user.target """ - import tempfile as _tf - with _tf.NamedTemporaryFile("w", suffix=".service", delete=False) as _tfh: - _tfh.write(_unit) - _unit_path = _tfh.name - try: - _unit_rc, _unit_out = await push_file( - container_name, - _unit_path, - "/etc/systemd/system/taos-agent-committer.service", - ) - finally: - os.unlink(_unit_path) - if _unit_rc == 0: - await exec_in_container( - container_name, - ["systemctl", "enable", "--now", "taos-agent-committer.service"], - ) - _active = await exec_in_container( - container_name, ["systemctl", "is-active", "taos-agent-committer.service"] - ) - if _active[0] == 0 and _active[1].strip() == "active": - steps.append("committer_installed") - _installed = True - else: - logger.warning( - "Deploy %s: committer systemd unit not active: %s", - req.name, _active[1].strip(), - ) - steps.append("committer_failed") - else: - logger.warning( - "Deploy %s: failed to push committer unit: %s", - req.name, _unit_out[-200:], - ) - if not _installed: - _nohup_rc, _nohup_out = await exec_in_container( - container_name, - [ - "bash", "-c", - "nohup python3 /root/.taos/agent_committer.py " - "> /root/.taos/committer.log 2>&1 &", - ], + import tempfile as _tf + with _tf.NamedTemporaryFile("w", suffix=".service", delete=False) as _tfh: + _tfh.write(_unit) + _unit_path = _tfh.name + try: + _unit_rc, _unit_out = await push_file( + container_name, + _unit_path, + "/etc/systemd/system/taos-agent-committer.service", + ) + finally: + os.unlink(_unit_path) + if _unit_rc == 0: + await exec_in_container( + container_name, + ["systemctl", "enable", "--now", "taos-agent-committer.service"], + ) + _active = await exec_in_container( + container_name, ["systemctl", "is-active", "taos-agent-committer.service"] + ) + if _active[0] == 0 and _active[1].strip() == "active": + steps.append("committer_installed") + _installed = True + else: + # Not terminal: the nohup fallback below still gets + # a turn, and only its failure disables versioning. + logger.warning( + "Deploy %s: committer systemd unit not active: %s", + req.name, _active[1].strip(), ) - if _nohup_rc == 0: - steps.append("committer_installed_nohup") - else: - _nohup_error = ( - f"nohup committer failed (rc={_nohup_rc}): " - f"{_nohup_out[-200:]}" - ) - logger.warning("Deploy %s: %s", req.name, _nohup_error) - steps.append("committer_failed") - versioning = False - versioning_error = _nohup_error else: - _push_error = ( - f"failed to push committer script (rc={_push_rc}): " - f"{_push_out[-200:]}" + logger.warning( + "Deploy %s: failed to push committer unit: %s", + req.name, _unit_out[-200:], ) - logger.warning("Deploy %s: %s", req.name, _push_error) - steps.append("committer_failed") - versioning = False - versioning_error = _push_error - else: - _missing_error = f"committer script not found at {_committer}" - logger.warning("Deploy %s: %s", req.name, _missing_error) - steps.append("committer_failed") - versioning = False - versioning_error = _missing_error + if not _installed: + _nohup_rc, _nohup_out = await exec_in_container( + container_name, + [ + "bash", "-c", + "nohup python3 /root/.taos/agent_committer.py " + "> /root/.taos/committer.log 2>&1 &", + ], + ) + if _nohup_rc != 0: + raise RuntimeError( + f"nohup committer failed (rc={_nohup_rc}): {_nohup_out[-300:]}" + ) + steps.append("committer_installed_nohup") except Exception as exc: logger.warning("Deploy %s: committer install failed: %s", req.name, exc) versioning = False diff --git a/tinyagentos/routes/agent_versions.py b/tinyagentos/routes/agent_versions.py index 27eef52d4..58f12580a 100644 --- a/tinyagentos/routes/agent_versions.py +++ b/tinyagentos/routes/agent_versions.py @@ -4,6 +4,15 @@ repo at /root. Container interactions go via ``agent_git`` helpers so the same code works for both LXC and Docker backends. +Scope +----- +The repo root is the agent's home directory, so what these routes can serve +is bounded by an allowlist rather than by the caller: only the state paths in +``agent_git._STATE_PATHS`` (workspace, memory, the per-framework AGENTS.md) +are versioned. Framework config carrying API keys and bridge tokens, shell +history and cache trees are never in history, so ``/diff`` cannot leak them +and ``/revert`` cannot roll the framework install back. + Routes ------ GET /api/agents/{name}/versions — list commits @@ -14,9 +23,11 @@ ------------------- 200 {status: "noop"} — sha is HEAD, nothing to do 200 {status: "reverted"} — success -400 — invalid sha format +400 — invalid sha format, or an unusable container target 404 — unknown revision -409 — sha not an ancestor of HEAD, or dirty tree +409 — sha not an ancestor of HEAD, dirty tree, the git + operation failed on repo state, or the container is + unreachable """ from __future__ import annotations @@ -30,10 +41,10 @@ from tinyagentos.agent_git import ( ContainerUnreachableError, DirtyTreeError, + GitOperationError, NotAncestorError, git_diff, git_log, - git_merge_base_is_ancestor, git_rev_parse, git_revert, ) @@ -43,21 +54,28 @@ router = APIRouter() -_SHA_RE = re.compile(r"^[0-9a-f]{7,40}$") -_REMOTE_RE = re.compile(r"^[A-Za-z0-9._-]+$") +# Hex object names are case-insensitive to git, and every copy-paste route a +# user has (git log, GitHub, an IDE) can hand over uppercase. +_SHA_RE = re.compile(r"^[0-9a-fA-F]{7,40}$") +# Both halves of a container target — the remote and the agent name — have to +# be plain tokens: "remote:container" is the qualified form, so a name that +# smuggles a colon would silently parse as a different remote. +_CONTAINER_TOKEN_RE = re.compile(r"^[A-Za-z0-9._-]+$") -class InvalidRemoteError(Exception): +class InvalidContainerTargetError(Exception): pass def _container_name(agent: dict) -> str: remote = agent.get("remote") name = agent["name"] + if not _CONTAINER_TOKEN_RE.match(name): + raise InvalidContainerTargetError(f"invalid agent name {name}") container = f"taos-agent-{name}" if remote: - if not _REMOTE_RE.match(remote): - raise InvalidRemoteError(f"invalid remote {remote} in agent {name}") + if not _CONTAINER_TOKEN_RE.match(remote): + raise InvalidContainerTargetError(f"invalid remote {remote} in agent {name}") return f"{remote}:{container}" return container @@ -95,7 +113,7 @@ async def list_versions(request: Request, name: str): try: container = _container_name(agent) commits = await git_log(container) - except InvalidRemoteError as exc: + except InvalidContainerTargetError as exc: return JSONResponse({"error": str(exc)}, status_code=400) except Exception as exc: logger.warning("versions list failed for %s: %s", name, exc) @@ -134,7 +152,7 @@ async def version_diff(request: Request, name: str, sha: str): try: container = _container_name(agent) patch = await git_diff(container, sha) - except InvalidRemoteError as exc: + except InvalidContainerTargetError as exc: return JSONResponse({"error": str(exc)}, status_code=400) except DirtyTreeError as exc: return JSONResponse({"error": str(exc)}, status_code=409) @@ -180,18 +198,24 @@ async def revert_version(request: Request, name: str, sha: str): try: container = _container_name(agent) + # Resolve the sha (404s an unknown one), then let git_revert decide + # noop vs reverted *inside* the state lock — comparing against a HEAD + # read out here races the auto-committer, which would answer "noop" + # while the tree sits on a commit the caller never asked for. resolved_sha = await git_rev_parse(container, sha) - # The noop-vs-reverted decision is made inside git_revert, under the - # container's state lock, so it stays correct even if a committer - # creates a new commit between sha resolution and lock acquisition. status = await git_revert(container, resolved_sha) return {"agent": name, "sha": resolved_sha, "status": status} - except InvalidRemoteError as exc: + except InvalidContainerTargetError as exc: return JSONResponse({"error": str(exc)}, status_code=400) except DirtyTreeError as exc: return JSONResponse({"error": str(exc)}, status_code=409) except NotAncestorError as exc: return JSONResponse({"error": str(exc)}, status_code=409) + except GitOperationError as exc: + # The container answered; git could not do the work. Reported apart + # from container_unreachable so a repo-state problem is diagnosable. + logger.warning("version revert failed for %s/%s: %s", name, sha, exc) + return JSONResponse({"error": str(exc)}, status_code=409) except ContainerUnreachableError: return JSONResponse({"error": "container_unreachable"}, status_code=409) except RuntimeError as exc: diff --git a/tinyagentos/scripts/agent_committer.py b/tinyagentos/scripts/agent_committer.py index 206cac39d..ab7a6c4ce 100644 --- a/tinyagentos/scripts/agent_committer.py +++ b/tinyagentos/scripts/agent_committer.py @@ -33,10 +33,13 @@ def _is_dirty() -> bool: def _changed_summary() -> str: + # Read from the index only, and only after `git add -A` has staged the + # tree: neither `git diff --cached --name-only` nor `git diff + # --name-only` lists untracked files, so a summary taken before staging + # is blind to the most common agent change — a new file under + # workspace/ — and silently falls back to "auto-commit". rc, out, _ = _git("diff", "--cached", "--name-only") - if rc != 0 or not out.strip(): - rc, out, _ = _git("diff", "--name-only") - lines = [l.strip() for l in out.strip().splitlines() if l.strip()] + lines = [l.strip() for l in out.strip().splitlines() if l.strip()] if rc == 0 else [] if not lines: return "auto-commit" if len(lines) == 1: @@ -51,11 +54,11 @@ def _commit() -> None: if not _is_dirty(): return ts = time.strftime("%Y-%m-%d %H:%M:%S") - summary = _changed_summary() - message = f"auto: {ts} | {summary}" rc, out, err = _git("add", "-A") if rc != 0: raise RuntimeError(f"git add failed: {err or out}") + summary = _changed_summary() + message = f"auto: {ts} | {summary}" rc, out, err = _git("commit", "-m", message) if rc != 0: raise RuntimeError(f"git commit failed: {err or out}")