Merge pull request #2789 from jaylfc/exec/tsk-xa76qz

agent versioning: allowlist the state paths instead of denylisting secrets (tsk-xa76qz)

Author: jaylfc

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`.

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".

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 <sha>..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 `<remote>: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`.

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,31 +58,35 @@ 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")
         assert ".env" not in log_out
         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}"
+        )