Corroborate a nonempty window instead of assuming it complete - #313
lamemustafa wants to merge 1 commit into
Conversation
`WindowRead::Complete` was asserted for any nonempty response, and that is not something a response can be inspected for: a silently short read returns rows that are all well-formed and all inside the window, and looks exactly like a full one. The voucher it omits then reads as `Absent` -- the one verdict in this engine that puts a duplicate in a client's book. So the range is re-read a day wider and the two reads are compared on Tally's own identities for the rows falling inside the original window. A row the wider read saw and the narrow one did not means the narrow read was short. A row carrying no `GUID` cannot be compared at all and is refused rather than assumed equal, and a row appearing between the two reads makes the window uncorroborated rather than absent. All of those fail towards `Partial`, which `BookWindow::observed` turns into `WindowIncomplete`. **The design is not mine.** It was rescued from an unowned worktree and is reviewed here rather than adopted: two corrections and a price. It did not build. The simulator cycle pins the request count and the run makes twenty-eight against the twenty-four asserted -- and the comment justifying that number said "the two extra are the wider voucher read and its paired status probe", when a corroborating read in this adapter is a *paired* read of six requests. The cost being pinned had been mis-estimated threefold. Two other simulator plans needed the extra read as well. **The price, measured against a live licensed 7.1 Silver book.** A one-day window on a dense corpus reads 895,888 bytes; its corroborating read of the same range widened a day either side reads 1,744,152 -- **1.95x** -- taking one presence call from ~896 KB to ~2.6 MB of Tally traffic. `max_bytes` is bounded at 5,000,000, so this roughly halves the widest window the tool can serve before corroboration alone fails the call. Wall-clock is not quoted: across runs on this lab it varied between six and twenty-six seconds for the same work, which is not a measurement of anything. The twenty-invoice replay still passes with it enabled -- sixteen present, four absent, unchanged. **What it establishes, and what it does not.** It catches a read that fell short because of *size*, which is the case the finding describes: a wider read is denser, so it truncates at least as hard, and a row the narrow read has and the wider one lacks proves the narrow read short. It does **not** catch a voucher both reads omit for a reason unrelated to size, and it is neither a source-side count nor a complete partitioning. It raises the bar; whether it clears the one the reviewer set is the reviewer's judgement, not mine. Pin set read off the base branch at reseal: none added, none removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1fc8ff510
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Closing this without merging. It does not do the thing it was built to do, and the two open findings on it are both correct. It cannot establish completeness, and ADR 0017 says why in a sentence I wrote before building it. A deterministic short answer agrees with itself across a pair. If the boundary days this adds happen to be empty, both reads drop the same suffix, the identity multisets match, and agreement becomes indistinguishable from correctness. The mechanism converts some false It is expensive. Measured on a licensed TallyPrime 7.1 Silver book: a one-day window reads 895,888 bytes, its widened corroboration reads 1,744,152 — 1.95× — taking one presence call from ~896 KB to ~2.6 MB. Against a And the widening introduces a hazard of its own. On Education-mode Tally an accepted boundary can widen into a date Tally silently reinterprets as the whole book, so a mechanism added to make a verdict safer would either reject a valid request or compare evidence from the wrong period. A detector with that price and that hazard is not worth shipping for the cases it does catch. What survives, and where
The source-side control total remains the only thing that would close the finding, and it is still a separate read contract needing its own live evidence. #294's corresponding P1 stays open — this PR does not narrow it. ProvenanceThe design was not mine. It was rescued from an unowned worktree, and what I added was two corrections, three simulator-plan updates and the price. Closing it is not a judgement on the person who wrote it — the idea was worth trying, and the reason it fails is only visible once you measure it and re-read the ADR. The branch |
1 participant
Stacked on #294. Do not merge before it. Sibling of #308, which is also on #294 — both touch
agent_presence.rsandagent_presence_tests.rs, so whichever lands second rebases.This is offered for judgement, not as a finished answer. It addresses the open P1 on #294 ("Withhold
Completeuntil source cardinality is proven"), and I do not claim it closes it — see What this does not establish below. It is a separate PR precisely so its cost can be weighed on its own rather than folded into a PR that is trying to land.Outcome and reason
WindowRead::Completeis asserted for any nonempty response. That is not something a response can be inspected for: a silently short read returns rows that are all well-formed and all inside the window, and looks exactly like a full one. The voucher it omits reads asAbsent— the one verdict in this engine that puts a duplicate in a client's book.The range is now re-read a day wider, and the two reads are compared on Tally's own identities for rows falling inside the original window. A row the wider read saw and the narrow one did not proves the narrow read short. A row carrying no
GUIDcannot be compared and is refused rather than assumed equal. A row appearing between the two reads makes the window uncorroborated. All fail towardsPartial, whichBookWindow::observedalready turns intoWindowIncomplete.Provenance
The design is not mine. It was found as uncommitted work in an unowned worktree, rescued to
rescue/voucher-presence-wip-20260912by the session holding the merge queue, and handed over unreviewed. I reviewed it rather than adopting it. What is mine here is two corrections, three simulator-plan updates, and the price.Scope, reuse, and impact
widened_window,validate_then_filter_voucher_rows,parse_agent_rows,row_in_windowandpost_read— the nonempty path is the empty path's counterpart and borrows its whole shape. No new request profile, no new XML.WindowRead::Completeassumption, and thelet mut reason = None/ conditional-assignment shape it needed.Absent, andAbsentauthorises an import.state: "partial"andWindowIncompletewhere they previously saw a verdict — that is the intended behaviour change and it fails closed.Validation and evidence
a1fc8ff5cargo fmt --all -- --checkclean;cargo clippy --locked --workspace --all-targets -- -D warningsclean;cargo test --locked --workspace1351 passed, 0 failed; gatecompatibility_gate_passed:unknown_claims=11:evidenced_claims=0, pin set read off the base at reseal — none added, none removed.present, fourabsent, unchanged.The price, measured
On a dense corpus against a licensed TallyPrime 7.1 Silver instance (
education_mode: false):One presence call goes from ~896 KB to ~2.6 MB of Tally traffic.
max_bytesis bounded at 5,000,000, so this roughly halves the widest window the tool can serve before corroboration alone fails the call.Wall-clock is deliberately not quoted. Across runs on this lab the same work varied between six and twenty-six seconds, which measures the lab rather than the change. I reported "6.5s against 6.3s" to a peer earlier today off two runs; that was under-supported and I am withdrawing it.
Two corrections to the rescued work
What this does not establish
It catches a read that fell short because of size — the case the finding describes. A wider read is denser, so it truncates at least as hard, and a row the narrow read has and the wider one lacks proves the narrow read short.
It does not catch a voucher that both reads omit for a reason unrelated to size, because two reads that agree prove only that they agree. And it is neither a source-side count nor a complete partitioning, which is what the finding actually asked for. It raises the bar. Whether it clears the one that was set is the reviewer's judgement, and I have not resolved that thread.
review-checklist.mdline is linked here: "New or changed commands validate inputs and surface user-safe errors." — the two new outcomes arewindow_short_readandwindow_identity_unreadable, both fixed strings carrying no book content, and both resolving toWindowIncompleterather than a verdict. Seeagent_tests.rs::a_nonempty_window_is_corroborated_rather_than_assumed_complete.Checklist
🤖 Generated with Claude Code