Skip to content

fix(*): let exec measure the files its command wrote - #829

Merged
0xKT merged 9 commits into
mainfrom
fix/exec_diff_in_exec_tool
Sep 30, 2026
Merged

0xKT merged 9 commits into
mainfrom
fix/exec_diff_in_exec_tool

Conversation

@arelchan

@arelchan arelchan commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

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 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 ...).

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:

  • The tool measures. raven/agent/tools/command_writes.py 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 result travels on a new ToolResult.written / ToolOutput.written (FileWrite on the tool paper, contract version 32 -> 33, and 33 -> 34 for FileRemoval.withheld), beside the existing removed.
  • The repo is handed in. Tools do not import the loop shell (import-linter), so ExecTool holds the shadow repo through a ShadowTree protocol 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-name exec replacement (raven-code's CodeExecTool) is handed it through measure_writes before it is registered, so it measures as the built-in does, as the loop-level listing it replaces did for any tool named exec. Both happen before the registry admits the tool, because the registry reads the ceiling off the spec it admits.
  • Every command stages afresh inside its own call, into a per-process index of its own in the shadow repo (never the index the per-turn commit reads), and waits up to 120 s. Whenever there is no snapshot to measure against -- the tree cannot be staged at all (checkpoint off, working_dir outside 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_writes raises 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.
  • Warm-up as a session opens. The first staging of a directory hashes every file. session.create / session.resume call ExecTool.warm for 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. warm returns at once and runs on the staging thread, so it overlaps the model's first reply.
  • One rule for what text may be shown. Every file a command created, rewrote or removed, including the ones its own removal watch caught by name, carries text only where the shadow repo's rules would store it (git check-ignore --no-index against its excludes and the user's .gitignore, the rules alone, not what the index holds). So a .env or 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.
  • Loop. The exec-only listing branch in turn_path.py is gone; the loop turns result.written into file_written. Besides that it has the resolver above and the one-line warm-up at turn start. The registry now keeps removed / written on 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 marked withheld (a new defaulted field on FileRemoval), 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.
  • Wire and page (unchanged from fix(*): show the diff of a file a shell command rewrote #826). file_written entries gain optional added / removed / diff; the diffs share a 512 KiB budget per call, past which the counts still go. Contract updated in rpc-schema/openrpc.json, raven/rpc/models.py and both generated clients. ui-web draws these rows with their hunks, so they open in the diff pane with +/- in the desk list, and fromUnified now skips only a real ---/+++ header pair, so a removed -- note line is no longer dropped.
  • Robustness. Stagings run on daemon threads with blocking git calls (an asyncio subprocess still starting when its loop closes hangs the close on CPython 3.12 macOS). add and write-tree run under one lock per index; the staging add has 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 with surrogateescape, 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's exec does 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-name exec that is not an ExecTool (none ships) reports no files, and existing sessions keep the rows they stored.

Type

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

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 on github/main on 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 covers check, or ignoring the turn's binding; the registry dropping written on unwrap or on an error result; the loop ignoring written; no turn-start warm-up; session.create / session.resume not warming; a failing warm-up failing the open; created-file text ignoring trackable; 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 a TimeoutError; warm ignoring record_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-name exec reports nothing; measure_writes not 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 serve from this branch, isolated RAVEN_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 first exec (append to a notes.md Raven had never touched, and create made_by_exec.txt) waited out the cold staging and took 37.8 s in total. The second exec took 3.7 s, with the staging under the 1 s sampling interval. The desk lists M notes.md +2 and A made_by_exec.txt +3; notes.md opens in the diff pane with both commands' hunks, and a reload draws the same rows.

Risk

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

User-visible changes:

  • An exec now 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.
  • Opening a session starts 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 there; files older than 7 days are pruned.
  • No file content is stored beyond what the checkpoint already commits, and the same excludes apply. With runtime.checkpoint.policy = "never" a command's rewrite is reported bare, as today, and a created file carries counts only.
  • The new FileWritten fields are optional, so older clients ignore them. ToolResult / ToolOutput gain a defaulted written field and FileRemoval a defaulted withheld; contract version 34.

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

Related Issues

Supersedes #826

arelchan and others added 3 commits September 29, 2026 20:35
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>
@arelchan
arelchan requested a review from LivXue as a code owner September 30, 2026 02:21
@arelchan
arelchan requested review from 0xKT and gloryfromca and removed request for LivXue September 30, 2026 02:21

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 because raven_everos was 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.

Comment thread raven/agent/tools/shell.py Outdated
…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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread raven/agent/tools/command_writes.py
Comment thread raven/agent/tools/removals.py
Comment thread raven/agent/loop/turn_path.py

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread raven/agent/tools/removals.py Outdated
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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread raven/contracts/tool.py
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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@0xKT

0xKT commented Sep 30, 2026

Copy link
Copy Markdown
Member

**Not a blocker: the budget log records a measurement that was already stale when the branch ended.

tests/test_kernel_budget.py raises CONTRACTS_LINE_CEILING 3,620 -> 3,660 and its running log
ends with "Measured at 3,627". That number was true at 57b44db, the branch's first commit.
77678e9 then added withheld and its paragraph, and 0d95b41 the version string, so the
surface at the branch head is 3,632.

Measured with the test's own algorithm (rglob("*.py") under raven/contracts, skipping
__pycache__, summing splitlines()):

base e6c0344c  3,594   ceiling 3,620   headroom 26
head 0d95b415  3,632   ceiling 3,660   headroom 28

Nothing is broken -- 3,632 is comfortably inside 3,660, and the raise is genuinely needed
rather than precautionary: the old ceiling would NOT have held this surface (3,632 > 3,620).
I checked that rather than take it on trust, because it is the thing the ceiling exists to
make someone check.

Why it is still worth a line: that log is the file's own record of every raise, and its
assertion says "a new paper is a reviewed change to this ceiling, not a bump in passing".
The next person raising it reads the last entry as the baseline, and 3,627 was never the
surface any commit shipped.

Everything else in the contract change holds. The pin bites -- I added a field to
FileRemoval without touching the version and test_contracts_two_tier_ledger went
1 failed / 8 passed; restored, 9 passed. PINNED_CONTRACT_SURFACE moved version and
rendered-surface hash together, and FileWrite is in the LEDGER's allowed names.

For the record, since it bears on this PR: all three findings I filed on a00c8f1 are fixed
on this head, verified with the same probes and their controls -- the staging arm, the
both-carriers-dropped arm, and the same-name exec replacement. The third one initially came
back red for me and that was MY probe's fault, not the fix's: the opt-in moved from the
constructor to the wiring step after plugin registration (wiring.py:1054-1058), and my probe
stopped at tools.register(). Pointed at the path production actually takes, it passes.

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>
@arelchan

Copy link
Copy Markdown
Contributor Author

@0xKT Fixed in 6bd97b9: the budget log's last entry now names the FileRemoval.withheld lines and reads "Measured at 3,632", the count at this head by the test's own algorithm; the ceiling stays 3,660.

🤖 Addressed by Claude Code

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread raven/agent/loop/wiring.py
@0xKT

0xKT commented Sep 30, 2026

Copy link
Copy Markdown
Member

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.

  1. One budget, written twice. DIFF_BUDGET_CHARS (raven/agent/tools/command_writes.py:39) is described as "the budget a call's removed bodies and whole-file changes already share", and that budget is _FILE_CHANGE_MAX_CHARS (raven/agent/loop/_shared.py:523). Both read 512 * 1024 today and nothing holds them equal; the cargo cannot import the loop's constant, which is exactly why MEASURE_SECONDS got a pin (tests/test_shell_command_writes.py:279). The same one-line pin closes this one.

  2. Three sentences the code has moved past. CONTEXT.md:568 still says the service is "cached by AgentLoop._turn_checkpoint() and keyed on the directory the running turn is bound to"; on this head the cache is _checkpoint_for (raven/agent/loop/turn_path.py:298), and _command_shadow (:315) makes a service with no turn bound, for the session-open warm-up the same entry describes a few lines later. raven/rpc/methods/session.py:331-333 still documents a stored file_written row as {path, created, size, lines}; the row is the tool-complete payload (turn_path.py:3159), which now carries added, removed and diff as well. And the rule this PR exists to add -- a file's text leaves only when the shadow repo's rules would store it, trackable -- is written out under FileWritten in raven/rpc/models.py:653 and in both generated clients, but has no name on the map; CLAUDE.md:400 asks for a coined term to be defined in CONTEXT.md in the same change.

  3. "waiting up to 120s" (CONTEXT.md:575) is the wait on the staging only. In stage_tree the deadline is taken at checkpoint.py:455 and used at :463, and _ensure_init at :459 sits between them unbounded by it: a first _init_repo, or a warm-up's setup awaited through _initializing, adds its own time on top. Each git call inside has its 30s bound, so this is precision rather than a hang, but the sentence promises a number the code does not hold.

  4. A staging that fails and one that times out are told apart only at DEBUG. The timeout is a WARNING (command_writes.py:108); a failed git add or write-tree is logger.debug in _stage_step (checkpoint.py:583) and reaches before as a plain None tree. A user whose shadow repo is broken gets every command measured without diffs, and nothing above DEBUG says why.

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>
@arelchan

Copy link
Copy Markdown
Contributor Author

@0xKT All four taken in 0db7c51:

  1. test_a_calls_diffs_share_the_budget_its_file_changes_and_removed_bodies_do pins DIFF_BUDGET_CHARS == _shared._FILE_CHANGE_MAX_CHARS.
  2. CONTEXT.md's Checkpoint entry now names _checkpoint_for as the cache, with _turn_checkpoint and _command_shadow as its two callers, and defines trackable (the repo's rules, judged without a tree; a refused removal goes out withheld). The session.read docstring lists added / removed / diff on the file_written row.
  3. CONTEXT.md now says the 120 s is the wait on the staging itself, and that a first use sets the repo up before that wait starts.
  4. before warns once per directory when a present shadow repo returns no tree (the shadow repo could not stage ...), pinned by a test; dropping the warning or warning on every command turns it red.

🤖 Addressed by Claude Code

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@0xKT
0xKT merged commit 22f161c into main Sep 30, 2026
32 of 33 checks passed
@0xKT
0xKT deleted the fix/exec_diff_in_exec_tool branch September 30, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants