feat(writer): allow CRON trigger block to be run - WF-881 #1250
Conversation
WalkthroughAdds generic trigger discovery and Cron-trigger support to blueprint execution. BlueprintRunner gains trigger-gatherers and Cron accessors; run_blueprint_via_api autonomously selects API or Cron triggers. serve.py broadens trigger checks to API or Cron and unifies task payloads. core.py no longer requires trigger_type in payloads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Serve as Serve.py
participant Runner as BlueprintRunner
participant Registry as TriggerRegistry
participant Worker
Note over Client,Serve: Client requests blueprint run (no trigger_type)
Client->>Serve: POST /run { blueprint_id, branch_id?, payload }
Serve->>Runner: run_blueprint_via_api(blueprint_id, branch_id, payload)
Runner->>Registry: _gather_api_blueprints() / _gather_cron_blueprints()
alt API trigger found
Registry-->>Runner: api_trigger_id
Runner->>Worker: invoke using API trigger (api_trigger_id, payload)
else Cron trigger found
Registry-->>Runner: cron_trigger_id
Runner->>Worker: invoke using Cron trigger (cron_trigger_id, payload)
else no trigger
Runner-->>Serve: raise ValueError (no supported trigger)
Serve-->>Client: 4xx error (lacks a supported trigger)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/writer/blueprints.py (1)
170-183: Implementation is consistent with the API counterpart.The static analysis tool flags that exception messages should ideally be defined within exception classes (TRY003). Since
get_blueprint_api_triggeruses the same pattern, this is consistent. Consider creating a custom exception class if you plan to catch these specifically, but this is optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/writer/blueprints.pysrc/writer/serve.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/writer/blueprints.py
180-182: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (8)
- GitHub Check: tests (firefox)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
🔇 Additional comments (6)
src/writer/serve.py (2)
388-396: Clean generalization of trigger detection.The refactored function cleanly extends trigger support to both API and Cron types. Using a tuple for
supported_triggersmakes the code extensible for potential future trigger types while keeping the logic readable.
524-529: LGTM!The validation and error message are correctly updated to reflect support for both trigger types. The existing bypass for
branch_idexecution is properly preserved.src/writer/blueprints.py (4)
72-74: LGTM!The property follows the established pattern of
api_blueprintsfor consistency.
144-153: LGTM!The implementation correctly mirrors
is_blueprint_api_availableand provides consistent API for checking Cron trigger availability.
185-230: Good refactoring to reduce duplication.The generalized
_gather_blueprints_by_triggermethod cleanly consolidates the logic for both API and Cron trigger collection, making the code more maintainable and extensible.
252-258: Verify the asymmetric fallback behavior is intentional.The fallback logic at line 254 has asymmetric behavior:
- If
trigger_type == "API"but API trigger is unavailable → silently falls back to Cron- If
trigger_type == "Cron"but Cron trigger is unavailable → raisesValueErrorThis works correctly with the current serve.py code (where
trigger_type="Cron"is only used whenbranch_idis provided, bypassing this logic), but could be confusing for direct callers of this method who might expect symmetric behavior.Consider either:
- Documenting this fallback behavior explicitly in the docstring
- Or making the fallback symmetric (try both before failing)
HackerOne Code Security Review🟢 Scan Complete: 3 Issue(s) Here's how the code changes were interpreted and info about the tools used for scanning. 📖 Summary of ChangesThe modifications across multiple files in the writer module focus on enhancing blueprint trigger handling. Changes include introducing methods for gathering Cron blueprints, refactoring trigger type detection, and expanding blueprint triggerability checks. The updates appear to improve the flexibility and robustness of blueprint execution mechanisms, with minimal alterations to the core implementation.
ℹ️ Issues DetectedNOTE: These may not require action! Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem. How will I know if something is a problem?
🧰 Analysis tools
⏱️ Latest scan covered changes up to commit 2ca3ee7 (latest) |
|
✅ Graham C reviewed all the included code changes and associated automation findings and determined that there were no immediately actionable security flaws. Note that they will continue to be notified of any new commits or comments and follow up as needed throughout the duration of this pull request's lifecycle.
Reviewed with ❤️ by PullRequest |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/writer/blueprints.py (1)
185-230: Nice refactor—generalizing trigger gathering reduces duplication.The
_gather_blueprints_by_triggermethod cleanly abstracts the common logic, and the derived_gather_api_blueprintsand_gather_cron_blueprintsmethods are concise.Minor consideration: Both
api_blueprintsandcron_blueprintsproperties recompute the component tree traversal on each access. If these are accessed frequently in hot paths, caching could be beneficial. However, this is likely acceptable for the current usage patterns.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/writer/blueprints.pysrc/writer/core.pysrc/writer/serve.py
💤 Files with no reviewable changes (1)
- src/writer/core.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:34:16.165Z
Learnt from: ramedina86
Repo: writer/writer-framework PR: 935
File: src/writer/blueprints.py:210-212
Timestamp: 2025-06-16T15:34:16.165Z
Learning: In src/writer/blueprints.py, execution environments passed to run_branch and run_blueprint methods are intentionally mutated during execution. The environments need to be created beforehand and should not be deep-copied, as the mutation is part of the intended workflow design.
Applied to files:
src/writer/blueprints.py
🧬 Code graph analysis (2)
src/writer/serve.py (2)
src/writer/app_runner.py (2)
AppRunner(767-1360)handle_event(1083-1084)src/writer/ss_types.py (1)
WriterEvent(175-180)
src/writer/blueprints.py (1)
src/writer/core.py (1)
run_blueprint_via_api(1272-1293)
🪛 Ruff (0.14.10)
src/writer/blueprints.py
180-182: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (8)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: tests (webkit)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: tests (firefox)
- GitHub Check: build (3.10)
- GitHub Check: tests (chromium)
🔇 Additional comments (7)
src/writer/serve.py (3)
388-396: LGTM! Clean refactor to support multiple trigger types.The function rename from
has_api_triggertois_blueprint_triggerablewith the expanded trigger type set is well-structured. The tuple-basedsupported_triggerslookup is efficient and easily extensible.
524-529: LGTM! Error message updated to reflect broader trigger support.The condition check and error message correctly align with the renamed function and new multi-trigger capability.
542-556: LGTM! Unified payload structure simplifies the event flow.Removing
trigger_typefrom the payload and delegating trigger selection torun_blueprint_via_apiin blueprints.py is a good design choice—it centralizes trigger resolution logic and keeps the API layer thin.src/writer/blueprints.py (4)
72-74: LGTM! Consistent pattern with existingapi_blueprintsproperty.The new
cron_blueprintsproperty mirrors the existingapi_blueprintsproperty structure, maintaining API consistency.
144-153: LGTM! Follows the established pattern fromis_blueprint_api_available.The method correctly checks availability by delegating to the
cron_blueprintsproperty.
170-183: LGTM! Mirrorsget_blueprint_api_triggerimplementation.The method correctly validates availability before returning the trigger ID. The static analysis hint about long exception messages (TRY003) is a minor style preference and acceptable here given the existing pattern used in
get_blueprint_api_triggerat lines 165-167.
250-267:and
…a_api and streamline trigger selection logic - AB-881
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/writer/blueprints.py (2)
170-183: Consider extracting exception messages to constants.The method logic is correct. However, static analysis suggests that long error messages could be extracted to constants or custom exception classes for better maintainability.
Optional refactor
+CRON_TRIGGER_NOT_FOUND_MSG = 'Cron trigger not found for blueprint "{}".' + def get_blueprint_cron_trigger( self, blueprint_id: str ): """ Retrieves the Cron trigger for a given blueprint key. :param blueprint_key: The blueprint identifier. :return: The Cron trigger component. """ if not self.is_blueprint_cron_available(blueprint_id): - raise ValueError( - f'Cron trigger not found for blueprint "{blueprint_id}".' - ) + raise ValueError(CRON_TRIGGER_NOT_FOUND_MSG.format(blueprint_id)) return self.cron_blueprints[blueprint_id]
250-268: Trigger resolution logic is sound with API prioritization.The trigger resolution correctly handles three scenarios:
- When
branch_idis provided, it determines the trigger type from the component- When
branch_idis None, it prioritizes API triggers over Cron triggers- When no triggers are available, it raises an appropriate error
The API-over-Cron prioritization when both exist is reasonable but could be documented in the docstring if this behavior is important to users.
Note: Static analysis suggests extracting the long exception message at line 268 to a constant, similar to the earlier comment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/writer/blueprints.pysrc/writer/core.pysrc/writer/serve.py
💤 Files with no reviewable changes (1)
- src/writer/core.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:34:16.165Z
Learnt from: ramedina86
Repo: writer/writer-framework PR: 935
File: src/writer/blueprints.py:210-212
Timestamp: 2025-06-16T15:34:16.165Z
Learning: In src/writer/blueprints.py, execution environments passed to run_branch and run_blueprint methods are intentionally mutated during execution. The environments need to be created beforehand and should not be deep-copied, as the mutation is part of the intended workflow design.
Applied to files:
src/writer/blueprints.py
🧬 Code graph analysis (1)
src/writer/blueprints.py (1)
src/writer/core.py (1)
run_blueprint_via_api(1272-1293)
🪛 Ruff (0.14.10)
src/writer/blueprints.py
180-182: Avoid specifying long messages outside the exception class
(TRY003)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (8)
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.13)
- GitHub Check: tests (chromium)
- GitHub Check: tests (webkit)
- GitHub Check: tests (firefox)
🔇 Additional comments (8)
src/writer/serve.py (3)
388-396: LGTM! Clear function name and implementation.The renamed function
is_blueprint_triggerablewith its updated docstring and implementation correctly checks for both API and Cron triggers. The use of a tuple forsupported_triggersis idiomatic and efficient.
413-413: LGTM! Consistent usage of renamed function.The call site correctly uses the renamed
is_blueprint_triggerablefunction.
542-556: LGTM! Unified task construction simplifies the flow.The unified approach to blueprint execution removes the need for explicit trigger type discrimination at the API layer, delegating trigger resolution to
run_blueprint_via_api. This simplifies the payload structure and aligns with the broader refactoring to make trigger selection automatic.src/writer/blueprints.py (5)
72-74: LGTM! Consistent with existing pattern.The
cron_blueprintsproperty follows the same pattern as the existingapi_blueprintsproperty, maintaining consistency in the API.
144-153: LGTM! Mirrors the API version.The method correctly checks for Cron trigger availability, maintaining symmetry with
is_blueprint_api_available.
185-214: LGTM! Good abstraction to reduce duplication.The generic
_gather_blueprints_by_triggermethod effectively consolidates trigger discovery logic, making it easy to support additional trigger types in the future.
216-230: LGTM! Clean delegation pattern.Both
_gather_api_blueprintsand_gather_cron_blueprintscleanly delegate to the shared_gather_blueprints_by_triggermethod, eliminating code duplication.
269-274: LGTM! Clear execution with informative title.The invocation of
run_branchcorrectly passes the resolved trigger information, and the title format clearly indicates which trigger type was selected.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.