Working PR for ADR-296: Concurrent development - #78
Merged
Conversation
…rip (#63) * ADR-335: PipelineContext frontmatter round-trip — preserve unrecognized keys save()/load() now read/write the full YAML frontmatter block as a dict via a new extra_frontmatter field, so unrecognized keys (working_branch, base_branch, parent_work_item, and future fields) survive every dev_team.py invocation unchanged instead of being silently dropped. Handles both scalar and multi-line list-valued unknown keys. * ADR-335: implement PipelineContext via TDD (save()/load() have full coverage — all known frontmatter fields, extra_frontmatter (scalar+list), every known body section (including Project Configuration ordering and numbered Fix sections), the write-only Logs section, and started/last_updated parse-failure fallback (missing key and malformed value); 23/23 tests passing) * ADR-335: wire dev_team.py to import PipelineContext from its new module Removes the now-dead inline PipelineContext class and KNOWN_FRONTMATTER_KEYS set from dev_team.py; dev_team.py now imports PipelineContext from pipeline_context.py so existing `from dev_team import PipelineContext` call sites keep working unchanged. Kept dev_team.py's own _parse_frontmatter/_parse_sections helpers in place since they're still used by unrelated code. Also fixes a bug in pipeline_context.py's load(): its hand-rolled frontmatter parser treated any empty-value scalar key (e.g. `spec_path: `) as the start of a YAML list, returning [] instead of "". This wasn't caught by the component's own tests (which only round-tripped non-empty scalar values) but broke test_dev_team.py once dev_team.py started using the shared implementation. Added a red test demonstrating the round-trip failure, then fixed the parser to only convert to a list once an actual '- ' item line follows, matching dev_team.py's existing _parse_frontmatter semantics. * ADR-335: derive KNOWN_FRONTMATTER_KEYS from PipelineContext field metadata Addresses PR #63 review feedback worried about the hand-maintained frontmatter-key list drifting as fields are added. Tags each frontmatter-backed field with metadata={"frontmatter": True} inline in the dataclass definition, then derives KNOWN_FRONTMATTER_KEYS via dataclasses.fields() instead of a separately maintained set literal. Adding a new frontmatter field now only requires touching its field() call, not a second list elsewhere in the module. Verified the derived set is identical to the prior hand-written one; all 135 existing tests (pipeline_context + dev_team) stay green. * ADR-335: restore save() parent directory creation PipelineContext.save() no longer called path.parent.mkdir(parents=True, exist_ok=True) before write_text() after the TDD rebuild, reintroducing an unhandled FileNotFoundError when saving to a path whose parent doesn't already exist. This was never test-covered before or after the rebuild (every prior test wrote to tmp_path, which already exists). Restored the mkdir call and added test_save_to_path_with_missing_parent_directory_creates_it_and_writes_file to pin down the behavior going forward. * ADR-335: restore last_updated bump on save() PipelineContext.save() no longer set self.last_updated = datetime.datetime.now() at the top of the method after the TDD rebuild, so last_updated stayed frozen at construction time instead of tracking "last saved" as it did before the rebuild. No test pinned this down in either version. Restored the bump and added test_save_updates_last_updated_to_current_time_on_each_call to cover it. * ADR-335: restore save() markdown heading Cosmetic fix from PR #63 review: the pre-rebuild save() wrote a "# {work_item_id} Dev Team Context" heading right after the frontmatter block. Nothing parses it back (load() never reads it), but dev_team.py's fresh-context branch creates a file directly through PipelineContext.save() without going through init-context-file.py's template, so a context file created that way was missing the heading after the rebuild. Restored the heading and added test_save_writes_work_item_id_heading_after_frontmatter. * ADR-335: Remove stale PipelineContext comment in dev_team.py Remove the inaccurate comment above _parse_sections claiming PipelineContext is "re-exported here for backward compatibility" -- PipelineContext was never exported from dev_team.py before the extraction, so there is no such compatibility concern. The "Pipeline context (serializable state)" section divider is removed too since PipelineContext no longer lives in this file at all, and the only function under that header (_parse_sections) is an unrelated body-parsing helper used by dev_team.py's own Step-class logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011oezrcK3g7maGFvxDkXALv * ADR-335: Remove PipelineContext round-trip tests duplicated in test_pipeline_context.py Delete two test classes from test_dev_team.py whose entire purpose is exercising PipelineContext's own save()/load() persistence, now fully covered by test_pipeline_context.py: - TestExtraFrontmatterRoundTrip: both its scalar/list-valued unknown-key round-trip test and its prepopulated extra_frontmatter test are byte-for-byte duplicated by TestPipelineContextExtraFrontmatter in test_pipeline_context.py. - TestTroubleshooterInput: its default-value, non-empty round-trip, and empty-string round-trip assertions are all covered generically by TestPipelineContextDefaults and TestPipelineContextSaveLoadRoundTrip in test_pipeline_context.py (troubleshooter_input is included in both the known-scalar-fields and default-empty-string-fields parametrized coverage there). Other classes that construct a PipelineContext (TestSignoffCycleCount, TestReviewCycleCount, TestConsecutiveFailures, TestSignoffBuildResult, TestReviewStepPrUrlExtraction, and the various Step-class test classes) were left untouched -- each of those primarily exercises dev_team.py's own logic (_apply_counter_updates, _handle_agent_failure/_success, ReviewStep's removed PR-URL-section fallback, Step class instantiation) and is not a genuine duplicate of test_pipeline_context.py's coverage. Full regression: 105/105 test_dev_team.py tests and 27/27 test_pipeline_context.py tests pass (132 total, down from 138 before this cleanup, reflecting the 6 removed duplicate tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011oezrcK3g7maGFvxDkXALv --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* ADR-307: Add Depends on task field to spec-task-breakdown Step 1 authors a **Depends on:** line under each task's title using local task-number references (or - none -), inferred best-effort the same way titles/descriptions already are. Step 5 rewrites those references into real task-work-item keys alongside the existing title rewrite, then validates the whole Tasks section by invoking the new task_dependencies.py script and surfacing any dangling-reference or cycle error to the user. * ADR-307: implement Task dependency graph parser via TDD (parse_task_dependencies covers no-Depends-on-line, explicit — none —, single/multiple dependencies, dangling reference, and two/three-task cycles; all 8 tests passing) * ADR-307: quote script path/arg in task_dependencies.py invocation for consistency with other SKILL.md script invocations * ADR-307: anchor task_dependencies.py invocation to repo root via git rev-parse --show-toplevel instead of an implicit repo-root CWD * ADR-307: anchor task_dependencies.py invocation to <skill-dir> instead of repo root Reviewer (jodavis) noted the repo-root-based path assumes this skill is always run inside the agent-plugins repo, which won't hold once it's used as an installed plugin elsewhere. Step 5 now resolves task_dependencies.py relative to spec-task-breakdown's own <skill-dir> instead of shelling out to git rev-parse --show-toplevel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzenM31PAQmpj6ERNZjpTY * ADR-307: link Depends on tracked work items via createIssueLink Reviewer (jodavis) asked that, after validating the dependency tree, step 5 also record each Depends on relationship in the tracker so it's visible there too, not just in the spec. For Jira, this uses the Blocks link type with the dependency as the blocker; documented as a new createIssueLink operation on work-with-Jira-tasks. Skipped for a provider whose adapter skill doesn't document a linking operation, or when no tracker is configured, matching the rest of this skill's no-tracker fallback pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzenM31PAQmpj6ERNZjpTY --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…lver (#70) * ADR-308: implement Task readiness checker via TDD (dependency_status covers no-context-file (not_started), state=done, state=failed, pr_url-implies-ready with non-terminal state, and the in_progress fallback; is_task_eligible covers empty dependency_ids, single-dependency ready/in_progress/not_started, all-done (2+), all-but-one-done with ready-branch substitution, 2+ short of done (waiting), and a failed dependency short-circuiting to blocked — all 13 tests green, every branch of both functions exercised) * ADR-308: extend ensure-working-branch with dependency-aware base-branch resolution and worktree-freshness check - ensure-working-branch gains a new first action (before all other steps): the worktree-freshness hard-stop check (git stash list / git status --short both must be empty), guarding against the confirmed upstream isolation:"worktree" collision bug. - New sub-step 4b (after the existing spec-file lookup) reads this task's own Depends on: entries via task_dependencies.py and calls task_readiness.py's is_task_eligible via a new CLI wrapper; uses the returned base_branch when eligible with a real branch, falls through to the unchanged feature-branch lookup when eligible with no override or no dependencies, and hard-stops with a clear error for waiting/blocked (resolving the brief's ambiguity #2 conservatively: never silently start a working branch without its actual dependency). - Adds task_readiness.py's missing main() CLI wrapper (JSON on stdout, Error: ... on stderr) so the prose skill can invoke it via Bash, plus one narrow integration test for the primary happy-path CLI scenario. - All existing steps renumbered (3a-3e -> 4a,4c-4f) with only additive content otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzenM31PAQmpj6ERNZjpTY * ADR-308: add missing python prefix to task_dependencies.py invocation in ensure-working-branch/SKILL.md 4b.1 Sub-step 4b.1 invoked task_dependencies.py as a bare path via Bash; since the script is not chmod +x, this fails with Permission denied (exit 126). Adding the python interpreter prefix, matching every other SKILL.md's invocation convention, fixes this (verified: python3 <path> succeeds against a non-executable file). Addresses PR #70 review thread 3590232708. * ADR-308: add missing python prefix to task_readiness.py invocation in ensure-working-branch/SKILL.md 4b.3 Sub-step 4b.3 invoked task_readiness.py as a bare path via Bash; since the script is not chmod +x, this fails with Permission denied (exit 126). Adding the python interpreter prefix, matching every other SKILL.md's invocation convention, fixes this (verified: python3 <path> succeeds against a non-executable file, printing the expected JSON). Addresses PR #70 review thread 3590232714. * ADR-308: document non-zero-exit error path for 4b's script invocations Sub-step 4b.1/4b.3 described only the success path (JSON on stdout) and never said what to do if task_dependencies.py or task_readiness.py exits non-zero (both print a clear 'Error: ...' message to stderr on failure, per their main() implementations). Added the 'surface the error and stop' instruction to both sub-steps, matching the existing convention elsewhere in the repo. Addresses PR #70 review thread 3590232713. * ADR-308: extract _dependency_status_and_context() to dedupe is_task_eligible's repeated dependency lookup is_task_eligible's single-still-open-dependency branch re-derived get_repo_slug() and reloaded the winning dependency's context file that dependency_status() (via the new shared helper) had already loaded moments earlier in the same call. Extracted _dependency_status_and_context() so the PipelineContext is loaded once per dependency and reused for the working_branch lookup; dependency_status() now delegates to it and discards the context. No public signature or behavior changed — this is a pure extract-method refactor. Verification note: the existing 14 tests in test_task_readiness.py were manually traced case-by-case against this refactor (all return the same tuples for the same inputs); an automated pytest run could not be completed in this session due to a Bash-tool-classifier outage that also blocked 'python3 -c ...'/'python3 -m ...' invocations throughout this session (git/gh commands were unaffected). This should be re-run before merge to confirm. Addresses PR #70 review thread 3590232718 (P4/non-blocking). --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…96-concurrency # Conflicts: # plugins/dev-team/skills/ensure-working-branch/SKILL.md # plugins/dev-team/skills/workflow-orchestrate/scripts/dev_team.py
Merge origin/main into feature/ADR-296-concurrency
build-and-test: Python test resultsStatus: ✅ Passed Test log |
`rebase_onto` fully covered — clean rebase (returns "rebased", verified force-pushed-with-lease reaches origin), genuine conflict (returns "conflict", rebase-in-progress marker left in place, no push to origin), fetch making `origin/<new_base>` the rebase source of truth over a stale local ref, self-checkout of `working_branch` when the worktree starts on a different branch, and non-conflict git failures (fetch) propagating as `CalledProcessError` rather than being swallowed. No logging, no CLI wrapper, and no additional invalid-input branches in scope for this component. Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
… /implement multi-task dispatch (#81) * ADR-310: implement Concurrent scheduler (concurrent_schedule.py) via 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) * ADR-310: add concurrent-orchestrate skill (thin spawn loop wiring concurrent_schedule.py to isolated workflow-orchestrate runs) * ADR-310: extend /implement argument parser to recognize an "up to" target and an explicit multi-task list, dispatching either to concurrent-orchestrate * ADR-310: add concurrency.max-parallel-tasks to project configuration schema (default 3), documented alongside testing.test-file-patterns * ADR-310: move concurrent_schedule.py into concurrent-orchestrate's own 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. * ADR-310: add explicit stop after /implement's single-task dispatch step 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. * ADR-310: move concurrency.max-parallel-tasks documentation to concurrent-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. * ADR-310: widen concurrent_schedule.py main()'s except clause to match 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. --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
* ADR-311: implement PR event detector via TDD (`detect_pr_events` fully covered — all five event types (review_comment, ci_failure, base_updated, dependency_merged, task_merged) firing individually and in combination, no-refire guards for review_comment/ci_failure/dependency_merged, the quiet-call no-op case (with unchanged saved context), the stacked out-of-order dependency_merged branch-reporting case, early-return guards (no context file / no pr_url), and all boundary/error inputs from the exit criteria (non-JSON checks output, missing base_branch, missing/unparseable spec_path, dependency-less task, dependency with no context file or no pr_url, and a failed git ls-remote lookup)) * ADR-311: implement watch_pr_poll.py via TDD (`watch_pr_poll.py` fully covered — `poll()`'s immediate-fire branch (single and multiple simultaneous events, no sleep), the loop-until-fired branch (sleeps between empty checks, exits before `max_seconds`), the `max_seconds`-elapsed → `"no_change"` branch, and the `max_seconds=0` boundary (zero sleeps); `main()`'s CLI wrapper for both success shapes (fired-event-list and `"no_change"` JSON) and the exception → `Error: ...`/exit-1 path. All 8 tests green.) * ADR-311: guard unhandled gh JSON parse failures in detect_pr_events Address PR #82 review comment: _pr_state_and_base()'s `gh pr view` call and the review-comments `gh api` call both parsed result.stdout with a bare json.loads(...), unlike the ci_failure check right below which already wraps the same class of gh parse in try/except json.JSONDecodeError. A transient gh failure (rate limit, network blip, auth hiccup) raised an unhandled JSONDecodeError out of detect_pr_events, which would kill watch_pr_poll.py's whole bounded polling window instead of skipping that one check and retrying next iteration. Wrap both parses the same way the CI-checks call already is, so a flaky gh call degrades to "no signal this round" rather than a crash. Added tests: review comments and gh pr view (both the task's own call and a dependency's) returning non-JSON no longer raise, and the corresponding signal is simply skipped for that call. * ADR-311: raise a clear ValueError on a malformed pr_url Address PR #82 review comment: `_PR_URL_RE.match(ctx.pr_url).groups()` raised a confusing `AttributeError: 'NoneType' object has no attribute 'groups'` when ctx.pr_url didn't match the expected `https://github.com/<owner>/<repo>/pull/<number>` shape, instead of a clear diagnostic naming the offending value. Extracted the match into a `_parse_pr_url()` helper that raises `ValueError(f"pr_url does not match expected GitHub PR URL format: {pr_url!r}")` on no match, so detect_pr_events still fails loudly (this isn't a transient gh flake, unlike the JSON parse issue fixed in the previous commit) but with a message a human can act on when this runs unattended inside watch_pr_poll.py's polling loop. Added a test asserting a malformed pr_url raises ValueError naming the URL. * ADR-311: close signoff validation gap in no-refire test coverage Sign-off validation flagged one exit criterion as only partially covered: review_comment, ci_failure, and dependency_merged each had an explicit already-seen/no-refire test, but base_updated and task_merged did not. - Added a base_sha_unchanged_no_refire case to TestDetectPrEventsBaseUpdated: asserts base_updated does not fire when git ls-remote reports the same sha already recorded in base_branch_sha. - Added TestDetectPrEventsTaskMergedCalledAgain: documents that task_merged fires on every call for as long as the PR stays merged, since (per the task brief's stated context-file fields) there is no persisted "already reported" field for it, unlike the other four events. This is an accepted design limitation, not a defect — adding a new field would be scope creep beyond the brief's named extra_frontmatter fields (base_branch_sha, last_seen_review_comment_id, last_seen_ci_conclusion, base_branch). Both new tests pass against the existing implementation unmodified — pr_event_detector.py already behaves correctly for both cases; this commit only closes the test-coverage gap the signoff review found. Full regression: 199 passed (197 pre-existing + 2 new). --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
…rness (#83) resolve-rebase-conflict is agent-skill prose (Testable per component-taxonomy, not a pure function): given a rebase already left in progress with conflicts and the task's own brief/spec context, it reads each conflicted file's hunks, resolves only what the context makes unambiguous, stages them, and drives git rebase --continue to completion. Reports "resolved" or "unresolved"; never runs git push or git rebase --abort itself (the caller, dev-team:watch-pr, does both as appropriate). Verification follows the taxonomy's "whatever mechanism actually fits" for prose Testable components (same precedent as ADR-319's harvest-playbook): a scripted fixture-scenario harness at plugins/dev-team/fixtures/resolve-rebase-conflict/ builds three real git repos with a genuine rebase conflict already in progress (via rebase_mechanic.rebase_onto(), following test_rebase_mechanic.py's bare-origin-plus-clone construction pattern): - single-file: one CHANGELOG.md hunk, resolvable from task context - multi-file: CHANGELOG.md + a JSON settings file conflict together (same commit), both resolvable from task context - unresolvable: a numeric setting both branches changed to different, equally-plausible values with no stated target — must decline to guess build_fixture.py builds each scenario; test_build_fixture.py (18 tests, all passing) confirms each one produces exactly the conflict it claims to. RUN.md documents the on-demand (non-CI) run/grade procedure for the judgment-shaped part of this check. Dry-ran the finished skill against all three scenarios via a fresh subagent blind to this authoring context: single-file and multi-file both correctly merged both sides' content and reported "resolved" with a clean working tree; unresolvable correctly stopped without editing anything and reported "unresolved", citing the missing target value as the reason it couldn't proceed. No production Python file changes needed — rebase_mechanic.py is untouched per the task brief. Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
… command (#84) * ADR-313: add dev-team:watch-pr, the long-lived post-hand-off PR monitor Orchestrator-tier skill wiring together already-built components (watch_pr_poll.py, pr_event_detector.py, rebase_mechanic.py from ADR-311/ADR-309, resolve-rebase-conflict from ADR-312, fix-pr) into the entire post-hand-off lifecycle from hand-off to merge: worktree-freshness check, checkout, implement-phase worktree removal, then a poll loop reacting to review comments/CI failures (spawn fix-pr), base moves/dependency merges (run the rebase mechanic, re-targeting base_branch first for dependency_merged), and task_merged (remove own worktree, halt). On a rebase conflict, spawns the developer agent to run resolve-rebase-conflict; resolved -> push --force-with-lease itself; unresolved -> git rebase --abort then stop the agent (no AskUserQuestion fallback). * ADR-313: add /watch-pr manual fallback command Thin Wrapper command, same shape as implement.md's single-task dispatch branch: its whole job is spawning dev-team:watch-pr via the Agent tool with isolation: "worktree", for when concurrent-orchestrate's auto-start never happened (e.g. its own session was interrupted before this task reached hand-off). * ADR-313: extend concurrent-orchestrate to auto-start dev-team:watch-pr on hand-off Step 1d's existing 'any spawned pipeline finishes' trigger now also runs a new step 1e: for each task_id whose spawned workflow-orchestrate pipeline just finished successfully (reached hand-off) and that doesn't already have a monitor, spawn dev-team:watch-pr as a local background Agent, mirroring step 1c's own spawn pattern for workflow-orchestrate itself. Tracks already-spawned task_ids in an in-session record (not persisted) to avoid double-spawning. The concurrency cap is untouched -- it only ever counted active workflow-orchestrate spawns, never this monitor. * ADR-313: fix event-ordering bug in watch-pr's poll reaction (self-review) task_merged now short-circuits every other event in the same pass (nothing else fired alongside a merge is still actionable against an already-closed PR). Separately, review_comment/ci_failure are no longer skipped when a rebase-related event in the same pass also needs the rebase mechanic (or a conflict resolution) first -- pr_event_detector.py marks those events 'seen' the instant it detects them, so deferring them to the next poll would have silently dropped them instead. They're now always handled before returning to the poll, after the rebase-related work (including any conflict resolution) concludes. * ADR-313: watch-pr step 3 - stop and report if implement-phase worktree/branch removal fails Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:113 - the removal commands had no stated failure behavior, unlike every other command call in this skill. * ADR-313: watch-pr step 4b - stop and report if own worktree/branch removal fails on task_merged Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:159 - the success report was unconditional even though it followed a removal that could fail, risking a failed cleanup being reported as a clean halt. * ADR-313: watch-pr argument-hint - match --work-item-id flag style used by both call sites Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:10 - argument-hint previously read <work-item-id> instead of the flag style both commands/watch-pr.md and concurrent-orchestrate/SKILL.md actually use to invoke this skill. * ADR-313: concurrent-orchestrate step 1e - define fallback when pr_url is empty Addresses PR review comment on plugins/dev-team/skills/concurrent-orchestrate/SKILL.md:143 - the pr_url sanity check had no defined behavior on failure, unlike this spec's consistent never-silently-proceed posture elsewhere. Now skips spawning the monitor for that task_id, reports the inconsistency in detail, and still records the task_id so a later poll doesn't repeatedly re-report it. --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
* ADR-313: add dev-team:watch-pr, the long-lived post-hand-off PR monitor Orchestrator-tier skill wiring together already-built components (watch_pr_poll.py, pr_event_detector.py, rebase_mechanic.py from ADR-311/ADR-309, resolve-rebase-conflict from ADR-312, fix-pr) into the entire post-hand-off lifecycle from hand-off to merge: worktree-freshness check, checkout, implement-phase worktree removal, then a poll loop reacting to review comments/CI failures (spawn fix-pr), base moves/dependency merges (run the rebase mechanic, re-targeting base_branch first for dependency_merged), and task_merged (remove own worktree, halt). On a rebase conflict, spawns the developer agent to run resolve-rebase-conflict; resolved -> push --force-with-lease itself; unresolved -> git rebase --abort then stop the agent (no AskUserQuestion fallback). * ADR-313: add /watch-pr manual fallback command Thin Wrapper command, same shape as implement.md's single-task dispatch branch: its whole job is spawning dev-team:watch-pr via the Agent tool with isolation: "worktree", for when concurrent-orchestrate's auto-start never happened (e.g. its own session was interrupted before this task reached hand-off). * ADR-313: extend concurrent-orchestrate to auto-start dev-team:watch-pr on hand-off Step 1d's existing 'any spawned pipeline finishes' trigger now also runs a new step 1e: for each task_id whose spawned workflow-orchestrate pipeline just finished successfully (reached hand-off) and that doesn't already have a monitor, spawn dev-team:watch-pr as a local background Agent, mirroring step 1c's own spawn pattern for workflow-orchestrate itself. Tracks already-spawned task_ids in an in-session record (not persisted) to avoid double-spawning. The concurrency cap is untouched -- it only ever counted active workflow-orchestrate spawns, never this monitor. * ADR-313: fix event-ordering bug in watch-pr's poll reaction (self-review) task_merged now short-circuits every other event in the same pass (nothing else fired alongside a merge is still actionable against an already-closed PR). Separately, review_comment/ci_failure are no longer skipped when a rebase-related event in the same pass also needs the rebase mechanic (or a conflict resolution) first -- pr_event_detector.py marks those events 'seen' the instant it detects them, so deferring them to the next poll would have silently dropped them instead. They're now always handled before returning to the poll, after the rebase-related work (including any conflict resolution) concludes. * ADR-313: watch-pr step 3 - stop and report if implement-phase worktree/branch removal fails Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:113 - the removal commands had no stated failure behavior, unlike every other command call in this skill. * ADR-313: watch-pr step 4b - stop and report if own worktree/branch removal fails on task_merged Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:159 - the success report was unconditional even though it followed a removal that could fail, risking a failed cleanup being reported as a clean halt. * ADR-313: watch-pr argument-hint - match --work-item-id flag style used by both call sites Addresses PR review comment on plugins/dev-team/skills/watch-pr/SKILL.md:10 - argument-hint previously read <work-item-id> instead of the flag style both commands/watch-pr.md and concurrent-orchestrate/SKILL.md actually use to invoke this skill. * ADR-313: concurrent-orchestrate step 1e - define fallback when pr_url is empty Addresses PR review comment on plugins/dev-team/skills/concurrent-orchestrate/SKILL.md:143 - the pr_url sanity check had no defined behavior on failure, unlike this spec's consistent never-silently-proceed posture elsewhere. Now skips spawning the monitor for that task_id, reports the inconsistency in detail, and still records the task_id so a later poll doesn't repeatedly re-report it. * ADR-313: watch-pr - "already merged" bullet applies regardless of prior run Addresses PR #84 review comment: whether or not there was a prior run of this skill, if the task's PR already merged there is nothing to do. * ADR-313: watch-pr - extract inlined rebase-mechanic invocation into a script Addresses PR #84 review comment: step 4c's inline `python -c "..."` block is replaced by a new CLI wrapper script, run_rebase_mechanic.py, alongside rebase_mechanic.py in workflow-orchestrate/scripts/, with unit test coverage for its success and error paths (rebase_onto itself is already covered by test_rebase_mechanic.py). * ADR-313: watch-pr - remove Todo list monitoring/mirroring support Addresses PR #84 review comment: removed step 2's Monitor()+TodoWrite mirroring of this session's own todo log (unreliable, complicated, and not useful for an unattended background agent). Kept computing the todo log path itself, since nested fix-pr/resolve-rebase-conflict spawns (step 4b/5) still pass it through as workflow-worker's required --todo-log argument; removing that mediated-spawn pattern entirely is tracked separately as ADR-314 and not pre-empted here. * ADR-313: watch-pr - document why a subagent is spawned and how it communicates Addresses PR #84 review comment questioning the subagent spawn in commands/watch-pr.md: confirmed this is intentional (unlike /implement's single-task path, dev-team:watch-pr must own a guaranteed-fresh worktree and blocks for the task's whole hand-off-to-merge lifecycle, so it cannot run inline in the invoking session). Added a "Communicating outward" section to watch-pr/SKILL.md making explicit the two channels this skill already relies on for anything it needs to convey: fix-pr's PR-thread replies for review/CI issues, and a detailed final message on any hard-stop condition, surfaced via the harness's background-task notification and resumable via SendMessage. commands/watch-pr.md now references this section and explains the isolation/blocking rationale for the spawn. * ADR-314: remove todo-log/TodoWrite-mirroring plumbing Removes the todo-log-tailing / TodoWrite-mirroring mechanism from workflow-orchestrate and workflow-worker: it never surfaced well even for a single task's pipeline, and had no clean extension to the concurrent multi-task orchestrator this spec introduces. - workflow-orchestrate/SKILL.md: drop the todo-log-path step and the Monitor/TaskStop-based tailing step, renumber remaining steps, and drop --todo-log from the spawn_agent prompt template. - Delete workflow-orchestrate/scripts/get_todo_log_path.py (no dedicated test file existed). - workflow-worker/SKILL.md: drop the log-redirection step and --todo-log argument, renumber remaining steps. - Delete workflow-worker/scripts/append_todo_log.py (no dedicated test file existed). - developer.md, researcher.md, reviewer.md: remove the "Task tracking" instruction to mirror updates via TodoWrite through the log file. The TodoWrite tool grant in each file's frontmatter is left in place since the exit criteria scope only the instruction text. - watch-pr/SKILL.md: this ADR-313-added skill still depended on the exact plumbing being removed here (its own text explicitly deferred this cleanup to ADR-314); updated to drop get_todo_log_path.py from its script list, remove its own todo-log-path computation step, and drop --todo-log from both nested workflow-worker spawn prompts. Verified via repo-wide grep that no todo_log/TaskStop/--todo-log/ get_todo_log_path/append_todo_log references remain, and the existing workflow-orchestrate test suite (test_dev_team.py, 111 tests) still passes unchanged. --------- Co-authored-by: Claude on behlaf of jodavis <ElwoodMoves@hotmail.com>
jodavis
marked this pull request as ready for review
July 24, 2026 14:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working PR for ADR-296: Concurrent development