Javi, here's my analysis. Both models tackled a genuinely hard problem, and both produced answers that look impressive on the surface but contain serious bugs underneath. Let me break it down. --- ## Qwen3-Coder **The good:** - Process group management is done right — uses `start_new_session=True` on `Popen` and `os.killpg()` for clean subprocess termination. This is the correct Linux/macOS pattern and the other model missed it entirely. - Non-blocking retry backoff via `threading.Timer` — doesn't tie up a worker thread while waiting. - Typed event dataclasses (`WorkflowStartedEvent`, `StepStateChangedEvent`, etc.) give some structure. - Kahn's algorithm for cycle detection is clean and correct. - The CLI matches the spec exactly (`run workflow.json`, `status `, etc.). **The not-so-good:** - **Event type serialization is fatally broken.** `event_to_dict` derives the type by doing `e.__class__.__name__.replace("Event", "").upper()`, which gives `"WORKFLOWSTARTED"`. But `dict_to_event` checks for `"WORKFLOW_STARTED"` (with underscore). These never match. Replay, restart, and status commands — the core value proposition of the engine — are all dead on arrival. - **`EventLog.append()` destroys the log.** It opens a `.tmp` file in append mode, writes one line, fsyncs, then `os.rename(tmp, main)`. On the second call, the `.tmp` file was already renamed away, so a fresh `.tmp` is created with only the new line, and the rename overwrites the entire log. Every event obliterates all previous events. - Retry state transitions are inconsistent. After a failure eligible for retry, the step stays in `RUNNING` state, but `_submit_step` logs a `PENDING → READY` transition. The logged `from_state` doesn't match reality. - `max_workers=128` is wildly high for a local workflow engine. - The `cancel` CLI command creates a *new* engine instance with no running subprocesses — it writes cancellation events to the log but can't actually kill anything running in a different process. - Daemon thread for the main loop means the "workflow continues in background" message on Ctrl+C is misleading; the daemon dies with the process. --- ## Qwen3.5-Opus **The good:** - Significantly better architecture: clean separation into `StepDefinition`, `StepStateTracker`, `StateMachine`, `WorkflowRunner`, and `EventStore`. Each class has a well-defined responsibility. - The `_emit_event` pattern (persist to store, then apply to state machine) is the textbook event-sourcing approach. - `RETRYING` as an explicit step state is a nice addition that makes the state machine more expressive. - Deterministic event IDs via SHA-256 hashing — good for idempotency and replay verification. - DFS-based cycle detection with proper three-color marking is correct. - The exposed reasoning block at the top shows solid architectural thinking before jumping into code. - Reasonable `MAX_WORKERS = 4`. **The not-so-good:** - **Same catastrophic `EventStore.append()` bug** as Qwen3-Coder — temp-file-then-rename with append mode destroys the log on the second write. Both models fell into the same trap. - Uses `subprocess.run()` (blocking) instead of `Popen`. This means there's no handle to the running process, so cancellation *cannot* kill in-flight subprocesses. The `_cancel_event.is_set()` check only fires before or after execution, not during. - No process group management at all — no `start_new_session`, no `os.killpg`. Even if you had a process handle, child processes would survive. - `time.sleep(backoff)` happens directly in the worker thread, blocking a ThreadPoolExecutor slot for up to 60 seconds. With 4 workers and multiple retries, this starves parallelism. - CLI deviates from the spec: `status`, `cancel`, and `replay` all require the workflow JSON file as an extra argument. The spec says just ``. - Cross-process cancel is broken — `cmd_cancel` creates a new `WorkflowRunner` and calls `.cancel()`, which sets a threading.Event on the new (idle) runner, not the one actually executing. --- ## Head-to-head comparison | Aspect | Qwen3-Coder | Qwen3.5-Opus | |---|---|---| | Architecture | Monolithic dict-based state | Clean OOP separation | | Event serialization | Broken (type names don't round-trip) | Works correctly | | Event log persistence | Destroys log on each write | Same bug | | Subprocess management | Good (Popen + process groups) | Weak (subprocess.run, no kill) | | Retry backoff | Non-blocking (Timer) | Blocking (sleep in worker) | | CLI spec compliance | Matches exactly | Requires extra workflow arg | | Cancellation (in-process) | Works (kills process groups) | Partially works (can't kill subprocesses) | | Cancellation (cross-process) | Broken | Broken | | Code readability | Decent, some confusion | Cleaner, better organized | | Cycle detection | Correct (Kahn's) | Correct (DFS 3-color) | --- ## Scores **Qwen3-Coder: 3.5 / 10** The event serialization bug is a showstopper — the code literally cannot reconstruct state from its own log. Combined with the append bug, the two headline features (replay and crash recovery) are both completely non-functional. The subprocess handling is the one area where it clearly outperforms the other model. **Qwen3.5-Opus: 5 / 10** Better architecture, and the event serialization actually works, so a single uninterrupted run would mostly succeed (minus the append bug). But the inability to kill running subprocesses undermines cancellation and timeout handling, and the CLI doesn't match the spec. The blocking backoff is a meaningful design flaw for a parallel engine. Neither solution is close to production-ready. The shared `EventLog.append` bug is particularly ironic given that both models explicitly call out "atomic writes via temp file + rename" as a design highlight — they both implemented the pattern backwards. The correct approach for an append-only log is simply: open in append mode, write, flush, fsync. The temp-file-rename pattern is for *replacing* a file atomically, not appending to one.