fix(*): show the diff of a file a shell command rewrote

[arelchan]

Summary

A file an exec command wrote reached the desk diff as a bare row. The runtime listed the working directory on either side of the command (#692), so it knew a file had changed but never what it changed from: a rewrite showed a bare M with no counts, and opening any such row fell back to the file viewer instead of the diff pane. File-tool edits were unaffected; this only concerned commands (python3 gen.py > out.json, echo x >> notes.md, sed -i ...).

The checkpoint's shadow git repo now supplies the missing half:

• Just before each command, CheckpointService.stage_tree stages the working tree into a per-process index of its own (never the index the per-turn commit reads). After the command, read_blobs reads back the old contents of every rewritten or removed file.
• file_written entries gain added / removed counts and a unified diff (created files are measured against nothing). A removal the listing found now carries the text it held. Wire contract updated in rpc-schema/openrpc.json, raven/rpc/models.py and both generated clients.
• ui-web draws those rows with their hunks, so they open in the diff pane like a file tool's row and show +/- in the desk list. A replayed session draws the same rows from the stored entry. Rows made by a listing are now marked listed rather than inferred from "has no hunk".

Cost and waiting (decided in review with the maintainer):

• The first staging of a directory the shadow repo has never indexed hashes every file: about 130 ms on a small directory, 7-8 s on a 4k-file / 250 MB repo, 14 s on a 17k-file one. It is started in the background on a directory's first turn in the process (warm returns at once; the repo setup runs on the staging thread too), so it overlaps the model's first reply. Later stagings are a stat walk (about 100-300 ms).
• A command waits at most 6 s for its staging. Past that it is not run: the call fails with a reply telling the model to run it again, and the staging carries on. A command whose tree cannot be staged at all (checkpoint off, working_dir outside the repo, git error) runs without a diff, as before.
• Stagings run on daemon threads with blocking git calls, because an asyncio subprocess still starting when its loop closes hangs the close on CPython 3.12 macOS (reproduced). add and write-tree run under one lock per index. The add has its own 600 s ceiling: under the shared 30 s one a cold staging of a large tree was killed and restarted forever, and every command was held back.
• The turn's own commit waits for a warm-up's repo setup instead of racing it on the config lock.
• The diffs share the event's 512 KiB budget; past it the counts still go and the diff is dropped whole.

Not covered: files the shadow repo excludes (*.log, .env, the user's .gitignore), text files over 256 KB, sub-agent lanes (#670 run records are unchanged), and existing sessions (no old contents were ever stored). Steering the model toward file tools in the exec description is left out because trajectory cassettes pin that description.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

• make test-python -- 27040 passed, 108 skipped, 3 failed. The 3 failures (test_rpc_files.py::test_a_host_without_libreoffice_says_so, test_simulation_scenario.py::test_a_fresh_trial_refreshes_the_subagent_homes_before_it_runs, test_simulation_suite.py::test_a_stopped_run_is_marked_in_its_record_and_finished_with_its_evidence) fail the same way on github/main on this machine.
• uv run --frozen --all-extras pytest tests/test_agent_loop_workdir.py tests/test_runtime_checkpoint.py tests/test_runtime_checkpoint_deep.py tests/test_agent_loop_session_stamps.py tests/test_rpc_message_tool_route.py -q -- 126 passed, three runs in a row, and five runs under added load for the timing-sensitive tests.
• make lint-python lint-types lint-imports -- clean; npm run gen:check --prefix ui-web, npm run type-check --prefix ui-web, npm run lint --prefix ui-web (0 errors, 4 pre-existing warnings), npm run lint:rpc --prefix ui-tui -- clean.
• npm test --prefix ui-web -- 3104 passed (including the fixture-shape gate); npm test --prefix ui-tui -- 2111 passed.
• Mutation checks, each restored afterwards and each turning at least one test red: staging into the shared index; no baseline; one baseline per turn; the warm-up's tree used as the command's; no wait budget; a second staging started while one runs; running the command unmeasured on timeout; returning no diff instead of holding back; the warm-up not started at turn start; the warm-up repeated every turn; the repo setup awaited inside warm; the setup run before waiting out the warm-up; the commit's setup not waiting for the warm-up's; the index not seeded; the stale lock kept; the 30 s ceiling on the staging add; removals ignoring the old contents; created files without a diff; the diff budget ignored; the counts dropped; the page ignoring the diff, the counts, or the listed flag; a measured change not appended to an existing row.
• Real gateway (raven serve from this branch, isolated RAVEN_HOME, real model), exec-only commands:
  • A small directory: the desk lists A demo.txt +3, M demo.txt +2 -1 and M readme.md +1 (a file Raven had never touched before). Each opens in the diff pane, and a reload draws the same rows. After a restart, the first command in the new process is measured too.
  • A cold 17k-file directory: the first two calls were held back with the retry reply, the model retried, and the third call ran and carried its diff.

Risk

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

User-visible changes:

• In a large directory the shadow repo has never indexed, the first turn's commands can fail with "the command was not run ... run the same command again" until the first staging finishes. The model retries on its own.
• Every turn that has exec and an active checkpoint may start one background git add per directory per process, writing into the same .raven/shadow.git the checkpoint already uses. Each process keeps its own exec-<pid>.index file there; files older than 7 days are pruned.
• No new file content is stored beyond what the checkpoint already commits, and the same excludes apply (credentials, .env, the user's .gitignore).
• New fields on FileWritten are optional, so older clients ignore them.

Rollback: revert the two commits. The staging indexes in .raven/shadow.git are inert without the code and are pruned by age.

Related Issues

N/A

[gloryfromca]

No blockers; this can merge as far as I am concerned.

I reviewed the full github/main...HEAD diff and both branch commits, then traced the surrounding turn execution, checkpoint staging/readback, RPC persistence and replay, and web workspace rendering paths. I also checked the repository rules and canonical context terms/boundaries, backward compatibility of the optional wire fields and unmeasured fallback, relevant history, and whether existing tests were weakened. I tried to refute the concurrency and compatibility concerns raised by the design before concluding that none causes a concrete failure in this revision.

Verification:

• uv run pytest tests/test_runtime_checkpoint.py tests/test_agent_loop_session_stamps.py -x - 68 passed after uv sync --all-extras (the initial environment lacked the declared raven_everos workspace dependency)
• uv run pytest tests/test_rpc_schema_match.py tests/test_rpc_tool_events.py -x - 428 passed
• npm test -- --run src/features/workspace/record.test.ts - 38 passed
• npm run type-check - passed
• npm run gen:check - generated client matches the contract
• git diff --check github/main...HEAD - passed

[0xKT]

Not a blocker. These are the other things I verified myself on 29b4e642, alongside the blocking thread on checkpoint.py. Grouping them here so one pass can pick up everything rather than two.

1. The page throws away the counts the server computed, and the substitute can be wrong. record.ts:177-181: when a hunk is present, c.add = hunk.add; c.del = hunk.del -- the wire's w.added/w.removed are used only in the else if branch, i.e. only when there is no diff. So the numbers the desk list shows are re-derived by parsing the unified text rather than taken from the side that measured them.

That substitute loses lines. hunks.ts:150 filters /^(---|\+\+\+)( |$)/ before counting. A removed line whose content begins -- is rendered in a unified diff as - plus -- foo, which is --- foo, and is dropped -- row and count both. Same for an added line beginning ++ . -- is an ordinary SQL comment and a common prose convention, so this is not exotic. Two sources for one number, and the authoritative one is the one being discarded.

2. CheckpointService._stage_index (checkpoint.py:600-610) is dead. grep -rn _stage_index raven/ tests/ returns only its own definition; the positive control on the same query shape, _stage_path, returns :432, :469, :608 and a test. It is worth mentioning beyond tidiness because it carries a written rationale for the per-process index that the live path no longer states anywhere.

3. _GIT_TIMEOUT_SECONDS's comment is no longer true of the file it lives in. :140 calls it "Upper bound on any single git subprocess". Since :159 this PR has a second, 20x larger bound (_STAGE_ADD_TIMEOUT_SECONDS = 600.0) on a git subprocess in the same module. The newer constant's own comment explains the split correctly; the older one was not updated to stop claiming the whole space.

One correction to something you may hear from elsewhere. A cancelled staging future was reported to me as leaving an unhandled InvalidStateError and a traceback on stderr each time a command is held back. I could not reproduce that on the refusal path with the shipped 6.0s budget: _stage calls result.set_running_or_notify_cancel() at :514 before doing any work, so by the time _within's cancel arrives six seconds later the future is already RUNNING and the cancel is a no-op. My probe over repeated refusals produced no traceback. Reaching it would need the cancel to land inside the microseconds between _start_stage creating the future and the thread's first line, which the shipped budget does not allow. Not reporting it as a defect; saying so explicitly so nobody spends time on it.

For completeness: the acceptance pass surfaced further non-blocking items I have not personally reproduced (two concurrent turns in one directory attributing each other's line counts; a binary file that happens to decode as UTF-8 receiving a diff of its bytes; coverage gaps where a mutation kills no test). I am deliberately not asserting those here, because I did not drive them myself. I will re-check them against whatever head answers the blocking thread.

[arelchan]

Thanks, all three taken in 5150f0a:

1. The page now uses the runtime's added/removed whenever they are sent and falls back to the patch only when they are not. Separately, fromUnified now skips only a real ---/+++ header pair (a --- line followed by a +++ line and then a @@ hunk or the end), so a removed -- note line is kept, row and count both. This also fixes the same drop for file-tool diffs, which use the same parser. Tests: keeps a removed line that reads like a file header, shows the runtime's counts over ones re-read from the patch.
2. _stage_index is removed; its per-process-index rationale now lives on _stage_path.
3. The _GIT_TIMEOUT_SECONDS comment now scopes itself to git calls the turn waits behind and points at the staging add's own ceiling.

On the cancelled-future note: it was reachable after all, on the warm-up path (the repo setup runs before the staging marks its future running). Details and the fix are in the checkpoint.py thread.

🤖 Addressed by Claude Code

[gloryfromca]

Blocking: the new timing test must satisfy the repository's strict CI idle ceiling.

The runtime blocker from the prior revision is fixed: the unchanged 6.25s staging reproduction now times out once at 6.01s and returns the staged tree immediately on retry. I also rechecked the revision delta, checkpoint callers and concurrency history, the AGENTS/CONTEXT architecture constraints, optional-wire backward compatibility, the three UI follow-ups, and whether tests were weakened; I found no other concrete failure.

This late-round blocker meets all three required conditions: this revision introduces the unmarked timing test, every ordinary CI shard runs with --idle-ceiling-strict, and the session fails with no merge path until the test is marked appropriately or its intentional wait is shortened.

Verification:

• original 6.25s staging probe: timeout then tree on retry
• uv run pytest tests/test_runtime_checkpoint.py tests/test_agent_loop_session_stamps.py -x: 74 passed, with the idle-ceiling warning
• uv run pytest tests/test_runtime_checkpoint.py::test_a_staging_under_the_budget_is_never_refused --idle-ceiling-strict: assertion passed but the session failed on 3.20s unmarked idle time
• npm test -- --run src/features/workspace/record.test.ts src/lib/hunks.test.ts: 52 passed
• npm run type-check: passed
• npm run gen:check: generated client matches the contract
• git diff --check github/main...HEAD: passed

[gloryfromca]

No blockers; this can merge as far as I am concerned.

The only revision delta adds the repository-defined production_timing marker, with reasons, to the four tests whose intentional staging delays are the property under test. This clears my CI blocker without changing production behavior or weakening assertions. I rechecked the delta against the prior full review, repository test rules, relevant marker implementation, and the standing discussion record; no unresolved item remains from my side. The original runtime thread belongs to 0xKT and remains theirs to resolve.

Verification:

• uv run pytest tests/test_runtime_checkpoint.py tests/test_agent_loop_session_stamps.py -x --idle-ceiling-strict: 74 passed
• git diff --check github/main...HEAD: passed

[0xKT]

Not a blocker -- a correction to myself. I was wrong about the cancelled-future crash, and I was wrong in the worst direction: I told you not to spend time on it. You were right that it was reachable, and the path you name is the one.

I have now reproduced it on 29b4e642, on the shipped 6.0s budget:

future right after warm(): running=False done=False
stage_tree: REFUSED while the repo setup was still running
THREAD CRASHES: ['InvalidStateError: CANCELLED_AND_NOTIFIED: <Future ... state=cancelled>']

And the same probe on 6e3687f6, unchanged, with the same 9s hold:

future right after warm(): running=True done=False
stage_tree: REFUSED while the repo setup was still running
THREAD CRASHES: none

Why my first probe could not have found it, which is the part worth recording. I slowed git add. But _stage calls set_running_or_notify_cancel() before it runs the add, so by the time a 6s wait expires the future is already RUNNING and the cancel is a no-op -- that much of what I wrote was true, and it is exactly why the finding looked unreachable from where I was standing. The vulnerable window is earlier: _warm_up runs _init_repo() first and only reaches _stage afterwards, so on the warm-up path the future stays PENDING for the whole repo setup, which is seconds rather than microseconds. My second attempt still missed it because I slowed subprocess.run, and _init_repo goes through the async _run instead. Only holding _init_repo itself opens the window.

So my negative rested on a probe that never entered the window, and I never checked that it could. A negative result needs a positive control -- proof that the probe can produce the thing under some conditions -- before it is worth anything, and I published one without.

The fix is verified from my side: moving set_running_or_notify_cancel() to the top of _warm_up closes it, and the future is RUNNING from the moment warm() returns.

Sorry for the detour I sent you on.

[0xKT]

Not a blocker. Graded A at 6e3687f6. Nothing blocking survived. Two things worth having, and two things a pass surfaced that I checked and am telling you NOT to chase -- the second list is the more useful one.

1. Half of the counts fix is unpinned. record.ts:169-170 now prefers the wire's numbers (w.added ?? hunk?.add), and the row-creation branch is tested. The merge branch at :175-176 is not. Measured:

mutation to record.ts	result
first-row branch prefers the patch again (hunk?.add ?? w.added)	1 failed
merge branch prefers the patch again (had.add += hunk.add)	39 passed, nothing dies

So a later edit could put the derived number back on the merge path and the suite would stay green. That path is where a command that writes the same file more than once accumulates its counts, which is where a wrong number is least likely to be noticed by eye.

2. CONTEXT.md:571 now names the wrong moment. It says stage_tree "stages it again just before each command". After 5150f0a2 a command's stage_tree usually issues no git add at all -- it hands back the warm-up's tree. That sentence is the one a reader consults to learn when a command's baseline is taken, and the answer changed in the same commit that left it standing. One clause.

Two things I am NOT reporting, having checked them myself.

A staging is not re-started per command, and a write that lands during the model's reply is therefore absent from the next command's baseline. I reproduced that: with nothing calling note_write, the baseline tree comes back holding only the pre-existing file; with a note_write after the write it holds both. But that is the design saying what it says. note_write's docstring scopes itself to writes the loop can know about, and its own example -- "a file the user saved between two messages" -- IS covered, because the turn-start call at turn_path.py:749 stamps before warm(). The uncovered case is a save DURING a reply, which the module comment already calls out by writing "when anything was last known to write". I nearly filed a sharper version of this on the grounds that the docstring claimed a scope it did not implement; reading the line properly showed the claim was accurate and mine was not.

A warm-up whose repo setup fails caches a None staging and never retries _ensure_init. The second half is untrue in production. My first probe reproduced it -- three commands, _init_repo called once -- but the probe omitted turn_path.py:1403, which calls note_write() after every tool call. Adding it, in the turn's real order, the init is retried on each following command (calls 1, 2, 3). What is left is that the first command after a failed warm-up runs without a diff, and that is the contract at checkpoint.py:26-27 working as written: a git failure costs the call its diff and nothing else.

Gates, all run by me on this head: gates.sh rc=0 (ruff / lint-imports / commit-lint / large-files / language); pyt.sh over the two touched test files 74 passed; ui-web npm ci from this tree's lockfile, gen:check (202 methods in sync), type-check exit 0, npm test 210 files / 3106 tests. Every mutation above ran on the reviewed tree and was reverted; git status clean afterwards.

[arelchan]

Both taken in ed71c7a:

1. New test adds the runtime's counts, not the patch's, to a row it already has pins the merge branch (the counts disagree with the patch on purpose). Putting had.add += hunk.add back now fails it.
2. The CONTEXT.md sentence now says a command's stage_tree takes the latest staging when it began after the last note_write (every tool call and every turn start) and stages afresh only otherwise.

Thanks also for writing down the two you checked and set aside.

🤖 Addressed by Claude Code

[gloryfromca]

No blockers; this can merge as far as I am concerned.

I reviewed the full revision delta against the prior accepted head, the workspace recorder implementation and callers, the checkpoint context contract, repository rules, backward compatibility, and whether the added test weakens or merely pins behavior. The new test correctly covers runtime-count preference when a command's measured change is merged into an existing row, and the context text now matches the implemented staging reuse/invalidation model. No production code changed and no prior settled issue regressed.

Verification:

• npm test -- --run src/features/workspace/record.test.ts: 40 passed
• npm run type-check: passed
• git diff --check github/main...HEAD: passed

[0xKT]

Not a blocker. Graded A at ed71c7ab. Both items from the last note are closed, and I checked the one that matters by re-running the mutation that survived rather than by reading the test.

The merge branch is pinned now. Last round, reverting record.ts:175-176 to had.add += hunk.add left all 39 tests green. On this head the same mutation fails adds the runtime's counts, not the patch's, to a row it already has. The control still holds too: reverting the first-row branch to prefer the patch fails 2 tests rather than 1, because the new test covers that path as well. Suite 40 passed unmutated.

CONTEXT.md:571 now names the rule rather than a moment that no longer happens -- "takes the latest staging if it began after the last note_write (every tool call and every turn start) or stages the tree afresh otherwise". That is both accurate and more useful than the sentence I complained about, because it puts the freshness rule where a reader looks for it instead of leaving it only in the docstring.

Gates on this head, all mine: gates.sh rc=0 (ruff / lint-imports / commit-lint / large-files / language) on ed71c7ab merged onto e6c0344c; ui-web npm ci from this tree's lockfile, npm run type-check exit 0, npm test 210 files / 3107 tests. The delta touches nothing under raven/, so the architecture answers from the previous pass stand unchanged; mutations ran on the reviewed tree and were reverted, git status clean.

For the record of this PR as a whole: one blocking finding, verified fixed and closed; one false negative of mine, corrected; three non-blocking items, all answered. Nothing is outstanding from my side.

[arelchan]

Superseded by #826. Review here found real defects in the short-budget, refuse-and-retry and staging-reuse machinery; #826 drops it for a simpler model: every exec stages the directory afresh inside its own call and waits up to 120s for it, with a warm-up started as the session opens. Thanks @0xKT and @gloryfromca for the reviews that drove the change.