Conversation
evomimic
left a comment
There was a problem hiding this comment.
Review Findings
Merge Approval on Hold
I’m holding off on approval for sequencing reasons rather than because the overall architectural direction looks wrong.
This PR is aligned with the MAP Runtime migration in that it removes obsolete dance builders/dispatch paths for operations we no longer want modeled as dances. Reviewing it with that migration plan in mind, I’m not treating the remaining TS references to create_new_holon / stage_new_holon / with_properties as blockers for this PR, since those paths are being retired across adjacent work and are expected to be cleaned up by follow-on PRs.
The issue that still matters for merge-readiness is the stale failing test in holochain_receptor:
host_mutation_precheck_blocks_create_new_holon_before_builder_side_effectsis still written against the old dance-builder seam forcreate_new_holon- the business rule it is protecting is still valid: creating a new transient holon must be blocked when host mutation entry is disallowed, and that guard must happen before any side effects occur
- but after this PR,
ClientDanceBuilderno longer permits that retired request path, so the test now fails for the wrong reason
So the invariant still needs coverage, but the probe is obsolete. This feels like it should either wait for the new NewHolon command path from PR 423 and then be retargeted there, or be updated in some equivalent way before merge so we’re not knowingly landing a stale red test around transaction gating.
Correct Ripple Effects
I’d also flag that the branch leaves test/docs language out of sync with the reduced dance surface. For example, tests/sweetests/tests/dance_tests.rs and fixtures such as abandon_staged_changes_fixture.rs and stage_new_version_fixture.rs still describe these retired operations as dances. Even if the actual cleanup is intentionally phased, that terminology drift is worth noting because it makes the current surface area harder to understand while the migration is in flight.
Here's is a checklist of changes needed to correct the test language, comments, and log text messages.
I did a more exhaustive pass specifically for stale “dance” terminology around the operations this PR is retiring as dances (abandon_staged_changes, with_properties, remove_properties, add_related_holons, remove_related_holons, stage_new_holon, stage_new_from_clone, stage_new_version, and the legacy create/new_holon seam). I’m not flagging still-valid dance terminology for operations that remain dances.
Here’s the correction list I’d want tracked during this migration:
tests/sweetests/tests/dance_tests.rs:1still opens withMAP Dance Test Cases, which now overstates the dance surface. The suite is exercising a mix of remaining dances and direct local mutation/staging operations, so the file-level framing should be updated.tests/sweetests/tests/dance_tests.rs:71-90still describes the whole suite in terms ofDanceTestCase/DanceTestStepand sayscargo test -p dances --test dance_tests. If the harness type names are staying for now, the prose should at least acknowledge that several of these steps are no longer dance-backed.tests/sweetests/tests/fixture_cases/abandon_staged_changes_fixture.rs:20saysTests abandon_staged_changes dance...even though this operation is now meant to be understood as a direct staged-holon operation rather than a dance.tests/sweetests/tests/fixture_cases/abandon_staged_changes_fixture.rs:45saysthe abandon dance succeeds, and:55has commented text sayingAttempt add_related_holon dance; both should be rewritten in mutation/operation language.tests/sweetests/tests/fixture_cases/simple_add_remove_properties_fixture.rs:95-100still explains the shape in terms ofRequestBody for the Dance. That is exactly the obsolete framing the feature is trying to remove; this should be reframed as legacy DSL/request-shape baggage rather than dance semantics.tests/sweetests/tests/fixture_cases/stage_new_version_fixture.rs:16still saysTests stage_new_version dance, which is now stale terminology.host/ui/src/app/models/map.request.ts:2still saysTypeScript equivalent of Rust DanceRequest and RequestBody.host/ui/src/app/models/map.request.ts:30still labels the sectionDANCE REQUEST STRUCTURE.host/ui/src/app/models/map.request.ts:35still says the request namecovers dance dispatch table eg stage_new_holon.host/ui/src/app/models/map.request.ts:36still references the RustDanceTypenaming.host/ui/src/app/clients/holons.client.ts:65-69contains a dead commented block withDanceRequestObject,dance_name, anddance_typeforstage_new_holon; it should just be removed.host/crates/holochain_receptor/src/holochain_receptor.rs:206-247is the remaining substantive issue: the testhost_mutation_precheck_blocks_create_new_holon_before_builder_side_effectsis still asserting through the old dance-builder seam forcreate_new_holon. The invariant it protects is still valid, but onceNewHolonis available via the MAP Runtime/Commands path this test should be retargeted there instead of continuing to encode the retiring builder path.
I did not find stale “dance” wording in the direct-mutation executors for these retired operations. Those are already phrased in terms of mutations/APIs rather than dances, which is good.
No description provided.