Record leases from the pre-tool hook, three CLI paper cuts, and the fork/exclude finding - #21
Conversation
|
| // answers with the session's turns, and a limit is a display | ||
| // concern. Newest first, matching `timeline`. | ||
| let hidden = turns.len().saturating_sub(limit); | ||
| for turn in turns.into_iter().take(limit) { |
There was a problem hiding this comment.
The daemon returns turns oldest-first, so take(limit) retains the oldest entries and hides the newest ones when a session exceeds the limit. The footer then incorrectly calls the hidden turns “older.” Reverse the iterator before applying the limit, as the MCP implementation already does.
| for turn in turns.into_iter().take(limit) { | |
| for turn in turns.into_iter().rev().take(limit) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/acyclic/src/main.rs
Line: 687
Comment:
**Limit keeps oldest turns**
The daemon returns turns oldest-first, so `take(limit)` retains the oldest entries and hides the newest ones when a session exceeds the limit. The footer then incorrectly calls the hidden turns “older.” Reverse the iterator before applying the limit, as the MCP implementation already does.
```suggestion
for turn in turns.into_iter().rev().take(limit) {
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let path = path | ||
| .map(|p| { | ||
| p.trim_start_matches(&*repo.to_string_lossy()) | ||
| .trim_start_matches('/') | ||
| }) | ||
| .filter(|p| !p.is_empty() && !p.contains('\t')) | ||
| .unwrap_or("*"); |
There was a problem hiding this comment.
This string-based prefix removal does not reliably produce repository-relative paths. On Windows, an absolute path can retain its leading backslash; on every platform, a sibling such as /work/repository/file can be mistaken for a child of /work/repo. The resulting lease looks precise but does not match the repository-relative diff path, so a scheduler consuming it can miss a conflict. Use component-aware path normalization and fall back to * for paths outside the repository.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/acyclic/src/hook.rs
Line: 309-315
Comment:
**Lease paths are misnormalized**
This string-based prefix removal does not reliably produce repository-relative paths. On Windows, an absolute path can retain its leading backslash; on every platform, a sibling such as `/work/repository/file` can be mistaken for a child of `/work/repo`. The resulting lease looks precise but does not match the repository-relative diff path, so a scheduler consuming it can miss a conflict. Use component-aware path normalization and fall back to `*` for paths outside the repository.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Anything scheduling work alongside an agent needs to know which paths the agent is in right now. Predicting that from a plan does not work: asked to declare its writes, one agent named a single file having written five, and a planned node predicted one path against nine observed. The pre-tool hook is already handed the exact path before the edit lands, so it records it. Two placement decisions, both forced by measurement. It rides on the existing pre-tool hook rather than being a hook of its own. A separate hook process costs about as much as this whole binary's hook path, so a second one roughly doubles what every Edit, Write and Bash pays to record a path this process already holds. Here it is one append. It writes into the store, not the repo. The obvious placement, <repo>/.speculation/leases, took the hook from 65ms to 120ms with a daemon running: a write inside the tree wakes the watcher, and this hook then waits for the checkpoint its own write caused. It would also have appeared in every blast radius as a changed path. Outside the tree, neither happens, and the cost is 1.4ms on a 65ms hook. A missing path records a wildcard, because a Bash command can touch anything and a reader should block rather than guess. ACYCLIC_NO_LEASES turns the whole thing off without a rebuild, which is also what made the measurement honest.
Three small things, each one a paper cut hit while driving the CLI from scripts. A daemon that exits mid-answer closes the socket, so the read succeeds with nothing and serde called that "decode: EOF while parsing a value at line 1 column 0". That reads like corruption; it means the daemon stopped. Running `stop` and then any other verb was enough to see it. The response parse moves into its own function so the shutdown case is covered by a test rather than a timing-dependent race — it could not be reproduced on demand in six tries. `turns` had no `--limit` while `timeline` did, so a long session printed everything. Trimmed on the rendering side, since the daemon already answers with the session's turns and a limit is a display concern, and the trim says how many turns it hid rather than quietly dropping history. `exclude` matches paths, not names, and the wrong form fails silently while looking like it worked: exclude = ["__pycache__"] leaves src/__pycache__ captured. The README now says so, with the measured cost of getting it wrong on a Rust tree — 1.4 GB of store and +29s per build against 14 MB and 35s. Also hardens the lease writer from the previous commit: it creates the store root if a tool call precedes `init`, and six tests cover repo-relative paths, the wildcard for tools that name no file, tab rejection, the off switch, append behaviour, and — the regression that cost 65ms to 120ms — that nothing is ever written inside the repo.
A speculative agent working inside a fork mount, with no shell and no LSP
tool, still ended up with a target/ in its fork: the host's edit-time
diagnostics ran cargo check for it. Nothing consults the exclude set on a
fork's overlay writes, so all of it went into the object store — 1.3 GB in
five minutes on a 2.4 MB source tree — and promote then snapshotted the fork,
target/ and all, for another 2.2 GB in one minute. The backend answered
"Objects capacity exhausted", the rewind's cleanup hit the same wall, and the
daemon fell into a recovery rescan it could not finish. Ten minutes after
init, restore and checkpoint both failed.
The design already says forks do not see excluded paths, and they do not:
the base generation holds none. The gap is paths created inside the fork.
The write-up records the store's own growth by minute, the promote message
that shows the seam from the other side ("target is excluded from
snapshots; no checkpoint holds it" — after capturing it as a fork path),
and three fixes in order of how much they change: apply exclude to fork
writes, have promote skip excluded paths, refuse a snapshot before it can
exhaust the store.
The product-name guard in CI caught a hardcoded `.acyclic` in the lease writer's test fixture. The helpers for exactly this already exist: product::repo_config_dir() and repo_config_file().
Stripping the repo as a string and then trimming '/' left a leading backslash on Windows (CI: '\src/report.py'), and a real Windows host would have recorded 'src\report.py', which never matches a path from `diff`. Strip the prefix as a Path and join the components with '/'.
7b126b5 to
a3b5339
Compare
Three commits from a day of driving the CLI from scripts against real repos, plus two CI fixes.
Record what a tool is about to touch, from the pre-tool hook (
d9423ae)Anything scheduling work alongside an agent needs to know which paths the agent is in right now. Predicting that from a plan does not work — measured: an agent declared one file having written five; a planned node predicted one path against nine observed. The pre-tool hook is already handed the exact path before the edit lands, so it records it.
Two placement decisions, both forced by measurement:
<repo>/.speculation/leases, took the hook from 65 ms to 120 ms with a daemon running: a write inside the tree wakes the watcher, and the hook then waits for the checkpoint its own write caused. In the store the cost is 1.4 ms on a 65 ms hook.ACYCLIC_NO_LEASESturns it off without a rebuild. A tool that names no file records a wildcard.Three paper cuts (
1fd26c9)decode: EOF while parsing a value at line 1 column 0, which reads like corruption. It now says the daemon stopped. The response parse is its own function so the case is unit-tested rather than a race.turns --limit, matchingtimeline, reporting how many turns it hid.excludematches paths, not names —["__pycache__"]does not excludesrc/__pycache__, and the wrong form fails silently. With the measured cost of getting it wrong on a Rust tree: 1.4 GB of store and +29 s per build against 14 MB and 35 s.The fork/exclude finding (
a240c74) — docs onlyexcludedoes not govern paths created inside a fork. A fork's overlay writes go straight into the object store;promotethen snapshots the fork. On a Rust tree, onecargo checkin a fork — run by the host's own edit-time diagnostics, with Bash and LSP both denied — was enough: 1.3 GB in five minutes, 2.2 GB more at the promote, thenObjects capacity exhaustedand a daemon stuck in recovery. Written up with the store's own growth by minute and three fixes ranked by how much they change. No code change here; it needs a design decision first.Two CI fixes (
da15624,7b126b5).acyclicin the lease test fixture; it now usesproduct::repo_config_dir()/repo_config_file()./-trim, which left\src/report.pyin CI and would have recordedsrc\report.pyon a real Windows host — never matching a path fromdiff. It is now stripped as aPathand rejoined with/.Tests
50 pass (was 40). The lease writer's six tests were mutation-checked: writing the lease inside the repo — the 65→120 ms regression — fails four of them.
timeline.shacceptance passes. clippy and fmt clean.Summary by cubic
Records what a tool is about to touch so anything scheduling work alongside an agent can see where the agent is working, fixes three paper cuts from driving the CLI from scripts, and documents a fork/exclude gap that can exhaust the object store.
Leases
timestamp,tool, and repo-relative path (normalized to/on Windows) to aleasesfile in the store; a command that names no file records*.ACYCLIC_NO_LEASESturns lease recording off without a rebuild.product::helpers so the CI product-name guard passes.CLI and docs
decode: EOF while parsing a value at line 1 column 0.turns --limitmatchestimeline --limitand reports how many older turns it hid.excludematches full paths, not names; the wrong form fails silently.excludedoes not govern paths created inside a fork; onecargo checkcan add gigabytes and wedge the store, and no fix is implemented yet.Written for commit a3b5339. Summary will update on new commits.