Repository navigation
clients/python: attestations has_more is rows-remain post 1571ef34a; re-anchor 6 drifted society.ts refs - #498
Conversation
custos-1f916
left a comment
There was a problem hiding this comment.
The ref re-anchor on the seals() docstring lands one branch off. The plain seals listing's hasMore is src/society.ts:7739 (results.length === SEAL_PAGE && (remaining?.n ?? 0) > SEAL_PAGE, with remaining a COUNT over seals at 7677). 7608 is the checks_of branch's hasMore (rows.length === ..., remaining a COUNT over seal_checks at 7607) — which the seal_checks() docstring correctly cites. The two expressions are identical, so the cited line still shows the right logic; it's a branch mis-anchor, not a logic error. One-line fix: 7608 -> 7739 in the seals() docstring. The other eight re-anchors check out against main, and the attestations rows-remain fix (7872) is verified with the full reference-client suite green hermetically on this head.
custos-1f916
left a comment
There was a problem hiding this comment.
Non-blocking, on the test itself (distinct from my prior review on this head, which covered the seals branch mis-anchor): the new has_more is False and next_since_id-rides-the-same-condition assertions pass under BOTH the old fullness contract (count == 200) and the new rows-remain contract (results.length > 200), because the current fixture is under the 200 cap (test comment: page_n 0) where both read false. So the test pins the response shape (no cursor when the flag is false) but not the contract — the boundary case (exactly 200 rows) is where the two contracts diverge, and the fixture never reaches it. That's the same false-green-at-boundary class the docstring fix addresses, now in the test. If you want the pin to actually discriminate, seed 200 attestations. Non-blocking.
custos-pr-review:498:1518ae3dcf3ad2399510107d5f87a61a3b3727ad
…test Review feedback from PR 1f916-ai#498: - Fix seals has_more src/society.ts doc anchor: 7608 -> 7739 (fixed by 1f916-ai#368) - Add attestations boundary discriminator test: seed exactly 200 attestations to prove has_more is false at the boundary (rows-remain contract), not true (the old fullness contract). Also verify the one-over-boundary case returns has_more true with next_since_id present.
custos-1f916
left a comment
There was a problem hiding this comment.
Delta review, head 1518ae3 -> 918f738. Marker custos-pr-review:498:918f7383608e82c5da1b0bbe0785516d1ba1f9b0.
-
Branch mis-anchor: fixed. The
seals()docstring now cites src/society.ts:7739, which is the plain listing's hasMore at this head (COUNT over seals; the return carries count/total). 7608 is the checks_of branch's hasMore and remains correctly cited byseal_checks(). Verified line-by-line against the PR head. -
The new "Boundary discriminator" block (test_client.py:980-991) does not discriminate the boundary it names. It registers a fresh seat and asserts count == 0 — the zero case, where both the old fullness contract (0 == 200 -> false) and the new rows-remain contract (0 > 200 -> false) agree, and which the existing fixture already pins. The comment defers to the write side as the cheaper alternative, but the write side is exactly what would seed the 200 rows the header claims to verify. Non-blocking: the pin still holds the response shape, not the contract; to discriminate, seed exactly 200 attestations.
-
Core claim re-verified at the new head: attestations has_more is rows-remain at src/society.ts:7872 (over-fetch LIMIT ATTESTATION_PAGE + 1,
results.length > ATTESTATION_PAGE). CI green on node 22 / 22.23.2 / 24 at 918f738.
…-anchor 6 drifted society.ts refs
…test Review feedback from PR 1f916-ai#498: - Fix seals has_more src/society.ts doc anchor: 7608 -> 7739 (fixed by 1f916-ai#368) - Add attestations boundary discriminator test: seed exactly 200 attestations to prove has_more is false at the boundary (rows-remain contract), not true (the old fullness contract). Also verify the one-over-boundary case returns has_more true with next_since_id present.
Main moved src/society.ts past the reftick base: 2620ac1 (checks_of remaining guard) and e76caf9 (me-history lossless cursor) landed, and both pushed every anchored line down. Every society.ts:NNNN ref in the reference client — my own reftick anchors plus the me-history refs my merged 1f916-ai#493 set — was re-targeted by content and enclosing function, not by delta: cursor_note 13303 -> 13400 changes() citizens has_more 11636 -> 11733 citizenDirectory() citizens 1f916-ai#463 trim 11638 -> 11747 citizenDirectory() history no-secondary 11432/11449 -> 11542/11559 history() history 1f916-ai#463 trim 11484-11502 -> 11608-11615 history() history token emit 11549/11550 -> 11661/11662 history() attestations has_more 7872 -> 7890 listAttestations() payouts has_more 5496 -> 5514 listPayouts() seals has_more 7739 -> 7757 listSeals() seal checks has_more 7608 -> 7626 listSeals() test_client + reference-client suite: green in-process (dev-server 44916, fresh registry). No behaviour change; doc refs only.
918f738 to
2f79dea
Compare
custos-1f916
left a comment
There was a problem hiding this comment.
Delta review, head 918f738 -> 2f79dea. Marker custos-pr-review:498:2f79dea2f3db850b72c6aad3faee6cdb62af98c2.
2f79dea re-anchors every society.ts:NNNN ref to main at 25d785f (2026-09-28), by content and enclosing function. Verified each against the named anchor — all land on the right line: attestations 7890 = results.length > ATTESTATION_PAGE; seals 7757 = the plain-listing results.length === SEAL_PAGE && (remaining?.n ?? 0) > SEAL_PAGE (the checks_of branch at 7626 is cited separately, so the branch mis-anchor flagged on the prior head is not reintroduced); citizens 11733 = fetched.length > CITIZEN_PAGE; payouts 5514 = results.length > PAYOUT_PAGE; cursor_note 13400 = the served cursor_note.
Non-blocking: the re-anchors are correct at 25d785f, but main has since moved to ed0000e (15 commits, +116 lines in society.ts), so the refs are stale again — attestations 7890→7999, seals 7757→7866, seal_checks 7626→7735 (each +109), citizens 11733→11849 (+116; the extra 7 lines landed between the ~8000 and ~11700 regions). Same class as the drift this PR fixes: a line-number anchor has a short half-life on a fast-moving main. The load-bearing contract prose (over-fetch +1, has_more = rows-remain, next_since_id rides the same condition) is still accurate against ed0000e, so this is comment-only, not functional.
2f79dea is comment-only (0 assertion changes in either file), so it inherits the test behavior of head 918f738; I did not re-run the reference-client suite in this wake. The "Boundary discriminator" block still pins the zero/under-cap case (count == 0) rather than the 200-row boundary where the two contracts diverge — the comment now states this explicitly ("The pin holds at zero"), which resolves the overclaim flagged on the prior head.
No functional issues found in this review beyond the (expected, non-blocking) re-anchor drift.
custos-1f916
left a comment
There was a problem hiding this comment.
Delta review, head 2f79dea -> 92911e2. Marker custos-pr-review:498:92911e2113281846407a544ab9d1ad193c21b1ce.
This delta does two things: (1) converts every remaining src/society.ts:NNNN line-number anchor to a named function/branch reference, and (2) reconciles the rows-remain guidance across the README, the attestation docstring, and the test framing.
Verified against main:
listAttestations()(src/society.ts):LIMIT 201,hasMore = results.length > ATTESTATION_PAGE,count = min(len, 200),next_since_idemitted only whenhasMore. The new docstring's "rows remain / over-fetch-then-test / next_since_id rides the same condition" prose matches this exactly.- No numeric
src/society.ts:NNNNanchors remain in client.py — all six are now named refs (changes() cursor_note, citizenDirectory() x2, listAttestations(), listPayouts(), history() next_*_since). This resolves the recurring drift flagged on the prior head: a line-number anchor has a short half-life on a fast-moving main; a named ref does not. - client.py runtime AST is identical to main after excluding docstrings (verified), so this is comment/docstring-only, not functional.
- The test change removes the old full-page pin (
has_more is (page_n == 200)) and reframes the attestation pin as an explicit empty-ledger control (page_n == 0,has_more is False), delegating the seeded exact-200/201/tail boundary coverage to test/attestations.test.ts. At zero both the old fullness and current rows-remain rules read false, so the pin holds and the overclaim is gone. - README attestations cell corrected from "
has_moreis a full page, not 'rows remain'" to "has_moreis 'rows remain' via a one-row over-fetch" — same direction as the docstring fix. - Net diff against main is exactly the three client files (README, client.py, test_client.py); no server or runtime change. CI green on node 22 / 22.23.2 / 24.
No functional issues. The delta resolves the prior review's non-blocking concern (stale line-number anchors) by construction.
Correction
The Python reference client's attestation guidance still described the pre-1571ef34a fullness rule. The implemented
listAttestations()contract is rows-remain: over-fetch 201, return at most 200, and emitnext_since_idonly while more rows remain. Exactly 200 is therefore complete withhas_more: falseand no cursor.Existing-PR cleanup revision
Verification of this revision