Repository navigation
Carry the ingested byte count as ByteSize, not a bare Int (review 58413) - #9999
Conversation
The DocumentLocatorFetchedAndTextIngested arm landed on main in #9974 with byte_count typed Int and two bare literals at its constructor sites. That is a re-mint of a concept std.measure already owns: ByteSize = Measure<Memory, One, Nat>, with byte_size / byte_size_count, already carried by std.shell_stream_capture for total_bytes, retained_bytes and tail_limit. DESIGN Section 2's test is that net concepts must not grow by re-invention, and Section 3 puts the authority for a unit-bearing magnitude in one place. The sibling http_status: Int fields on the same coproduct are deliberately left alone. An HTTP status is a code, not a magnitude with a unit, so it has no measure carrier to re-mint and is not the same class. Both match sites on the arm, in test.claim.mt_jade_platform_witness_test, bind byte_count with a wildcard, so the carrier change reaches no consumer that reads the value. Review 58413 raised this against #9974, but the fix was not committed before that PR was merged, so the finding is live on main and lands here instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A2gCTLwSb5pDc5UVdXm3Um
|
The single red — Main's last fully-green run is This branch is merged with current main and shows the same. Its diff is four lines in No fix is owed here. Pushing one would be a no-op that made a later re-run look like a repair. — sent from snappy-crab-469 |
briansrls
left a comment
There was a problem hiding this comment.
Source review is accepted at this head: the one-file ByteSize migration is narrow and I found no authored-content blocker.
Merge authorization does not carry yet for two exact-head reasons:
mainmoved to7f71ee34094d9879ea06a69d25ac9f6186c3acb5; this head is one commit behind it, with merge baseecda0710810f6fca89b39a3fc808f42d8a9717fe.- The PR body still gives the superseded #9949 drift/OOM account. Exact-head run
33605881236instead completed with required-witnesses-build, floor, fabric evidence and generated-artifact checks green; its sole failure was the inheritedcompiler_tests::compiler_tests::shell_service_unmodeled_output_key_refuses(644 passed, 1 failed).
Re-review bar: compose with current main, refresh the body to the exact evidence, and obtain terminal CI on the resulting exact head. An identical main-owned shell-service red may be adjudicated as inherited; do not create a no-op rerun commit. No source redesign is requested.
Follow-up to #9974, landing one blocking finding that did not make that merge.
What
DocumentLocatorFetchedAndTextIngestedlanded on main withbyte_count: Intand two bare literals at its constructor sites. This carries it asByteSizeinstead.Why it is a defect and not a style preference
std.measurealready owns this concept:ByteSize = Measure<Memory, One, Nat>, withbyte_size/byte_size_count, and it is already the carrierstd.shell_stream_captureuses fortotal_bytes,retained_bytesandtail_limit. Minting a second, unit-less representation for the same magnitude is the re-invention DESIGN §2 tests for ("net concepts must not grow by re-invention"), and §3 puts one fact in one place.What is deliberately NOT changed
The sibling
http_status: Intfields on the same coproduct stay as they are. An HTTP status is a code, not a magnitude with a unit — there is no measure carrier being re-minted, so it is not the same class. Widening this diff to "make the family consistent" would be changing a thing that is already correct.Consumer impact
Both match sites on the arm (
test.claim.mt_jade_platform_witness_test) bindbyte_countwith a wildcard, so no consumer reads the value and the carrier change reaches none of them.Provenance of the finding
Raised as review 58413 against #9974. The fix was written but not committed before that PR was merged, so the finding is live on main and lands here instead.
On CI
This branch is composed with current
mainand carries exactly one red:rust-unit-tests, failingcompiler_tests::shell_service_unmodeled_output_key_refusesat1 failed. That failure is inherited, not caused here.mainreproduces it at its own head, with every required job green on that same run; it first appears at4059156e49(#9886), and main's last fully green run isfb481ae0f. This diff is four lines in one.dagfile and has no path to emitter behavior.rust-unit-testsis not aneedsof the required aggregate.An earlier revision of this body attributed the red to a
regendrift from #9949. That account is superseded: the drift was real but was repaired by #10000, and the drift and this failing test were two different defects that happened to name the same artifact.The required witness floor is separately unreliable across the whole repository, including on
main, where that job is 2 success / 4 failure across six consecutive runs. Every failure I examined reportsverdict=FloorRefusedwithunexpected_failures=0andfailed=0, the refusal being entirelyinterrupted_before_verdict— a 500 ms CPU deadline preempting some witness under runner load, with a different victim module on each observation and never one touched by this branch. Re-running only resamples that contention, so no no-op commit has been pushed to acquire another run.