Repository navigation
feat(time-machine): add Time Machine, a git snapshot of every turn to go back to with /timeline - #5
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cbbf80793
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!result.isStdoutTruncated) return result.stdout | ||
| return result.stdout.slice(0, result.stdout.lastIndexOf('\0') + 1) |
There was a problem hiding this comment.
When a jump produces more than 4 MiB of diff metadata, whole() returns only the complete records before the output cutoff, but move() derives the ignored-file safety check from that partial list while read-tree still applies the full target. A target-added path after the cutoff can therefore collide with an ignored on-disk file, never reach check-ignore, and be overwritten even though no snapshot contains its current contents. Git 2.43's git read-tree -h describes -u as “update working tree with merge result”; keep the safety enumeration complete/chunked even if UI summaries remain truncated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6debea1. The guard no longer reads the summary from changes(). It builds its own list of added files with git diff --name-only --diff-filter=A -z, and if that listing comes back cut at the 4 MiB output limit, the move is refused (MoveBlocked with no paths) before read-tree -u runs. The UI summary can still be truncated; the safety check never is. Test: a cut listing refuses the move and leaves the files untouched.
| const parts = status.split('\0').filter(Boolean) | ||
| for (let i = 0; i + 1 < parts.length; i += 2) statusOf.set(parts[i + 1], parts[i]) | ||
| return numstat.split('\0').filter(Boolean).map(line => { | ||
| const [added, removed, path] = line.split('\t') |
There was a problem hiding this comment.
When a target-added pathname contains a tab, git diff --numstat -z emits one NUL-terminated record whose pathname still contains that tab, but this destructuring keeps only the text before it. The statusOf lookup then misses and defaults the add to M, so move() omits it from the ignored-file guard; if an ignored file with that name exists, read-tree -u overwrites data that no snapshot holds. Git 2.43's git diff -h documents -z as producing NUL-terminated output, so parse only the first two tab separators and retain the rest as the pathname.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6debea1. Each numstat record is split on its first two tabs only; the rest is the path. The ignored-file guard now uses --name-only -z (fix above), so it no longer depends on this parse, but the pane shows the right path too. Test (skipped on Windows, which has no tabs in file names): an ignored a\tb.txt is listed whole as A, and a jump that would overwrite it is refused.
| async function goTo ($: Engine, id: number, read0?: { tree: string; head: string | null }): Promise<string> { | ||
| if (!repo) return 'Time Machine: this folder is not a git repository.' | ||
| // Mid-turn, Claude works on files it has read: moving them under it would go unnoticed | ||
| if (turn && !turn.outside) return 'Time Machine: a turn is running. Jump when it ends.' |
There was a problem hiding this comment.
When a background agent invokes a mutating tool after the main turn has ended, startTurn() creates an outside turn before the downstream tool runs, but this condition explicitly allows /timeline to proceed. The exclusive snapshot section has already ended while a long-running Bash tool is still changing files, so a concurrent jump can snapshot a partial result and run move() underneath the agent, after which the agent continues writing over the selected state. Track active outside tool calls and refuse the jump until they complete, as is already done for a main turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6debea1. Every tool call that may write counts itself while it runs (incremented before next(e), decremented in a finally), a background agent's included, and a jump refuses with "a tool is still changing files" until the count is back to zero. Test: a tool held open after the main turn blocks the jump; once it ends, the jump goes through.
| jumped = false | ||
| modelTree = null |
There was a problem hiding this comment.
After a /timeline jump, jumped and modelTree are the only state used by prompt.submit to inject the reread warning. A resumed session restores its states and cursor but clears both values here, so if the user jumps, exits or hot-reloads, and then sends the next real prompt after resume, jumpNote() is skipped and Claude receives no list of files that changed beneath its previous context. Persist and restore the pending baseline, or reconstruct it, rather than clearing it on every start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6debea1. The tree Claude last saw and whether a jump moved it since are now saved with the session's states, and restored when the session starts again (resume or hot reload) instead of being cleared. Test: jump, restart the session, and the next prompt carries the note, once.
midudev
left a comment
midudev
left a comment
There was a problem hiding this comment.
Thanks Diego, this is a really nice piece of work. The git layer is careful (an index of its own per session, refs instead of branches, the ignored-file guard), the tests run against real git and against the engine, and node --test, claude plugin test, claude plugin validate and a tsc -p plugins/time-machine all pass on my side. It also merges cleanly with current main.
A few things inline before merging. The first two are the ones I care most about; the rest are small.
| // Mid-turn, Claude works on files it has read: moving them under it would go unnoticed | ||
| if (turn && !turn.outside) return 'Time Machine: a turn is running. Jump when it ends.' | ||
| // A background agent's tool may be writing right now: the files would move under it | ||
| if (writing > 0) return 'Time Machine: a tool is still changing files. Jump when it ends.' |
There was a problem hiding this comment.
A jump while a background agent's "outside" turn is still open leaves a phantom state.
Repro with the test harness (world() from register.test.tsx):
await turnOf($, 'first')
await turnOf($, 'second')
// the main turn is over; a background agent edits
await $.tool.call({ tool: 'Bash', tool_use_id: 'bg', command: 'npm run build' })
await timeline($, '1')
await turnOf($, 'third', ['Read'])/timeline then shows:
◉ #5 (outside a turn) ~ src/a.ts
○ #4 your changes, kept before a jump ~ src/a.ts
● #3 second
● #2 first
● #1 start of the session
- The agent's changes are kept as feat: Context Meter plugin (context window, cost, session tokens, running agents) #4, "your changes, kept before a jump" ("Your changes are safe, Doc"), though the person didn't make them.
- When the next turn starts,
finishTurncloses the stale outside turn against the tree the jump left: feat(time-machine): add Time Machine, a git snapshot of every turn to go back to with /timeline #5 is a copy of image-preview: "not stored in this session" on Windows — wrong images folder #1's tree, a child of test(image-preview): make the engine tests pass on Windows hosts #3, and the cursor moves onto it, soundogoes to test(image-preview): make the engine tests pass on Windows hosts #3 instead of back where the person came from.
Closing the outside turn before the jump fixes it (goTo already runs inside exclusive):
if (writing > 0) return 'Time Machine: a tool is still changing files. Jump when it ends.'
// A background agent's changes since the turn ended become a state of their own first
if (turn?.outside && turn.start) await finishTurn($)With it, the same repro gives #4 (outside a turn) as #3's child, the cursor on #1 and no phantom row, and the existing 43 tests still pass. Worth keeping the repro as a test.
There was a problem hiding this comment.
Fixed in 60733fe. Before a jump, an open outside turn is closed first, so with your repro the agent's changes become #4 (outside a turn), a child of #3, and the cursor ends on #1 with no phantom row. Your repro is a test now.
Closing it there had a side effect, fixed in the same commit: finishTurn also reset what Claude last saw, so a jump that then failed dropped the note of the jump before, and the note compared with the agent's files instead of Claude's. A background agent's turn now leaves that alone. The jump also reuses the tree finishTurn just read instead of walking it again.
| if (blocked.length > 0) throw new MoveBlocked(blocked) | ||
| } | ||
| const moved = await changes(run, repo, from, to) | ||
| await git(run, ['read-tree', '-m', '-u', from, to], { cwd: repo.top, env: indexEnv(repo) }) |
There was a problem hiding this comment.
A jump moves the files but not HEAD, and nothing says so.
If a turn ran git commit (Claude itself, or /commit) or a git checkout, going to a state from before it puts the old files back while HEAD stays on the new commit or branch. With real git and src/git.js:
commit "base" → snapshot #1 → edit + git commit -am "claude commit" → move(now, #1)
$ git status --short
M a.txt # the reverse of "claude commit", as an unstaged change
It's consistent with "never touches HEAD" and nothing is lost, but it's surprising: it reads as if Claude's commit had been reverted, and after a branch switch the old branch's files land on the new one.
Suggestion: keep the HEAD each state was taken on (its commit's parent already is) and, when it differs from the current one, say it in the jump message, e.g. HEAD has moved since #3 (a commit or a checkout): the files went back, HEAD and the branch did not; git status shows the difference. Plus a line about it in Limits.
There was a problem hiding this comment.
Done in fcc33d4. Each state keeps the HEAD it was taken on (head, optional so states saved before still load). When it differs from the current one, the jump says: HEAD has moved since #3 (a commit or a checkout): the files went back, HEAD and the branch did not; git status shows the difference. Plus a line in Limits. Test: a commit, a jump back, and the message says so; a jump to a state on the current HEAD says nothing.
|
|
||
| ## What it never touches | ||
|
|
||
| - **Your history**: snapshots are commits no branch points at, kept by refs under `refs/time-machine/<session>/<n>`. `git log` does not show them. |
There was a problem hiding this comment.
git log doesn't show them, but git log --all, gitk --all and the graph views (VS Code Git Graph, lazygit, Fork…) do, since every ref under refs/ counts: one commit per state, branching off HEAD, with the prompt as its message. And git push --mirror would publish them, untracked (not ignored) files and prompts included. I'd say it here, with the way to hide them: git log --exclude='refs/time-machine/*' --all.
Nit on the next line: the index is now one per session, .git/time-machine-<session>.index, not .git/time-machine.index. Related: as this is the first release, the shared time-machine.index never shipped, so its clean-up in pruneIndexes and the INDEX fallback in git.js could go.
There was a problem hiding this comment.
Done in 6f2a550 and 5e87e14. The README says what git log --all and the graph views show, how to hide them with --exclude='refs/time-machine/*', and what push --mirror would publish. One correction to the wording: a snapshot's message is the turn's label (the first 60 characters of the prompt), not the whole prompt. I also added a line on files of secrets that are not in .gitignore.
The index path is .git/time-machine-<session>.index now. The INDEX fallback and the shared-index clean-up are gone: locate(run, cwd, session) returns the session's index, and git.js throws if a repo comes without one, rather than staging into the person's own index.
| 'AskUserQuestion', 'ToolSearch', 'Skill', 'ListMcpResourcesTool', 'ReadMcpResourceTool', 'ReadMcpResourceDirTool', | ||
| 'TaskGet', 'TaskList', 'TaskOutput', 'ExitPlanMode', 'EnterPlanMode', 'LSP']) | ||
| // An MCP tool whose name says it only reads: `mcp__github__get_issue`, `mcp__fs__list_directory` | ||
| const MCP_LOOKS = /^mcp__.+__(?:get|list|read|search|find|fetch|query|view|show|describe|lookup|check|count|stat)(?:_|$)/i |
There was a problem hiding this comment.
The prefix match lets some writers through: mcp__editor__find_and_replace, mcp__fs__search_and_replace and mcp__x__check_and_fix all give mayWrite(...) === false (checked). If one of them is the first write of a turn, the turn's "before" snapshot is taken after it (or not at all), so undoing the turn doesn't undo that change, and it shows up later as "changes outside Claude" or "your changes".
In the spirit of "when unsure, it may write", a write verb anywhere in the tool's name could win:
const MCP_WRITES = /(?:^|_)(?:write|edit|replace|create|update|delete|remove|move|rename|apply|fix|set|save|patch|insert|put)(?:_|$)/i
export function mayWrite (tool, input = {}) {
if (tool === 'Bash') return !(typeof input.command === 'string' && readOnly(input.command))
if (LOOKS.has(tool)) return false
return !(MCP_LOOKS.test(tool) && !MCP_WRITES.test(tool.replace(/^mcp__.+?__/, '')))
}A false positive only costs a snapshot.
There was a problem hiding this comment.
Fixed in aa8183b with your regex, plus a few verbs a second review turned up (add, append, upsert, store, upload, copy, format, generate, install, import…): fetch_and_store and check_and_format were still getting through. Tests cover your three and those.
| void exclusive(() => liveRefresh($)).catch(() => undefined) | ||
| } | ||
| return result | ||
| }).catch(async ($, e, next) => { |
There was a problem hiding this comment.
Nit: this handler also runs when the hook fails after next(e) (e.g. the LIVE update right after the tool), and then the toast says the snapshot failed when it was taken. next.called tells the two apart:
}).catch(async ($, e, next) => {
if (!next.called) await $.ui.toast('Time Machine could not snapshot this turn; it runs without a way back.')
return next(e)
})There was a problem hiding this comment.
Done in 7462ad4. While there: a tool whose snapshot failed ran through the catch's next(e) without counting as writing, so a jump could move the files under it. It counts now. The running row's updates can no longer fail the hook either, so the toast only ever means a failed snapshot.
|
Thanks for the review! All five are in, one commit per topic. Besides those:
Every commit passes |
… go back to with /timeline Every turn that changes files is a git snapshot off the history, Bash changes included, and /timeline opens them as an undo tree to go back and forth. Snapshots live under refs/time-machine/ in an index of their own per session, so branches, HEAD, the staging area and ignored files are never touched. Hidden in the root README until it is on the web.
a23089e to
5e7dd15
Compare
|
¡Muchísimas gracias crack @diegomarty! 🙌 Qué currazo de plugin y qué rápido dejaste finos todos los puntos de la review. ¡A tope! 🚀 |
Time Machine
A Claude Code mod: every turn that changes files is a git snapshot off your history, and
/timelinetakes the working tree to any of them and back.rm), not only the file tools; they outlive the session.u/rundo and redo.refs/time-machine/*, built in an index of their own per session. Ignored files are never saved nor overwritten.Screenshots with example data.
Hidden in the root README until it is on the web, like the other new plugins.
Tests
node --test: the git layer against real git (renames, binaries, merge conflicts, linked worktrees, ignored files), the rest pure.claude plugin test: the hooks and the pane against the engine.npm testandnpm run validatepass locally.