Skip to content

feat: add node fallback #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leavesster wants to merge 1 commit into
base: main
Choose a base branch
from
+71 −0
Open

feat: add node fallback #362

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions manifest_reader/src/manifest/node/definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ use super::task::{TaskNode, TaskNodeBlock};
use super::value::ValueNode;
use serde::Deserialize;

#[derive(Deserialize, Debug, Clone)]
pub struct FallbackNode {
#[serde(default = "fallback_node_id")]
pub node_id: NodeId,
}
Comment on lines +12 to +16
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

修复：untagged 变体充当“吞掉一切”的兜底，导致任意 YAML 条目都会被反序列化为 Fallback

当前 FallbackNode 只有一个带默认值的字段，且未拒绝未知字段。配合 #[serde(untagged)]，它会匹配几乎任何映射（例如测试里的 comment: ...），从而静默吞掉拼写错误/未知节点，削弱配置校验，并改变现有变体的匹配优先级。必须要求出现显式的 fallback 键，且拒绝未知字段，避免误匹配。

建议修改如下：

-#[derive(Deserialize, Debug, Clone)]
-pub struct FallbackNode {
-    #[serde(default = "fallback_node_id")]
-    pub node_id: NodeId,
-}
+#[derive(Deserialize, Debug, Clone)]
+#[serde(deny_unknown_fields)]
+pub struct FallbackNode {
+    // 要求存在 `fallback` 键，以免该变体在 untagged 下成为“捕获所有”
+    pub fallback: serde::de::IgnoredAny,
+    #[serde(default = "fallback_node_id")]
+    pub node_id: NodeId,
+}

Also applies to: 18-20


fn fallback_node_id() -> NodeId {
NodeId::from("fallback".to_owned())
}

#[derive(Deserialize, Debug, Clone)]
#[serde(untagged)]
pub enum Node {
Expand All @@ -18,6 +28,7 @@ pub enum Node {
Service(ServiceNode),
Condition(ConditionNode),
Value(ValueNode),
FallBack(FallbackNode),
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

命名一致性：将枚举变体从 FallBack 重命名为 Fallback

现有命名与其它变体（Task/Subflow/Service/...）不一致。该变体是新公开 API，建议立即更正，避免之后的 API 震荡。

-    FallBack(FallbackNode),
+    Fallback(FallbackNode),
@@
-            Node::FallBack(fallback) => &fallback.node_id,
+            Node::Fallback(fallback) => &fallback.node_id,
@@
-            Node::FallBack(_fallback) => default_concurrency(),
+            Node::Fallback(_fallback) => default_concurrency(),
@@
-            Node::FallBack(_) => None,
+            Node::Fallback(_) => None,
@@
-            Node::FallBack(_) => true,
+            Node::Fallback(_) => true,
@@
-            Node::FallBack(_) => false,
+            Node::Fallback(_) => false,

Also applies to: 43-43, 55-55, 66-66, 77-77, 92-92

🤖 Prompt for AI Agents 
In manifest_reader/src/manifest/node/definition.rs around lines 31, 43, 55, 66,
77 and 92, the enum variant is named "FallBack" which is inconsistent with other
variants and public API; rename the variant to "Fallback" everywhere it appears
(declaration and all usages), update any pattern matches, constructors, and
documentation/comments to use the corrected spelling, and run cargo check/tests
to ensure no remaining references to the old "FallBack" identifier remain.

}

impl Node {
Expand All @@ -29,6 +40,7 @@ impl Node {
Node::Service(service) => &service.node_id,
Node::Condition(condition) => &condition.node_id,
Node::Value(value) => &value.node_id,
Node::FallBack(fallback) => &fallback.node_id,
}
}

Expand All @@ -40,6 +52,7 @@ impl Node {
Node::Service(service) => service.concurrency,
Node::Condition(condition) => condition.concurrency,
Node::Value(_value) => default_concurrency(),
Node::FallBack(_fallback) => default_concurrency(),
}
}
pub fn inputs_from(&self) -> Option<&Vec<NodeInputFrom>> {
Expand All @@ -50,6 +63,7 @@ impl Node {
Node::Service(service) => service.inputs_from.as_ref(),
Node::Condition(condition) => condition.inputs_from.as_ref(),
Node::Value(_) => None,
Node::FallBack(_) => None,
}
}
pub fn should_ignore(&self) -> bool {
Expand All @@ -60,6 +74,7 @@ impl Node {
Node::Service(service) => service.ignore,
Node::Condition(condition) => condition.ignore,
Node::Value(value) => value.ignore,
Node::FallBack(_) => true,
}
}

Expand All @@ -74,6 +89,62 @@ impl Node {
Node::Service(_) => false,
Node::Condition(_) => false,
Node::Value(_) => false,
Node::FallBack(_) => false,
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use serde_yaml;

#[test]
fn test_fallback_node() {
let yaml = r#"
a: c
"#;

let node: FallbackNode = serde_yaml::from_str(yaml).unwrap();
assert_eq!(node.node_id, NodeId::from("fallback".to_owned()));
}

Comment on lines +97 to +111
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

测试用例应要求显式的 fallback 键，避免误匹配

当前用 a: c 也能反序列化为 FallbackNode，恰好暴露了“吞掉一切”的问题。建议改成要求出现 fallback 键的形状。

-        let yaml = r#"
-        a: c
-        "#;
+        let yaml = r#"
+        fallback: {}
+        "#;
🤖 Prompt for AI Agents 
In manifest_reader/src/manifest/node/definition.rs around lines 97 to 111, the
FallbackNode test and current deserialization accept any mapping (e.g., `a: c`)
which masks a lax Deserialize implementation; change the deserialization to
require an explicit top-level `fallback` key (either by adjusting the struct to
have a `fallback` field or implementing custom Deserialize that errors when the
`fallback` key is missing), update the test YAML to include the `fallback:` key
with the appropriate value, and add/adjust tests to ensure deserialization fails
for mappings that do not contain `fallback`.

#[test]
fn test_nodes() {
let yaml = r#"
- task: example_task
node_id: example_node
inputs_from:
- handle: input_handle
value: null
concurrency: 5
ignore: false
- slot: example_slot
node_id: slot_node
- service: example_service
node_id: service_node
- condition:
handle: condition_handle
node_id: condition_node
- value:
key: example_key
value: example_value
node_id: value_node
- fallback:
node_id: fallback_node
- comment: This is a comment and should be ignored
"#;

let nodes: Vec<Node> = serde_yaml::from_str(yaml).unwrap();
assert_eq!(nodes.len(), 7);
assert_eq!(nodes[0].node_id(), &NodeId::from("example_node".to_owned()));
assert_eq!(nodes[1].node_id(), &NodeId::from("slot_node".to_owned()));
assert_eq!(nodes[2].node_id(), &NodeId::from("service_node".to_owned()));
assert_eq!(
nodes[3].node_id(),
&NodeId::from("condition_node".to_owned())
);
assert_eq!(nodes[4].node_id(), &NodeId::from("value_node".to_owned()));
assert_eq!(nodes[5].node_id(), &NodeId::from("fallback".to_owned()));
}
Comment on lines +112 to +149
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

YAML 形状不一致且隐藏配置错误：fallback 的 node_id 放在嵌套层里，且“comment”被吞掉

  • 其它节点的 node_id 在顶层；fallback 却把 node_id 放在嵌套的 fallback: 下，导致该值被丢弃，进而使用默认 fallback，与直觉不符。
  • “comment” 项本不应作为 Node 被接受；当前实现会被 Fallback 吞掉。建议去掉该条，或定义专门的 Comment 变体/预处理逻辑明确忽略。

建议统一形状并修复断言：

-        - fallback:
-            node_id: fallback_node
-        - comment: This is a comment and should be ignored
+        - fallback: {}
+          node_id: fallback_node
@@
-        assert_eq!(nodes.len(), 7);
+        assert_eq!(nodes.len(), 6);
@@
-        assert_eq!(nodes[5].node_id(), &NodeId::from("fallback".to_owned()));
+        assert_eq!(nodes[5].node_id(), &NodeId::from("fallback_node".to_owned()));
🤖 Prompt for AI Agents 
In manifest_reader/src/manifest/node/definition.rs around lines 112 to 149, the
test YAML mixes shapes: most nodes put node_id at top-level but the fallback
node nests node_id under "fallback", and a raw "comment" entry is being parsed
as a Node; update the test YAML so every node has node_id at the top level (move
the fallback's node_id out of the nested map) and remove the "comment" entry (or
replace it with an explicit comment line) so it is not parsed as a Node, then
adjust the expected nodes.len() and subsequent assertions to match the corrected
7-node structure and the correct node_id strings (e.g., fallback node_id should
be "fallback_node" if you rename it consistently).

}
Loading