tasks: allow running multi-level sub-tasks

[sandydoo]

For example, for 3 level tasks, you can now run the second level:

devenv tasks run ci:lint

Fixes #2017.

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review Tasks

• Read and analyze the changed files
• Review the task resolution logic changes
• Examine test coverage and quality
• Identify potential issues or improvements
• Provide comprehensive review feedback

Code Review Feedback

I've reviewed the changes for multi-level sub-task support. The implementation looks solid overall, but here are some areas for improvement:

devenv-tasks/src/tasks.rs

Issue 1: Potential Performance Impact (resolve_namespace_roots:96-101)

let matching_tasks: Vec<_> = task_indices
    .iter()
    .filter(|(task_name, _)| task_name.starts_with(&*search_prefix))
    .map(|(_, &index)| index)
    .collect();

For large task sets, this linear scan could be slow. Consider:

• Using a trie or prefix tree for efficient prefix matching
• Pre-indexing tasks by namespace segments
• At minimum, add a comment about the O(n) complexity

Issue 2: String Allocation in Hot Path (resolve_namespace_roots:90-94)

let search_prefix: Cow<str> = if trimmed_name.ends_with(':') {
    Cow::Borrowed(trimmed_name)
} else {
    Cow::Owned(format!("{}:", trimmed_name))
};

Good use of Cow to avoid unnecessary allocations, but the format! could be optimized by checking if we can reuse existing strings.

Issue 3: Input Validation Logic (resolve_namespace_roots:78-82)

if trimmed_name == ":" || trimmed_name.starts_with(':') || trimmed_name.contains("::") {
    return Err(Error::TaskNotFound(name.clone()));
}

The validation logic mixes syntax errors with "not found" errors. Consider:

• Using a more specific error type like Error::InvalidNamespace
• The error message TaskNotFound is misleading for syntax errors like ::

Issue 4: Inconsistent Error Handling (resolve_namespace_roots:75,108)

if trimmed_name.is_empty() {
    return Err(Error::TaskNotFound(name.clone()));
}
// vs
return Err(Error::TaskNotFound(name.clone()));

Both cases use the same error type but represent different failure modes (empty vs not found).

devenv-tasks/src/tests/mod.rs

Issue 5: Test Flakiness Concerns (test_preserved_output_on_skip:866-872)

if let TaskStatus::Completed(TaskCompleted::Skipped(Skipped::Cached(_))) =
    &tasks2.graph[tasks2.tasks_order[0]].read().await.status
{
    println!("Task was correctly skipped on second run");
} else {
    println!("Warning: Task was not skipped on second run");
}

Several tests have relaxed assertions "for CI stability". This suggests:

• Potential race conditions in the caching logic
• Tests that depend on timing/filesystem behavior
• Consider using more deterministic test approaches or mocking

Issue 6: Resource Management (test_file_state_updated_after_task:1008-1009)

let cache = crate::task_cache::TaskCache::with_db_path(db_path.clone()).await?;

Database connections are created but not explicitly closed. While Rust's RAII handles this, consider:

• Using explicit connection pools for better resource management
• Ensuring connections are properly closed in test cleanup

Issue 7: Hardcoded Sleep Values (create_basic_script:2360)

"#!/bin/sh\necho 'Task {tag} is running' && sleep 0.1 && echo 'Task {tag} completed'"

Hardcoded sleeps can cause test flakiness. Consider:

• Using synchronization primitives instead of sleep
• Making sleep durations configurable
• Using deterministic delays in tests

Positive Aspects

• Good error handling: Uses thiserror and proper error propagation
• Comprehensive test coverage: Tests cover many edge cases including namespace resolution, caching, file modification detection
• Memory efficiency: Good use of Cow<str> to avoid unnecessary allocations
• Async design: Proper use of async/await throughout

Security Considerations

• Path traversal: The namespace resolution doesn't seem to have path traversal vulnerabilities
• Resource limits: No obvious resource exhaustion issues in the namespace matching logic
• Input validation: Reasonable validation of namespace names

Recommendations

1. Performance: Consider optimizing the namespace matching for large task sets
2. Error types: Use more specific error types for different failure modes
3. Test stability: Address the test flakiness by removing timing dependencies
4. Documentation: Add comments explaining the O(n) complexity of namespace matching
5. Monitoring: Consider adding metrics for namespace resolution performance

The implementation correctly handles the core requirement of allowing multi-level sub-task execution like devenv tasks run ci:lint. The extensive test coverage provides good confidence in the functionality.

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	4966727
Status:	✅  Deploy successful!
Preview URL:	https://8e0797c4.devenv.pages.dev
Branch Preview URL:	https://sub-tasks.devenv.pages.dev

View logs