Feature/model registry and metadata schema#3978
Feature/model registry and metadata schema#3978Chetansahney wants to merge 5 commits intoGraphiteEditor:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ai-models library for managing AI model manifests and registries, and implements "marching ants" animations for selection tools by integrating animation time into the overlay system. Additionally, it extends the vector stroke system to support gradient paints in both SVG and Vello renderers. Feedback includes correcting a dimension-handling bug in the tensor shape logic, aligning the new library's Rust edition with the workspace, and improving consistency and logic within the stroke style builder and visibility checks.
I am having trouble creating individual review comments. Click here to see my feedback.
libraries/ai-models/src/manifest.rs (19-25)
The batched function appears to have a bug. It replaces the first dimension of the input shape with None (dynamic), effectively losing that dimension's size. For example, batched([3, 1024, 1024]) for an image would result in a shape of (None, 1024, 1024), losing the channel information (3).
The expected behavior for adding a batch dimension is typically to prepend a dynamic dimension, so the resulting shape would be (None, 3, 1024, 1024). This is critical for correctly defining model input shapes. The corresponding test tensor_shape_batched_first_dim_is_none should also be updated to reflect the corrected behavior.
pub fn batched(dims: impl IntoIterator<Item = usize>) -> Self {
let mut shape = vec![None];
shape.extend(dims.into_iter().map(Some));
Self(shape)
}libraries/ai-models/Cargo.toml (4-5)
The Rust edition and version are set to future/unstable values. The 2024 edition is not yet stable, and Rust version 1.88 has not been released. This can cause compatibility issues and prevent the project from building on stable Rust toolchains.
It's recommended to align this with the rest of the workspace, which likely uses the 2021 edition and a lower MSRV (Minimum Supported Rust Version). Please update these to match the workspace configuration (e.g., edition = "2021" and a stable rust-version like 1.75).
rust-version = "1.75"
edition = "2021"
node-graph/libraries/vector-types/src/vector/style.rs (433-439)
The signature of with_color is inconsistent with other builder methods on Stroke like the newly added with_gradient and existing with_weight. It returns Option<Self> instead of Self, and takes &Option<Color> as a parameter instead of Option<Color>.
For consistency and better ergonomics with method chaining, this should be updated to match the pattern of the other methods.
pub fn with_color(mut self, color: Option<Color>) -> Self {
self.color = color;
if self.color.is_some() {
self.gradient = None;
}
self
}node-graph/libraries/vector-types/src/vector/style.rs (493-502)
The logic in has_renderable_stroke does not correctly reflect the rendering priority. The SVG and Vello renderers prioritize gradients over solid colors. If a Stroke has a transparent gradient and an opaque solid color, this function will return true, but the renderers will use the transparent gradient and render nothing.
The logic should follow the same priority as the renderers: check the gradient first, and only if it's not present, check the solid color.
pub fn has_renderable_stroke(&self) -> bool {
if self.weight <= 0. {
return false;
}
if let Some(gradient) = &self.gradient {
gradient.stops.color.iter().any(|color| color.a() != 0.)
} else if let Some(color) = self.color {
color.a() != 0.
} else {
false
}
}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 16 files
Confidence score: 3/5
- There is some merge risk due to concrete rendering behavior regressions in
node-graph/libraries/vector-types/src/vector/style.rs:Stroke::lerpreportedly handles gradient→solid transitions asymmetrically, which can make strokes briefly disappear, andhas_renderable_strokemay mis-handle gradient-vs-color precedence. libraries/ai-models/src/registry.rsandlibraries/ai-models/src/manifest.rsinclude medium-severity contract/invariant mismatches (download progress range enforcement, SPDX/serde encoding mismatch, andTensorShape::batchedaccepting empty input) that could surface as interoperability or state-consistency bugs.- This is not a clear merge blocker, but the number of medium/high-confidence findings (mostly 6–7/10 with 8–10/10 confidence) suggests validating behavior before merge; the PR title-format issue appears process-related and should not heavily affect runtime risk.
- Pay close attention to
node-graph/libraries/vector-types/src/vector/style.rs,libraries/ai-models/src/registry.rs, andlibraries/ai-models/src/manifest.rs- rendering transitions/precedence and model manifest/registry invariants are the main user-impacting risk areas.
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="libraries/ai-models/src/registry.rs">
<violation number="1" location="libraries/ai-models/src/registry.rs:1">
P2: Custom agent: **PR title enforcement**
PR title is not in imperative leading-verb form and uses a prefix-like `Feature/` format, violating the PR title convention.</violation>
<violation number="2" location="libraries/ai-models/src/registry.rs:164">
P2: `set_status` stores `ModelStatus::Downloading` progress without enforcing the documented `[0.0, 1.0]` invariant.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/vector/style.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/style.rs:366">
P1: `Stroke::lerp` handles gradient transitions asymmetrically, causing gradient->solid transitions to lose both paint fields and briefly disappear.</violation>
<violation number="2" location="node-graph/libraries/vector-types/src/vector/style.rs:523">
P2: `has_renderable_stroke` violates stroke paint precedence by treating color and gradient as additive even though gradient overrides color.</violation>
</file>
<file name="libraries/ai-models/src/manifest.rs">
<violation number="1" location="libraries/ai-models/src/manifest.rs:21">
P2: `TensorShape::batched` silently accepts empty input and returns an empty shape, contradicting its documented contract to set a first (batch) dimension.</violation>
<violation number="2" location="libraries/ai-models/src/manifest.rs:33">
P2: `License` serde encoding does not match the SPDX-style identifiers described by docs/Display, risking manifest JSON interoperability.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/select_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/select_tool.rs:1159">
P2: Animation-frame redraw subscription is started unconditionally for drawing, causing unnecessary per-frame overlay redraws in non-animated selection modes like `Touched`.</violation>
</file>
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1142">
P2: Stroke gradient support duplicates the fill gradient-construction pipeline instead of reusing a shared helper, increasing long-term drift/regression risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| pub fn lerp(&self, other: &Self, time: f64) -> Self { | ||
| Self { | ||
| color: self.color.map(|color| color.lerp(&other.color.unwrap_or(color), time as f32)), | ||
| gradient: match (&self.gradient, &other.gradient) { |
There was a problem hiding this comment.
P1: Stroke::lerp handles gradient transitions asymmetrically, causing gradient->solid transitions to lose both paint fields and briefly disappear.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/style.rs, line 366:
<comment>`Stroke::lerp` handles gradient transitions asymmetrically, causing gradient->solid transitions to lose both paint fields and briefly disappear.</comment>
<file context>
@@ -359,6 +363,12 @@ impl Stroke {
pub fn lerp(&self, other: &Self, time: f64) -> Self {
Self {
color: self.color.map(|color| color.lerp(&other.color.unwrap_or(color), time as f32)),
+ gradient: match (&self.gradient, &other.gradient) {
+ (Some(a), Some(b)) => Some(a.lerp(b, time)),
+ (Some(a), None) if time < 0.5 => Some(a.clone()),
</file context>
| .get_mut(model_id) | ||
| .map(|e| { | ||
| log::debug!("Model '{}' status → {status}", model_id); | ||
| e.status = status; |
There was a problem hiding this comment.
P2: set_status stores ModelStatus::Downloading progress without enforcing the documented [0.0, 1.0] invariant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/ai-models/src/registry.rs, line 164:
<comment>`set_status` stores `ModelStatus::Downloading` progress without enforcing the documented `[0.0, 1.0]` invariant.</comment>
<file context>
@@ -0,0 +1,310 @@
+ .get_mut(model_id)
+ .map(|e| {
+ log::debug!("Model '{}' status → {status}", model_id);
+ e.status = status;
+ })
+ .ok_or_else(|| RegistryError::NotFound(model_id.to_string()))
</file context>
| let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.); | ||
| let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.)); | ||
|
|
||
| has_color_alpha || has_gradient_alpha |
There was a problem hiding this comment.
P2: has_renderable_stroke violates stroke paint precedence by treating color and gradient as additive even though gradient overrides color.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/style.rs, line 523:
<comment>`has_renderable_stroke` violates stroke paint precedence by treating color and gradient as additive even though gradient overrides color.</comment>
<file context>
@@ -488,7 +513,14 @@ impl Stroke {
+ let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.);
+ let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.));
+
+ has_color_alpha || has_gradient_alpha
}
}
</file context>
| has_color_alpha || has_gradient_alpha | |
| if self.gradient.is_some() { has_gradient_alpha } else { has_color_alpha } |
| /// (batch) and the rest as fixed. | ||
| pub fn batched(dims: impl IntoIterator<Item = usize>) -> Self { | ||
| let mut shape: Vec<Option<usize>> = dims.into_iter().map(Some).collect(); | ||
| if let Some(first) = shape.first_mut() { |
There was a problem hiding this comment.
P2: TensorShape::batched silently accepts empty input and returns an empty shape, contradicting its documented contract to set a first (batch) dimension.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/ai-models/src/manifest.rs, line 21:
<comment>`TensorShape::batched` silently accepts empty input and returns an empty shape, contradicting its documented contract to set a first (batch) dimension.</comment>
<file context>
@@ -0,0 +1,178 @@
+ /// (batch) and the rest as fixed.
+ pub fn batched(dims: impl IntoIterator<Item = usize>) -> Self {
+ let mut shape: Vec<Option<usize>> = dims.into_iter().map(Some).collect();
+ if let Some(first) = shape.first_mut() {
+ *first = None;
+ }
</file context>
| /// Only the three variants listed in [`License::is_permissive`] are considered | ||
| /// compatible with Graphite's licensing standards. | ||
| #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "kebab-case")] |
There was a problem hiding this comment.
P2: License serde encoding does not match the SPDX-style identifiers described by docs/Display, risking manifest JSON interoperability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/ai-models/src/manifest.rs, line 33:
<comment>`License` serde encoding does not match the SPDX-style identifiers described by docs/Display, risking manifest JSON interoperability.</comment>
<file context>
@@ -0,0 +1,178 @@
+/// Only the three variants listed in [`License::is_permissive`] are considered
+/// compatible with Graphite's licensing standards.
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+pub enum License {
+ /// MIT Licence
</file context>
| } else { | ||
| let selection_shape = if input.keyboard.key(lasso_select) { SelectionShapeType::Lasso } else { SelectionShapeType::Box }; | ||
| // Subscribe to animation frames so the marching ants selection border animates continuously. | ||
| tool_data.start_marching_ants(responses); |
There was a problem hiding this comment.
P2: Animation-frame redraw subscription is started unconditionally for drawing, causing unnecessary per-frame overlay redraws in non-animated selection modes like Touched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/select_tool.rs, line 1159:
<comment>Animation-frame redraw subscription is started unconditionally for drawing, causing unnecessary per-frame overlay redraws in non-animated selection modes like `Touched`.</comment>
<file context>
@@ -1125,6 +1155,8 @@ impl Fsm for SelectToolFsmState {
} else {
let selection_shape = if input.keyboard.key(lasso_select) { SelectionShapeType::Lasso } else { SelectionShapeType::Box };
+ // Subscribe to animation frames so the marching ants selection border animates continuously.
+ tool_data.start_marching_ants(responses);
SelectToolFsmState::Drawing { selection_shape, has_drawn: false }
}
</file context>
| tool_data.start_marching_ants(responses); | |
| if !matches!(tool_action_data.preferences.get_selection_mode(), SelectionMode::Touched) { | |
| tool_data.start_marching_ants(responses); | |
| } |
| scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path); | ||
| if kurbo_stroke.width > 0. { | ||
| let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() { | ||
| let mut stops = peniko::ColorStops::new(); |
There was a problem hiding this comment.
P2: Stroke gradient support duplicates the fill gradient-construction pipeline instead of reusing a shared helper, increasing long-term drift/regression risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1142:
<comment>Stroke gradient support duplicates the fill gradient-construction pipeline instead of reusing a shared helper, increasing long-term drift/regression risk.</comment>
<file context>
@@ -1115,34 +1115,90 @@ impl Render for Table<Vector> {
- scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path);
+ if kurbo_stroke.width > 0. {
+ let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() {
+ let mut stops = peniko::ColorStops::new();
+ for (position, color, _) in gradient.stops.interpolated_samples() {
+ stops.push(peniko::ColorStop {
</file context>
| @@ -0,0 +1,310 @@ | |||
| //! Model registry – the centralised service that tracks every model's lifecycle. | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title is not in imperative leading-verb form and uses a prefix-like Feature/ format, violating the PR title convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/ai-models/src/registry.rs:
<comment>PR title is not in imperative leading-verb form and uses a prefix-like `Feature/` format, violating the PR title convention.</comment>
<file context>
@@ -0,0 +1,310 @@
+//! Model registry – the centralised service that tracks every model's lifecycle.
+use std::collections::HashMap;
+
+use thiserror::Error;
+
+use crate::manifest::ModelManifest;
+
+/// The lifecycle state of a single model.
+#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]
</file context>
1 participant
#Feature
This PR introduces the ai-models library, a core architectural layer designed to manage AI/ML model metadata, licensing, and lifecycle states within Graphite. This crate serves as the "source of truth" for the node engine to identify, validate, and prepare for inference requests.
By implementing this as a pure-Rust, zero-dependency library, we establish the cross-platform schema required for both local Python bundles and browser-based WebGPU execution