Auto-load workflow templates and tighten PR template#106
Auto-load workflow templates and tighten PR template#106
Conversation
There was a problem hiding this comment.
Pull request overview
Consolidates workflow templates from external repos into a new workflows package and integrates template selection + self-healing retries into OrchestrationEngine, along with supporting docs and tests.
Changes:
- Added a declarative workflow template catalog + registry helpers (
workflows/) and wired template materialization into orchestration. - Implemented step-level retry/self-heal behavior and failure recording hooks during workflow execution.
- Added documentation and tests validating template coverage and orchestrator template selection; updated test dependencies for async tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| workflows/templates.py | Introduces WorkflowTemplate and consolidated template definitions. |
| workflows/self_correcting.py | Adds helper to apply retry policy defaults to workflow steps. |
| workflows/registry.py | Provides template listing, intent-to-template matching, and materialization. |
| workflows/init.py | Exposes registry functions as package API. |
| orchestrator.py | Integrates template generation, applies self-correction, adds retry loop + failure recording. |
| tests/test_workflow_templates.py | Adds tests for template sources, materialization, and orchestrator selection. |
| tests/test_mcp_compliance.py | Adjusts MCP fixtures to be synchronous. |
| tests/conftest.py | Ensures repo root is on sys.path for test imports. |
| requirements-test.txt | Adds pytest-asyncio to test requirements. |
| mcp_server/real_mcp_server.py | Adds tool registry + lightweight RealMCPServer wrapper for tests. |
| docs/workflows/CONSOLIDATION.md | Documents consolidation and template-driven orchestration usage. |
| README.md | Mentions the new consolidated workflow catalog. |
| MCP_INTEGRATION_PLAN.md | Clarifies “no mocks” policy statement. |
| config/init.py | Adds config package marker. |
| .github/dependabot_test.py | Adds helper module used by tests for dependabot config assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,5 @@ | |||
| pytest>=6.0.0 | |||
There was a problem hiding this comment.
pytest-asyncio>=0.23.0 is likely incompatible with pytest>=6.0.0, and it also diverges from requirements-ci.txt (which requires pytest>=7). To avoid resolver conflicts and keep local/CI aligned, bump the pytest lower bound here to match CI (or adjust pytest-asyncio version accordingly).
| pytest>=6.0.0 | |
| pytest>=7.0.0 |
| policy = workflow.get("self_correction", {}).get("policy", {}) | ||
| max_attempts = policy.get("max_attempts", 2) | ||
|
|
||
| for step in workflow.get("steps", []): | ||
| retry = step.get("retry_policy") or {} | ||
| retry.setdefault("max_attempts", max_attempts) | ||
| retry.setdefault("strategy", "self_heal") | ||
| step["retry_policy"] = retry |
There was a problem hiding this comment.
apply_self_correction ignores the self_correction.enabled flag that templates set, and it also applies a default retry policy even when self_correction is absent. This makes it impossible to opt out of retries and leaves enabled as dead metadata. Consider short-circuiting when self_correction.enabled is false (or missing), and only defaulting max_attempts when self-correction is enabled.
|
|
||
| for step in self.steps: | ||
| step_copy = dict(step) | ||
| step_copy.setdefault("retry_policy", default_retry) |
There was a problem hiding this comment.
default_retry is a single dict reused across all steps via setdefault, so multiple steps will share the same retry_policy object. If any code later mutates one step’s retry policy (e.g., per-step backoff, attempts), it will unintentionally affect all steps. Create a new dict per step (copy/merge) when assigning the default retry policy.
| step_copy.setdefault("retry_policy", default_retry) | |
| if "retry_policy" not in step_copy: | |
| step_copy["retry_policy"] = default_retry.copy() |
| "construction", | ||
| "daily log", | ||
| "rfi", | ||
| "wrklfw", |
There was a problem hiding this comment.
The tag wrklfw looks like a misspelling of wrkflw (repo name). Since match_template_for_intent does substring matches on tags, this typo can prevent intent matching based on that tag.
| "wrklfw", | |
| "wrkflw", |
| async def store_execution(self, data: Dict): | ||
| """Store execution data in graph""" | ||
| node_id = data["workflow_id"] | ||
| self.graph[node_id] = { | ||
| "data": data, | ||
| "relationships": [], | ||
| "insights": [], | ||
| "failures": [], | ||
| } |
There was a problem hiding this comment.
store_execution overwrites any existing node for the same workflow_id (including failures recorded during execution), resetting failures to an empty list. This loses failure history that _handle_step_failure just recorded. Consider merging into an existing node (preserve failures/relationships/insights) instead of replacing it.
| node = self.graph.setdefault( | ||
| workflow_id, | ||
| {"data": {}, "relationships": [], "insights": [], "failures": []}, | ||
| ) |
There was a problem hiding this comment.
record_step_failure can create a graph node with data: {}. find_similar_executions later assumes node['data']['intent'] exists, which will raise a KeyError if called before store_execution populates data. Either ensure data includes the intent when creating the node, or guard against missing intent in find_similar_executions.
| ```python | ||
| from orchestrator import OrchestrationEngine | ||
|
|
||
| engine = OrchestrationEngine() | ||
| result = asyncio.run( | ||
| engine.execute_intent( | ||
| "Generate a construction daily log workflow", | ||
| options={"workflow_template": "construction_daily_log"}, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The example uses asyncio.run(...) but doesn’t import asyncio, so the snippet won’t run as written. Add the missing import (or adjust the snippet).
| attempt = 0 | ||
| last_error = None | ||
|
|
||
| while attempt < attempts: |
There was a problem hiding this comment.
Bug: If a step's retry_policy has max_attempts set to 0, the execution loop is never entered, causing the step to be silently skipped without any logging or errors.
Severity: HIGH
Suggested Fix
Validate the max_attempts value when loading or materializing workflow templates. Ensure that max_attempts is a positive integer, defaulting to 1 if it is not set or is invalid. Reject or correct templates where max_attempts is 0.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: orchestrator.py#L289
Potential issue: The retry logic in the step executor uses a `while attempt < attempts:`
loop. If a workflow step is configured with `"max_attempts": 0`, this condition is
immediately false. Consequently, the step's execution block is never entered. This
results in the step being silently skipped without any execution, logging, or error
reporting. The workflow proceeds as if the step succeeded, eventually marking the entire
workflow as "completed". This can lead to silent failures and incomplete data
processing, particularly since the pull request allows loading external templates which
may not have validated `max_attempts` values.
Did we get this right? 👍 / 👎 to inform future reviews.
Title
Auto-load workflow templates and tighten PR template
Summary
workflows/data/), seeded WRKFLW-derived construction log template, and refreshed PR template for concise summaries.Type
Checklist
Sections
workflows/data(yaml/json) into the catalog.construction_daily_log_externaltemplate.python -m pytest(new external-template coverage included).datetime.utcnow()remain unchanged.Original prompt