fix(hooks): a folded reply carries no second accounting of itself - #777
Conversation
…the fold Two accountings of one payload landed in one blob and their sum exceeded the input. On a re-run of git log --oneline -40 the distiller cut a single byte, the ledger folded the reply to a few lines, and the banner announced '1 bytes omitted' on top of fold markers that already covered all 40. Every number was true and the arithmetic a reader can do was not. The affordability test read final_out, which is the ledger's output, so the saving the ledger made bought a marker describing the distiller's cut. It reads distilled_len now: the numbers in that marker describe the distiller, so the test that decides whether to print it has to describe the distiller. The first version of this fix stopped there and broke the saving. The else arm throws the whole reply away, which was correct when the distiller was the only stage that touched it and is not correct now the ledger runs first: the same re-run turned a 3,257 byte reply folded to 325 into a passthrough. It passes through only when the ledger left the reply alone. Also: 1 bytes, 1 lines. Singulars. Closes #775
… other The first cut of this asked only whether the distiller's cut could pay for the marker, and dropped the question of whether the reply that results is still smaller than the command's own bytes. Those are different questions and the second one covers bytes this function appends after the distiller's numbers are taken, including a [Partial signal] banner. Caught by never_hands_back_more_bytes_than_the_command_produced, which drives process_payload with no store, so the ledger never runs and my new arm was not even involved: 104 bytes back for 99. The repo's own invariant found it, not review, and not me.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else if ledger_folded && final_out.len() < content.len() { | ||
| // The marker cannot pay for the distiller's cut, so it is not | ||
| // printed. The ledger's fold is a different saving with its own | ||
| // markers and its own handles, and throwing it away here cost the | ||
| // whole of it: on a re-run of `git log --oneline -40` this arm | ||
| // turned a 3,257 byte reply that had been folded to 325 into a | ||
| // passthrough, because the distiller had cut one byte. | ||
| // | ||
| // Nothing is left unreachable. The handle is cleared because no | ||
| // marker names it, and every folded run carries its own. | ||
| // | ||
| // The size test is not decoration. `never_hands_back_more_bytes_than_ | ||
| // the_command_produced` caught the first version of this arm handing | ||
| // 104 bytes back for 99: a fold on a tiny payload can cost more than | ||
| // it saves, and the old passthrough was what capped that. Keeping the | ||
| // fold is only right when the fold is smaller. | ||
| rewind_hash.clear(); |
There was a problem hiding this comment.
When the distiller removes less than its marker costs and the ledger also folds the reply, this branch keeps the folded output but clears the only handle covering the raw pre-distillation content. The ledger handles recover only runs from the already-distilled text, so the agent cannot recover bytes removed by the distiller.
Knowledge Base Used: Tool hook integration
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/post_tool.rs
Line: 1047-1063
Comment:
**Distiller cuts lose recovery**
When the distiller removes less than its marker costs and the ledger also folds the reply, this branch keeps the folded output but clears the only handle covering the raw pre-distillation content. The ledger handles recover only runs from the already-distilled text, so the agent cannot recover bytes removed by the distiller.
**Knowledge Base Used:** [Tool hook integration](https://app.greptile.com/weekndlabs/-/custom-context/knowledge-base/fajarhide/omni/-/docs/tool-hook-integration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Valid, and it is the rule this pipeline is not allowed to break, so it is worth being exact
about the size and about why I am not fixing it in this PR.
The bound. That arm is only reached when distilled_len + marker.len() >= content.len(),
so the distiller's cut is smaller than the marker that would announce it: under about 80
bytes. In the reported repro it was a single byte, almost certainly a trailing newline.
Small, and still unannounced, which is the part that matters.
Three fixes tried, and each one broke something the repository already guards.
- Keep the handle and print the marker anyway. That is the double accounting this PR
exists to remove. - Refuse a cut too small to announce, before the ledger runs, so there is nothing
unreachable to keep.a_dropped_reply_books_no_savingwent red: revertingfinal_out
beforerecord_tracemakes the raw and distilled columns identical, which silently
removes the only corpus that measures distiller behaviour without going through OMNI's
own books. That test's own comment says a previous fix made exactly this mistake. - Snapshot the distiller's output for the trace and revert the delivered text. That works
only if the trace is meant to hold the pre-ledger string, and today it holds the
post-ledger one, so the change would quietly alter whatexecution_tracesmeans.
So the question is a design call rather than a patch: either the trace's distilled
column becomes explicitly pre-ledger, or the ledger is given the raw content when the
distiller's cut is below the floor, or the sub-marker cut is accepted as a documented and
bounded loss. The first two are real changes with their own measurements; the third needs
saying out loud rather than leaving in a comment.
What this PR does ship is strictly better than main: the false claim is gone, the
arithmetic sums, and the fold survives. What it does not fix is a pre-existing class made
visible by fixing that, and it is now written down instead of discovered again in six
months.
Filing it as its own issue with these three options and the bound, and leaving this PR to
the defect it names.
There was a problem hiding this comment.
That distinction is correct. The fallback is bounded to cuts smaller than the marker cost, but those bytes are still genuinely unreachable once the raw rewind handle is cleared. Printing the marker would reintroduce the double-accounting defect, while moving the distiller boundary would change the established execution_traces.distilled semantics or break a_dropped_reply_books_no_saving.
I agree this is a valid follow-up design issue rather than a reason to block this PR: this change fixes the false accounting while preserving the ledger fold, and the remaining loss is explicit, bounded, and documented. The follow-up should capture the invariant that every distiller-removed byte must either have a retrieval handle or be intentionally accounted for, along with the three design options you listed.
1 participant
A folded reply carried a second accounting of itself, and the two did not add up to the
input. On a re-run of
git log --oneline -40, through the installed 0.7.9 with an isolateddatabase:
22 + 17 + 1 = 40, the whole input, and then a fourth line claims a byte more. That is the
false-claim class: nothing was missing and the output says otherwise.
The mechanism is narrower than the issue's own diagnosis. The banner's numbers come
from the distiller, correctly and by design (#519). What did not was the test deciding
whether to print it, which read
final_out, and by thenfinal_outis the ledger'soutput. So the ledger's saving bought a marker describing the distiller's cut, and here
that cut was one byte, almost certainly a trailing newline.
Three commits, because the first two were wrong in ways worth keeping in the history.
distilled_len. Correct, and on its own it turned a 3,257 bytereply folded to 325 into a passthrough: the
elsearm throws the whole reply away, whichwas right when the distiller was the only stage to touch it and is not right now the
ledger runs first.
ledger left the reply alone. Then
never_hands_back_more_bytes_than_the_command_producedwent red at 104 bytes back for 99 raw. That test drives
process_payloadwith no store,so the ledger never runs and the new arm was not involved at all: the leak was
[Partial signal], appended after the distiller's numbers are taken, whose cost hadsilently left the affordability test.
must still be smaller than the command's own bytes. Those are different questions and
answering only one of them is what each earlier commit did.
After all three the same command delivers 252 bytes for 3,257, and the markers sum to
exactly 40 of 40.
Also
1 bytesand1 lines. Singulars.The issue's second claim is not fixed and the issue stays open for it.
[Partial signal]over a complete answer did not reproduce in three attempts on 0.7.9 withisolated databases: the post-hook on the reported command, the same through
omni exec, anda constructed 130 line log payload aimed at the partial route. It needs
route == Softtogether with a partial rather than whole-output fold, and every fixture I built folded
whole or never reached Soft. Not a verdict that it is unreal; the reporter saw it. The
detail is in the issue.
Verification:
make cigreen,smoke_test.sh70/70, and the reported command re-runagainst the release binary.
Refs #775
Greptile Summary
This PR adjusts post-tool rewind affordability so ledger savings cannot fund a marker describing a separate distiller cut, preserves useful ledger folds when that marker is rejected, and fixes singular marker units.
git logoutput and documents the fix in the changelog.Confidence Score: 4/5
The PR should not merge until retained ledger folds preserve a retrieval path for any bytes removed earlier by the distiller.
The new fallback keeps a reduced host-visible result while clearing the only handle that archives the raw pre-distillation output, making the distiller's omitted bytes unreachable from the response.
Files Needing Attention: src/hooks/post_tool.rs
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Raw[Raw command output] --> Distiller[Distiller] Distiller --> Distilled[Distilled output] Distilled --> Ledger[Session ledger fold] Ledger --> Final[Final folded output] Raw --> Archive[Raw-output rewind archive] Archive --> Marker{Marker affordable?} Distilled --> Marker Final --> Marker Marker -->|Yes| DeliverMarker[Deliver fold plus rewind marker] Marker -->|No, ledger folded| DeliverFold[Deliver ledger fold] Marker -->|No fold| Passthrough[Keep raw host output]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(hooks): the marker has to pass both ..." | Re-trigger Greptile
Context used: