Skip to content

Commit 88f9a36

Browse files
committed
Address third review: fan-out IDs, catalog guards, shell coercion, docs
- Fan-out generates unique per-item step IDs and collects results - Catalog merge skips non-dict workflow entries (malformed data guard) - Shell step coerces run_cmd to str after expression evaluation - urlopen timeout=30 for catalog workflow installs - yaml.dump with sort_keys=False, allow_unicode=True for catalog configs - Document streaming timeout as intentionally unbounded (user Ctrl+C) - Document --allow-all-tools as required for non-interactive + future enhancement - Update test docstring and PUBLISHING.md to 10 step types with prompt
1 parent 2dace14 commit 88f9a36

8 files changed

Lines changed: 34 additions & 9 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4394,7 +4394,7 @@ def workflow_add(
43944394
from urllib.request import urlopen # noqa: S310 — URL comes from catalog
43954395

43964396
workflow_dir.mkdir(parents=True, exist_ok=True)
4397-
with urlopen(workflow_url) as response: # noqa: S310
4397+
with urlopen(workflow_url, timeout=30) as response: # noqa: S310
43984398
workflow_file.write_bytes(response.read())
43994399
except Exception as exc:
44004400
if workflow_dir.exists():

src/specify_cli/integrations/base.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,9 @@ def dispatch_command(
173173
cwd = str(project_root) if project_root else None
174174

175175
if stream:
176+
# No timeout when streaming — the user sees live output and
177+
# can Ctrl+C at any time. The timeout parameter is only
178+
# applied in the captured (non-streaming) branch below.
176179
try:
177180
result = subprocess.run(
178181
exec_args,

src/specify_cli/integrations/copilot/__init__.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ def dispatch_command(
8383
agent_name = f"speckit.{stem}"
8484

8585
prompt = args or ""
86+
# NOTE: --allow-all-tools is required for non-interactive execution
87+
# so the agent can perform file edits and shell commands. The
88+
# workflow engine's gate steps serve as the human approval mechanism.
89+
# Making tool approval configurable (e.g. via workflow config or
90+
# env var) is tracked as a future enhancement.
8691
cli_args = [
8792
"copilot", "-p", prompt,
8893
"--agent", agent_name,

src/specify_cli/workflows/catalog.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,11 +371,15 @@ def _get_merged_workflows(
371371
# Handle both dict and list formats
372372
if isinstance(workflows, dict):
373373
for wf_id, wf_data in workflows.items():
374+
if not isinstance(wf_data, dict):
375+
continue
374376
wf_data["_catalog_name"] = entry.name
375377
wf_data["_install_allowed"] = entry.install_allowed
376378
merged[wf_id] = wf_data
377379
elif isinstance(workflows, list):
378380
for wf_data in workflows:
381+
if not isinstance(wf_data, dict):
382+
continue
379383
wf_id = wf_data.get("id", "")
380384
if wf_id:
381385
wf_data["_catalog_name"] = entry.name
@@ -468,7 +472,7 @@ def add_catalog(self, url: str, name: str | None = None) -> None:
468472

469473
config_path.parent.mkdir(parents=True, exist_ok=True)
470474
with open(config_path, "w", encoding="utf-8") as f:
471-
yaml.dump(data, f, default_flow_style=False)
475+
yaml.dump(data, f, default_flow_style=False, sort_keys=False, allow_unicode=True)
472476

473477
def remove_catalog(self, index: int) -> str:
474478
"""Remove a catalog source by index (0-based). Returns the removed name."""
@@ -488,6 +492,6 @@ def remove_catalog(self, index: int) -> str:
488492
data["catalogs"] = catalogs
489493

490494
with open(config_path, "w", encoding="utf-8") as f:
491-
yaml.dump(data, f, default_flow_style=False)
495+
yaml.dump(data, f, default_flow_style=False, sort_keys=False, allow_unicode=True)
492496

493497
return removed.get("name", f"catalog-{index + 1}")

src/specify_cli/workflows/engine.py

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -596,23 +596,35 @@ def _execute_steps(
596596
):
597597
return
598598

599-
# Fan-out: execute nested step template per item
599+
# Fan-out: execute nested step template per item with unique IDs
600600
if step_type == "fan-out" and result.output.get("items"):
601601
template = result.output.get("step_template", {})
602602
if template:
603-
for item_val in result.output["items"]:
603+
fan_out_results = []
604+
for item_idx, item_val in enumerate(result.output["items"]):
604605
context.item = item_val
606+
# Create a per-item copy with a unique ID
607+
item_step = dict(template)
608+
base_id = item_step.get("id", "item")
609+
item_step["id"] = f"{base_id}-{item_idx}"
605610
self._execute_steps(
606-
[template], context, state, registry,
611+
[item_step], context, state, registry,
607612
step_offset=-1,
608613
)
614+
# Collect per-item result for fan-in
615+
item_result = context.steps.get(item_step["id"], {})
616+
fan_out_results.append(item_result.get("output", {}))
609617
if state.status in (
610618
RunStatus.PAUSED,
611619
RunStatus.FAILED,
612620
RunStatus.ABORTED,
613621
):
614-
return
622+
break
615623
context.item = None
624+
# Update fan-out step output with collected results
625+
result.output["results"] = fan_out_results
626+
context.steps[step_id]["output"] = result.output
627+
state.step_results[step_id]["output"] = result.output
616628

617629
def _resolve_inputs(
618630
self,

src/specify_cli/workflows/steps/shell/__init__.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
2121
run_cmd = config.get("run", "")
2222
if isinstance(run_cmd, str) and "{{" in run_cmd:
2323
run_cmd = evaluate_expression(run_cmd, context)
24+
run_cmd = str(run_cmd)
2425

2526
cwd = context.project_root or "."
2627

tests/test_workflows.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
- Step registry & auto-discovery
55
- Base classes (StepBase, StepContext, StepResult)
66
- Expression engine
7-
- All 9 built-in step types
7+
- All 10 built-in step types
88
- Workflow definition loading & validation
99
- Workflow engine execution & state persistence
1010
- Workflow catalog & registry

workflows/PUBLISHING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ steps:
9090
- ✅ `version` follows semantic versioning (X.Y.Z)
9191
- ✅ `description` is concise
9292
- ✅ All step IDs are unique
93-
- ✅ Step types are valid: `command`, `shell`, `gate`, `if`, `switch`, `while`, `do-while`, `fan-out`, `fan-in`
93+
- ✅ Step types are valid: `command`, `prompt`, `shell`, `gate`, `if`, `switch`, `while`, `do-while`, `fan-out`, `fan-in`
9494
- ✅ Required fields present per step type (e.g., `condition` for `if`, `expression` for `switch`)
9595
- ✅ Input types are valid: `string`, `number`, `boolean`
9696
- ✅ Enum values match declared type

0 commit comments

Comments
 (0)