fold CodeRabbit findings on #2714 (tsk-fjmxzo): Agent state versioning: git-in-container with auto-commit + history/revert API - #2717
fold CodeRabbit findings on #2714 (tsk-fjmxzo): Agent state versioning: git-in-container with auto-commit + history/revert API#2717jaylfc wants to merge 2 commits into
Conversation
- Initialise a git repo inside each agent container at deploy time with
a .gitignore that excludes secrets and bulk artefacts, and commit
identity set to the agent slug.
- Ship a small debounced auto-committer script that runs as a background
loop inside the container, committing dirty trees with a timestamp +
changed-file-summary message.
- Add controller API routes: GET /api/agents/{name}/versions,
GET /api/agents/{name}/versions/{sha}/diff,
POST /api/agents/{name}/versions/{sha}/revert.
- Add changelog fragment and tests for committer, routes, and deployer
steps.
Docs-Reviewed: agent-coordination.md has no route table; new /api/agents/{name}/versions routes are self-documenting via the route file.
…, 2) git_revert uses single operation, 3) agent_committer excludes Git stat footer
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jaylfc
added
the
lead-blocked
|
Lead review: holding this PR ( Fix-forward card tsk-yn5gze carries all four with the acceptance bar; it builds on |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return out | ||
|
|
||
|
|
||
| async def git_revert(container: str, sha: str) -> None: |
There was a problem hiding this comment.
CRITICAL: git_revert --no-edit <sha> is a reverse-apply of one commit, not a snapshot restore. The PR title and changelog promise "revert the state repo to a prior commit", but with this implementation reverting the initial commit deletes the files it added instead of restoring the working tree to that earlier state. The lead reviewer already flagged this on the PR; the fix-forward card tsk-yn5gze is supposed to address it. Either change semantics to git reset --hard <sha> (with the obvious caveats) or rename the endpoint and document the inverse-commit semantics.
|
|
||
|
|
||
| async def git_revert(container: str, sha: str) -> None: | ||
| rc, out = await _git(container, ["revert", "--no-edit", sha]) |
There was a problem hiding this comment.
CRITICAL: Externally reachable argument injection via git argv. The sha path parameter from /api/agents/{name}/versions/{sha}/{diff,revert} is concatenated straight into the git argv (["revert", "--no-edit", sha]). A user-supplied value starting with - (e.g. --upload-pack=..., --exec=...) is interpreted as a git option. Validate sha against ^[0-9a-fA-F]{4,64}$ (and ideally --end-of-options before the user value) before passing it through. The same issue applies to git_diff on line 100 and git_log's f"--format={fmt}" is safer but the formatted string still embeds | separators vulnerable to author-name | content (see git_log).
| return rc == 0 and bool(out.strip()) | ||
|
|
||
|
|
||
| async def git_log(container: str) -> List[dict]: |
There was a problem hiding this comment.
WARNING: git_log parses output with line.split("|", 4) over a user-controlled --format=%H|%s|%an|%ae|%ai. Author name or email containing | (legal in git) shifts columns silently; the resulting dict will have wrong fields and the loop will drop lines via the len(parts) == 5 guard. Use a delimiter that cannot appear in any field (e.g. NUL \0 with -z, or %x1f unit-separator) and split with split("\0", 4) / split("\x1f", 4).
|
|
||
|
|
||
| async def git_diff(container: str, sha: str) -> str: | ||
| rc, out = await _git(container, ["show", "--format=", "--patch", sha]) |
There was a problem hiding this comment.
WARNING: git_diff raises RuntimeError for any non-zero rc, but the route maps it to HTTP 404 ("not found"). When the container is down or git is broken, callers get a misleading 404 instead of a 5xx/409. Distinguish "bad sha" (parse stderr for unknown revision → 404) from "git command failed for other reasons" (→ 409 container_unreachable, like the list endpoint).
|
|
||
| def main() -> None: | ||
| while True: | ||
| try: |
There was a problem hiding this comment.
WARNING: except Exception: pass in the committer loop silently eats every failure — git lock contention, missing git binary, ENOSPC, permission errors, even programmer bugs in _changed_summary. The committer will appear healthy while doing nothing. At minimum log to stderr (the redirect already routes to committer.log), e.g. logger.exception(...) or print(..., file=sys.stderr, flush=True).
| return rc == 0 and bool(out.strip()) | ||
|
|
||
|
|
||
| def _changed_summary() -> str: |
There was a problem hiding this comment.
WARNING: _changed_summary is called before _git("add", "-A") (see line 54). The first _git("diff", "--cached", "--stat") therefore always returns empty for a dirty-but-unstaged tree (which is the common case, since git add only runs after the summary is captured). It then falls back to _git("diff", "--stat") against the working tree, which works but bypasses the intended staged-stats path. Either stage first then read --cached --stat, or call _git("add", "-A") once at the top of _commit before building the message.
| if not lines: | ||
| return "auto-commit" | ||
| # Exclude the Git stat footer (e.g., "2 files changed") | ||
| if lines and "files changed" in lines[-1]: |
There was a problem hiding this comment.
SUGGESTION: The "exclude stat footer" heuristic (if "files changed" in lines[-1]: lines = lines[:-1]) silently corrupts a perfectly legitimate file summary whose content happens to contain the substring files changed (e.g. an agent memory note titled "2 files changed in Q3 review.md"). Parse git diff --stat with --numstat (TSV, no footer) or check line.split() for the trailing token pattern instead of substring matching.
| from pathlib import Path as _P | ||
| _committer = _P(__file__).parent / "scripts" / "agent_committer.py" | ||
| if _committer.exists(): | ||
| _push_rc, _push_out = await push_file( |
There was a problem hiding this comment.
WARNING: push_file to /root/.taos/agent_committer.py will fail if /root/.taos/ does not already exist (fresh container, or any prior deploy that never created the dir). The whole block is wrapped in try/except Exception that only logs a warning, so the failure is invisible: committer_installed is silently absent from steps, but success: True is returned. Add await exec_in_container(container_name, ["mkdir", "-p", "/root/.taos"]) before the push_file, and surface push failures as steps.append("committer_install_failed").
| "/root/.taos/agent_committer.py", | ||
| ) | ||
| if _push_rc == 0: | ||
| await exec_in_container( |
There was a problem hiding this comment.
SUGGESTION: chmod +x on a file that is always invoked as python3 /root/.taos/agent_committer.py (see line 757) is dead code. Either drop the chmod or drop the shebang/+x requirement; doing both is redundant and adds a container round-trip per deploy.
| container_name, | ||
| [ | ||
| "bash", "-c", | ||
| "nohup python3 /root/.taos/agent_committer.py " |
There was a problem hiding this comment.
WARNING: nohup ... & from inside an exec_in_container shell does not survive the exec session on every backend. On LXC (depending on lxc-attach flavor) the spawned process can be killed when the exec command returns. There is also no supervisor: if the committer dies (or the container restarts), nothing relaunches it. Either run it under a small init/loopback (e.g. an entrypoint hook or a systemd-style watchdog) or document the restart requirement.
Code Review SummaryStatus: 10 Issues Found | Recommendation: Address before merge Overview
Lead reviewer (jaylfc) already placed a Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Reply with Reviewed by minimax-m3:free · Input: 41.4K · Output: 6K · Cached: 314.2K |
|
Closed mechanically: superseded by #2751.
Evidence ( No work is lost. This closes the fix-forward accounting gap the per-repo throttle already assumed was closed ( — @taOS-dev ( |
|
Superseded by #2751. |
1 participant
CARD TITLE (intent, not commit subject): fold CodeRabbit findings on #2714 (tsk-fjmxzo): Agent state versioning: git-in-container with auto-commit + history/revert API
Autonomous build of board card tsk-4vogow.
REVISION: built on
exec/tsk-fjmxzo(cut at9d0e2949008aafb780d60b1a36f773de767376a1), not ondev. That branch'scommits are ancestors of this one. Verified by
git merge-base --is-ancestorbefore the PR was opened.
Files:
tests/test_deployer.py | 50 ++++++++++
tests/test_routes_agent_versions.py | 119 +++++++++++++++++++++++
tinyagentos/agent_git.py | 109 +++++++++++++++++++++
tinyagentos/deployer.py | 46 +++++++++
tinyagentos/routes/init.py | 3 +
tinyagentos/routes/agent_versions.py | 84 ++++++++++++++++
tinyagentos/scripts/agent_committer.py | 68 +++++++++++++
9 files changed, 569 insertions(+)