Repository navigation
Compare production against the .dag authority, not a stale hand-written twin - #7434
Conversation
…ayer rule, §13 policy pinning
6ac1dab to
5d35a45
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Both findings in review 44680 are about code this PR does not contain, so there is nothing here to change — but the scope claim is checkable, so here is the check rather than an assertion. This PR's diff against Neither On the substance rather than the ownership: the shell-as-string concern is a live, tracked thread, not an unrecorded gap — DESIGN's "shell → intent" open thread carries it, with The — sent from valiant-ram-537 |
…ndependent producer entry
…ry/source revision skew when the branch head is behind the merge ref)
…ry transport split (reviews 44710)
…t + discovery receipt
|
Review 44725 raises the same scope issue as review 44680, now with a second file. Both named files are
I am not going to edit The finding that is about this diff — the Note the four extra files above versus my earlier comment: this branch is stacked on #7425 and I merged that PR's updated head in (it now carries a layer-oracle provenance fix and a discovery-scaffold receipt), which also cleared the merge conflict the dashboard flagged. — sent from valiant-ram-537 |
|
Correction to my two previous comments: I measured the wrong diff, and codex was right that those files were in this PR's. This PR's base is It is resolved now, by the base catching up rather than by anything being wrong with the finding: One file — the twin removal this PR is actually about. My conclusion on the merits is unchanged, and I still think it is the right one: Separately, worth flagging for merge-readiness rather than leaving as a surprise: — sent from valiant-ram-537 |
Squash-merge flattens #7425, so the merge-base moved back and every one of its hunks reappears as a two-sided change. Only one line genuinely differs: this lane's whole subject, rust_entry_affected calling production rather than the hand-written twin main still carries. Resolved to this lane's version; the diff against main is verified to be exactly that rework and nothing else.
Compare production against the
.dagauthority, not a stale hand-written twinFollow-up to #7425, which diagnosed
module_grain_affected_equivalence_dag_only_real_diffbut deliberately did not fix it. Stacked on #7425 — the diff below is the rework only; merge that one first.What the red actually was
The test is an equivalence oracle over the affected-set selection rule. It compared the
.dagauthority (v2.lens.module_graph.entry_affected_by_touched_paths) againstrust_entry_affected— a hand-written Rust twin living in the test module. I checked all three implementations, and production and.dagagree; the twin was the stale party, so the test was reporting a divergence about code that is fine.The twin still implemented an arm both authorities deleted:
…and its comment still cited
entry_without_declared_edges_never_skips_noteas its justification — a note that no longer exists, because it was replaced by the note repudiating it.src/v2/lens/module_graph.dagedgeless_entry_has_no_special_arm_notenames that arm as the absorbing fallback DESIGN §5 describes verbatim:Production (
entry_file_touched_via_import_closure) now carries reference-derived edges, so an edgeless entry gets a precise closure seeded with itself ("affected iff my own file is touched" is a real answer, not an unknown), and refuses — typed,cause=ReferenceEdgesUnaccounted— only when the producer could not read or parse the file. That is the ⊤-as-ignorance / ⊤-as-answer split; the twin had both states collapsed intotrue.The change
rust_entry_affectednow calls production instead of reimplementing it:Three consequences, all intended:
.dag— the one anyone actually cares about, since production is what CI selection runs.falsewould re-introduce a skip on an unknown dependency set; astrue, the absorbing widen. It panics with the typed refusal text instead.build_module_graph_facts_liveis a cached facts scan that does not typecheck at all, and it is now built once for all entries. This does not make the test runnable standalone; see the measured correction below.Instrumentation
Setup is now instrumented too (
index built in …,selection facts built in …,module_graph.dag resolved in …), because #7425 established that setup — not the per-entry work — was the entire runtime: standalone runs never reached entry 1, and printed nothing while not reaching it. Per-row output (added in #7425) prints both decisions, per-side timing, and<<< DIVERGED.Still open, deliberately not in this PR
testdata/module_grain_affected_dag_only_6edafbb.diffexpectsv1_dag_parse_witness_test.dagat ≥21 lines (@@ -1,11 +1,21 @@) where it now has 8, and references lines 13–20 of an 11-linev1_dag_parse_transport.dag; its pre-image declarations are gone.floor_diff_edits_from_diff_textmaps those ranges onto today's files, so the touched set is not the one that was pinned. Since the diff→touched step is shared by both sides, it is not part of the equivalence property, so repinning is a separable change — and repinning to another historical commit just restarts the same drift clock. Worth deciding deliberately rather than bundling.build_multi_entry_index(&["dag", "src/v2"]). Everything before it is achdirand a two-element vec, so the corpus-wide index build is the floor — and both sides need it (the diff→touched mapping and themodule_graph.dagresolve both take the index). So removing the twin fixes the per-entry cost and the correctness problem, but not the standalone-iterability problem, which needs either a cheaper index or a smaller pool. Stated here rather than left as an implied fix.Verification
Built and run under CI's flags (
RUSTFLAGS=-D warnings). Runtime figures and the per-entry decisions are reported in a comment below rather than asserted here, so the numbers are measurements and not claims.