Repository navigation
floor: one base-tree commit per run for every base-side reader (discharges #13344's row) - #13347
Conversation
…arge a_roster_edit_judged_against_the_base_tip_not_the_merge_base Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…inputs Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ntrol reaches the window Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…to be converted by whichever lands second Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at the REQUESTED exact head 6f50b747f451e837996e9dbb5289cfdce52c384c, against DESIGN.md §§3, 4b/4d, 5 and 6b. The migrated production reads are accepted; the remaining issues are the claimed assurance/discharge and the exact-head mechanism receipt. #13344 was not re-reviewed.
Accepted production repair
FreezeBaselineComparison::resolve_window resolves both endpoints as commits, preserves Direct comparisons, applies merge-base only on MergeBase, and refuses an unreadable endpoint or missing common ancestor. The frozen-deferral gate now shares this relation implementation rather than copying it.
run_required_floor creates one lazy FloorBaseTree. Its OnceCell retains the successful window or its refusal, and the migrated base-side consumers borrow that same value: interface planning, unimported-provider roster edit, cost-debt witness-source edit budgets, base test-declaration census, and the one-file reader through non-fold-residue planning. The latter two are real additional members of the defect class. Their existing .dag readers and refusal handling are retained. I found no remaining tip selection in those migrated paths.
The real-history test meaningfully distinguishes the departure point from the target tip and retains the genuine retirement-reversal control. Its local-only execution standing is explicitly stated; I am not requesting restoration of the whole Rust-unit-test CI lane here.
1. Deleting base() is not the claimed semantic construction wall
The CEILING/DISCHARGED receipt says the change leaves no consumer able to read the tip under a merge-base relation, and relies on that construction in place of executing regression coverage. But the old value is still exported as pub(crate) fn base_ref(&self) -> &str, and the actual read helpers still accept ordinary strings. ResolvedComparisonWindow also has public String fields; its type does not establish that resolve_window produced it.
A consumer can therefore restore the wrong selection without changing either the resolver or the read API:
let base = floor_diff_comparison_readout()?.base_ref().to_string();
let source = unimported_bare_provider_roster_source_at_base(source_roots, &base)?;The same substitution is possible at the cost-edit read. This is a source-derived counterexample to the claimed compiler wall, not an executed mutant or a claim that the current migrated call sites still do this. Deleting the old method does usefully force outstanding old callers, including #13332, to be edited; it does not make a tip read ill-typed. A diagnostics-only doc comment is not a restriction on consumption.
The new roster_edit_is_judged_at_the_merge_base_not_the_base_tip test calls resolve_window, then performs its own git show and judge_edit. It never exercises FloorBaseTree or the production gate's selection of the base. Reverting a production reader to the expression above would leave that test's path unchanged. It establishes the resolver/classifier composition, not the claimed confinement of readers.
Correct the assurance at its actual boundary. Either enforce the resolved-window requirement at the base-tree reader interface and discriminate rejection of a raw/unresolved base there, or record this as the source-level repair it currently is, with the remaining reader-enforcement/CI obligation still explicit rather than asserting the structural ceiling is discharged. For regression evidence of the wiring, reuse the real-history specimen through the production base-selection/read composition so a tip-read substitution at that boundary fails it. A new wrapper name, or a compile probe for the deleted spelling alone, is not that discriminator. This does not require a new CI system or another open-ended v1 program.
2. The requested tree still contains #13344's inaccurate mechanism sentence
At 6f50b747f4, the MECHANISM receipt still groups the unimported-provider gate and the cost-edit budget arm as reading the roster through unimported_bare_provider_roster_at_base. The second arm reads the affected WITNESS SOURCE through cost_debt_changed_witness_ceilings / floor_cost_debt_edit::cost_debt_changed_witness_ceilings_at_base. The historical #13332 admission is a third reader, of COST-ROSTER membership. Preserve that distinction in this row.
The PR has already advanced to 7628274e106bc248f3b62b029b260d10738edf47; comparison shows that later delta changes only two receipt lines. Do not recreate a correction already present there, but that later tree is not substituted for the requested review head.
Reviewed the complete four-file exact-commit diff, exact-head DESIGN, the resolver and lazy carrier, migrated call sites, and the complete new real-history test. I did not run the tests or mutations; local/remote results remain author-reported. The requested-head witnesses run 37261461059 is cancelled, not a passing exact-head CI receipt.
…build; real-history control drives FloorBaseTree and the gate's judgment; row names three inputs Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the REQUEST_CHANGES review in 63a76fe. 1. Reader interface (option a).
Your counterexample ( Two limits, both stated in the receipt (ceiling 3, not 4):
Real-history control through the production path.
The only supplied seam is the 2. Three inputs. The MECHANISM receipt now names (a) the roster (unimported gate), (b) the witness source (cost-edit arm) and (c) cost-roster membership (#13332's admission). The DISCHARGED receipt covers (a) and (b) only, and says (c) is not covered until #13332's |
… BaseTreeCommit, delete its per-site merge base; discharge covers input (c) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Head
No floor helper resolves its own merge base any more. The RED Local run: 24 tests pass (cost_debt, roster_edit, real base read, unimported roster, frozen_roster), and clippy |
briansrls
left a comment
There was a problem hiding this comment.
APPROVE at exact requested head 165da6683e4d7fc53413849635a2e17e36dc8361, against DESIGN.md §§3, 4b/4d, 5 and 6b. This resolves my change request 5409975133 on 6f50b747f4, including the integration required because #13332 landed first.
The reader-interface repair is substantive. comparison_window::BaseTreeCommit has a field private to its defining module; ResolvedComparisonWindow is not an open pair of strings. The resolver applies Direct versus MergeBase, resolves endpoints to commits, and preserves unreadable-endpoint/unrelated-history refusals without falling back to the tip. The roster source reader, cost-edit source reader and cost-roster archive extraction accept the resolved commit carrier. The roster judgment and base-file/declaration readers receive FloorBaseTree. My former raw base_ref().to_string() substitution therefore no longer fits those interfaces. This is a scoped interface guarantee, not a claim that arbitrary Git calls elsewhere cannot read another ref.
run_required_floor creates one lazy FloorBaseTree, retaining either the resolved window or its failure, and lends it through changed/enrolled-witness planning, the unimported-provider gate, interface planning, the base declaration census, non-fold-residue base reads and cost-edit budgeting. The frozen-deferral gate reuses the same relation resolver instead of retaining another Direct/MergeBase implementation.
The real-history roster control now drives FloorBaseTree::for_comparison and the production unimported_bare_provider_edit_refusals composition. It distinguishes the base tip's false retirement reversal from the merge-base result and keeps the true un-retirement refusal. The fixture supplies Git show in its temporary repository; the sibling end-to-end control still executes the actual .dag-backed roster reader with a resolved commit. This satisfies the bounded correction requested: the test no longer rebuilds only a resolver/classifier composition independent of the production judgment's selection.
#13332's integration is present, not merely promised. cost_debt_comparison_base_commit is deleted, cost_debt_admitted_identities takes the shared FloorBaseTree, and cost_debt_base_tree_extract requires BaseTreeCommit. The archive-backed isolated evaluation, head dependency-closure trigger, admission merge and semantic-verdict behavior are not replaced or weakened. The retained cost-row control now resolves a real diverged Git history through FloorBaseTree: merge-base membership admits nothing; reading the tip supplies the discriminating false admission. The archive/provider evaluation controls remain separate and retained. The head-only closure reader correctly remains a head reader.
The committed mechanism receipt now distinguishes all three inputs: unimported-provider roster, changed witness source for edit budgeting, and cost-roster membership for admission. Its discharge records the last integration and explicitly preserves the limits: direct Git/general namespace utilities remain outside the typed reader wall; the cost-edit arm has no separate behavioral discriminator; the raw-ref compile-fail probe was one-off; Rust unit tests remain off CI under the existing declared standing. Approval does not discharge those coverage limitations. The PR body's older '#13332 not landed' paragraph is superseded by its integration comment and the exact-head code/receipt, not the current integration state.
Verified exact-head workflow 37344676456: floor, generated, emit-build and aggregate witnesses all succeeded. The local Rust controls and E0308 probe are author-run evidence; I reviewed their source and composition but did not execute the tests or mutants. No CI-lane restoration, budget change or additional v1 program is requested. The requested head remained unchanged and mergeable before submission. Land through normal required landing-head checks.
…election test; receipt evidence updated
…pplied values, within the unit budget); merge of main incl #13452 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…the hand-built main merge (review 77008) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
review 77008: confirmed and fixed in |
briansrls
left a comment
There was a problem hiding this comment.
APPROVE / LAND at eb2ef6d6a9df7c678ad6c0f396f7ff57a042771c. No blocking findings. This re-review replaces the evidence assessment in my approval at 165da6683e; the deleted heavy tests are not credited as current coverage.
The production repair remains coherent. comparison_window::resolve applies Direct versus MergeBase once and constructs the private-field BaseTreeCommit. run_required_floor creates one FloorBaseTree, whose OnceCell retains the resolved window (or refusal) and supplies interface planning, roster admission/edit readers, historical declaration/file readers, and the changed-witness budget. The old cost-debt-specific merge-base helper is deleted rather than retained as a second authority. The two comparison modes remain distinct; unrelated history refuses under MergeBase rather than silently reading the tip.
The replacement RED exercises the relevant production selection, not just an equivalent test helper. cost_debt_admitted_identities calls cost_debt_admitted_at_base_tree; that function obtains base_tree.commit(), passes that typed commit to the reader, and folds the returned rows. The test drives that same function through a real divergent Git history. Only the expensive archive/corpus read and .dag fold are supplied: the fixture reader actually runs git show at the commit it receives. The unchanged branch admits nothing after main retires t.retired; the genuinely branch-added row admits exactly t.new. The second test independently checks MergeBase versus Direct and refusal for unrelated histories. Those distinctions agree with the reported tip-reading mutant; I did not independently rerun that mutant.
Required execution verified. The exact-head workflow enables both pull_request and merge_group, runs cargo test --release -p v1-compiler --lib, and its witnesses aggregate refuses a missing/non-success Rust-unit result for this same-repository PR. The retrieved unit log checks out bb6d19e58eed940e894ca2d28a9fc647ac4c58f3, explicitly merging this requested head into ed0e5afac4731319d6ad98145ae8fe6ea6293a6e. It records both required_floor_runner::floor_base_tree_tests tests as ok, including cost_debt_row_retired_at_the_tip_but_carried_at_the_merge_base_is_not_admitted. The branch-added positive control is the second assertion in that executed RED. Suite result: 1,085 passed, 0 failed, 54 ignored, 0 filtered out, 5.04s. Neither new test is ignored. The exact-head witnesses workflow is successful. This is observed PR-merge execution plus merge-group enrollment, not a claim that a later queue run has already happened.
Evidence boundary retained: this does not restore end-to-end execution of the deleted historical-roster judgment or witness-source budget controls, nor does it execute the production archive/.dag fold inside the new RED. For those reader paths I checked the current wiring and typed interfaces; I am not promoting signature evidence into runtime evidence. The narrowed control does restore execution of the central stale-base selection and its delivery into the admission reader, which is the defect this replacement is meant to discriminate. No request to re-add whole-corpus v1 tests or another test lane.
Small receipt correction, non-blocking: 401bd7249a to this head contains two .dag ledger-file changes, not only one evidence edit: the updated stale-base receipt and restoration of comparison_operands_never_judged_in_v2_infer.dag. There are no Rust or build-configuration changes in that comparison. Clippy remains an author-reported run on 401bd7249a, not a separately executed exact-head clippy run. Likewise, 0.05s is the reported targeted-run timing; the CI log establishes execution and the suite total, not an individual <100ms timing certificate. I did not build, run clippy, or time tests locally.
Discharges the recurring_failure_mode row filed in #13344 (
a_roster_edit_judged_against_the_base_tip_not_the_merge_base).Root.
FreezeBaselineComparison::resolve_window(v1_compiler.cli_run) is now the only place the relation is read for a tree: two-dot → the base ref, merge-base → the departure point, with typed refusals (FreezeBaselineUnobservable,FreezeBaselineUnrelatedHistory) instead of a fallback to a ref. The barebase()accessor is deleted;base_ref()remains for printing only.One resolution per floor run.
required_floor_runner::FloorBaseTreeresolves that window lazily, at most once per run (run_required_floorcreatesfloor_base_tree), and is lent to every base-tree reader:interface_consumer_planning(its own merge-base call removed)unimported_bare_provider_gate(read the tip before)floor_base_test_decl_census,floor_base_file_read→non_fold_residue_changed_row_subjects(both read the tip before too: same class, not named in the row)The frozen path-deferral gate uses
resolve_windowinstead of keeping its own copy of the arm. No per-site merge-base calls were added.#13332. Not landed. Its
cost_debt_admitted_identitiesreads.base(), so it won't compile against this PR whichever lands second, which is the intended loud failure. I've messaged crisp-ram-667 with the conversion: take&FloorBaseTree, read at.commit().Evidence.
pure_producer_share_tests::roster_edit_is_judged_at_the_merge_base_not_the_base_tipbuilds a real git history: the branch departs at P, then main moves a retirement the branch never touched.RosterRetirementChanged (Retired FileDeleted -> Retired ImportsFixed).RosterRetirementChanged (Retired ImportsFixed -> ActiveDebt).The new test and the existing
unimported_bare_provider_roster_edit_is_judged_across_framespass locally under a 24 GiB cgroup. In-process tests can't run on BuildBuddy (HostBudgetUnreadable; the existing sibling fails there the same way). Remote clippy-D warningsis clean. All 12frozen_roster_*controls pass remotely, including unrelated-history and the non-fast-forward push.Row. This PR carries the whole row: the MECHANISM receipt is verbatim from #13344's head 5153e3d, plus a DISCHARGED receipt naming both inputs (the roster in (a), the changed witness source in (b)). The receipt states the rung honestly: the Rust test runs on no CI path while
rust_unit_tests_off_the_merge_pathstands, and (b) is repaired by construction, with no discriminating control of its own. #13344 is to be closed as superseded.🤖 Generated with Claude Code