DO NOT MERGE!: feat(huntsman): Task executor prototyping.#301
Open
LinZhihao-723 wants to merge 8 commits intoy-scope:mainfrom
Open
DO NOT MERGE!: feat(huntsman): Task executor prototyping.#301LinZhihao-723 wants to merge 8 commits intoy-scope:mainfrom
LinZhihao-723 wants to merge 8 commits intoy-scope:mainfrom
Conversation
Contributor
WalkthroughA comprehensive Task Definition Language (TDL) system is introduced, enabling Rust-based task definitions with automatic serialization, C-FFI boundaries for dynamic loading, wire-format marshalling, procedural macro support, and runtime task execution via a package loader and handler registry. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Executor as Task<br/>Executor
participant Loader as TdlPackage<br/>Loader
participant DynLib as Dynamic<br/>Library
participant Registry as Task<br/>Registry
Client->>Executor: load(shared_lib_path)
Executor->>DynLib: dlopen(path)
DynLib-->>Executor: library_handle
Executor->>DynLib: __spider_tdl_package_get_name()
DynLib-->>Executor: package_name
Executor->>Loader: insert(package_name, library)
Loader-->>Executor: &str reference
Client->>Executor: execute_task(pkg_name, task_name, ctx, inputs)
Executor->>Loader: get(package_name)
Loader-->>Executor: &TdlPackage
Executor->>DynLib: __spider_tdl_package_execute(task_name, ctx, inputs)
DynLib->>Registry: lookup(task_name)
Registry->>Registry: TaskHandlerImpl::execute_raw(ctx, inputs)
Registry->>Registry: deserialize TaskContext + params
Registry->>Registry: call user Task::execute()
Registry->>Registry: serialize result/error
Registry-->>Registry: ExecutionResult
Registry-->>DynLib: TaskExecutionResult
DynLib-->>Executor: result_bytes
Executor->>Executor: decode ExecutionResult
Executor-->>Client: Ok(output_bytes) or Err(TdlError)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
components/spider-tdl/src/error.rs (1)
51-55: Strengthen round-trip tests to assert variant identity, not just display text.At Line 54, string comparison can pass even if the decoded variant is wrong but renders the same message.
Proposed test-strengthening diff
-#[derive(Debug, thiserror::Error, serde::Serialize, serde::Deserialize)] +#[derive(Debug, PartialEq, Eq, thiserror::Error, serde::Serialize, serde::Deserialize)] pub enum TdlError { @@ - let original_display = original.to_string(); let encoded = rmp_serde::to_vec(&original)?; let decoded: TdlError = rmp_serde::from_slice(&encoded)?; - assert_eq!(decoded.to_string(), original_display); + assert_eq!(decoded, original); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/spider-tdl/src/error.rs` around lines 51 - 55, The current round-trip test only compares decoded.to_string() with original.to_string(), which can hide variant mismatches; change the assertion to verify the decoded variant equals the original variant (e.g., use assert_eq!(decoded, original) if TdlError implements PartialEq, or match on the TdlError enum variants to compare discriminants/fields), updating the test that constructs original, encodes with rmp_serde::to_vec, decodes to TdlError, and then assert variant identity for decoded vs original instead of comparing display text.components/spider-tdl-derive/src/task_macro.rs (1)
180-209: Consider documenting the intentional permissiveness ofTaskContextvalidation.The validation checks only the last segment of the type path (line 201), meaning it would accept
foo::TaskContext,spider_tdl::TaskContext, or any other path ending inTaskContext. While this appears intentional for flexibility, a brief doc comment explaining this design choice would help future maintainers understand it's not an oversight.📝 Suggested documentation addition
+/// Validates that the first parameter's type ends with `TaskContext`. +/// +/// This check is intentionally permissive: it matches any type path whose final segment is +/// `TaskContext` (e.g., `TaskContext`, `spider_tdl::TaskContext`, `crate::TaskContext`). +/// This allows flexibility in how users import the type. fn validate_first_param_is_task_context(param: &FnArg) -> syn::Result<()> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/spider-tdl-derive/src/task_macro.rs` around lines 180 - 209, Add a doc comment to validate_first_param_is_task_context explaining that the function intentionally only checks the last segment of the type path (the last_segment.ident == "TaskContext" check) so it accepts TaskContext with or without module prefixes (e.g., foo::TaskContext or spider_tdl::TaskContext) for flexibility; update the doc above the function to state this design decision and why full path validation was avoided.claude/task-exec-prototyping/design-doc.md (1)
33-42: Consider adding language specifiers to fenced code blocks.Several fenced code blocks in this design document lack language specifiers (e.g., lines 33, 67, 373, 408, 486, 684, 854). For ASCII diagrams and directory structures, using
textorplaintextwould satisfy linters and improve rendering consistency.📝 Example fix for the dependency graph block
-``` +```text spider-tdl-derive (proc-macro: syn, quote, proc-macro2) │ ▼ spider-tdl (rmp-serde, serde, thiserror; re-exports spider-tdl-derive)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude/task-exec-prototyping/design-doc.md` around lines 33 - 42, The fenced ASCII diagrams (e.g., the dependency graph containing symbols like spider-tdl-derive, spider-tdl, spider-executor, and example-tdl-package) lack language specifiers; update those code fences (including the block shown and the other similar blocks referenced) by adding a language label such as "text" or "plaintext" after the opening ``` so linters and renderers treat them as plain text (e.g., change ``` to ```text for each ASCII/diagram/directory structure block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/spider-tdl/src/register.rs`:
- Around line 37-44: The registry currently uses HashMap::insert which silently
overwrites handlers when two tasks share the same <$task as $crate::Task>::NAME;
change the insertion to a fail-fast check using the map.entry(...) API (or an
explicit contains_key check) and panic (or return Err) if the name already
exists, emitting a clear message that includes the duplicate name (<$task as
$crate::Task>::NAME) and the task type (e.g. stringify!($task)) so duplicates
are detected during registry construction; update the macro block that creates
the Box::new($crate::TaskHandlerImpl::<$task>::new()) as the value to use this
guarded insertion instead of plain insert.
In `@taskfiles/test.yaml`:
- Line 217: Replace the hardcoded ".so" suffix for SPIDER_TDL_PACKAGE_COMPLEX
with an OS-aware template or variable so macOS uses ".dylib" and Linux uses
".so"; for example compute a SPIDER_LIB_SUFFIX earlier (or use a template
conditional like {{if eq .OS "darwin"}}.dylib{{else}}.so{{end}}) and then set
SPIDER_TDL_PACKAGE_COMPLEX to
"{{.G_RUST_BUILD_DIR}}/release/libexample_tdl_package_complex{{.SPIDER_LIB_SUFFIX}}"
(or use the direct conditional) so the correct dynamic library extension is
chosen at runtime.
In `@tests/tdl-integration/tests/complex.rs`:
- Around line 19-21: The current lib_path() returns Option and allows tests to
silently skip when SPIDER_TDL_PACKAGE_COMPLEX is unset; change lib_path() to
return a String and call
std::env::var("SPIDER_TDL_PACKAGE_COMPLEX").expect("SPIDER_TDL_PACKAGE_COMPLEX
must be set for integration tests") (or otherwise panic with a clear message) so
missing prerequisites hard-fail; update all call sites that currently do `let
Some(path) = lib_path() else { return Ok(()); };` to call lib_path() directly
(or let path = lib_path();) and remove the early-return pattern so the test
fails loudly when the env var is not provided.
---
Nitpick comments:
In `@claude/task-exec-prototyping/design-doc.md`:
- Around line 33-42: The fenced ASCII diagrams (e.g., the dependency graph
containing symbols like spider-tdl-derive, spider-tdl, spider-executor, and
example-tdl-package) lack language specifiers; update those code fences
(including the block shown and the other similar blocks referenced) by adding a
language label such as "text" or "plaintext" after the opening ``` so linters
and renderers treat them as plain text (e.g., change ``` to ```text for each
ASCII/diagram/directory structure block).
In `@components/spider-tdl-derive/src/task_macro.rs`:
- Around line 180-209: Add a doc comment to validate_first_param_is_task_context
explaining that the function intentionally only checks the last segment of the
type path (the last_segment.ident == "TaskContext" check) so it accepts
TaskContext with or without module prefixes (e.g., foo::TaskContext or
spider_tdl::TaskContext) for flexibility; update the doc above the function to
state this design decision and why full path validation was avoided.
In `@components/spider-tdl/src/error.rs`:
- Around line 51-55: The current round-trip test only compares
decoded.to_string() with original.to_string(), which can hide variant
mismatches; change the assertion to verify the decoded variant equals the
original variant (e.g., use assert_eq!(decoded, original) if TdlError implements
PartialEq, or match on the TdlError enum variants to compare
discriminants/fields), updating the test that constructs original, encodes with
rmp_serde::to_vec, decodes to TdlError, and then assert variant identity for
decoded vs original instead of comparing display text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aee06e81-462a-4447-b153-7195e10facee
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.tomlclaude/task-exec-prototyping/context.mdclaude/task-exec-prototyping/design-doc.mdclaude/task-exec-prototyping/struct-serde.mdcomponents/spider-task-executor/Cargo.tomlcomponents/spider-task-executor/src/error.rscomponents/spider-task-executor/src/lib.rscomponents/spider-task-executor/src/loader.rscomponents/spider-tdl-derive/Cargo.tomlcomponents/spider-tdl-derive/src/lib.rscomponents/spider-tdl-derive/src/task_macro.rscomponents/spider-tdl/Cargo.tomlcomponents/spider-tdl/src/error.rscomponents/spider-tdl/src/ffi.rscomponents/spider-tdl/src/lib.rscomponents/spider-tdl/src/register.rscomponents/spider-tdl/src/task.rscomponents/spider-tdl/src/task_context.rscomponents/spider-tdl/src/tdl_types.rscomponents/spider-tdl/src/wire.rsexamples/example-tdl-package-complex/Cargo.tomlexamples/example-tdl-package-complex/src/lib.rstaskfiles/test.yamltests/tdl-integration/Cargo.tomltests/tdl-integration/src/lib.rstests/tdl-integration/tests/complex.rs
Comment on lines
+37
to
+44
| $( | ||
| map.insert( | ||
| <$task as $crate::Task>::NAME, | ||
| Box::new($crate::TaskHandlerImpl::<$task>::new()) | ||
| as Box<dyn $crate::TaskHandler>, | ||
| ); | ||
| )* | ||
| map |
Contributor
There was a problem hiding this comment.
Detect duplicate task names during registry construction.
At Line 38, HashMap::insert will silently replace an existing handler if two task types resolve to the same Task::NAME, leading to unexpected dispatch.
Proposed fail-fast guard
$(
- map.insert(
+ let previous = map.insert(
<$task as $crate::Task>::NAME,
Box::new($crate::TaskHandlerImpl::<$task>::new())
as Box<dyn $crate::TaskHandler>,
);
+ assert!(
+ previous.is_none(),
+ "duplicate task name in register_tasks!: {}",
+ <$task as $crate::Task>::NAME
+ );
)*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $( | |
| map.insert( | |
| <$task as $crate::Task>::NAME, | |
| Box::new($crate::TaskHandlerImpl::<$task>::new()) | |
| as Box<dyn $crate::TaskHandler>, | |
| ); | |
| )* | |
| map | |
| $( | |
| let previous = map.insert( | |
| <$task as $crate::Task>::NAME, | |
| Box::new($crate::TaskHandlerImpl::<$task>::new()) | |
| as Box<dyn $crate::TaskHandler>, | |
| ); | |
| assert!( | |
| previous.is_none(), | |
| "duplicate task name in register_tasks!: {}", | |
| <$task as $crate::Task>::NAME | |
| ); | |
| )* | |
| map |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/spider-tdl/src/register.rs` around lines 37 - 44, The registry
currently uses HashMap::insert which silently overwrites handlers when two tasks
share the same <$task as $crate::Task>::NAME; change the insertion to a
fail-fast check using the map.entry(...) API (or an explicit contains_key check)
and panic (or return Err) if the name already exists, emitting a clear message
that includes the duplicate name (<$task as $crate::Task>::NAME) and the task
type (e.g. stringify!($task)) so duplicates are detected during registry
construction; update the macro block that creates the
Box::new($crate::TaskHandlerImpl::<$task>::new()) as the value to use this
guarded insertion instead of plain insert.
| MARIADB_DATABASE: "{{.MARIADB_DATABASE}}" | ||
| MARIADB_USERNAME: "{{.MARIADB_USERNAME}}" | ||
| MARIADB_PASSWORD: "{{.MARIADB_PASSWORD}}" | ||
| SPIDER_TDL_PACKAGE_COMPLEX: "{{.G_RUST_BUILD_DIR}}/release/libexample_tdl_package_complex.so" |
Contributor
There was a problem hiding this comment.
Use an OS-aware dynamic library suffix for SPIDER_TDL_PACKAGE_COMPLEX.
Line 217 hardcodes .so, which fails on macOS (.dylib). This can break local/unit test execution for part of the supported Unix-like dev flow.
🔧 Proposed fix
spider-huntsman-unit-tests-executor:
internal: true
+ vars:
+ TDL_DYLIB_EXT:
+ sh: 'case "$(uname -s)" in Darwin) echo "dylib" ;; *) echo "so" ;; esac'
env:
MARIADB_PORT: "{{.MARIADB_PORT}}"
MARIADB_DATABASE: "{{.MARIADB_DATABASE}}"
MARIADB_USERNAME: "{{.MARIADB_USERNAME}}"
MARIADB_PASSWORD: "{{.MARIADB_PASSWORD}}"
- SPIDER_TDL_PACKAGE_COMPLEX: "{{.G_RUST_BUILD_DIR}}/release/libexample_tdl_package_complex.so"
+ SPIDER_TDL_PACKAGE_COMPLEX: "{{.G_RUST_BUILD_DIR}}/release/libexample_tdl_package_complex.{{.TDL_DYLIB_EXT}}"Based on learnings: development tasks/scripts under taskfiles/ are Unix-like only (Linux/macOS), so Linux-only artifact assumptions can break macOS workflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfiles/test.yaml` at line 217, Replace the hardcoded ".so" suffix for
SPIDER_TDL_PACKAGE_COMPLEX with an OS-aware template or variable so macOS uses
".dylib" and Linux uses ".so"; for example compute a SPIDER_LIB_SUFFIX earlier
(or use a template conditional like {{if eq .OS
"darwin"}}.dylib{{else}}.so{{end}}) and then set SPIDER_TDL_PACKAGE_COMPLEX to
"{{.G_RUST_BUILD_DIR}}/release/libexample_tdl_package_complex{{.SPIDER_LIB_SUFFIX}}"
(or use the direct conditional) so the correct dynamic library extension is
chosen at runtime.
Comment on lines
+19
to
+21
| fn lib_path() -> Option<String> { | ||
| std::env::var("SPIDER_TDL_PACKAGE_COMPLEX").ok() | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid silent pass when integration test prerequisites are missing.
At Line 19, returning Option enables all tests to early-return Ok(()); this can hide broken CI wiring and skip coverage without signal.
Proposed hard-fail helper (root cause fix)
-fn lib_path() -> Option<String> {
- std::env::var("SPIDER_TDL_PACKAGE_COMPLEX").ok()
+fn lib_path() -> String {
+ std::env::var("SPIDER_TDL_PACKAGE_COMPLEX")
+ .expect("SPIDER_TDL_PACKAGE_COMPLEX is required for tdl-integration tests")
}Then replace each let Some(path) = lib_path() else { return Ok(()); }; with:
- let Some(path) = lib_path() else {
- return Ok(());
- };
+ let path = lib_path();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn lib_path() -> Option<String> { | |
| std::env::var("SPIDER_TDL_PACKAGE_COMPLEX").ok() | |
| } | |
| fn lib_path() -> String { | |
| std::env::var("SPIDER_TDL_PACKAGE_COMPLEX") | |
| .expect("SPIDER_TDL_PACKAGE_COMPLEX is required for tdl-integration tests") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tdl-integration/tests/complex.rs` around lines 19 - 21, The current
lib_path() returns Option and allows tests to silently skip when
SPIDER_TDL_PACKAGE_COMPLEX is unset; change lib_path() to return a String and
call
std::env::var("SPIDER_TDL_PACKAGE_COMPLEX").expect("SPIDER_TDL_PACKAGE_COMPLEX
must be set for integration tests") (or otherwise panic with a clear message) so
missing prerequisites hard-fail; update all call sites that currently do `let
Some(path) = lib_path() else { return Ok(()); };` to call lib_path() directly
(or let path = lib_path();) and remove the early-return pattern so the test
fails loudly when the env var is not provided.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes