ADR-313: Add dev-team:watch-pr post-hand-off PR monitor and /watch-pr command - #84
Merged
Merged
Conversation
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).
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).
…r 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.
…iew) 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.
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
commented
Jul 23, 2026
jodavis-claude
left a comment
jodavis-claude
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed against the ADR-313 task brief's exit criteria and _spec_ConcurrentDevelopment.md. The three components (dev-team:watch-pr, /watch-pr, the concurrent-orchestrate auto-start hook) implement all listed exit criteria and match the spec's Data Flow/Interfaces sections closely — spawn shapes mirror FixPrStep's and workflow-orchestrate's existing patterns, rebase_onto()/watch_pr_poll.py/pr_event_detector.py are called with correct signatures and left unmodified, and the event-ordering logic (task_merged short-circuit, dependency_merged re-target before base_updated, review_comment/ci_failure always handled before returning to poll) is sound. CI passes; diff is scoped to exactly the three files described. No production Python was touched, consistent with the brief's "no unit tests" note.
A few fault-tolerance gaps worth tightening before merge (see inline comments), plus one low-priority doc-consistency nit.
…e/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.
…moval 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.
…d 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.
… 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.
jodavis-claude
commented
Jul 23, 2026
jodavis-claude
left a comment
jodavis-claude
left a comment
Collaborator
Author
There was a problem hiding this comment.
Sign-off review: all 4 prior review threads verified resolved.
- watch-pr SKILL.md:113 (step 3 worktree/branch removal) - hard-stop language added, matches convention. Resolved.
- watch-pr SKILL.md:161 (step 4b bullet 0 own-worktree removal on task_merged) - success report now conditional on both removal commands succeeding; non-zero exit is now a hard stop. Resolved.
- concurrent-orchestrate SKILL.md:143 (empty pr_url fallback) - explicit skip-and-report-and-record-anyway behavior added. Resolved.
- watch-pr SKILL.md:10 (argument-hint style) - now
--work-item-id <work-item-id>, matching both call sites. Resolved.
Scanned both modified files (watch-pr/SKILL.md, concurrent-orchestrate/SKILL.md) in full for new issues introduced by the fix commits (734a5b5, b344a80, ee1bc99, 22c758c) - none found. No production Python was touched by this fix pass, consistent with the developer's summary. Approving sign-off.
jodavis-claude
marked this pull request as ready for review
July 23, 2026 15:07
jodavis
requested changes
Jul 23, 2026
jodavis
approved these changes
Jul 24, 2026
jodavis
pushed a commit
that referenced
this pull request
Jul 24, 2026
* 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>
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.
Work item
ADR-313— builddev-team:watch-pr, the long-lived, user-started post-hand-off PR monitorskill, plus
/watch-pr, its manual entry point, so that once a task's PR is handed off to ahuman reviewer something keeps it in sync (rebasing on a moved base or a merged dependency,
fixing review comments and CI failures) until it merges, without any human polling it by hand.
Depends on ADR-309, ADR-311, ADR-312, and ADR-335, all already merged on
feature/ADR-296-concurrency.Changes
plugins/dev-team/skills/watch-pr/SKILL.md— new.dev-team:watch-pr, the long-livedpost-hand-off PR monitor (Orchestrator). Runs the worktree-freshness check (copied verbatim
from
ensure-working-branch), resolves the context file,git fetch origin && git checkout <working_branch>, removes the implement-phase worktree/branch and recordswatch_worktree_path/watch_worktree_branch, then loops onwatch_pr_poll.py, reacting toevery fired event (
task_mergedshort-circuits everything else; otherwise rebase-relatedevents first via
rebase_mechanic.rebase_onto(), thenreview_comment/ci_failurespawnfix-pr). On a rebase conflict, spawns the developer agent (nested, no isolation) to runresolve-rebase-conflict, then pushes--force-with-leaseon"resolved"or runsgit rebase --abortand stops itself on"unresolved"(noAskUserQuestionfallback).plugins/dev-team/commands/watch-pr.md— new. Thin/watch-pr <task-key>command (Wrapper),same shape as
implement.md's single-task dispatch branch: spawnsdev-team:watch-prvia theAgenttool withisolation: "worktree".plugins/dev-team/skills/concurrent-orchestrate/SKILL.md— modified. Added step 1e: thedev-team:watch-prauto-start hook. Step 1d's existing "any spawned pipeline finishes" triggernow also runs 1e, which keeps an in-session (not persisted) record of task_ids already given a
monitor, and for each newly-finished-successfully task_id, spawns
dev-team:watch-pras alocal background
Agent(isolation: "worktree",run_in_background: true), mirroring step1c's own
workflow-orchestratespawn pattern.watch_pr_poll.py,pr_event_detector.py,rebase_mechanic.py,resolve-rebase-conflict, andfix-prare all reused unmodified from ADR-309/ADR-311/ADR-312.Design decisions
concurrent-orchestratecorrelates a finished background pipeline to itstask_idby keepingan in-session record of task_ids already given a monitor, and sanity-checks that task's own
context file
pr_urlfield before spawningdev-team:watch-pr— no new persisted bookkeeping./implement <key>run still gets no monitor unless the user runs/watch-prmanually afterward — auto-start is attributed solely toconcurrent-orchestrate,per the spec.
git worktree remove <path> --force+git branch -D <branch>for both the implement-phase removal and the monitor's own self-removal on
task_merged.dev-team:watch-pris spawned via a bareAgent(subagent_type: "claude", isolation: "worktree", ...)call with no dedicated agent persona file, matching every other prose-onlyOrchestrator skill in this repo.
fix-pr/rebase-conflict spawns write to distinctly-named sections(
Post-Handoff Fix <n>,Rebase Conflict <n>, in-session counters) rather than reusingdev_team.py'swork_summaries-basedFix Nnumbering, sincedev-team:watch-prnever runsthrough that state machine.
task_mergedshort-circuits every other event ina pass, and
review_comment/ci_failureare always handled before returning to the poll, evenafter a rebase-related event's own conflict-resolution detour concludes — otherwise an
already-fired review/CI event could be silently dropped since
pr_event_detector.pymarksevents "seen" the instant they're detected, not when acted on.
Testing completed
No production Python files were touched (
watch_pr_poll.py,pr_event_detector.py, andrebase_mechanic.pyare all reused unmodified). This task's three components(
dev-team:watch-pr,/watch-pr, and theconcurrent-orchestrateextension) areWrapper/Orchestrator-tier agent-skill prose with no existing E2E/Gherkin harness in this repo and
no precedent of one being added for equivalent prior work (ADR-310's
concurrent-orchestrateand its
/implementextension were each a single prose-only commit with no test file). Permissing-test-harness, no new test harness was invented; verification is via the task brief'sown Given/When/Then exit-criteria scenarios plus code review, not new automated coverage.