Decide which proposed vouchers are already in the book - #294
lamemustafa wants to merge 91 commits into
Conversation
Every failure across four document-import engagements was binding a document's entities to the target book's masters, never reading the document. Bridge was growing two answers to it: an MCP-private prefix matcher that named one near-miss candidate as `exact_live_spelling`, and a desktop screen that correctly ranks nothing but narrows nothing either. Add `bridge_tally_core::master_binding` as the single contract both surfaces consume, per ADR 0016. It matches an identifier embedded in a master name before the name itself, binds only where a rule is unique on both sides, and never resolves a near-miss: it reports candidates with the rule that surfaced each, and no score. An empty catalogue is a typed refusal rather than a report full of "missing". Deletes `master_match` and `master_key` from agent_import and moves unicode-normalization down a layer with them. The write gate is unchanged: build_import_xml and the approved-post recheck still admit byte-exact names only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…scale Self-review found a real defect: a catalog name was trimmed at the boundary, so a bound row reported a spelling the book does not contain. The write gate compares byte-exact against Tally's own name, so that would have refused with no explanation. Observed names are now retained verbatim; only source names are trimmed. Names differing solely in surrounding whitespace are an ambiguity, not a refused catalog. Adds a characterization suite over one fabricated 200-master book with the recorded naming pathologies. The assertion that matters is that no entity binds to a master a human would not have chosen; the counts are pinned underneath so loosening a threshold has to move a number. The mutation sweep was checked against two positive controls rather than trusted for passing: resolving a near-miss to its first candidate trips it, and binding a lone candidate does not. Both results are recorded in the test, so it is read as "no mutation reaches the wrong master" and not as "no rule change can loosen binding". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rzakhani-94af40 # Conflicts: # docs/tally/compatibility/compatibility-matrix.json # docs/tally/compatibility/compatibility-surface.json
Ten review findings, read against the tree rather than taken at face value, and all reproduced. The severe one: bind_one selected Identifier before ExactName, so a ledger carrying a number, requested byte-exactly, returned match_state identifier while the write gate admits exact only. Every ledger with a phone or account number in its name was permanently unimportable — the exact population this contract was built for. When the two signals agree it now reports the byte-level fact. Also: neither side trims a name any more, so a trailing space cannot claim byte equality it does not have; digits inside a mixed code are no longer emitted as a standalone numeric; all admitted eight-digit date orders are excluded, not just year-first; more identifiers than the bound is refused rather than truncated, which could hide a conflict; the source-entity bound now covers what the source parser admits and the desktop states whether narrowing ran; desktop candidate bytes are capped in aggregate; a binding refusal keeps the catalogue evidence it already read; and master_binding.rs is sealed into the compatibility surface, which needed a deliberate one-file cap raise since it now decides admission outcomes. The larger finding came from running the binder over 470 real ledger names from all 16 synthetic books. Prefix matching offered a median of 40 candidates, 63% of the catalogue, and omitted the right master a third of the time: a truncated name reaches a whole family, and an alphabetically capped slice of DN Party 001..120 does not contain DN Party 057. A prefix matching more masters than the cap is now counted and deliberately not listed. Re-measured on the same names: where candidates are listed the right master is present in 403 of 403 rows, median list length 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The remaining review finding, and the one I had triaged but not fixed. FallbackBinding::assign took an EntityBinding detached from its report plus any catalog, so a stock-item binding could be parked against a ledger catalog and the result carried no provenance for anything downstream to detect. Assignment is now a method on BindingReport taking an index into its own entities, so an entity from another report cannot be named at all, the catalog class is checked, and the binding carries its class forward. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A fix is a change and generates its own findings; the re-review of the previous commits raised four, all reproduced. An unusable ledger name in a parsed draft was skipped while the response still claimed a complete narrowing pass, so the rows that vanished were exactly the ones worth looking at; the pass is now reported unavailable. The reported candidate total took the larger of the suppressed family and the retained candidates, which under-reports when they are different masters; it is now their union. An identifier hint reached extraction without the bound applied to every other name. And the operator workflow in docs/agent/README.md still told readers to correct only near_miss rows, which now leaves a bound-but-not-exact row refused at the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two further findings from the re-review, both reproduced. `Purchases FY25` and `Sales FY25` both yielded the code identifier `FY25`, so identifier-first matching bound the source to whichever existed before it ever compared the names. A fiscal-period label identifies a period, not a party, and is now excluded by shape; the minimum code length also rises from four to six, since a four-character mixed token is weak evidence of identity and the failure mode here is money against the wrong party. Re-measured against the same 470 live ledger names: no change to the distribution, so the tightening costs nothing observed. Candidate collection recounted every prefix from the start of the name, making it quadratic in a field the source parser lets reach 4 KiB. It now carries the character count forward in one pass. 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. |
No book on the instance carried an embedded identifier: across 470 live ledger names from all 16 loaded companies, zero yielded a numeric identifier and exactly one a code identifier. The rule that separates this from fuzzy matching had no live coverage at all. Ten `MB ` ledgers now exist in BRIDGE CORPUS OPENING, parented to Suspense A/c so no receivable, payable or ageing measurement moves, and documented in TEST_CORPUS.md section 9 with the import method and the company-choice reasoning. BRIDGE PROBE B SANDBOX was rejected as the target despite the manufacturing precedent: it shares a GUID with a second loaded company and Bridge's own reads refuse it as company_identity_ambiguous. Within minutes the pair sharing one identifier exposed a defect no fabricated fixture had produced. A byte-exact request for a ledger whose embedded number is shared with another was refused as IdentifierConflict, making that ledger permanently unimportable, since the write gate admits exact only. Byte equality with an observed master name is now decisive: it names exactly one master, and an ambiguous identifier does not undermine it. Only a decisive identifier pointing elsewhere still outranks an exact name, and that stays a reported conflict. Re-measured over 485 live names, 2,330 cases: identifier binds 3 -> 11, every uppercase mutation now binds, and where candidates are listed the right master is present in 434 of 434 rows at a median list length of 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8ded397 to
7bc66ad
Compare
![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: 8ded39786b
ℹ️ 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".
| ScenarioPlan::new(Fixture::SyntheticXml(body)) | ||
| .with_encoding(WireEncoding::Utf16Le) | ||
| .with_framing(ResponseFraming::ContentLength) |
There was a problem hiding this comment.
The only end-to-end cycle feeds this hand-authored voucher window through Fixture::SyntheticXml, so it verifies the parser and decision rules against data this repository invented rather than against an actual Tally response; the ADR itself states that no verdict from this contract has been checked against a real book. Because this change adds a large production decision engine whose Absent result authorizes importing vouchers, replace or precede this synthetic cycle with captured live evidence before treating the component as shippable.
AGENTS.md reference: AGENTS.md:L79-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and not fixed — this one cannot be closed by code, so I want to be plain rather than argue it away.
You are right about AGENTS.md P1, and ADR 0017 states the same limit in its own Consequences: the fixtures are fabricated from a placeholder alphabet, and no verdict from this contract has been checked against a real book. Binding is pure computation over already-observed data, so P1's live-evidence requirement is satisfied upstream by the two qualified reads that feed it — but that argument covers the reads, not the decision rules, and it does not cover the claim that the output is useful on the 214-ledger book that motivated it.
I have surfaced this to the repository owner as a merge decision rather than resolving it myself, with the concrete next step: replay the twenty-invoice engagement that motivated the work, where fifteen should return present and the one posted short by a dropped GST head should return present with an amount difference. That is a measurement, not a test, and it belongs to someone with access to a real book.
Leaving this thread open deliberately.
There was a problem hiding this comment.
Accepted, and not closed. I want to be exact about what has and has not changed here rather than resolve this on adjacent work.
Where you are right. The only end-to-end cycle drives Fixture::SyntheticXml — a voucher window this repository wrote — so it exercises the parser and the decision rules against data we invented. It cannot show that a real Tally response parses into the rows this engine assumes, and the ADR says as much. For a component whose Absent result authorizes an import, that is the wrong kind of evidence to ship on.
What has changed since you filed this, and why it does not close it. Your sibling thread on WindowRead::Complete turned out to be the sharp end of the same problem: the engine was asserting that a nonempty read saw its whole window, which is precisely the assumption a synthetic fixture cannot test, because a synthetic fixture is never short. That is now corroborated against a second, wider read rather than assumed. It is a better failure mode, and it is still not live evidence.
What is blocking the rest. Both lab endpoints (9000 and 9001) went down mid-session and have not returned. I have a watch on 9001. So I cannot capture the slice now, and I am not going to argue that fixtures plus a corroboration rule add up to a live cycle.
What would close it, so it is actionable by whoever gets there first: a captured Vouchers response from a lab company with known contents, retained as bytes, replayed through this engine, with the verdicts checked against what that book actually holds — including at least one proposal that is present and one that is absent. The MB -prefixed ledgers seeded for the binder are synthetic and in a lab company, so the data stays fabricated while the response becomes real, which is the distinction your thread is drawing.
Until that exists this thread stays open, and the ADR keeps saying no verdict has been checked against a real book.
There was a problem hiding this comment.
The replay ran. Against a licensed TallyPrime 7.1 Silver instance (education_mode: false), over a real book, through the shipped vouchers read profile and this decision engine.
totals: {"requested":20, "present":16, "possibly_present":0, "absent":4}
book: {"window_voucher_count":82, "unmatched_book_vouchers":66,
"duplicate_number_group_count":0, "unbalanced_voucher_count":0}
Fifteen invoices the book already holds came back present by manual_voucher_number, every one with an empty differences list. One short by the engagement's 36.13 came back present with an amount difference -- the finding that engagement needed and nobody had asked for. Four new-customer invoices came back absent.
Committed as replay_the_twenty_invoice_engagement on claude/presence-live-replay, branched from this PR's head. It is #[ignore]d and environment-driven, following this repository's existing live-test convention.
Three things about it you should weigh rather than take on trust.
It is read-only, deliberately. Seeding a book would need a disposable company and this MCP cannot create one, so the proposals are built from the book's own rows -- the present ones are present by construction. That makes the present arm a weaker claim than a seeded book would give: it proves the rules identify a voucher they were shown, not that they would identify one posted independently.
It inverts the shortfall. The engagement's voucher was short in the book; read-only, the harness puts the shortfall on the proposal side. The difference detected is the same one, the side missing the GST head is reversed, and the test says so in place rather than only here.
It does not close your second finding. A silently short window still needs a source-side control total; this replay is over a window whose completeness rests on the same unproven cardinality, so it is evidence about the rules, not about the read.
Two earlier runs of this harness produced wrong answers, both my fault, and one is worth recording because it looked like a finding: with the invented proposals accidentally carrying an unmodified amount, the engine returned possibly_present / resembles_book_voucher rather than absent. That was correct -- in a one-day window every row shares the date, so a known party resembles something on date and party and absent is properly withheld. I nearly wrote it up as engine behaviour worth questioning; it is the contract working, and the NoDiscriminatingCandidate pressure ADR 0017 predicted from the 470-name measurement showing up on a real book.
I am leaving this thread open for you: whether a read-only replay on a lab book meets the bar you set is your call, not mine to close.
7bc66ad to
6294652
Compare
Two findings from the latest re-review, both reproduced. Enumerating the period shapes that must not become identifiers was a losing game: FY25 was fixed, then APR2025 and 2025Q1 were still binding two unrelated ledgers that merely share a period. The rule is now a shape — every run in the token is a short alphabetic marker or a number reading as a year or small ordinal, at most three runs — and a code identifier additionally needs eight alphanumerics, three digits and two letters. Requiring real length is the part that does not depend on having thought of every label. Measured against 485 live ledger names, exactly one yields a code identifier at all, and it still does. The aggregate candidate budget was applied to the consumer's copy, so the report's own clones were already allocated by then; capping the copy bounded only the copy. The budget now lives in bind() and is spent in entity order, and the desktop's second budget is deleted as redundant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The voucher-presence lane needs the same fold for voucher numbers and voucher-type names that master names use, and is exposing this function crate-wide to get it. That is the right call — a second, subtly different normaliser is the divergence ADR 0016 exists to end, and it would diverge silently, agreeing on every name tested by hand and differing on the punctuation nobody thinks to try. Records that obligation at the function, and its corollary: changing what this folds changes every consumer's notion of sameness at once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c56a03765
ℹ️ 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".
e4004bd to
bff99de
Compare
A client-supplied REMOTEID is not readable back — Tally overwrites the attribute with its own value (IMPLEMENTATION_GUIDE.md §3.3a, fourth property, verified). Nothing here uses it, but the doc on unresolved_identity said only that the identity is retained "for later reallocation" without naming the channel, and the obvious wrong choice fails silently: an amount parked with its identity in a write-only field is unreallocatable, and nothing about the write says so. Says narration, and says why. Surfaced by the voucher-presence lane, which had to correct three claims of its own after reading the reference's §9.3 heading as the general case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a192c875b
ℹ️ 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".
…facts candidates can be empty because nothing resembles the name, because a family resembles it and none is separable, or because the list was cut — and those mean opposite things to whoever decides what to do next. The disambiguators are reason, candidate_count and candidates_truncated, and reading the empty vector alone is wrong in two cases out of three. Recorded here rather than left to each consumer because it has already been got wrong twice by different lanes: the preparation screen rendered "0 possible ledgers are listed first" over a family of 120, and the voucher-presence contract needed a paired test to stop its rule collapsing into "no candidates means unknown". Same defect class this ADR was written against — a refusal whose neighbouring value reads as an answer. Also records why this is a doc and not a type. An enum of Listed / Truncated / Withheld / None is the stronger fix and the one P2 asks for, but it is breaking, a stacked consumer already depends on candidates_truncated as a predicate and holds the boundary with tests, and forcing that rework mid-review trades an improvement for a regression risk. Revisit once both have merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3a192c8 to
f5bd780
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbe2934151
ℹ️ 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".
…isting Five items in one round, because review reopens once either way and the marginal cost of the rest once it is reopened is small. Tally's master-name matching is measured, not guessable: IMPLEMENTATION_GUIDE.md §3.3b found it case-insensitive AND separator-insensitive — a hyphen matches a space — and otherwise exact. The binder was stricter, which is not the safe direction it looks like: it refused names Tally accepts, and `X - Y` is a common ledger convention. A separate master_identity_key follows §3.3b and stops where Tally stops; `AND` for `&`, a missing suffix word and a singular for a plural still refuse. It is separate from comparison_key rather than a widening of it, because that one is shared with voucher numbers and voucher-type names and §3.3b says nothing about those. Measured live: 16 of 16 hyphenated masters now bind from the spelling Tally itself accepts, where all 16 were near-misses before. Candidates becomes None | Listed | Truncated | Withheld. An empty vector was three different facts and a consumer reading is_empty() was wrong in two of them, a shape already got wrong twice by different lanes. Taken before merge because the contract has not shipped and this is the cheapest it will ever be; the consumer who pays for it measured thirty lines and reported the change improves its code. The MCP result gains an explicit listing discriminator, since a model is the caller that would read an empty array as "no such ledger exists"; the desktop DTO stays flat, where the screen already distinguishes the cases and is tested. Also: FallbackBinding says reallocate with a Journal and never Alter or Cancel, which §9.7 measured as duplicating with the target untouched while reporting success; the ADR records that identifier coverage is bimodal by client (42%, 0%, 0%, 0%) so the rule is a first-pass check and never a primary key; and BindingStatus says what a Bound does not establish — not that the master still exists, not that the requested name may be written, not that it is right in business terms, and no authority at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex reviewed #294 and I could not invalidate a single finding. The four that mattered all failed in the silent direction this contract exists to avoid. One book voucher could satisfy two proposals. Each identity basis enforced its own uniqueness and nothing stopped two proposals reaching the same voucher by different bases — one by REMOTEID, another by a manual number — so a consumer would exclude two source vouchers against one book row. Every claimant of a contested voucher is now demoted. A proposal's REMOTEID was discarded and could still be called absent. The adapter sets every observed voucher's id to None because the read profile does not fetch it, so a proposal carrying a REMOTEID had its strongest key silently skipped and could still be reported absent on the keys that happened to remain. The report-level remote_id_observed flag did not stop that — a status field does not neutralise the verdict printed beside it, which is the exact defect ADR 0017 cites elsewhere and had reproduced. A window now declares RemoteIdEvidence, and NotRead withholds absent structurally. The catalogue was never corroborated after the voucher read. A ledger renamed between the two reads would bind proposals to the old name while the rows carry the new one, removing the only resemblance. Now re-read and refused on drift, matching the selected-voucher read. A number match contradicted by a different REMOTEID settled anyway. Reported as a conflict now, per ADR 0016's identifier-versus-name rule. Five more, none silent: candidates dropped by the response cap were counted as book vouchers no proposal reached; the party diagnostic tested every entry ledger while reporting the party field, so a real disagreement went unreported; the two cross-input refusals ran after three Tally reads despite the stated pre-read guarantee; nested schema bounds and additionalProperties were advertised and not enforced; and the tool description still carried the flat no-idempotency claim the ADR had already corrected. Nine regression tests, one per finding plus the contrasts that keep each rule from drifting. 865 lib and 157 core green, clippy and fmt clean, reseal run and gate passing. Two findings are not addressed here because they are not code: no live evidence, and window completeness resting on cardinality this read cannot prove. Both are already stated as limits in ADR 0017 and both are answered on their threads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf421524d1
ℹ️ 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".
Three were the same shape recurring: an identifier built from something that identifies a period rather than a party. A token carrying letters now never yields a standalone numeric, whether or not it qualified as a code — `Part A12345678` was reaching an unrelated `Bank 12345678` through the one-letter gap the code test rejects. Period labels are recognised by their numbers rather than their words, which catches `SEPTEMBER2025` and `2025QUARTER1` that no cap on the alphabetic run ever would: a month name can be any length, a year cannot. And a fiscal range is excluded before its digits are fused, since `2025-2026` strips to an eight-digit run no calendar reading rejects. Fallback assignment now checks catalog provenance, not just class: two ledger catalogs are both Ledger, and a fallback drawn from the one the report never saw names a master that was never a candidate. Candidate collection selects by index and clones only what it retains, instead of cloning every match before the cap and the budget discard most of it. ADR 0016 quoted thresholds this module stopped using two rounds ago, and it is the contract two surfaces integrate against. Synced — and a test now reads the ADR and asserts it quotes the live constants, so the next drift fails rather than waiting to be noticed. Verified against a positive control: changing a constant without the document fails it. It also caught a false positive of its own on first run, which was the detector being too strict about `(10%)` rather than the ADR being wrong. Live re-measure over 485 names, 2,330 cases: 434 of 434 listed rows still contain the right master, median listed length 2, no wrong binds. Identifier binds 11 -> 8 with bound and unbound totals unchanged: three mutations that had bound to themselves through a leaked numeric now bind by name instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ializable Both reproduced by execution before fixing, and both re-verified against the same reproduction after. A token shared across a name family IS the family. Where the catalog is large enough that the token stays under the common-token threshold — 30 rows among 330 is 9% — the shared-token pass re-offered exactly the rows the prefix pass had withheld, restoring the arbitrary capped slice that withholding exists to prevent. Measured: the case reported near_miss with 25 of 30 listed and the intended master outside the slice; it now reports no_discriminating_candidate with the family counted and none listed. Weaker rules now skip the withheld set; decisive rules still reach a family member on its own evidence, because the identifier or the whole key is what separates that one from its siblings. Candidates::Listed was a newtype variant carrying a sequence, and under Serde's internally tagged representation a tag cannot be merged into a sequence — so serializing the MOST COMMON unresolved result failed at runtime while the other three variants succeeded. Now a struct variant. No test caught it because none had ever serialized an Unresolved, only a Bound; every variant now round-trips in a test. Live re-measure unchanged: 434 of 434 listed rows contain the right master, median listed length 2, no wrong binds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39d6da2de5
ℹ️ 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".
Codex reviewed #294 and I could not invalidate a single finding. The four that mattered all failed in the silent direction this contract exists to avoid. One book voucher could satisfy two proposals. Each identity basis enforced its own uniqueness and nothing stopped two proposals reaching the same voucher by different bases — one by REMOTEID, another by a manual number — so a consumer would exclude two source vouchers against one book row. Every claimant of a contested voucher is now demoted. A proposal's REMOTEID was discarded and could still be called absent. The adapter sets every observed voucher's id to None because the read profile does not fetch it, so a proposal carrying a REMOTEID had its strongest key silently skipped and could still be reported absent on the keys that happened to remain. The report-level remote_id_observed flag did not stop that — a status field does not neutralise the verdict printed beside it, which is the exact defect ADR 0017 cites elsewhere and had reproduced. A window now declares RemoteIdEvidence, and NotRead withholds absent structurally. The catalogue was never corroborated after the voucher read. A ledger renamed between the two reads would bind proposals to the old name while the rows carry the new one, removing the only resemblance. Now re-read and refused on drift, matching the selected-voucher read. A number match contradicted by a different REMOTEID settled anyway. Reported as a conflict now, per ADR 0016's identifier-versus-name rule. Five more, none silent: candidates dropped by the response cap were counted as book vouchers no proposal reached; the party diagnostic tested every entry ledger while reporting the party field, so a real disagreement went unreported; the two cross-input refusals ran after three Tally reads despite the stated pre-read guarantee; nested schema bounds and additionalProperties were advertised and not enforced; and the tool description still carried the flat no-idempotency claim the ADR had already corrected. Nine regression tests, one per finding plus the contrasts that keep each rule from drifting. 865 lib and 157 core green, clippy and fmt clean, reseal run and gate passing. Two findings are not addressed here because they are not code: no live evidence, and window completeness resting on cardinality this read cannot prove. Both are already stated as limits in ADR 0017 and both are answered on their threads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca673ee40b
ℹ️ 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".
| // The adapter has no source-side cardinality for nonempty | ||
| // windows. A later catalogue reread cannot change the fixed | ||
| // `Partial` state into a complete observation, so avoid the | ||
| // extra endpoint load and fail with the evidence already in | ||
| // hand. A future qualified nonempty path can continue to the | ||
| // paired-snapshot checks below. | ||
| return Err(PresenceError::WindowIncomplete |
There was a problem hiding this comment.
For every nonempty voucher window, this branch unconditionally returns presence_window_incomplete, so the shipped surface still has no working end-to-end presence slice while this change adds the full decision engine and extensive simulator/unit coverage. Fresh evidence after the earlier documentation fix is that merely recording the qualification gap does not satisfy the repository rule prohibiting component growth beyond a live-proven slice; establish source-side completeness and live evidence first, or defer the unexercisable implementation.
AGENTS.md reference: AGENTS.md:L86-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead47bf3aa
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ce6e0c9b9
ℹ️ 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".
1 participant
HELD for operational qualification. Source-side completeness evidence for nonempty voucher windows remains unavailable. Current local tests do not establish live presence verdicts or authorize importing vouchers. Addresses the presence review debt tracked by #317.
Functional summary
The presence contract compares proposals with an observed book window using bounded identity and diagnostic evidence. The production adapter refuses unqualified nonempty windows with
presence_window_incomplete; even agreeing bounded reads do not establish source cardinality. Only the existing qualified empty-window control can construct Complete.A supplied, observed REMOTEID conflicts with a numbered voucher whose REMOTEID is missing or different. Unread identity evidence withholds a decision before conflict classification; duplicate proposal numbers and unobserved voucher types retain their existing rules. A remote-identified row cannot settle against a contradictory absent manual number. No candidate is chosen automatically. An unread proposed REMOTEID remains the reported reason for withholding a decision even when resemblance or nondecisive-number candidates exist; those candidates remain visible. An identified voucher with a different exact voucher type reports both spellings through a
voucher_typedifference.Admission caps the proposal/window product at 1,000,000 and indexed resemblance work at 5,000,000 units. The weighted bound includes posting-list walks, pooled candidates and party-key fanout, including work for identity-settled proposals. Exact observed ledger and party spellings must exist in the catalogue; raw spellings and folded comparison keys remain separate and bounded. Numbering declarations retain count and aggregate-byte limits, and internal voucher-number whitespace remains significant. A shared raw-observation budget additionally limits voucher count, 100,000 raw entries and 4 MiB of ledger/amount bytes before parsing, cloning or folding entries. The adapter applies it before allocating entry descriptors, and the public core batch boundary protects direct callers. Retained book metadata has a separate 4 MiB bound. Raw proposals have their own 100,000-entry, 4 MiB entry-byte and 4 MiB metadata budgets before conversion; their positions must be unique. The core request accepts only an opaque admitted
ProposedBatch, so callers cannot concatenate separately converted vectors around admission. ADR0017 preserves voucher-number Unicode form with outer trimming only, keeps voucher-type spelling exact, and documents missing or unequal observed REMOTEID as conflict.Test or reproduction command
Candidate
17bfe9786bf48794b0f2a0a197042e8be29f5e0e, based onmaster. Master06528eeb615b827896b9649ea476034143bbf7f9is in its ancestry.cargo test --locked --manifest-path src-tauri/Cargo.toml -p bridge-tally-core --lib: 263 passed on final runtime/test bytes. Controls cover all three nondecisive candidate classes with unread REMOTEID and serialize the exact voucher-type difference.-D warningsand direct core-manifest rustfmt checks passed.cargo test --locked --manifest-path src-tauri/Cargo.toml -p bridge --lib agent::presence::tests: 19 passed, 1 existing authorized-live test ignored. The adapter preserves voucher-type differences through party marking. No live replay was run.cargo test --locked --manifest-path tools/Cargo.toml -p bridge-tally-compatibility: 24 library and 4 binary tests passed with the shell's normal umask 022. A separate existing CLI mode-preservation defect under umask 077 is tracked for rectification; this pin change does not alter that writer.agent_catalog.rsadmission schema is now a required compatibility pin. The bound rises by exactly one for that file; 216 unique paths match actual bytes. The required-path omission regression rejects dropping it. Rehash/seal/repoint/gate andgit diff --checkpassed.Migration compatibility
No destructive database migration or persisted-schema change. Direct core callers must use
BookWindow::from_observationsandProposedVoucher::from_inputs, which returns an opaqueProposedBatchrequired byPresenceRequest::new. Individual proposal construction exists only for crate tests; per-voucher and preconverted-window construction remains crate-private. Admitted batches expose read-only views, preserving the aggregate boundary. Incomplete reads remain typed refusals. The operational live-evidence gate remains open; clean status against a feature branch does not establish master acceptance.Rollback notes
Revert the PR to remove this presence contract and endpoint extension. Preserve unknown-outcome lookup and never automatically replay financial writes. Windows interactive/package qualification and source-complete nonempty presence behavior remain unproven.
Security impact
This is a read-only comparison path. Company admission, exact catalogue coverage, bounded declarations and refusal on incomplete evidence remain enforced. No credential changes, live Tally write, import, or client financial mutation occurred in this rectification.
Windows validation evidence: native/package CI passed on the previous 6ce6e0c candidate; final-head hosted checks and interactive qualification remain outstanding.
macOS validation evidence: the local Rust 1.96 core, adapter and tools suites above passed; nonempty live qualification remains held.