fix(backend): add locks to inline preprocessor/failure/tool modules in flows

[hugocasa]

Important

Add locks to handle dependencies in preprocessor_module and failure_module in flow modules, updating traversal and locking logic.

• Behavior:
  • Add locks to preprocessor_module and failure_module in FlowValue in flows.rs.
  • Update lock_flow_value() in worker_lockfiles.rs to process preprocessor_module and failure_module.
  • Modify traverse_leafs() in flows.rs to handle preprocessor_module and failure_module.
• Dependency Map:
  • Update ScopedDependencyMap::fetch_maybe_rearranged() in scoped_dependency_map.rs to handle new module types.
  • Add entries for preprocessor_module and failure_module in dependency_map.sql.
• Tests:
  • Update relative_imports.rs to test new locking behavior for preprocessor_module and failure_module.
  • Add SQL entries in dependency_map.sql for new module types.

^{This description was created by for 189e7f2. You can customize this summary. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	f08bb25
Status:	✅  Deploy successful!
Preview URL:	https://ebe8e4c8.windmill.pages.dev
Branch Preview URL:	https://hc-preprocessor-failure-tool.windmill.pages.dev

View logs

[claude]

Claude finished @hugocasa's task —— View job

---

Code Review: Add locks to inline preprocessor/failure/tool modules in flows ✅

---

Summary

This PR successfully addresses a gap in Windmill's dependency management system by ensuring that inline scripts in flow preprocessor modules, failure modules, and AI agent tools now properly generate lock files and are tracked in the dependency map. This prevents stale dependencies and ensures these special module types stay synchronized when their imported scripts are updated.

Key Changes

1. flows.rs - Data Structure Updates

• ✅ Added FlowNodeFlow struct to wrap FlowValue with an optional summary field
• ✅ Extended AIAgent enum variant to include modules_node: Option<FlowNodeId> for storing references to flow nodes
• ✅ Added resolution logic for AI agent tools in resolve_module function

2. worker_lockfiles.rs - Core Lock Generation Logic

• ✅ Modified insert_flow_modules signature to accept summary: Option<String> and force_insert: bool parameters
• ✅ Added special handling for AI agent tools with force_insert: true to always create flow nodes
• ✅ Updated all callsites of insert_flow_modules throughout the codebase to pass new parameters
• ✅ Modified lock_modules to properly preserve and handle the modules_node field for AI agents

3. scoped_dependency_map.rs - Dependency Tracking

• ✅ Critical Fix: Modified rebuild_map to include failure and preprocessor modules when traversing flows
• ✅ Previously only checked flow_data.flow.modules, now also explicitly includes failure_module and preprocessor_module
• ✅ This ensures dependency map entries are created for imports in these special modules

4. Test Updates

• ✅ Added 4 expected dependency map entries for preprocessor module (3 imports)
• ✅ Added 1 expected dependency map entry for failure module
• ✅ Tests validate that preprocessor with importer_node_id='preprocessor' and failure with importer_node_id='failure' are tracked

---

Detailed Analysis

✅ Correctness Assessment

Strengths:

1. ✅ The core fix in scoped_dependency_map.rs correctly addresses the root issue by including preprocessor and failure modules in dependency traversal
2. ✅ Logic flow is consistent across all three special module types (preprocessor, failure, AI agent tools)
3. ✅ Test updates properly validate the new behavior with concrete expected values
4. ✅ Changes maintain backward compatibility through Option types for new fields
5. ✅ Proper use of Rust's type system prevents many potential bugs at compile time

Minor Concerns:

1. ⚠️ The force_insert: true parameter for AI agent tools bypasses normal simplicity checks without documentation
2. ⚠️ The FlowNodeFlow wrapper struct lacks doc comments explaining its purpose and usage patterns

🔍 Potential Issues & Edge Cases

Issue 1: Asymmetric Code Paths

The preprocessor and failure modules are handled in different code locations:

• Lock generation: Handled separately in lock_preprocessor_and_failure before main flow processing
• Dependency tracking: Handled together with main modules in traverse_leafs during rebuild_map

This asymmetry could cause maintenance problems if someone updates one path but forgets the other.

Recommendation: Add cross-reference comments linking lock_preprocessor_and_failure in worker_lockfiles.rs with the preprocessor/failure handling in scoped_dependency_map.rs

Issue 2: Missing Validation (Acknowledged by TODO)

Line 723 in worker_lockfiles.rs has a TODO comment: "TODO: unsafe. Importer Node Ids are not checked."

The code passes importer_node_ids directly without validating they exist in the database:

args.insert(
    "components_to_relock".to_string(),
    to_raw_value(&s.importer_node_ids), // No validation
);

Impact Assessment: 🟡 Low-Medium

• Worst case: Invalid IDs cause unnecessary work or failed lookups during dependency recomputation
• Not a security issue: No privilege escalation or data corruption possible
• Already tracked: The TODO comment shows this is a known limitation

Recommendation: Consider adding at minimum a warning log for invalid node IDs to aid debugging.

Issue 3: Force Insert Behavior Undocumented

AI agent tools use force_insert: true which bypasses this check:

if !force_insert 
    && (modules.is_empty() || is_simple_modules(modules, failure_module)) {
    return Ok(tx);
}

Question: Why can't AI agents use the simplicity optimization? Possible reasons:

1. AI agent configuration needs to be persisted regardless of simplicity?
2. The summary field needs to be stored in the database?
3. Future extensibility requirements?

Recommendation: Add an inline comment explaining the reasoning for this design decision.

🚀 Performance Analysis

Positive Impacts:

1. ✅ No additional database queries in the hot path - changes only affect dependency job processing
2. ✅ Traversal of preprocessor/failure modules doesn't change algorithmic complexity (already O(n) in module count)
3. ✅ Proper use of transactions prevents race conditions without sacrificing performance

Neutral Impacts:

1. 🟡 AI agent tools will always create flow_node database rows even for simple cases
   • Impact: Minimal - likely only a few extra KB per flow with AI agents
   • Trade-off: Consistency and future extensibility outweigh minor storage cost
2. 🟡 Dependency map now has more entries (preprocessor + failure modules)
   • Impact: Negligible - map lookups are still indexed and fast

Performance Verdict: ✅ No performance concerns. Changes are well-optimized.

🔒 Security Assessment

Security Review: ✅ No security issues identified

The changes:

• ✅ Don't modify authentication or authorization logic
• ✅ Don't introduce new external inputs or attack vectors
• ✅ Use existing parameterized queries (SQL injection safe)
• ✅ Don't change permission checks or access controls
• ✅ Maintain proper transaction boundaries
• ✅ Don't expose sensitive information in logs

Specific Security Checks:

1. ✅ SQL queries use sqlx::query! macro with parameter binding
2. ✅ No user-supplied strings concatenated into SQL
3. ✅ Transaction commits preserve ACID properties
4. ✅ Error messages don't leak sensitive path information

📝 Code Quality Assessment

Excellent:

• ✅ Consistent naming conventions following Rust and Windmill codebase standards
• ✅ Proper error handling with Result<T, Error> types throughout
• ✅ Appropriate use of async/await for database operations
• ✅ Good use of Rust's ownership system (e.g., modules_node.take() for move semantics)
• ✅ Test coverage updated to match new behavior

Good:

• 🟢 Follows existing patterns for similar functionality
• 🟢 Uses transactions appropriately to maintain atomicity
• 🟢 Minimal code duplication

Could Be Improved:

• 🟡 Missing documentation on new public structs (FlowNodeFlow)
• 🟡 Magic behavior (force_insert special case) lacks explanation
• 🟡 TODO comments indicate validation logic could be more robust
• 🟡 The inline comment "we only include summary for ai agents modules" could be more prominent (e.g., as a const or in function docs)

---

Specific Recommendations

🔴 High Priority (Documentation)

1. Document FlowNodeFlow struct: Add a doc comment explaining:

   /// Wrapper for FlowValue that includes an optional summary.
   /// Used when storing flow modules in the database, particularly for
   /// AI agent tool groups where the summary provides context.
   pub struct FlowNodeFlow { ... }

2. Document force_insert parameter: Add doc comment to insert_flow_modules:

   /// - `force_insert`: If true, always create a flow node even if modules are "simple".
   ///   Used for AI agents which need persistent storage for their configuration.
   

3. Link related code sections: Add comments connecting lock generation and dependency tracking:

   • In lock_preprocessor_and_failure: // See also: rebuild_map in scoped_dependency_map.rs for dependency tracking
   • In rebuild_map: // See also: lock_preprocessor_and_failure in worker_lockfiles.rs for lock generation

🟡 Medium Priority (Robustness)

4. Address validation TODO: At minimum, add warning logs for invalid node IDs:

   if let Some(ids) = &s.importer_node_ids {
       // Validate IDs exist before passing them
       // TODO: Add validation query
   }

5. Make special cases explicit: Consider a constant or enum:

   const AI_AGENT_REQUIRES_STORAGE: bool = true; // Always store AI agent modules

🟢 Low Priority (Future Improvements)

6. Type safety: Use newtype pattern for different ID types:

   pub struct FlowNodeId(pub i64);
   pub struct ModuleId(pub String);

7. Consider consolidation: The handling of preprocessor/failure/AI agent could potentially share more code through a common abstraction

---

Testing Assessment

✅ Test Coverage - Excellent

The test updates are thorough and properly validate the new behavior:

1. ✅ Test fixture updates: Added 4 new dependency map entries covering preprocessor and failure modules
2. ✅ Expected values: Test validates exact importer_node_id values ('preprocessor', 'failure')
3. ✅ Correctness verification: The CORRECT_DMAP constant now includes all new entries
4. ✅ Rebuild test: Existing relative_imports_test_rebuild_correctness will validate the new traversal logic

🟡 Additional Test Suggestions

While current tests are good, consider adding:

1. Dependency Update Test: Create flow with preprocessor → update dependency → verify preprocessor gets relocked
2. AI Agent Tools Test: Validate AI agent tools with inline scripts and dependencies
3. Integration Test: Full workflow test exercising all three module types together
4. Edge Case Test: Flow with preprocessor, failure, AND AI agent all importing the same script

---

Reproduction Instructions for QA Testing

To thoroughly verify these changes through the Windmill application interface:

Test 1: Preprocessor Module Lock Generation

Setup:

1. Navigate to the Flow Editor and create a new flow named "test-preprocessor-locks"
2. Click "Add Preprocessor" to add a preprocessor module to your flow
3. In the preprocessor, select "Inline script" and choose Python
4. Add code that imports a workspace script:

   from f.utils.data_processor import clean_data
   
   def preprocessor(event):
       result = clean_data(event)
       return {"cleaned": result}

5. First, make sure f/utils/data_processor exists in your workspace (create it if needed)
6. Deploy the flow

Verification:

1. Go to your workspace settings → Troubleshooting → View Dependency Map
2. Search for "test-preprocessor-locks" in the dependency map
3. Expected: You should see an entry showing:
   • Importer: test-preprocessor-locks (flow)
   • Imported: f/utils/data_processor
   • Node ID: preprocessor
4. Now update the f/utils/data_processor script (add a comment or change logic)
5. Return to your flow - it should automatically have a new version with updated lock
6. Expected: The flow can still execute and uses the new version of the processor

Test 2: Failure Module Dependency Tracking

Setup:

1. Create a new flow named "test-failure-locks"
2. Add at least one regular step (can be simple identity)
3. Click "Add Failure Module" at the flow level
4. Add inline Python or TypeScript code with imports:

   from f.monitoring.error_handler import log_error
   from f.notifications.slack import send_alert
   
   def main(message: str, name: str, step_id: str):
       log_error(message, step_id)
       send_alert(f"Flow failed at {step_id}: {message}")
       return {"logged": True}

5. Ensure the imported scripts exist
6. Deploy the flow

Verification:

1. Check the dependency map for "test-failure-locks"
2. Expected: Two entries with importer_node_id='failure':
   • test-failure-locks → f/monitoring/error_handler (failure)
   • test-failure-locks → f/notifications/slack (failure)
3. Update one of the imported scripts (e.g., add logging to error_handler)
4. Expected: Flow automatically gets a new version with updated dependencies
5. Trigger a failure in the flow (make a step fail)
6. Expected: Failure module executes with the updated dependency code

Test 3: AI Agent Tools with Dependencies

Setup:

1. Create a flow with an AI Agent step
2. In the AI Agent configuration, add a custom tool
3. Make the tool an inline script with dependencies:

   from f.data.fetcher import get_user_data
   import wmill
   
   def main(user_id: str):
       data = get_user_data(user_id)
       return {"user": data, "timestamp": wmill.get_current_time()}

4. Configure the AI agent with appropriate prompts and settings
5. Deploy the flow

Verification:

1. Check dependency map for the flow
2. Expected: Entry showing flow → f/data/fetcher with the tool's node ID
3. Update the f/data/fetcher script
4. Expected: Flow regenerates locks for the AI agent's tools
5. Execute the AI agent step
6. Expected: The tool uses the updated dependency and runs successfully

Test 4: Combined Scenario (Comprehensive)

Setup:
Create a single flow that uses all three features:

1. Preprocessor importing f/validation/input_validator
2. Regular steps doing some work
3. Failure module importing f/error/handler
4. AI Agent with tools importing f/data/enricher

Verification:

1. Deploy the flow and verify all dependency map entries exist
2. Update all three imported scripts simultaneously
3. Expected: Flow automatically creates new version with all three modules' locks updated
4. Expected: Dependency map shows all three special modules tracking their respective imports
5. Run the flow end-to-end to ensure all modules work with updated dependencies

---

Overall Assessment

✅ Approval Recommendation: APPROVE with minor suggestions

This is a well-implemented fix for an important gap in Windmill's dependency management system. The code is:

• ✅ Correct: Solves the stated problem effectively
• ✅ Safe: No security concerns, proper error handling
• ✅ Performant: No performance regressions
• ✅ Well-tested: Good test coverage with clear validation

Minor improvements suggested (mostly documentation):

• Document the FlowNodeFlow struct and force_insert parameter
• Add cross-reference comments between related code sections
• Consider addressing the validation TODO for node IDs

Impact: This change improves system reliability by ensuring preprocessor, failure, and AI agent tool modules stay synchronized with their dependencies, preventing subtle bugs from stale lock files.

---

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to e19eba6 in 2 minutes and 36 seconds. Click for details.

• Reviewed 358 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 11 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/worker_lockfiles.rs:1591

• Draft comment:
  Avoid repeatedly calling remove_dir_all and create_dir_all on the job_dir within the loop. This could be expensive if many modules are processed. Consider moving the directory reset outside the per-module loop.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. backend/windmill-worker/src/worker_lockfiles.rs:1336

• Draft comment:
  Using Box::pin() repeatedly to invoke async recursive calls (e.g. in lock_modules) may be unnecessary. Consider using an async recursion macro or refactoring to simplify the async call structure.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. backend/windmill-worker/src/worker_lockfiles.rs:1203

• Draft comment:
  The transaction variable 'tx' is reassigned several times after async calls. Ensure that the transaction state is correctly propagated and consider refactoring into smaller helper functions for clarity.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. backend/windmill-worker/src/worker_lockfiles.rs:1198

• Draft comment:
  raw_deps is cloned repeatedly and passed to several function calls. If raw_deps constitutes a large structure, consider passing it by reference to avoid unnecessary cloning.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% While the comment is technically correct that raw_deps is cloned multiple times, I need to consider: 1. HashMap cloning could be expensive if the map is large 2. However, raw_deps appears to be a mapping of language to lockfile content, which is likely small 3. The alternative would be passing references, but that would complicate the recursive function signatures with lifetimes 4. The cloning happens in an async dependency resolution context where performance is not super critical I may be underestimating the size of the raw_deps HashMap. If it contains large lockfiles, the cloning could be more expensive than I assume. Even if raw_deps is larger than expected, the complexity cost of adding lifetimes to handle references would likely outweigh the performance benefit, especially since this is not in a hot path. The comment raises a valid point but the suggested change would add more complexity than it's worth. The current approach of cloning is acceptable given the context.

5. backend/windmill-worker/src/worker_lockfiles.rs:1871

• Draft comment:
  In the RawScript match arm, the use of 'else { unreachable!() }' after std::mem::replace makes the code brittle. Consider handling this error case explicitly instead of assuming it is unreachable.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. backend/windmill-worker/src/worker_lockfiles.rs:1282

• Draft comment:
  The lock_modules function is highly nested and handles multiple flow module variants. Consider splitting this function into smaller helper functions to improve readability and maintainability.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. backend/windmill-worker/src/worker_lockfiles.rs:1710

• Draft comment:
  When updating the hash computation (e.g. including relative imports in the hash), ensure that the handling of optional lock and flow content is consistent to avoid unintended cache misses.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. backend/windmill-worker/src/worker_lockfiles.rs:2930

• Draft comment:
  There is extensive branching for different ScriptLang variants in capture_dependency_job. Ensure that inline documentation explains the rationale for bypassing or processing raw_deps for each language variant.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. backend/windmill-worker/src/worker_lockfiles.rs:1324

• Draft comment:
  Typo alert: The variable name 'nmodified_ids' may be a typo. Consider renaming it to 'modified_ids' (or clarify the intended meaning, e.g., if it should be 'new_modified_ids') for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The 'n' prefix pattern is used consistently throughout the codebase for temporary variables that hold "new" values. While 'new_modified_ids' might be more explicit, the current naming follows an established pattern in this file. The meaning is clear from context and consistent with the codebase style. The comment has a point that more explicit naming could improve readability for new developers. The 'n' prefix is a bit terse and its meaning isn't immediately obvious without seeing the pattern. However, consistency within a codebase is also important for readability. The 'n' prefix is used systematically throughout this file, and changing just one instance would actually reduce consistency. The variable name follows an established pattern in the codebase and changing it would reduce consistency. The comment should be deleted.

10. backend/windmill-worker/src/worker_lockfiles.rs:1325

• Draft comment:
  Typo alert: The variable name 'nerrors' might be a typo. Verify if the intended name is 'errors' or 'new_errors', and adjust accordingly.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

11. backend/windmill-worker/src/worker_lockfiles.rs:1646

• Draft comment:
  Typo: The code uses e.id.clone() but in this branch the error is bound as error. Should this be error.id.clone() to match the binding?
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_uYSAKg3D8Yn2xGWh

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[claude]

Good fix! This ensures that inline scripts in both preprocessor and failure modules are now tracked in the dependency map, which was the main goal of this PR.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 59f6353 in 2 minutes and 46 seconds. Click for details.

• Reviewed 42 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/tests/fixtures/dependency_map.sql:67

• Draft comment:
  Changed inline AI tool: replaced tool id 'm' with 'mytool1' and updated its content to import from f/rel/leaf_2. Consider adding an explicit 'lock' property for consistency with other inline modules.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. backend/tests/fixtures/dependency_map.sql:101

• Draft comment:
  New dependency map entry for tool 'mytool1' has been added. Verify that this entry aligns with the updated tool id in the flow definition.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify alignment between a new dependency entry and an updated tool id. This falls under asking the author to double-check or ensure something, which is against the rules.

Workflow ID: wflow_2RvYeDbZFLal54bP

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 189e7f2 in 1 minute and 31 seconds. Click for details.

• Reviewed 57 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/tests/fixtures/dependency_map.sql:101

• Draft comment:
  Renamed tool id from 'mytool1' to 'qtool1' in the aiagent module and corresponding dependency map row. This ensures consistent locking for inline modules.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. backend/tests/relative_imports.rs:157

• Draft comment:
  Updated expected dependency tool id from 'mytool1' to 'qtool1' to match the inline module change.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_JPspS2d6tBZSdk3L

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[hugocasa]

/ai-fast review this PR

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run