Skip to content

Runtime deltas - #4388

Open
Keavon wants to merge 5 commits into
masterfrom
runtime-deltas-2
Open

Runtime deltas#4388
Keavon wants to merge 5 commits into
masterfrom
runtime-deltas-2

Conversation

@Keavon

@Keavon Keavon commented Jul 28, 2026

Copy link
Copy Markdown
Member

No description provided.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 issues found across 10 files

Confidence score: 2/5

  • In editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, same-gesture replace/remove flows can delete a resource that the final graph still references because liveness is checked before the full batch projection; this can create broken references after edit application — evaluate resource liveness against the post-batch graph state before emitting removals.
  • SetInput/SetExport handling in editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs and nested-network export staging in document/graph-storage/src/from_runtime.rs can persist references without persisting the referenced ResourceId, leaving unresolved exports or missing assets when loading/runtime conversion runs — collect referenced resources from input/export edits and nested export slots and emit corresponding AddResource ops.
  • In editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, compound removals may emit duplicate structural removal operations because the deduplicated batch closure is computed but not used; this increases churn and can make downstream op processing brittle — emit removals from the deduplicated closure (or suppress already-covered removals).
  • editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta_tests.rs currently masks declaration-persistence regressions by discarding ConstructedOps::declaration_bytes, so a key failure mode can slip through tests, while document/graph-storage/src/session.rs has a minor ergonomics mismatch in stage_computed_ops input typing — assert declaration bytes in parity tests and align the signature with IntoIterator to reduce call-site friction.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="document/graph-storage/src/session.rs">

<violation number="1" location="document/graph-storage/src/session.rs:134">
P3: `stage_computed_ops` takes `Vec<RegistryDelta>` while `stage_ops` (the method it delegates to) accepts `impl IntoIterator<Item = RegistryDelta>`. Matching the more flexible signature would let callers pass arrays, slices, iterator adapters, or other collections without an intermediate allocation.</violation>
</file>

<file name="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs">

<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs:107">
P1: Resource-valued input/export edits can persist a reference without persisting the referenced resource. The SetInput and SetExport paths would benefit from collecting referenced `ResourceId`s and emitting `AddResource` before the value-changing op, as structural additions already do.</violation>

<violation number="2" location="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs:124">
P2: Compound removals can generate duplicate structural removal ops because the batch-level closure is calculated but ignored. Using the deduplicated batch closure for removal emission, or otherwise suppressing already-covered nodes, would avoid replaying removals for group descendants.</violation>

<violation number="3" location="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs:167">
P2: Adding a nested network can leave resources referenced only by its export slots out of storage. Extending this collection to scan `scratch.networks` export targets would keep structural additions consistent with whole-document conversion.</violation>

<violation number="4" location="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs:319">
P1: Replacing or removing nodes in the same gesture can delete a resource that the resulting graph still references. Resource liveness should be evaluated against the post-batch projection, including structural additions and SetInput/SetExport changes, rather than only the pre-batch working registry.</violation>
</file>

<file name="document/graph-storage/src/from_runtime.rs">

<violation number="1" location="document/graph-storage/src/from_runtime.rs:684">
P2: Adding a nested-network node can leave a resource-valued network export unresolved because the scoped conversion exposes the export but the staging path only discovers resources from node inputs. Including resource references from the converted network export slots (or exposing an equivalent scoped helper) would keep structural additions self-contained.</violation>
</file>

<file name="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta_tests.rs">

<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta_tests.rs:33">
P3: The structural-addition parity test can pass even when declaration extraction is broken because this helper discards `ConstructedOps::declaration_bytes`, which the staging path must persist separately from registry ops. Retaining the full `ConstructedOps` in the helper and asserting the declaration-byte result would cover this required side effect.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

ops.push(RegistryDelta::ChangeNodeInput {
id: resolver.node_id(network_path, *node_id),
index: (*input_index).try_into().map_err(|_| ConversionError::IndexOverflow(*input_index))?,
new_input: resolver.convert_input_at(input, network_path)?,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Resource-valued input/export edits can persist a reference without persisting the referenced resource. The SetInput and SetExport paths would benefit from collecting referenced ResourceIds and emitting AddResource before the value-changing op, as structural additions already do.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, line 107:

<comment>Resource-valued input/export edits can persist a reference without persisting the referenced resource. The SetInput and SetExport paths would benefit from collecting referenced `ResourceId`s and emitting `AddResource` before the value-changing op, as structural additions already do.</comment>

<file context>
@@ -0,0 +1,452 @@
+				ops.push(RegistryDelta::ChangeNodeInput {
+					id: resolver.node_id(network_path, *node_id),
+					index: (*input_index).try_into().map_err(|_| ConversionError::IndexOverflow(*input_index))?,
+					new_input: resolver.convert_input_at(input, network_path)?,
+				});
+			}
</file context>

candidates.dedup();

for candidate in candidates {
let still_referenced = working

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Replacing or removing nodes in the same gesture can delete a resource that the resulting graph still references. Resource liveness should be evaluated against the post-batch projection, including structural additions and SetInput/SetExport changes, rather than only the pre-batch working registry.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, line 319:

<comment>Replacing or removing nodes in the same gesture can delete a resource that the resulting graph still references. Resource liveness should be evaluated against the post-batch projection, including structural additions and SetInput/SetExport changes, rather than only the pre-batch working registry.</comment>

<file context>
@@ -0,0 +1,452 @@
+	candidates.dedup();
+
+	for candidate in candidates {
+		let still_referenced = working
+			.node_instances
+			.iter()
</file context>

ops.push(RegistryDelta::AddResource { id: *id, entry: entry.clone() });
}
}
let mut tagged: Vec<ResourceId> = scratch.node_instances.values().flat_map(node_value_resource_refs).collect();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Adding a nested network can leave resources referenced only by its export slots out of storage. Extending this collection to scan scratch.networks export targets would keep structural additions consistent with whole-document conversion.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, line 167:

<comment>Adding a nested network can leave resources referenced only by its export slots out of storage. Extending this collection to scan `scratch.networks` export targets would keep structural additions consistent with whole-document conversion.</comment>

<file context>
@@ -0,0 +1,452 @@
+			ops.push(RegistryDelta::AddResource { id: *id, entry: entry.clone() });
+		}
+	}
+	let mut tagged: Vec<ResourceId> = scratch.node_instances.values().flat_map(node_value_resource_refs).collect();
+	tagged.sort();
+	tagged.dedup();
</file context>

}
}

let _ = batch_removed_nodes;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Compound removals can generate duplicate structural removal ops because the batch-level closure is calculated but ignored. Using the deduplicated batch closure for removal emission, or otherwise suppressing already-covered nodes, would avoid replaying removals for group descendants.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs, line 124:

<comment>Compound removals can generate duplicate structural removal ops because the batch-level closure is calculated but ignored. Using the deduplicated batch closure for removal emission, or otherwise suppressing already-covered nodes, would avoid replaying removals for group descendants.</comment>

<file context>
@@ -0,0 +1,452 @@
+			}
+		}
+
+		let _ = batch_removed_nodes;
+		Ok(())
+	}
</file context>

/// Converts the node with `local_id` in the network at `local_path` into `registry` under its
/// stable IDs. `recurse` also converts the contents of any nested network the node implements;
/// without it only the node itself is converted, with its nested network referenced by ID.
pub fn convert_node_at(&mut self, registry: &mut Registry, local_path: &[RuntimeNodeId], local_id: RuntimeNodeId, doc_node: &DocumentNode, recurse: bool) -> Result<(), ConversionError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Adding a nested-network node can leave a resource-valued network export unresolved because the scoped conversion exposes the export but the staging path only discovers resources from node inputs. Including resource references from the converted network export slots (or exposing an equivalent scoped helper) would keep structural additions self-contained.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/graph-storage/src/from_runtime.rs, line 684:

<comment>Adding a nested-network node can leave a resource-valued network export unresolved because the scoped conversion exposes the export but the staging path only discovers resources from node inputs. Including resource references from the converted network export slots (or exposing an equivalent scoped helper) would keep structural additions self-contained.</comment>

<file context>
@@ -564,10 +601,158 @@ fn convert_implementation<M: NodeMetadataSource + ?Sized>(
+	/// Converts the node with `local_id` in the network at `local_path` into `registry` under its
+	/// stable IDs. `recurse` also converts the contents of any nested network the node implements;
+	/// without it only the node itself is converted, with its nested network referenced by ID.
+	pub fn convert_node_at(&mut self, registry: &mut Registry, local_path: &[RuntimeNodeId], local_id: RuntimeNodeId, doc_node: &DocumentNode, recurse: bool) -> Result<(), ConversionError> {
+		let owner_path = self.resolver.owner_path(local_path);
+		let location = NodeLocation {
</file context>


/// Stages ops computed outside the session (an incremental runtime projection) as hot ops, the
/// same way `stage_from_runtime` stages a diff's ops.
pub fn stage_computed_ops(&mut self, ops: Vec<RegistryDelta>) -> Result<Vec<HotOp>, CrdtError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: stage_computed_ops takes Vec<RegistryDelta> while stage_ops (the method it delegates to) accepts impl IntoIterator<Item = RegistryDelta>. Matching the more flexible signature would let callers pass arrays, slices, iterator adapters, or other collections without an intermediate allocation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/graph-storage/src/session.rs, line 134:

<comment>`stage_computed_ops` takes `Vec<RegistryDelta>` while `stage_ops` (the method it delegates to) accepts `impl IntoIterator<Item = RegistryDelta>`. Matching the more flexible signature would let callers pass arrays, slices, iterator adapters, or other collections without an intermediate allocation.</comment>

<file context>
@@ -129,6 +129,12 @@ impl Session {
 
+	/// Stages ops computed outside the session (an incremental runtime projection) as hot ops, the
+	/// same way `stage_from_runtime` stages a diff's ops.
+	pub fn stage_computed_ops(&mut self, ops: Vec<RegistryDelta>) -> Result<Vec<HotOp>, CrdtError> {
+		self.stage_ops(ops)
+	}
</file context>

fn construct(editor: &EditorTestUtils, deltas: &[EditorDelta], working: &Registry) -> Vec<RegistryDelta> {
construct_batch(deltas, working, &editor.active_document().resources.registry, PEER)
.expect("construction should succeed")
.ops

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The structural-addition parity test can pass even when declaration extraction is broken because this helper discards ConstructedOps::declaration_bytes, which the staging path must persist separately from registry ops. Retaining the full ConstructedOps in the helper and asserting the declaration-byte result would cover this required side effect.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta_tests.rs, line 33:

<comment>The structural-addition parity test can pass even when declaration extraction is broken because this helper discards `ConstructedOps::declaration_bytes`, which the staging path must persist separately from registry ops. Retaining the full `ConstructedOps` in the helper and asserting the declaration-byte result would cover this required side effect.</comment>

<file context>
@@ -0,0 +1,241 @@
+fn construct(editor: &EditorTestUtils, deltas: &[EditorDelta], working: &Registry) -> Vec<RegistryDelta> {
+	construct_batch(deltas, working, &editor.active_document().resources.registry, PEER)
+		.expect("construction should succeed")
+		.ops
+}
+
</file context>

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