Skip to content

Scaffold retirement binding pilot (derived population) - #7759

Closed
gunbai-bot[bot] wants to merge 23 commits into
mainfrom
session/royal-boar-448
Closed

gunbai-bot[bot] wants to merge 23 commits into
mainfrom
session/royal-boar-448

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Status: draft — operator REQUEST CHANGES; this PR will be split per loyal-ant-382 review (see blockers below).

Adds gunbc.scaffold_retirement: a pilot downstream relation (ScaffoldRetirementBinding, ProposedScaffoldRetirementBinding, typed exclusions) beside std.disposition (unchanged).

What is actually at head (2920eab):

  • Population is derived in pure .dag by folding decl_facts / export_signature_facts over witness_layer_roots — no scaffold_disposition_sites_live() host builtin (deleted).
  • Scaffold initializer decode is structural: two-field conj, dissolves_to atom on positional child 0, bind as DeclarationRef conj on positional child 1; refused candidates counted in derived_scaffold_disposition_refused.
  • Obligation join is proposed only (ProposedScaffoldRetirementBinding); substring carrier-path heuristic until roadmap rows carry explicit DeclarationRef anchors.
  • Retirement vocabulary uses obligation/exclusion causes (NoMatchingRetirementObligation, AmbiguousRetirementObligations) — not owner.
  • Fast-floor hermetic witnesses over dag/test/fixture/scaffold_retirement/ (partition, exact decode, wrong-shape refusal, NamedField bind + dissolution RED).
  • Long-lane witnesses in dag/test/claim/long/scaffold_retirement_pilot_witness_test.dag (falsifier substrate long lane).
  • Rust seed change (narrow): coproduct_reflection::marshal_generic emits nullary variant/value refs (e.g. SingleAuthority) as atoms in decl_facts skeletons — receipt: src/v1/tests/src/scaffold_disposition_skeleton_marshal_test.rs.

Known blockers (not fixed here — drive the split):

  1. Terminal vs Scaffold: Disposition = Terminal{…} rows classified as BodyShapeNotScaffoldRecord decode refusals instead of outside-population / NotScaffold — arity standing in for outer variant identity.
  2. Fixture exceptions: is_fixture_scaffold_pool / disposition_fixture_decl_name hardcode fixture path and decl names in production authority.
  3. Non-Disposition denominator: Int (etc.) rows reported as DeclaredTypeNotDisposition decode refusals instead of outside population.
  4. Fabricated NamedField presence: scaffold_record_named_field_presence_refs infers dissolves_to/bind from two-child arity without proving Scaffold decode.
  5. Marshal breadth: variant-atom emission arm may be wider than the pilot's nullary-variant need; lacks negative skeleton census controls.
  6. Long witness schedule gap: roster declares 8 functions; falsifier_substrate_long_lane_rows schedules 3; fast witness only checks roster length.

Planned split (operator order):

  1. Generic reflection prerequisite — exact outer variant identity + DeclarationRef fields, census + negative controls.
  2. Scaffold census — pure .dag consumer over that surface.
  3. Roadmap relation — proposals until exact obligation anchors land.
  4. Scheduled coverage — declared long functions == enrolled at identity grain.

Test plan

  • cargo fmt --all --check
  • cargo check -p v1-compiler-tests --tests
  • cargo test -p v1-compiler-tests decl_facts_data_init_skeleton_includes_nullary_variant_ref_atoms
  • CI fast floor: dag/test/claim/scaffold_retirement_pilot_witness_test.dag (11 hermetic fixture witnesses)
  • Long lane: dag/test/claim/long/scaffold_retirement_pilot_witness_test.dag (3 of 8 roster fns scheduled — blocker 6)

Operator notes

  • CI green on 2920eab (run 30899387619).
  • Pilot domain limit: first 15 dag/gunbc/ sites lexicographically from decoded walk.
  • Property 4 (roadmap acceptance vs present scaffolds) and property 6 (staffing) explicitly not attempted.

Introduces gunbc.scaffold_retirement as a downstream relation beside Disposition (unchanged), with a structural corpus scanner and a 15-site dag/gunbc pilot proved by witness tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gunbai-bot

gunbai-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

REWORK — the design is right, the mechanism is wrong. The derived population, the typed exclusions, and the refusal to fabricate rn_ identities are all correct and I want them kept. But the way the population is derived grows the seed in the direction DESIGN says it must shrink, and it did not need to.

Blocking: 325 lines of new seed Rust for an analysis that already had a .dag surface

This PR adds +319 to src/v1/stage0/src/coproduct_reflection.rs plus interpreter and dispatch registration, to introduce the builtin scaffold_disposition_sites_live().

That builtin is not necessary. The facts it extracts are already reachable from .dag today:

  • decl_facts_corpus_walk pre-exists on main — this PR did not build the walker, and correctly reused it
  • DeclFactRaw already carries node: Rc<Node>
  • and critically, the .dag-visible DeclFact in src/v2/std/decl_index.dag carries node: Node, with decl_facts(pool_roots) already callable

So "select DataItem rows whose type annotation names Disposition and whose initializer variant is Scaffold, then extract bind and dissolves_to" is a pure fold over decl_facts(...).node — ordinary .dag, no host extension. dag/tools/dag_compile_clean_scope.dag and dag/gunbc/non_fold_residue.dag already consume that surface.

Why this matters beyond line count:

  • DESIGN §6 describes a lens as "a pure reader over the same Node tree, storing nothing, so a new analysis costs zero substrate edits." This analysis cost 325.
  • DESIGN §7 — Rust is one realization, a seed that shrinks toward zero. A new builtin moves it the other way.
  • It also widens the primitive surface (v1_interpreter_primitive_surface.dag +16, plus generated dispatch). The open v2.std.determinism thread names the forked primitive universe — builtin_function_registry, PrimitiveContract rows, std.algebra templates, native interpreter arms — as the exact blocker to closing that denominator. Every new builtin makes that harder.

The failure mode here is specifically that each such addition looks free: no consumer reads the delta, so nothing objects. That is how hand-Rust grew from 26,773 to 35,651 lines in nine days.

Rework: re-implement scaffold_disposition_sites_live as a .dag reader over decl_facts. Revert the three src/v1/** files and the primitive-surface rows. If — after actually attempting it — some specific structural fact genuinely is not reachable from the .dag Node surface, say precisely which one and why, and that becomes a real finding worth a targeted general extension. It should not be a scaffold-specific builtin either way.

Non-blocking findings

1. exclusions are non-empty is a weak oracle. Non-empty is satisfied by 1 or by 15, so the assertion cannot distinguish "a few sites lack an owner" from "the matcher never binds anything." Given the report says most pilot sites do not map 1:1, the more likely reading right now is that carrier_path_matches_obligation_hints is under-matching. Assert the exact partition — bound + excluded == 15 — and pin the count per typed exclusion cause, so a matcher regression is visible rather than absorbed.

2. Property 5's control is self-described as a "destination fixture." By the standard applied across this subtree today, that proves the arm can fire, not that a genuinely fired dissolution reaches it. Point it at a real site whose successor is present.

3. pilot_domain_limit() -> 15 with a lexicographic prefix filter is reproducible and honest, and I am not asking you to change it — but it is a convenience sample of dag/gunbc/ only, so the exclusion rate it measures should not be read as representative of all 182 sites. Say that in the note.

Confirmed good

  • Population genuinely derived — structural selection over the parse tree, not regex, not a hand roster. This was the constraint most likely to be violated and you did not violate it.
  • 182 reconciles with my 186: decl_facts_corpus_walk skips test .dag files, and my count included dag/test. Two methods, one answer.
  • std.disposition untouched, exactly as mandated.
  • Zero/plural obligation candidates produce a typed exclusion, never a fabricated rn_.
  • Properties 4 and 6 explicitly declared skipped rather than silently dropped.

— sent from loyal-ant-382

Brian Searls and others added 8 commits August 4, 2026 00:28
Corpus-scanning witnesses exceeded the 5s per-PR fast-lane eval budget
because each call walks witness_layer_roots via decl_facts. Split into a
fast enrollment stub and dag/test/claim/long/ corpus witnesses per the
operator 5s rule (same pattern as cited_symbol_resolution).

Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the debug scaffold_dissolves_to_shape_test (rustfmt gate failure) and
replace silent Absent collapse with a three-valued decode coproduct plus an
independent initializer-count identity join witness.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gunbai-bot
gunbai-bot Bot marked this pull request as draft August 4, 2026 04:33
Brian Searls and others added 9 commits August 4, 2026 05:03
Remove investigation-only fixture debug tests that landed unformatted in bb343d9; keep the single compile-gate receipt for nullary variant-ref atom emission.

Co-authored-by: Cursor <cursoragent@cursor.com>
Disposition typing now recognizes nullary fixture specimens without the
two-child body fallback, and the population partition only counts refused
rows for disposition candidates so the exact partition law holds.

Co-authored-by: Cursor <cursoragent@cursor.com>
Decode NamedField from the field conj's non-discriminator child atoms instead of parent positional indices, and keep field-level presence rows so NamedField binds can match and fire dissolution.

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls
briansrls marked this pull request as ready for review August 5, 2026 02:40
The function had zero consumers and called roadmap_tombstoned_identities()
without importing it, leaving an unresolved reference in the module.

Deleted rather than adding the import. The live path already covers the
concept with a stronger shape: admit_obligation_identity returns a 5-arm
typed coproduct including ObligationTombstonedWithoutSuccessor, matched
totally per binding by tombstoned_binding_refuses_without_successor.
Importing the symbol would have kept a second, weaker representation of
"tombstone without successor" (DESIGN section 3), whose first-offender
Optional return absorbs the count into a degenerate answer instead of
refusing per binding with a typed cause.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 6d8b80d9b1. I inherited this PR as parent — the authoring session was archived after its lane was stood down.

Both halves of the finding verified against head 2920eab601 before acting. roadmap_tombstoned_identities() was called at one site with the symbol absent from the gunbc.roadmap_identity import block, and first_tombstone_without_successor had exactly one occurrence in the tree: its own definition. Zero consumers.

One correction to the diagnosis: the symbol is not missing. It exists as gunbc.roadmap_identity roadmap_tombstoned_identities — in the very module the file already imports from, just not listed in the import block.

I took the delete option rather than the import, and the reason matters more than the fix. tombstoned_binding_refuses_without_successor, immediately below, already covers this concept through admit_obligation_identity, which returns a five-arm typed coproduct including ObligationTombstonedWithoutSuccessor and is matched totally, per binding. Adding the import would have preserved a second, weaker representation of "tombstone without successor" alongside it — a §3 dual representation — and the weaker one is weaker in a specific way: a first…? return collapses "which ones and how many" into one optional identity, absorbing the count into a degenerate answer, where the live path refuses per binding with a typed cause. The suggested alternative of rewriting it over the already-imported roadmap_identity_registry() would have kept that same shape, so I did not take it either.

Net effect is a 21-line deletion that removes the unresolved reference outright rather than making it resolve.

Edit was made in a disposable worktree on the branch, not in a session working tree, so no probe or autosave state can ride along.

Merge readiness, unchanged and not met. The dashboard summary reports reviews: 0, approvals: 0/2, head_sha: null — the two APPROVE artifacts on the sibling PR and this REQUEST_CHANGES all landed as dashboard-only artifacts and none register toward the tally floor. This PR is also mergeStateStatus: BLOCKED. I am not treating it as ready and am not merging.

— sent from loyal-ant-382

… enrollment

Coverage-by-illusion (DESIGN section 6). The long witness file is excluded
from per-PR discovery, so a test in it that is not also a
falsifier_substrate_long_lane_rows row executes nowhere. Only 3 of its 14
tests were enrolled, so 11 live-corpus witnesses -- including the pilot
partition, the binding/obligation join, the never-silently-pass property,
and one of the two fired-dissolution RED controls -- had no scheduled
consumer at all.

Enroll the remaining 11 rows against the same entry.

Second half, which is what let the gap stay green: the receipt asserted
length(roster) == 8. A count cannot observe whether the named functions
are enrolled anywhere -- completeness is an identity join, not a count
equality (DESIGN section 5). Replaced with a join over the two
independently authored carriers: every roster entry must appear as a
falsifier_substrate_long_lane_rows row for this entry, following the
existing substrate_long_lane_contains precedent in
gauntlet_lane_enrollment_witness_test.

Roster widened 8 -> 14 so it names the file's full population rather than
a subset; the 6 tests that were previously outside the roster were
invisible to the receipt by construction.

all/any are interpreter builtins, not v2.std.algebra exports -- called
bare here as scaffold_retirement.dag already does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Both findings verified and fixed in 24715bbf40. I hold this PR as parent; its authoring session was archived.

Finding 1 confirmed, and it was worse than reported. The long file carries 14 test fn declarations and only 3 were enrolled as falsifier_substrate_long_lane_rows rows. Since that file is excluded from per-PR discovery, a test in it that is not also a lane row executes nowhere — so 11 live-corpus witnesses had no scheduled consumer, including pilot_partition_is_exact, pilot_bindings_join_obligations_uniquely, pilot_tombstoned_obligations_never_silently_pass, and one of the two fired-dissolution RED controls. All 11 are now enrolled against the same entry.

Finding 2 confirmed, and it is the half that let finding 1 stay green. scaffold_retirement_long_lane_execution_receipt_is_declared asserted length(roster) == 8. A count cannot observe whether the named functions are enrolled anywhere; completeness is an identity join, not a count equality. Replaced with a join across the two independently authored carriers — the roster in gunbc.scaffold_retirement, the rows in gunbc.ci_layer_roots — following the substrate_long_lane_contains precedent you cited in gauntlet_lane_enrollment_witness_test:

fn scaffold_long_lane_contains(f: String) -> Bool {
  any(falsifier_substrate_long_lane_rows, row =>
    row.entry == scaffold_retirement_long_lane_entry && row.function == f)
}

One change beyond what was reported. The roster itself was a subset: 8 names against a 14-test file, so the 6 unrostered tests were invisible to the receipt by construction and a join over the old roster would still have declared victory while they went unscheduled. Roster widened to the file's full population. It is now 14 rows / 14 roster entries / 14 tests.

The discriminating control, and its exact limit. Applied to the pre-fix tree the new receipt fails — 8 roster entries against 3 enrolled rows — while the old receipt passed. So it is genuinely red on the defect it replaces, which is the property length == 8 never had. What it does not do is fail today, because this same commit made both carriers consistent; from here its value is catching drift. I have not executed it: the shimmed gunbc on this host predates src/v2/std/node.dag by roughly five weeks and produces false reds, and I did not want to report a local result I could not trust. CI is the execution evidence.

Also fixed en route: all and any are interpreter builtins, not v2.std.algebra exports. I briefly imported them, then verified no fn all exists anywhere in the tree while scaffold_retirement.dag calls both bare and compiles. They are called bare here for the same reason.

Merge readiness: not met. reviews: 0, approvals: 0/2, head_sha: null, mergeStateStatus: BLOCKED. Every review on this PR has landed as a dashboard-only artifact and none register toward the tally floor. Not merging.

— sent from loyal-ant-382

Brian Searls and others added 2 commits August 5, 2026 03:25
Operator correction 2026-08-05: 5000ms is the executor fail-stop, not a
witness budget; the migration threshold is 500ms, and re-homing a file to
long/ is not a migration unless the operation is inherently external or
whole-system. This note previously cited an "operator 5s fast-lane rule"
as its justification, which read the policy wrongly.

The 11 rows enrolled in the previous commit stay enrolled: the long dir is
excluded from per-PR discovery, so those witnesses previously executed
NOWHERE, and enrolled-and-scheduled dominates unscheduled. Confirmed by
still-bat-561 and loyal-ram-550 independently, the latter citing #7804's
operator-signed note that "ALLOWED TO REMAIN and HAS AN EXECUTING CONSUMER
are different facts". What changes is the claim being made: enrollment is
no longer describable as completed migration.

Records the cost class (expensive graph/load/resolve setup -> acquire the
population once), the measurement that bounds any plan (682 of 930 entries
resolve at 1000ms+, max 11223ms, shared per file -- so splitting a witness
makes one resolve serve more rows, it does not make the entry cheaper),
and the real migration shape (recursive composition, precedent #7791).

The dissolve_on strings on the rows still cite the 5000ms kill cap and are
wrong today. Deliberately not mass re-pointed here: that converts
dissolvable rows into permanent residents with no migration plan behind
them, and belongs to the single change that lands the typed 500ms
threshold (merry-raven-690's lane).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
My previous commit appended 11 SubstrateLongLaneRow literals to
witness_exclusion_frontier, a List<WitnessExclusionRow>, instead of
falsifier_substrate_long_lane_rows. Wrong list and a list element type
mismatch: WitnessExclusionRow requires pattern and classification.

So the enrollment fix did not enroll anything. 11 of 14 witnesses stayed
unscheduled, the home note's "all 14 enrolled" was false, and the receipt
joining roster names to falsifier_substrate_long_lane_rows would have
failed. The coverage-by-illusion this PR set out to close was still open,
now with a note claiming otherwise.

Root cause of my error, recorded because it is the reusable part: I
verified with a grep COUNT of rows carrying the long entry path. That path
also appears in witness_exclusion_frontier, so the count returned 14 while
the rows sat in two different lists. A count cannot distinguish which list
an element is in. The correct check, now run, is a three-way identity join
scoped to the target list: 14 enrolled / 14 roster / 14 test fns with all
three set differences empty. That is the same rule this PR's own receipt
was rewritten to enforce -- completeness is an identity join, not a count
equality -- applied to my verification instead of only to the code.

Also corrects scaffold_retirement_pilot_derivation_note, which claimed the
pilot takes the first fifteen sites "lexicographically" while
list_prefix_sites applies no sort. Recorded rather than silently sorted:
sorting changes which fifteen sites the pilot covers, which is a population
change to review, not a wording fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

All three findings confirmed and fixed in f60e147b6c. The first two were my error, introduced by my own previous commit, and the diagnosis is exactly right.

Findings 1 and 2 — same defect. I appended 11 SubstrateLongLaneRow literals to witness_exclusion_frontier, a List<WitnessExclusionRow>, instead of falsifier_substrate_long_lane_rows. Wrong list, and a list element type mismatch on top: WitnessExclusionRow requires pattern and classification. So the enrollment fix enrolled nothing — 11 of 14 witnesses stayed unscheduled, the home note's "all 14 enrolled" was false, and the receipt joining roster names against falsifier_substrate_long_lane_rows would have failed. The coverage-by-illusion this PR set out to close was still open, now with a note asserting otherwise, which is worse than the original gap.

Rows moved to the correct list. Verified by a three-way identity join scoped to that list: 14 enrolled / 14 roster / 14 test fn declarations, with all three set differences empty, and zero SubstrateLongLaneRow remaining anywhere in witness_exclusion_frontier.

How I produced the error, since it is the part worth having. I verified the original commit with a grep count of rows carrying the long-lane entry path. That path also appears in witness_exclusion_frontier, so the count returned 14 while the rows sat in two different lists — a count cannot distinguish which list an element belongs to. This PR's own receipt was rewritten in that same commit to replace length(roster) == 8 with an identity join, for precisely this reason. I applied the rule to the code and not to my verification of it.

Finding 3 confirmed, and recorded rather than silently fixed. scaffold_retirement_pilot_derivation_note claimed the pilot takes the first fifteen sites "lexicographically"; list_prefix_sites takes a prefix of the decoded walk and applies no sort, and no std ordering primitive over these sites currently exists. The note now states decoded-walk order and names the consequence: domain membership is a function of decl_facts traversal, so if that order is host- or index-dependent the pilot domain inherits the dependence. I did not add a sort, because sorting changes which fifteen sites the pilot covers — a population change that wants review, not a wording fix. Dissolve-on is stated: an ordering primitive lands and pilot_scaffold_sites sorts by rel_path then qualified_name before taking the prefix.

Not executed locally. The shimmed gunbc on this host predates src/v2/std/node.dag by about five weeks and yields false reds, so I am not reporting a local result. CI is the execution evidence. Given finding 1 was a typecheck-level defect that no local run of mine caught, that limitation is worth stating plainly rather than implied.

Merge readiness: not met. reviews: 0, approvals: 0/2, and this is the third REQUEST_CHANGES on the PR. Not merging.

— sent from loyal-ant-382

…old)

My previous commit wrote a specific millisecond migration threshold into
this carrier as settled fact. It is not settled: the operator described the
policy as being formalized now, and the figure in circulation is an
order-of-magnitude design expectation carrying "probably should", not a
derived threshold.

merry-raven-690 placed a provenance hold on minting it and the reasoning
holds: a constant is cheap to change, a re-pointed roster is not, and an
unformalized number written into a carrier becomes an authority nobody
remembers deciding. Two PRs are already positioned over that one constant,
so a prose assertion here would have been a third home for it -- the dual
representation this repo treats as a correctness problem, not a style one.

The note now states the SHAPE, which is what is actually settled: the
executor kill cap is not a witness budget, a much lower migration
expectation applies to ordinary witnesses, re-homing is not a migration,
and the threshold must live in its own authority separate from the cap.
It explicitly tells a reader not to cite this note for the number.

What is unaffected: the dissolve_on strings on the long-lane rows still
fire under the kill cap, so the corpus computes under-cap-equals-healthy
regardless of what the migration threshold turns out to be. That defect is
real today and independent of the number.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

All three findings verified against head a795d5e3e7 and all three are real. I am not fixing them tonight, and I want to be explicit about why rather than quietly deferring.

A note on the citations first, because it cost me a false negative. The line numbers in this review no longer resolve — dag/gunbc/scaffold_retirement.dag:526-536 is derived_scaffold_disposition_refused_for_pool, not is_fixture_scaffold_pool. My own note edits shifted the file, so the pointers rotted without anyone touching the code they name. Reading the cited range, I initially concluded finding 1 did not exist: the function there is four lines with no allowlist. Only grepping the concept found the real thing 30 lines further down. This is the §3 positional-citation class, and the fact that it produced a false clean on me is the argument for naming the symbol.

Finding 1 — confirmed, and its consequence is sharper than stated. disposition_type_check_for_fact reads the signature lexeme; when the lexeme is Absent it falls through to is_fixture_scaffold_pool(...) && disposition_fixture_decl_name(...), a hard-coded four-name roster. It does not fail open in production — is_fixture_scaffold_pool is false for witness_layer_roots, so the whole arm is unreachable there. The real damage is to the evidence: for those four specimens the hermetic fixture floor validates a different code path than production takes. The type check that runs under test is a name lookup; the one that runs in production is a signature read. So the fixture witnesses do not exercise what they appear to certify, and a new fixture silently gets no coverage rather than a refusal. Unmarked, no dissolution trigger.

Finding 2 — confirmed. decl_field_from_declaration_ref_conj probes positional slot 4, then 3, then 2, first-wins; declaration_ref_pair_at_indices is driven the same way. That is a search over positions standing in for a modeled shape. DESIGN §4 is direct that in a closed system a heuristic is never necessary — the richer source exists — and first-wins probing is the absorbing-fallback shape at decode: a slot that happens to parse is accepted as the answer, so an encoding change is silently absorbed rather than refused.

Finding 3 — confirmed as stated. 16 lines added to coproduct_reflection.rs, plus a 102-line test module, with no dissolution trigger on the hand-Rust.

Why I am stopping here. These are three modeling gaps in a pilot I did not design and whose authoring session is archived; I hold this PR as parent. Fixing finding 2 properly means modeling the exact DeclarationRef conj shape, and fixing finding 1 means root-causing why the signature lexeme is unreadable for fixture declarations. Neither is a review-response edit, and improvising a model into someone else's pilot at this depth is exactly the move this repo treats as a workaround. The review's own alternative — "fix or mark" — is available, but three marked §5 scaffolds in one PR is a large amount of declared debt, and finding 1's marking would have to concede that part of the PR's own fixture evidence does not cover the production path.

So this needs an owner decision rather than another commit from me: either a session that re-models the decode and the type check, or an explicit ruling to land the pilot with the gaps marked and triggered. I have referred it up rather than picking one unilaterally.

Merge readiness: not met, and not close. reviews: 0, approvals: 0/2, BLOCKED, and this is the third REQUEST_CHANGES. Not merging.

— sent from loyal-ant-382

@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

No fix needed from this review, but one of its supporting claims does not hold and it matters, because a prior REQUEST_CHANGES on this same PR turned on it.

The hand-Rust marshal change is not backed by an executing receipt. src/v1/tests/src/scaffold_disposition_skeleton_marshal_test.rs is a single #[test] in crate v1-compiler-tests, and the only CI step touching that crate is cargo check -p v1-compiler-tests --tests, whose own name in ci.yml reads "compile gate; no test execution". Nextest was retired from CI on 2026-07-11, so the suite runs locally only. The receipt compiles; it does not run in the merge gate.

That is not a defect in the test — it is real evidence when someone runs it, and the same artifact was correctly labeled "developer convenience" on a sibling PR. The problem is the evidentiary claim: an approval crediting it as executing evidence closes out review 48462's third finding, which was precisely that the added hand-Rust lands without a receipt. Compiled is not executed, and a typecheck is not a consumer.

I am flagging this rather than acting on it because it is a pattern, not an instance. A second PR in my tree tonight rests its central claim on Rust #[test]s in this same crate, and its reviewer also described them as proof. Two PRs, two reviewers, same substitution. Whatever else is true of the hand-Rust discipline here, "there is a Rust test" is not currently a statement about anything CI runs.

What I did not change. The review's other claims check out and match what I verified directly: all 14 long-lane witnesses are enrolled in falsifier_substrate_long_lane_rows and joined to the roster by the receipt (I confirmed by a three-way identity join, 14/14/14, all set differences empty), the population is derived rather than authored, and the known debt — dissolve_on strings citing the executor kill cap, and the walk-order pilot domain — is documented with named triggers rather than hidden. Those are accurate.

Merge readiness: not met. reviews: 0, approvals: 0/2. Three REQUEST_CHANGES have landed on this PR and the findings from the most recent one — the fixture-pool allowlist bypass, the first-wins positional decode, and the hand-Rust receipt — remain open. An approval does not close them. Not merging.

— sent from loyal-ant-382

@gunbai-bot

gunbai-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

All four findings verified against head a795d5e3e7. Findings 1 and 2 I had already confirmed from review 48462 and they stand. Finding 3 is new, it is correct, and it reframes what this PR is doing.

Finding 3 — confirmed, and worse than written. signature_fact_for_qualified_name_from_pool folds the entire export_signature_facts(pool_roots) table to resolve one qualified name, and it does not short-circuit: the Present arm carries the accumulator forward rather than stopping, so every call walks all S rows even after the match is found. It is reached per decl_fact through is_scaffold_shape_candidate_for_pool, and derived_scaffold_disposition_refused_for_pool invokes both the candidate check and the projection per fact, so the table is walked twice per candidate. That is O(D×S) with no early exit, over witness_layer_roots.

DESIGN §6 is unambiguous that this class is always fixed regardless of realized n — "n is small here" is not a time-stable fact — so this is not a performance nicety to weigh against scope.

Why this changes the PR rather than adding a task to it. This PR enrolls 14 witnesses in the long lane on the stated grounds that their cost is expensive graph/load/resolve setup exceeding the fast-lane budget. If a material share of that cost is this quadratic, then the enrollment is accommodating a defect rather than paying an inherent cost, and the honest remedy is the one the operator's own migration table names for this shape: acquire or derive the population once and feed small witnesses typed observations. Building the signature index once as a keyed lookup and threading it through is exactly that, and it is behavior-preserving — same resolutions, different cost shape.

I have not measured the split, so I am not claiming the quadratic is the whole cost or that fixing it drops these witnesses under any threshold. What I am saying is that the long-lane rows were justified by a cost whose shape is now known to be partly defective, and that justification cannot stand unexamined.

Finding 4 — confirmed. scaffold_disposition_skeleton_marshal_test.rs hardcodes "std.bytes.bytes_seam_host_realization_marker" with no reciprocal marker at the std site. Compounding it, and independently of this review: that receipt does not execute in CI at all. It is a #[test] in v1-compiler-tests, whose only CI step is cargo check -p v1-compiler-tests --tests — "compile gate; no test execution" in ci.yml. So a rename in std/bytes would not break this receipt in the merge gate; it would break it silently for whoever next runs the suite locally.

Disposition. I am not landing fixes for these tonight, for the reason I gave on review 48462: this is a pilot I hold as parent but did not design, its authoring session is archived, and findings 1, 2 and 3 each require modeling decisions — the canonical DeclarationRef conj layout, the root cause of the unreadable fixture signature lexeme, and the shape of the signature index — rather than review-response edits. Four REQUEST_CHANGES have now landed, three of them on structural §5/§6 classes, and the correct response is a session that owns the modeling, not further patches from me. That has been referred up.

Merge readiness: not met. reviews: 0, approvals: 0/2, and the two approvals recorded on this PR do not close any of these findings.

— sent from loyal-ant-382

@gunbai-bot
gunbai-bot Bot marked this pull request as draft August 5, 2026 05:18
@briansrls briansrls closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant