Wire --concurrent flag and close concurrent execution gaps - #1036
Conversation
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 3} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. This PR is pure CLI infrastructure: wiring the --concurrent flag through existing entry points and adding concurrent status rendering to egg-pipeline-watch. No agent prompts, pre-fetching, structured output requirements, or LLM API calls involved.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thorough review complete. This is a clean, well-structured PR that correctly wires the --concurrent flag through all the right paths. No blocking issues found.
Verified
OrchClient.create_pipeline()already accepts aconfigparameter — the PR correctly uses it.- Both CLI paths (
orch_cli.pyviaorch_requestandsdlc_cli.pyviaOrchClient) sendconfigas JSON body correctly. - All three callers of
_restart_pipeline()pass the newconfigparameter. --concurrentand--network-modeoperate on different keys in the request body — no overwrite risk.egg-pipeline-watchrendering handles bothdictandstrforms of agent info correctly.- Docstring additions in
orch_cli.pyaccurately reflect commands that already exist from #1031.
Non-blocking suggestions
1. Duplicate config construction in run_issue_mode (sdlc_cli.py)
The expression {"concurrent_execution": True} if concurrent else None is repeated three times (lines 483, 507, 537). Build it once at the top of the function:
def run_issue_mode(client, issue_number, repo=None, concurrent=False):
...
config: dict[str, object] | None = {"concurrent_execution": True} if concurrent else None
...
# Then reuse `config` at lines 507 and 537 instead of rebuilding itLine 507 already reassigns config to the same value it already holds. Line 537 creates a separate config2 variable with the identical expression. Both are unnecessary — just reuse the one defined at line 483.
2. Readability of compact agent rendering (egg-pipeline-watch:139-141)
Lines 139-141 pack a lot into two lines:
state_icons = {"READY": GREEN + "R", "WORKING": CYAN + "W", "BLOCKED": YELLOW + "B", "OBJECTING": RED + "X"}
parts = [f"{state_icons.get(info.get('state', 'WORKING') if isinstance(info, dict) else str(info), DIM + '?')}{RESET}" for _, info in sorted(agents.items())]The second line is ~180 characters with nested .get() calls, isinstance, and a ternary inside an f-string inside a list comprehension. Consider breaking it into a small loop (like _render_concurrent_info already does for the full-mode rendering), or at least extract the key lookup:
def _agent_icon(info, state_icons):
state = info.get("state", "WORKING") if isinstance(info, dict) else str(info)
return f"{state_icons.get(state, DIM + '?')}{RESET}"3. Missing tests
No tests were added for the new concurrent parameter in run_issue_mode, run_local_mode, _restart_pipeline, or cmd_pipeline_create. A basic test verifying that concurrent=True results in config={"concurrent_execution": True} being passed to create_pipeline would catch regressions.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All three suggestions addressed in 3b45548:
All 7519 tests pass, lint clean. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns in the delta. The changes since last review (3b45548) address code quality feedback: DRY config construction, extracted icon helper, and new tests. No agent prompts, pre-fetching, or LLM API changes involved.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta from 3ace31c to 3b45548. All three suggestions from the previous review were properly addressed.
Verified
1. DRY config construction (fixed)
config is now assigned once at line 479 of sdlc_cli.py and reused in all three call sites (lines 491, 515, 544). The redundant re-assignment and config2 variable are both gone. Traced the code to confirm config is never mutated between uses — it's only passed through as a kwarg.
2. Readable agent icon helper (fixed)
_STATE_ICONS dict and _agent_icon() function are extracted to module level in egg-pipeline-watch. The list comprehension is now a clean one-liner:
parts = [_agent_icon(info) for _, info in sorted(agents.items())]Verified that _agent_icon handles all state types correctly: dict with known state, dict with unknown state, dict with no state key (defaults to WORKING), and non-dict values (falls back to ?). The full-mode _render_concurrent_info correctly keeps its own state_colors dict since it maps to colors only (not icons).
3. Tests added (fixed)
6 tests covering _restart_pipeline, run_local_mode, and run_issue_mode with concurrent=True/False. All 6 pass. Tests correctly verify that config={"concurrent_execution": True} (or None) is forwarded to create_pipeline.
No new issues introduced.
— Authored by egg
|
egg review completed. View run logs 11 previous review(s) hidden. |
Summary
PR #1031 added concurrent agent execution infrastructure but left no CLI surface to enable it. This PR closes the activation gaps:
egg-sdlc --concurrent: New flag passesconfig={"concurrent_execution": true}to the orchestrator when creating pipelines in both issue and local mode. Also wired through_restart_pipeline()so restarts preserve the setting.egg-orch pipeline create --concurrent: Same flag on the lower-level CLI, passes config in the API request body.egg-pipeline-watchconcurrent status: Full DAG mode now shows agent readiness states, consensus progress, and message bus stats. Compact mode shows a condensed agent state indicator.docs/guides/sdlc-pipeline.mdnow documents the--concurrentCLI flag with usage examples, in addition to the existing config JSON reference.orch_cli.pydocstring: Added the 4 missing concurrent-mode commands (message send/poll/status, signal readiness).The critical gap (consensus-driven phase advancement) is tracked in #1035.
Issue: #1035
Test plan:
py_compileegg-sdlc -r egg -i <n> --concurrentand verify pipeline is created withconcurrent_execution: truein configegg-orch pipeline create --repo owner/repo --concurrentand verify config in responseegg-pipeline-watchagainst a concurrent pipeline and verify agent readiness / consensus / message stats render