fix(*): let exec measure the files its command wrote - #829
Conversation
A file an exec command wrote reached the desk diff as a bare row: the loop listed the working directory on either side of the call, so it knew a file changed but never what it changed from, and such a row opened in the file viewer instead of the diff pane. The exec tool now does the measuring itself, around the command it runs, and hands the result back on ToolResult.written beside the removals it already reported: - command_writes lists the directory before and after the command and, where the checkpoint's shadow repo covers it, stages the tree first so a rewritten or removed file is shown against what it held. The repo reaches the tool through a protocol (ShadowTree) the loop hands in; tools do not import the loop shell. - Every command stages the tree afresh inside its own call, into a per-process index of its own, and waits up to 120s. Past that the command is not run and the call fails, rather than running unmeasured. The tool's registry ceiling rises to 780s to cover the wait plus the 600s command cap. - A created file carries its text only where the shadow repo would store it (git check-ignore against its rules), so a .env or a key a command writes never enters a diff or the stored session. - ExecTool.warm starts the first staging as a session opens (session.create / session.resume), or at the first turn of a session opened by its first message, so it overlaps the model's first reply. - The loop loses its exec-only listing branch and forwards result.written as file_written; the registry keeps removed and written on a result it rewrites as an error. file_written entries gain optional added / removed / diff, and ui-web draws them with hunks so they open in the diff pane. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
…one rule Two defects review found in the exec diff. A turn cancelled while waiting on the warm-up's repo setup cancelled the setup's own future: it was never marked running. The warm-up thread then failed setting its result before it settled the staging it had already published in _STAGING, and every later command in that directory waited out the full 120s and was refused as if the filesystem had stopped answering. The setup future is now running from creation, and both the warm-up and a command's own staging settle their futures whatever happens on their thread. Only a created file's text was checked against the shadow repo's rules. A rewritten or removed file was read from the staged tree unfiltered, and a file staged before an ignore rule named it stays in the index, so its old text reached the diff and the removal body. Now one rule (trackable, the repo's rules alone) decides for every file a command created, rewrote or removed, including the files its own removal watch caught by name; a file the rules keep out goes out with its counts at most. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
A command whose tree had not finished staging within the 120s wait was not run, and the call failed. A tree that could not be staged at all already let the command run without a diff, so the two ways of having no snapshot were handled differently, and the stricter one bought nothing: refusing the command does not make its diff any more exact, it only stops the user's work. Both now end the same way: the command runs, it is still listed, and its files are reported without the text they held. The late staging is left running, so a later command usually finds it done. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: account for post-command measurement in ExecTool's registry timeout.
I reviewed the repository rules, the full diff, the affected exec/checkpoint/RPC/UI call paths, all three branch commits, backward compatibility of the optional RPC fields, test changes for weakening, and the checkpoint/tool-layer architecture. The rest of the change is internally consistent, including ignored-file filtering and generated RPC clients.
Verification:
uv run pytest tests/test_shell_command_writes.py tests/test_runtime_checkpoint.py tests/test_agent_loop_session_stamps.py tests/test_rpc_session.py tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py -x- 274 passed after syncing the declared dev extra. The first attempt could not collect becauseraven_everoswas not installed.npm test -- --run src/features/workspace/record.test.ts src/lib/hunks.test.ts- 53 passed.npm run type-check- passed.npm run gen:check- passed.git diff --check github/main...HEAD- passed.
…iling The registry's 780s ceiling covered the staging wait and the 600s command cap but not the measuring that follows the command. A command near its cap could finish and change files, then be cut off while its files were still being read, losing both its output and the record of what it wrote. Everything after the command is now bounded short of the ceiling. The shadow repo's reads (trackable, read_blobs) get 60s; past that every file goes out without text, a named removal's included. The whole post-command step (second walk, those reads, reading the written files) gets 120s; past that the call returns the command's output with no record of its files. The ceiling is now 900s: the 120s staging wait, the 600s command cap, the 120s measuring bound and a margin. A git read whose caller stops waiting now kills its process instead of leaving it running. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The timeout blocker from the prior revision is fixed: the command's post-processing now has a 120-second bound inside a 900-second outer ceiling, slow shadow reads fail closed on file text, and cancellation terminates the Git subprocess. I rechecked the full change and the new commit against AGENTS.md and CONTEXT.md, the exec/checkpoint callers and architecture, backward-compatible RPC payload additions, branch history, and the tests for weakening; I found no further issue worth raising.
Verification: uv run pytest tests/test_shell_command_writes.py tests/test_runtime_checkpoint.py tests/test_agent_loop_session_stamps.py tests/test_rpc_session.py tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py -x passed, 277 tests. git diff --check github/main...HEAD also passed.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three newly reported file-accounting defects must be addressed before merge.
The new evidence invalidates my earlier clean stance on this unchanged revision. I independently reproduced all three reports: failed staging bypasses the trackability gate for a named secret removal; the turn-level removal watch restores a body that the gate withheld; and the shipped same-name code-flow exec replacement performs filesystem changes while returning no write record, after this PR removes the prior loop-level fallback. I added the measurements to the existing threads rather than opening duplicate findings.
Verification: the direct probes reproduced all three failures. uv run pytest tests/test_shell_command_writes.py tests/test_removal_watch.py tests/test_agents_code_flow_exec.py tests/test_agents_code_tools_plugin.py tests/test_agent_loop_session_stamps.py -x passed, 128 tests; that suite currently lacks the cross-boundary cases above.
Three holes in what an exec command reports about its files: - A staging that failed or ran late dropped the shadow repo along with its tree, so a named removal such as `rm .env` kept its body. Whether text may be shown is the repo's rules (`trackable`), which need no tree; `Before` now keeps the repo whenever one was handed in, and only the earlier text of a rewrite or removal needs the tree. - The turn's removal watch refilled a removal the tool had reported without its body, which is exactly the blank the gate writes. The watch now takes the tool's report as given, as it did before. - Measuring was switched on by a constructor argument at the one place the built-in exec is built, so a plugin's same-name replacement (raven-code's CodeExecTool) reported no files. The loop now asks whatever answers to `exec` once every tool is registered (`ExecTool.measure_writes`), and that call raises the tool's registry ceiling by what measuring may take, so the replacement's own ceiling keeps its margin too. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: preserve the turn watch's last-known body for bare removals when no checkpoint tree can supply one.
The three prior blockers are fixed on this revision: policy filtering survives a missing tree, explicitly withheld removal bodies are no longer refilled, and the final registered exec replacement is opted into measurement with the required timeout budget.
One regression remains in the interaction between the second fix and checkpoint-disabled operation, marked inline. It satisfies the round-five blocker bar: this PR changes the ordering that introduces it, same-turn write-and-cleanup is ordinary while checkpointing is off or unavailable, and once hit the file is gone and the only retained body has been discarded.
Verification: uv run pytest tests/test_shell_command_writes.py tests/test_removal_watch.py tests/test_agent_loop_session_stamps.py tests/test_runtime_checkpoint.py tests/test_agents_code_flow_exec.py tests/test_agents_code_tools_plugin.py tests/test_rpc_session.py tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py -x passed, 353 tests. A direct cross-boundary probe produced tool_report=[(..., None)] and after_turn_watch=[(..., None)], confirming the missing case. git diff --check github/main...HEAD passed.
The previous fix stopped the turn's removal watch from refilling any removal the tool reported without a body. That closed the secret refill but also dropped the only copy of a file the turn wrote and a command then removed unseen with the checkpoint off, where no tree can supply the text. `FileRemoval` gains `withheld`: set when the command tool kept the text back on purpose (the repo's rules would not store the file, or no rule was read in time). The watch fills in a bare removal from what the turn wrote only when it is not withheld. Only a present shadow repo can withhold, so with the checkpoint off every bare removal stays fillable, as before this PR. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The checkpoint-off removal regression is fixed: FileRemoval.withheld distinguishes policy suppression from missing text, RemovalWatch restores only the latter, and the wire payload continues to omit the internal marker. I rechecked the full change and this delta against AGENTS.md/CONTEXT.md, affected callers and history, backward-compatible RPC behavior, test changes for weakening, and the contract/tool-layer architecture.
One nonblocking bookkeeping suggestion is inline: the contract surface digest moved without the required contract version bump.
Verification: uv run pytest tests/test_shell_command_writes.py tests/test_removal_watch.py tests/test_agent_loop_session_stamps.py tests/test_runtime_checkpoint.py tests/test_agents_code_flow_exec.py tests/test_agents_code_tools_plugin.py tests/test_rpc_session.py tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py -x passed, 357 tests. git diff --check github/main...HEAD passed.
The new field changed the contract tier's declared surface, and the version moves with every shape change, not once per pull request. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The contract-version suggestion is fixed: version 34 now accompanies the reviewed FileRemoval.withheld surface and its pinned digest. I rechecked the full revision record, the two-line delta, contract architecture, callers, compatibility, history, and test coverage; nothing remains outstanding from my side, and my threads are resolved.
Verification: uv run pytest tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py tests/test_shell_command_writes.py tests/test_removal_watch.py tests/test_agent_loop_session_stamps.py tests/test_runtime_checkpoint.py tests/test_agents_code_flow_exec.py tests/test_agents_code_tools_plugin.py tests/test_rpc_session.py -x passed, 357 tests. git diff --check github/main...HEAD passed.
|
**Not a blocker: the budget log records a measurement that was already stale when the branch ended.
Measured with the test's own algorithm ( Nothing is broken -- 3,632 is comfortably inside 3,660, and the raise is genuinely needed Why it is still worth a line: that log is the file's own record of every raise, and its Everything else in the contract change holds. The pin bites -- I added a field to For the record, since it bears on this PR: all three findings I filed on a00c8f1 are fixed |
The budget log's last entry was measured before FileRemoval gained its withheld flag and the version moved to 34; the surface now stands at 3,632 lines, inside the 3,660 ceiling. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
|
@0xKT Fixed in 6bd97b9: the budget log's last entry now names the 🤖 Addressed by Claude Code |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the delta from 0d95b4153ade: it only corrects the contracts budget ledger to include the five FileRemoval.withheld lines. An independent count using the test's algorithm is 3,632, matching the updated record, while the ceiling remains 3,660. uv run pytest tests/test_kernel_budget.py tests/test_contracts_two_tier_ledger.py -x passes 14/14, and git diff --check github/main...HEAD passes.
Covered the repository rules, this revision's diff and commit history, the full-PR file surface, backward compatibility, test integrity, and the previously reviewed architecture/caller constraints. This prose-only correction changes no runtime behavior or contract surface, and it does not weaken a test. All threads I opened are already resolved.
|
Not a blocker -- four small things from a re-read of the loop side on 6bd97b9 merged onto the current tip. None of these holds the merge; the one that does is the review thread anchored on wiring.py.
|
The registry reads a tool's ceiling off the spec it admits, never off the tool afterwards. Measuring was switched on after every tool had been registered, so the 240 s it adds never reached the spec and a measured command was still cut off at 660 s. The built-in takes record_writes and the shadow resolver at construction again, and a plugin's same-name exec replacement is asked to measure before it is registered. Tests now pin the admitted spec for both. Also from the same review: pin the diff budget to the loop's file change budget it shares, warn once per directory when the shadow repo cannot stage (the failure was only logged at debug), and bring CONTEXT.md and the session.read docstring up to date (the checkpoint cache, the staging wait, the trackable rule, the file_written row). Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
|
@0xKT All four taken in 0db7c51:
🤖 Addressed by Claude Code |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Verified the prior timeout blocker is fixed at the registry boundary: the built-in enables measurement at construction, plugin exec replacements enable it before registration, and the admitted ToolSpec now carries the raised ceiling (900s for the built-in; the plugin path is pinned separately). An independent probe confirms the built-in's live and admitted ceilings are both 900s.
The additional changes correctly pin the shared diff budget, align the checkpoint/session documentation with current callers and payloads, define trackability, and warn once per directory when staging fails. The affected exact-head suite passes 352/352, and git diff --check github/main...HEAD passes. I checked the repository rules, revision diff and history, callers/dynamic replacement path, backward compatibility, test integrity, and the previously reviewed architecture and disclosure constraints. All threads I opened are resolved; the timeout thread is also resolved by its opener.
Summary
A file an
execcommand wrote reached the desk diff as a bare row. The loop 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 bareMwith 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 ...).This supersedes #826. That PR measured the change in the loop, around a call named
exec. Here the exec tool measures it itself, around the command it runs, and hands it back the way it already hands back the files a command removed; the loop only forwards it.How it works:
raven/agent/tools/command_writes.pylists the directory before and after the command and, where the checkpoint's shadow repo covers it, stages the tree first, so a rewritten or removed file is shown against what it held. The result travels on a newToolResult.written/ToolOutput.written(FileWriteon the tool paper, contract version 32 -> 33, and 33 -> 34 forFileRemoval.withheld), beside the existingremoved.ExecToolholds the shadow repo through aShadowTreeprotocol and a resolver the loop passes in (AgentLoop._command_shadow: the turn's repo, only where it covers the command's directory). The built-in is built with it, and a plugin's same-nameexecreplacement (raven-code'sCodeExecTool) is handed it throughmeasure_writesbefore it is registered, so it measures as the built-in does, as the loop-level listing it replaces did for any tool namedexec. Both happen before the registry admits the tool, because the registry reads the ceiling off the spec it admits.working_diroutside the repo, git error) or it did not finish within the wait -- the command runs all the same and is still listed, so its files are reported without the text they held; where a repo covers the directory, its rules still decide what text may be shown. A late staging is left running, so a later command usually finds it done. What follows the command is bounded too: the shadow repo's reads get 60 s (past it every file goes out without text) and the whole post-command step 120 s (past it the call returns the command's output with no record of its files), so a slow measurement never costs the output.measure_writesraises the tool's registry ceiling by what measuring may take (the 120 s wait and the 120 s bound), so the built-in's goes from 660 s to 900 s and a replacement's own ceiling keeps its margin. A git read whose caller stops waiting kills its process.session.create/session.resumecallExecTool.warmfor the directory the session resolves to, and a session opened by its first message (a channel's) is warmed at the start of that turn.warmreturns at once and runs on the staging thread, so it overlaps the model's first reply.git check-ignore --no-indexagainst its excludes and the user's.gitignore, the rules alone, not what the index holds). So a.envor a key never enters the event or the stored session, even one the repo staged before an ignore rule named it, and even when the staging failed: the rules need no tree. Where no shadow repo can vouch for a file, a created file carries its counts only.turn_path.pyis gone; the loop turnsresult.writtenintofile_written. Besides that it has the resolver above and the one-line warm-up at turn start. The registry now keepsremoved/writtenon a result it rewrites as an error (a command whose output begins with "Error" still wrote what it wrote). A removal whose body the rules kept back goes out markedwithheld(a new defaulted field onFileRemoval), and the turn's removal watch fills in a bare removal from what the turn wrote only when it is not: with the checkpoint off every bare removal stays fillable, as before, and a withheld body is never put back.file_writtenentries gain optionaladded/removed/diff; the diffs share a 512 KiB budget per call, past which the counts still go. Contract updated inrpc-schema/openrpc.json,raven/rpc/models.pyand both generated clients. ui-web draws these rows with their hunks, so they open in the diff pane with +/- in the desk list, andfromUnifiednow skips only a real---/+++header pair, so a removed-- noteline is no longer dropped.addandwrite-treerun under one lock per index; the stagingaddhas its own 600 s ceiling; the turn commit waits for a warm-up's repo setup instead of racing it on the config lock. The setup's future and every staging future are running from creation, so a cancelled waiter cannot cancel them, and the warm-up and staging threads settle them whatever happens, so no staging is left published for nobody to finish (the cancellation 0xKT and gloryfromca measured on fix(*): show the diff of a file a shell command rewrote #826). Git queries now encode paths withsurrogateescape, so a POSIX file name that is not UTF-8 no longer raises after the command has run (the follow-up gloryfromca raised on fix(*): show the diff of a file a shell command rewrote #826).Behaviour that moved with the measurement: staging now happens after the permission gate has ruled, so a denied command is never staged; a background command (
run_in_background) takes no listing, since its files land after the call returns; a sub-agent'sexecdoes not list (its runner lists every call itself, as before).Not covered: files the shadow repo excludes (
*.log,.env, the user's.gitignore) get no diff, text files over 256 KB get no counts, a same-nameexecthat is not anExecTool(none ships) reports no files, and existing sessions keep the rows they stored.Type
Verification
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
make test-python-- 27077 passed, 108 skipped, 3 failed (on the previous head; the focused set below is on this one). 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 ongithub/mainon this machine.uv run --frozen --all-extras pytest tests/test_runtime_checkpoint.py tests/test_runtime_checkpoint_deep.py tests/test_agent_loop_session_stamps.py tests/test_agent_loop_workdir.py tests/test_rpc_session.py tests/test_shell_command_writes.py tests/test_shell_file_removals.py tests/test_tool_registry_execute.py tests/test_tool_registry_channels.py tests/test_tool_registry_session_overlay.py tests/test_rpc_spine.py tests/test_workdir_snapshot.py tests/test_shell_approval.py tests/test_contracts_two_tier_ledger.py tests/test_kernel_budget.py tests/test_removal_watch.py tests/test_agents_code_flow_exec.py tests/test_agents_code_tools_plugin.py tests/test_agents_code_launcher.py tests/test_subagent_dag_runner.py tests/test_agents_research_ask_user_text.py tests/test_living_docs.py tests/test_tracing_compact.py -q --idle-ceiling-strict-- 1231 passed.make lint-python lint-types lint-imports-- clean (10 import contracts kept).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-- 3107 passed.Mutation checks, each restored afterwards and each turning at least one test red (48): a staging timeout not falling back to running the command without a diff; the wiring not asking the tool to record writes, or not handing it the repo; the resolver dropping its
coverscheck, or ignoring the turn's binding; the registry droppingwrittenon unwrap or on an error result; the loop ignoringwritten; no turn-start warm-up;session.create/session.resumenot warming; a failing warm-up failing the open; created-file text ignoringtrackable; the diff budget ignored; removals losing the staged text; staging a directory too large to list; reading the written files on the event loop; a named removal reported twice; the ceiling under the sum of the wait, the command cap and the measuring bound; no bound on the shadow repo's reads after the command, or on the whole post-command step; a late read keeping a named removal's text; a cancelled git read left running; the checkpoint's timeout not being aTimeoutError;warmignoringrecord_writes; a background command measured; the setup future not marked running; the warm-up or a command's staging not settling its future on an unexpected error; a rewritten or removed file read without the rule; a named removal's text kept without the rule; the repo dropped when the staging failed, or its rules applied only where there is a tree; a tree-less staging still read for blobs; the removal watch refilling a withheld body, or never refilling an unknown one; a named, listed or timed-out removal not marked withheld where the repo kept its body back, or a listed one marked where no repo ruled; measuring switched on only for the built-in, so a plugin's same-nameexecreports nothing;measure_writesnot raising the ceiling, or raising it on every call; the built-in or the replacement measured only after the registry admitted it, so the admitted ceiling stays at 660 s; a failed staging not warned about, or warned about on every command.Real gateway (
raven servefrom this branch, isolatedRAVEN_HOME, real model), in a cold 12,775-file / 428 MB directory on a machine running endpoint protection at load average 13-14. The first message opened the session and the warm-up started with it; the model's firstexec(append to anotes.mdRaven had never touched, and createmade_by_exec.txt) waited out the cold staging and took 37.8 s in total. The secondexectook 3.7 s, with the staging under the 1 s sampling interval. The desk listsM notes.md +2andA made_by_exec.txt +3;notes.mdopens in the diff pane with both commands' hunks, and a reload draws the same rows.Risk
User-visible changes:
execnow runs only after the working directory has been staged (or the 120 s wait has passed). On a warm directory that is well under a second; the first command in a large directory the shadow repo has never indexed waits for the first staging (tens of seconds on a slow disk), which the session-open warm-up usually hides. Past 120 s the command runs without a diff.git addper directory per process, writing into the same.raven/shadow.gitthe checkpoint already uses. Each process keeps its ownexec-<pid>.indexthere; files older than 7 days are pruned.runtime.checkpoint.policy = "never"a command's rewrite is reported bare, as today, and a created file carries counts only.FileWrittenfields are optional, so older clients ignore them.ToolResult/ToolOutputgain a defaultedwrittenfield andFileRemovala defaultedwithheld; contract version 34.Rollback: revert the commit. The staging indexes in
.raven/shadow.gitare inert without the code and are pruned by age.Related Issues
Supersedes #826