feat: add node fallback #362
Open
+71
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary by CodeRabbit
Walkthrough在节点定义文件中新增 Fallback 节点类型，作为 Node 枚举的新变体 FallBack。更新相关行为以识别该变体，并新增针对 YAML 解析的单元测试，包括解析单个 Fallback 节点与包含 FallBack 的节点序列。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
Comment |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new FallbackNode type to the node system, extending the existing node enumeration with fallback functionality. The implementation provides a default node ID of "fallback" and integrates the new node type across all existing node operations.
- Introduces
FallbackNodestruct with default node ID functionality - Extends the
Nodeenum to include the newFallBackvariant - Updates all node methods to handle the fallback case with appropriate default behaviors
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifest_reader/src/manifest/node/definition.rs (1)
22-32: 在 manifest_meta/src/node/definition.rs 中补齐 Fallback 变体及匹配分支manifest_meta 的 Node 枚举缺少 FallBack(FallbackNode) 变体及相应的匹配分支（如 node_id、concurrency、inputs_from、ignore 等），请同步添加以保持与 manifest_reader 一致。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
manifest_reader/src/manifest/node/definition.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
manifest_reader/src/manifest/node/definition.rs (2)
manifest_reader/src/manifest/node/common.rs (1)
default_concurrency(23-25)manifest_meta/src/node/definition.rs (1)
node_id(51-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: rust-test
- GitHub Check: rust-test
🔇 Additional comments (1)
manifest_reader/src/manifest/node/definition.rs (1)
47-57: 行为定义 LGTM
- concurrency: Fallback 默认并发=1，合理。
- should_ignore: Fallback 恒为 true，符合“只用于分支控制，不参与执行”的预期。
- should_spawn: Fallback 恒为 false，合理。
Also applies to: 69-79, 81-94
Comment on lines
+12
to
+16
| #[derive(Deserialize, Debug, Clone)] | ||
| pub struct FallbackNode { | ||
| #[serde(default = "fallback_node_id")] | ||
| pub node_id: NodeId, | ||
| } |
There was a problem hiding this comment.
修复：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
| Service(ServiceNode), | ||
| Condition(ConditionNode), | ||
| Value(ValueNode), | ||
| FallBack(FallbackNode), |
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+97
to
+111
| #[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())); | ||
| } | ||
|
|
There was a problem hiding this comment.
测试用例应要求显式的 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`.
Comment on lines
+112
to
+149
| #[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())); | ||
| } |
There was a problem hiding this comment.
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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.