feat(version): tag, diff and roll back an agent's config - #109
Conversation
An agent is already a git repo, so a known-good configuration is just a tag — but restoring one meant hand-running git plumbing and knowing which paths are safe to touch. Adds a CLI over those primitives; no new storage. - gitagent version save/list/show/diff/rollback, dispatched like `plugin` before parseArgs. Versions are annotated tags under refs/tags/agentcfg/, namespaced so they never collide with a repo's release tags. - Scoped to config: agent.yaml, SOUL.md, RULES.md, DUTIES.md, AGENTS.md, config/, tools/, hooks/, knowledge/, examples/, compliance/, agents/, workflows/, schedules/, plugins/. memory/ and skills/ are deliberately excluded — memory commits on every save (src/tools/memory.ts) and skill_learner rewrites skills/ at runtime, so those commits interleave with config commits and restoring them would destroy what the agent learned after the tag was cut. - Rollback is forward-only: it writes a new commit rather than rewriting history, so the rollback is itself visible in git log and revertible. No reset --hard anywhere. - Restore is planned from `git diff --name-status --no-renames -z <tag> HEAD`, then remove-before-restore. A plain `git checkout <tag> -- tools/` leaves files added after the tag in place, which yields a merge of old and new config rather than a rollback; the ordering also handles file<->directory flips. test/version.test.ts covers both. - Adds src/git.ts: every call is execFileSync with an argv array, so quoting and injection bugs are structurally impossible. Also handles --no-pager, a 64MiB buffer, closed stdin, and per-invocation identity/no-gpgsign so commits work in a repo with no user.email without touching git config. Only isGitRepo is migrated here; porting the other 13 execSync sites is a separate change. - Establishes the temp-git-repo test fixture the suite lacked (hermetic via GIT_CONFIG_GLOBAL=/dev/null). 29 new cases, full suite 94 passing.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Solid feature with well-thought-out design decisions. The injection-safe git layer, the config-only scope, and the forward-only rollback are all correct. Four findings below, one of which is a real correctness bug in error handling.
Security pass: no new dependencies introduced, all git invocations go through execFileSync with argv arrays (no shell), version names are validated against a strict allowlist regex before any git call touches them. No CVEs, no secrets, no injection surfaces in the new code. Clean.
The unescaped execSync at src/session.ts:138 (acknowledged out of scope) remains the highest-risk site in the repo — worth a dedicated follow-up.
Review follow-up. A failure in the rm/checkout phase of applyRollback threw a raw GitError that escaped the CLI's VersionError handler, so the user got a stack trace and no recovery guidance while the repo sat half-mutated. - Split the failure modes: RESTORE_FAILED (plan applied partway, nothing committed, reset --hard recovers) vs COMMIT_FAILED (plan fully staged, git commit finishes it). Both now warn that reset --hard also discards uncommitted memory/, which the old message glossed over. - Drop RollbackOptions.force — it was never read; force is consumed entirely by assertRollbackReady before applyRollback is called. - Cover the rollback INDEX_DIRTY path, which had no test. Also pins that --force covers dirty config and detached HEAD but never a dirty index, since that guard is what stops staged memory/ riding along in the commit. - Note why gitStream's exit status is ignored in diff: git diff exits 1 when differences exist and hasConfigDiff already established there are some. Full suite 95 passing.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
All four findings addressed — the follow-up commit is clean.
RESTORE_FAILED vs COMMIT_FAILED split — exactly right. The try/catch wrapping the rm+checkout phase now catches partial-restore failures and throws a distinct RESTORE_FAILED error with accurate guidance. The commit path stays as COMMIT_FAILED. Both messages now note that reset --hard also discards uncommitted memory/, which the old message glossed over. Better than the minimal fix I suggested.
RollbackOptions.force removed — correct call. Removal (not dead-field documentation) is the right answer; the field was never semantically part of applyRollback's contract. The CLI call site is updated accordingly.
INDEX_DIRTY rollback test added — the new test covers both force=false and force=true, and explicitly locks in that --force does not bypass a dirty index. That's an important behavioral contract to pin in tests.
gitStream exit status comment — the comment is accurate and saves the next reader from the same question.
95 tests passing. No new concerns. Approving.
2 participants
Summary
An agent is already a git repo, so a known-good configuration is just a tag — but restoring one meant hand-running git plumbing and knowing which paths are safe to touch. This adds an ergonomic CLI over those primitives. No new storage: versions are annotated git tags in the agent's own repo.
save [<name>]v+versionfromagent.yamllist--jsonfor scripting)show <name>diff <a> [<b>]rollback <name>Three design decisions worth reviewing
1. Rollback is config-only —
memory/andskills/are never touched.src/tools/memory.tscommits on every memory save andskill_learnerrewritesskills/at runtime, so those commits interleave with config commits throughout history. A whole-tree restore would silently destroy everything the agent learned after the tag was cut.Versioned paths:
agent.yaml,SOUL.md,RULES.md,DUTIES.md,AGENTS.md,config/,tools/,hooks/,knowledge/,examples/,compliance/,agents/,workflows/,schedules/,plugins/.2. Rollback is forward-only.
It writes a new commit rather than rewriting history, so the rollback is visible in
git logand revertible like anything else. Noreset --hard, no branch switching — safe with a remote or other clones.3. Restore is diff-planned, not a blanket checkout.
A plain
git checkout <tag> -- tools/leaves files added after the tag in place, producing a merge of old and new config rather than a rollback. Instead the plan comes fromgit diff --name-status --no-renames -z <tag> HEAD -- <pathspec>, then remove-before-restore (that ordering also handles file↔directory flips). Because every path is computed by git under the config pathspec, it is structurally impossible formemory/to enter the plan.Also adds
src/git.tsEvery call is
execFileSyncwith an argv array, so the quoting/injection bug class is structurally impossible. It also handles--no-pager(otherwisediffopens a pager and appears to hang), a 64 MiB buffer, closed stdin, and per-invocation identity +gpgsign=falseso commits work in a repo with nouser.emailwithout ever writing to the user's git config.Scope note: only
isGitRepois migrated to it. The other 13 ad-hocexecSyncgit sites — including the unescaped commit message atsrc/session.ts:138— are left alone deliberately; they are on untested hot paths, and porting them belongs in a follow-up whose diff reads as a refactor rather than being buried in a feature.Test plan
npm run buildclean;npm test94 passing (29 new)GIT_CONFIG_GLOBAL=/dev/nullso CI and dev machines agreesrc/tools/memory.tsgitagent pluginstill works after the shared--dirextractionDocs
README gains a
## Versioning & Rollbacksection;Documentation.mdgains a full### Version CLIreference with edge-case behavior and known limitations.Out of scope
--push/ remote tag sync, versioningmemory/orskills/, per-agent tag namespacing (agentcfg/<agent>/<version>), and porting the remainingexecSyncsites. Tags stay local until pushed manually —saveprints the exact command.