Skip to content

Commit e681ffe

Browse files
committed
Address sixth review: operator precedence, fan_in cleanup, registry resilience, docs
- Fix or/and operator precedence (or parsed first = lower precedence) - Restore context.fan_in after fan-in step completes - Catch JSONDecodeError in registry load for corrupted files - Replace print() with on_step_start callback (library-safe) - Gate validation warns when on_reject set but no reject option - Shell step: document shell=True security tradeoff - README: sdd-pipeline → speckit, parallel → sequential for fan-out - ARCHITECTURE.md: parallel → fan-out/fan-in in diagram
1 parent 97dcf01 commit e681ffe

9 files changed

Lines changed: 47 additions & 23 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4101,6 +4101,7 @@ def workflow_run(
41014101

41024102
project_root = Path.cwd()
41034103
engine = WorkflowEngine(project_root)
4104+
engine.on_step_start = lambda sid, label: console.print(f" \u25b8 [{sid}] {label} \u2026")
41044105

41054106
try:
41064107
definition = engine.load_workflow(source)
@@ -4164,6 +4165,7 @@ def workflow_resume(
41644165

41654166
project_root = Path.cwd()
41664167
engine = WorkflowEngine(project_root)
4168+
engine.on_step_start = lambda sid, label: console.print(f" \u25b8 [{sid}] {label} \u2026")
41674169

41684170
try:
41694171
state = engine.resume(run_id)

src/specify_cli/workflows/catalog.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,12 @@ def __init__(self, project_root: Path) -> None:
7373
def _load(self) -> dict[str, Any]:
7474
"""Load registry from disk or create default."""
7575
if self.registry_path.exists():
76-
with open(self.registry_path, encoding="utf-8") as f:
77-
return json.load(f)
76+
try:
77+
with open(self.registry_path, encoding="utf-8") as f:
78+
return json.load(f)
79+
except (json.JSONDecodeError, ValueError):
80+
# Corrupted registry file — reset to default
81+
return {"schema_version": self.SCHEMA_VERSION, "workflows": {}}
7882
return {"schema_version": self.SCHEMA_VERSION, "workflows": {}}
7983

8084
def save(self) -> None:

src/specify_cli/workflows/engine.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,7 @@ class WorkflowEngine:
297297

298298
def __init__(self, project_root: Path | None = None) -> None:
299299
self.project_root = project_root or Path(".")
300+
self.on_step_start: Any = None # Callable[[str, str], None] | None
300301

301302
def load_workflow(self, source: str | Path) -> WorkflowDefinition:
302303
"""Load a workflow from an installed ID or a local YAML path.
@@ -504,10 +505,11 @@ def _execute_steps(
504505
{"event": "step_started", "step_id": step_id, "type": step_type}
505506
)
506507

507-
# Print progress so the user sees which step is running
508-
508+
# Log progress — use the engine's on_step_start callback if set,
509+
# otherwise stay silent (library-safe default).
509510
label = step_config.get("command", "") or step_type
510-
print(f" ▸ [{step_id}] {label} …", flush=True)
511+
if self.on_step_start is not None:
512+
self.on_step_start(step_id, label)
511513

512514
step_impl = registry.get(step_type)
513515
if not step_impl:

src/specify_cli/workflows/expressions.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -150,19 +150,20 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
150150
):
151151
return expr[1:-1]
152152

153-
# Boolean operators
154-
if " and " in expr:
155-
parts = expr.split(" and ", 1)
156-
left = _evaluate_simple_expression(parts[0].strip(), namespace)
157-
right = _evaluate_simple_expression(parts[1].strip(), namespace)
158-
return bool(left) and bool(right)
159-
153+
# Boolean operators — parse 'or' first (lower precedence) so that
154+
# 'a or b and c' is evaluated as 'a or (b and c)'.
160155
if " or " in expr:
161156
parts = expr.split(" or ", 1)
162157
left = _evaluate_simple_expression(parts[0].strip(), namespace)
163158
right = _evaluate_simple_expression(parts[1].strip(), namespace)
164159
return bool(left) or bool(right)
165160

161+
if " and " in expr:
162+
parts = expr.split(" and ", 1)
163+
left = _evaluate_simple_expression(parts[0].strip(), namespace)
164+
right = _evaluate_simple_expression(parts[1].strip(), namespace)
165+
return bool(left) and bool(right)
166+
166167
if expr.startswith("not "):
167168
inner = _evaluate_simple_expression(expr[4:].strip(), namespace)
168169
return not bool(inner)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
2727
results.append(step_data.get("output", {}))
2828

2929
# Resolve output expressions with fan_in in context
30+
prev_fan_in = getattr(context, "fan_in", None)
3031
context.fan_in = {"results": results}
3132
resolved_output: dict[str, Any] = {"results": results}
3233

@@ -36,6 +37,9 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
3637
else:
3738
resolved_output[key] = expr
3839

40+
# Restore previous fan_in state
41+
context.fan_in = prev_fan_in
42+
3943
return StepResult(
4044
status=StepStatus.COMPLETED,
4145
output=resolved_output,

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,11 @@ def validate(self, config: dict[str, Any]) -> list[str]:
103103
f"Gate step {config.get('id', '?')!r}: 'on_reject' must be "
104104
f"'abort', 'skip', or 'retry'."
105105
)
106+
if on_reject in ("abort", "retry") and isinstance(options, list):
107+
reject_choices = {"reject", "abort"}
108+
if not any(o.lower() in reject_choices for o in options):
109+
errors.append(
110+
f"Gate step {config.get('id', '?')!r}: on_reject={on_reject!r} "
111+
f"but options has no 'reject' or 'abort' choice."
112+
)
106113
return errors

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
2525

2626
cwd = context.project_root or "."
2727

28+
# NOTE: shell=True is required to support pipes, redirects, and
29+
# multi-command expressions in workflow YAML. Workflow authors
30+
# control commands; catalog-installed workflows should be reviewed
31+
# before use (see PUBLISHING.md for security guidance).
2832
try:
2933
proc = subprocess.run(
3034
run_cmd,

workflows/ARCHITECTURE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ flowchart TD
2323
H -- "if" --> L["IfThenStep.execute()"]
2424
H -- switch --> M["SwitchStep.execute()"]
2525
H -- "while/do-while" --> N["Loop steps"]
26-
H -- "fan-out/fan-in" --> O["Parallel steps"]
26+
H -- "fan-out/fan-in" --> O["Fan-out/fan-in"]
2727
2828
I --> P{Result status?}
2929
J --> P
@@ -198,7 +198,7 @@ src/specify_cli/
198198
│ ├── switch/ # Multi-branch dispatch
199199
│ ├── while_loop/ # While loop
200200
│ ├── do_while/ # Do-while loop
201-
│ ├── fan_out/ # Parallel dispatch
201+
│ ├── fan_out/ # Sequential per-item dispatch
202202
│ └── fan_in/ # Result aggregation
203203
└── __init__.py # CLI commands: specify workflow run/resume/status/
204204
# list/add/remove/search/info,

workflows/README.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,10 @@ For detailed architecture and internals, see [ARCHITECTURE.md](ARCHITECTURE.md).
3232
specify workflow search
3333

3434
# Install a workflow from the catalog
35-
specify workflow add sdd-pipeline
35+
specify workflow add speckit
3636

3737
# Run a workflow with inputs
38-
specify workflow run sdd-pipeline --input feature_name="user-auth"
38+
specify workflow run speckit --input feature_name="user-auth"
3939

4040
# Check run status
4141
specify workflow status
@@ -44,19 +44,19 @@ specify workflow status
4444
specify workflow resume <run_id>
4545

4646
# Get detailed workflow info
47-
specify workflow info sdd-pipeline
47+
specify workflow info speckit
4848

4949
# Remove a workflow
50-
specify workflow remove sdd-pipeline
50+
specify workflow remove speckit
5151
```
5252

5353
## Running Workflows
5454

5555
### From an Installed Workflow
5656

5757
```bash
58-
specify workflow add sdd-pipeline
59-
specify workflow run sdd-pipeline --input feature_name="user-auth"
58+
specify workflow add speckit
59+
specify workflow run speckit --input feature_name="user-auth"
6060
```
6161

6262
### From a Local YAML File
@@ -68,7 +68,7 @@ specify workflow run ./my-workflow.yml --input feature_name="user-auth"
6868
### Multiple Inputs
6969

7070
```bash
71-
specify workflow run sdd-pipeline \
71+
specify workflow run speckit \
7272
--input feature_name="user-auth" \
7373
--input scope="backend-only"
7474
```
@@ -193,7 +193,7 @@ Execute steps at least once, then repeat while condition holds:
193193

194194
### Fan-Out Steps
195195

196-
Parallel dispatch over a collection:
196+
Dispatch a step template for each item in a collection (sequential):
197197

198198
```yaml
199199
- id: parallel-impl
@@ -207,7 +207,7 @@ Parallel dispatch over a collection:
207207

208208
### Fan-In Steps
209209

210-
Aggregate results from parallel steps:
210+
Aggregate results from fan-out steps:
211211

212212
```yaml
213213
- id: collect

0 commit comments

Comments
 (0)