ADR-310: Concurrent task scheduler, concurrent-orchestrate skill, and /implement multi-task dispatch - #81
Conversation
…TDD (closure computation for "up to" targets, explicit-list upfront validation, repo-wide concurrency-cap enforcement across multiple target files, waiting/complete/blocked status computation)
…current_schedule.py to isolated workflow-orchestrate runs)
…rget and an explicit multi-task list, dispatching either to concurrent-orchestrate
…schema (default 3), documented alongside testing.test-file-patterns
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
There was a problem hiding this comment.
Review: ADR-310 — Concurrent scheduler, /implement argument parser, concurrent-orchestrate
Reviewed against _spec_ConcurrentDevelopment.md's exit criteria (lines 826-851), CONTRIBUTING.md, and the existing task_readiness.py/task_dependencies.py/dev_team.py conventions this PR builds on.
Exit criteria — all satisfied:
/implementrecognizes a single key (unchanged, byte-for-byte identical step-2 body), the case-insensitiveup to <key>phrase, and an explicit comma/"and"-separated list of two-or-more keys; either multi-item form dispatches toconcurrent-orchestrate.concurrent_schedule.py'scompute_next_batch(target)computes the "up to" closure or takes the explicit list as-is, persists to~/.dev-team/<repo-slug>/concurrent-<target-slug>.json.- Explicit-list upfront validation (
_validate_explicit_list) rejects a dependency neither in the list nor alreadydone, naming the offending task/dependency, before anything is persisted or spawned. - Concurrency cap is enforced repo-wide (
_repo_wide_active_spawn_countscans everyconcurrent-*.jsonunder the state dir, excluding terminal spawns) — confirmed correct via the multi-target-file tests. get-project-configurationgainsconcurrency.max-parallel-tasks(default 3), documented consistently with the existingtesting.test-file-patternsstyle;_max_parallel_tasksdefensively converts the merged config's string scalar toint, with a fallback — a real bug this test suite would have caught if the conversion were missing."complete"/"blocked"/"waiting"semantics match the spec precisely, including the two blocked sub-cases (immediate vs. deferred-until-active-spawns-reach-terminal) — both explicitly tested.concurrent-orchestrate/SKILL.mdis a thin, spec-faithful spawn loop: pre-populatesbase_branchonly when notNone, spawns withisolation: "worktree", records worktree path/branch, re-invokes every 60s or on spawn completion, stops on"blocked"/"complete".
Testing — 17 new unit tests in test_concurrent_schedule.py, following the established test_<scenario>_<expected_result> / AAA / one-Test<Behavior>-class-per-scenario-group conventions (mirrors test_task_readiness.py closely). Ran the full suite locally: all 17 new tests pass, the full workflow-orchestrate/scripts suite (182 tests) and get-project-configuration/scripts suite (81 tests) both still pass, and scripts/validate.sh is clean.
Documentation — no _doc_*.md update needed for this task; the spec itself defers _doc_ConcurrentDevelopment.md authorship to "the final documentation task once implementation completes" (not this task).
One minor, non-blocking fault-tolerance note left inline. Also noting for the record (style only, not blocking): concurrent-orchestrate/SKILL.md's Agent(subagent_type: "claude", isolation: "worktree", ...) example uses colon-style key syntax, where the sibling workflow-orchestrate/SKILL.md uses subagent_type="claude" (equals-sign). Purely cosmetic, doesn't affect the instructions' meaning.
Overall: solid, well-tested implementation that matches the spec's interfaces and design decisions closely, including sensible resolutions to all five "Known ambiguities" the brief flagged. Approved.
…n scripts/ folder Addresses PR #81 review comment: concurrent-orchestrate shouldn't reach into workflow-orchestrate's scripts/ folder for its own script. Moved concurrent_schedule.py and test_concurrent_schedule.py into plugins/dev-team/skills/concurrent-orchestrate/scripts/, updated the script's internal sys.path insertion to point at the sibling workflow-orchestrate/scripts/ directory for its shared-module imports (dev_team, get_context_path, task_dependencies, task_readiness), and updated concurrent-orchestrate/SKILL.md's invocation path to <skill-dir>/scripts/concurrent_schedule.py.
Addresses PR #81 review comment: without an explicit stop, the agent could fall through from step 2 (single-task workflow-orchestrate dispatch) into step 3 (concurrent-orchestrate) and try to invoke a list of tasks it doesn't have. Mirrors the existing 'Then stop.' already used in step 1's no-match branch.
…ent-orchestrate Addresses PR #81 review comment: the general-purpose get-project-configuration skill doc shouldn't make every agent that reads project configuration think about task concurrency. Removed the concurrency.max-parallel-tasks subsection from get-project-configuration/SKILL.md and added it to concurrent-orchestrate/ SKILL.md instead, next to concurrent_schedule.py where the key is actually consumed. The schema/default itself stays unchanged in get-project-configuration/assets/default-config.yaml, since merge_config.py's structural merge still needs it there.
… its docstring Addresses non-blocking self-review comment: main()'s docstring promises a clean 'Error: ...' message on 'any other failure', but the except tuple didn't cover json.JSONDecodeError (a corrupted concurrent-*.json data file) or subprocess.TimeoutExpired (a merge_config.py subprocess timeout inside _max_parallel_tasks) — either would still exit non-zero but with a raw traceback. Added both to the except tuple. Added two new failing-first unit tests (test_main_corrupted_data_file_prints_clean_error_and_exits_nonzero, test_main_merge_config_subprocess_timeout_prints_clean_error_and_exits_nonzero) that exercise main() directly (not via subprocess) with monkeypatched sys.argv, confirmed red before the fix, green after.
Work item
ADR-310: Implement the concurrent scheduler (
concurrent_schedule.py), the extended/implementargument parser, and the newconcurrent-orchestrateskill — together these let a human queue up multiple dependency-ordered task-work-items (an inclusive "up to" target or an explicit list) and have them run as parallelworkflow-orchestratepipelines, respecting a repo-wide concurrency cap, without any single-task pipeline internals changing.Changes
plugins/dev-team/skills/workflow-orchestrate/scripts/concurrent_schedule.py(new) —compute_next_batch(target) -> dictcomputes the "up to" target's transitive dependency closure (or takes an explicit list as-is, no expansion), validates an explicit list's dependencies upfront (raisingConcurrentScheduleErrornaming the offending task/dependency), persists a per-target data file at~/.dev-team/<repo-slug>/concurrent-<target-slug>.json, refreshes each task's status via the existing Task readiness checker (dependency_status/is_task_eligible), enforces the concurrency cap repo-wide (scanning everyconcurrent-*.jsonfile under the state dir), and returns"complete"/"blocked"(namingblocked_tasks) /"waiting".main()is a thinargparseCLI wrapper (--up-to <key>/--list <key1,key2,...>, mutually exclusive).plugins/dev-team/skills/workflow-orchestrate/scripts/test_concurrent_schedule.py(new) — 17 unit tests covering closure computation, list-mode no-expansion, upfront list validation (reject/accept),"complete","blocked"(immediate and deferred-until-active-spawns-reach-terminal), repo-wide cap enforcement (including across multiple target files),max-parallel-tasksdefault/override, and CLI wrapper happy/error paths.plugins/dev-team/skills/concurrent-orchestrate/SKILL.md(new) — sibling Orchestrator skill toworkflow-orchestrate: a thin loop that invokesconcurrent_schedule.py, pre-populates a spawned task'sbase_branchviause-context-fileonly when it isn'tNone, spawns each newly-eligible task as a backgrounded, isolatedworkflow-orchestraterun (isolation: "worktree"), records the returned worktree path/branch back into that task's context file, re-invokes the script every 60 seconds or on spawn completion, and stops/reports on"complete"or"blocked".plugins/dev-team/commands/implement.md(modified) — step 1 now recognizes, in order: the case-insensitiveup to <key>phrase (dispatches toconcurrent-orchestratewith--target-mode up-to), two-or-more comma/"and"-separated keys (dispatches with--target-mode list), or a single key (unchanged dispatch toworkflow-orchestrate).plugins/dev-team/skills/get-project-configuration/assets/default-config.yaml(modified) — addedconcurrency: max-parallel-tasks: 3.plugins/dev-team/skills/get-project-configuration/SKILL.md(modified) — documented the newconcurrency.max-parallel-taskskey, following the existingtesting.test-file-patternsstyle.Design decisions
concurrent_schedule.pydirectly via TDD (test-first, confirmed red, then green) rather than the heavierimplement-tddsub-agent trio, given the component's moderate size.workflow-orchestrate/ensure-working-branchprecedents. The two Given/When/Then exit criteria are covered byconcurrent_schedule.py's own unit tests instead.concurrent_schedule.py:argparsewith a mutually-exclusive--up-to/--listgroup; the spec file is located viadev_team.find_spec_file(), never passed on the CLI.concurrent-orchestratechosen as 60 seconds (not specified in the spec)._max_parallel_tasksexplicitly converts the configured value tointwith a fallback to the default, sincemerge_config.py's minimal YAML parser returns scalars as strings.isolation:"worktree"parent-HEAD-leak risk (Claude Code issue #55708) is accepted as residual, out-of-scope risk, consistent with the spec's own risk write-up.Testing completed
plugins/dev-team/skills/workflow-orchestrate/scripts/test_concurrent_schedule.py: 17 new tests, all passing.workflow-orchestrate/scriptssuite (182 tests) andget-project-configuration/scriptssuite (81 tests) both still pass unmodified.