Add Concurrent CI Review Gate - #143
Conversation
30c8e82 to
6daf65b
Compare
eshulman2
added
enhancement
4fc89db to
5dc629f
Compare
22133a4 to
6761477
Compare
70913d3 to
32846a6
Compare
Add pending_ci_event to WorkflowState for buffering CI webhooks that arrive while the workflow is at human_review_gate. Remove the orphaned PR status comments test file. Note: this change is not backward compatible with in-flight workflows checkpointed at wait_for_ci_gate. Those workflows will need to be manually restarted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
…tion Merge wait_for_ci_gate into human_review_gate so CI checks and human review run concurrently instead of sequentially. human_review_gate now handles initial-entry housekeeping (posting status comments, setting review labels) and routes pending CI events to ci_evaluator. Add Phase 0 CI attribution check so ci_evaluator can distinguish build-breaking changes from pre-existing failures before attempting fixes. Fix double-increment of ci_fix_attempt in attempt_ci_fix. Remove the now-unused wait_for_ci_gate node and clean up stale references across implement_review and pr_creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
Replace duplicated post-PR node/edge wiring across bug, feature, and task-takeover graphs with a shared post_pr module exposing add_post_pr_nodes() and add_post_pr_edges(). This eliminates ~300 lines of triplicated graph construction code while updating all graphs to use human_review_gate instead of the removed wait_for_ci_gate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
Extend worker CI webhook routing to deliver check-suite and check-run events to human_review_gate (in addition to ci_evaluator and attempt_ci_fix). When the workflow is at human_review_gate, the event is stored as pending_ci_event so the gate can route to ci_evaluator on its next invocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
Integration-style tests verifying the end-to-end concurrent gate behavior: CI events arriving while at human_review_gate are buffered and processed, review approval works independently of CI state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
Align three test files with renamed/moved symbols: - _route_ci_evaluation moved from feature.graph to post_pr - wait_for_ci_gate replaced by human_review_gate - CI routing: pending now goes to human_review_gate instead of END - attempt_ci_fix now posts two comments (attribution + fix attempt) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
32846a6 to
f35d221
Compare
The pending_ci_event flag was never set because the test state lacked pull_requests data, and the early current_node override unconditionally switched to ci_evaluator before the pending_ci_event branch could run. - Skip the ci_evaluator override when already at human_review_gate - Add pull_requests + repository to the test fixture so event_targets_pull_request returns True - Update multi-repo test to expect the new human_review_gate behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The PR deleted TestHandleResumeEventReviewGates (15 tests) when removing the review_response_gate inline-reply handling. Of those, 12 tested behavior that still exists in the codebase with no equivalent coverage elsewhere. Re-add them: - Login caching (_get_forge_github_login) - Self-authored review suppression (prevents infinite loops) - changes_requested/commented/approved at review_response_gate - PR merge at review_response_gate - Review ID vs fallback API branching for inline comments - changes_requested at human_review_gate - is_paused=False guard (review ignored when not paused) - _FRESH_INVOKE_NODES invariant - Handler-to-router integration (routes to implement_review) The 3 correctly deleted tests covered inline-reply thread resolution at review_response_gate, which was intentionally removed by this PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eshulman2
left a comment
eshulman2
left a comment
There was a problem hiding this comment.
Automated review: found several correctness regressions in the CI/human-review lifecycle rework that should be addressed before merge. Two additional issues fall outside this diff's changed lines and couldn't be attached inline, noting them here:
src/forge/orchestrator/worker.py (around line 886, in _handle_resume_event) — The dedicated handler that resumed the workflow when a human replied inline to a specific contested-comment thread at review_response_gate (matching in_reply_to_id against contested_comments and clearing only that thread) was deleted with no replacement; only the unrelated PRD/spec-proposal-PR inline-reply path remains (is_proposal_reply check). Failure scenario: a human posts an inline reply on a GitHub review thread that Forge previously contested on an implementation PR. No handling matches this event for review_response_gate anymore, so is_rejected/is_approved are never set and the workflow isn't resumed — the reply is silently ignored. Two tests exercising exactly this behavior (test_inline_reply_resumes_only_its_contested_thread, test_standalone_inline_comment_is_actionable_at_response_gate) were deleted rather than adapted.
src/forge/workflow/pr_state.py (line ~100, save_active_pull_request / _PR_LIFECYCLE_NODES) — This file wasn't touched by the diff but is broken by it: save_active_pull_request still defaults a new PR record's lifecycle_node to the now-deleted "wait_for_ci_gate" node, and _PR_LIFECYCLE_NODES still lists it. Failure scenario: in a multi-repo ticket, create_pull_request calls save_active_pull_request with current_node="teardown_workspace", which isn't in _PR_LIFECYCLE_NODES, so lifecycle_node gets stamped as "wait_for_ci_gate" for every non-final repo. A later /forge skip-gate <name> comment targeting an earlier repo restores that dead node, which is no longer in worker.py's local _CI_STAGES tuple, so the skip-gate command is silently dropped even though the ticket is genuinely at a CI-related stage.
| next_node = "review_response_gate" | ||
| else: | ||
| next_node = "wait_for_ci_gate" if unpushed else "human_review_gate" | ||
| next_node = "human_review_gate" |
There was a problem hiding this comment.
next_node is hardcoded to "human_review_gate" even when objections exist, contradicting this function's own docstring and the code above that still builds contested_comments and posts objections.
Failure scenario: the analysis container marks a review comment as "contest"; Forge posts the objection to the PR/Jira as designed, but the workflow proceeds straight to human_review_gate instead of pausing at review_response_gate. Combined with the deleted inline-reply handler in worker.py (see review summary), a human replying to the contested thread has no mechanism to resolve it.
| await jira.close() | ||
| logger.info(f"Pausing {ticket_key} at human_review_gate after PR creation") | ||
| else: | ||
| logger.info(f"Pausing {ticket_key} at human_review_gate (ci_status={ci_status!r})") |
There was a problem hiding this comment.
human_review_gate now unconditionally sets is_paused: True, removing the prior explicit is_paused: False branch for pr_merged; nothing downstream resets it.
Failure scenario: PR merges and routing correctly proceeds to current_node="complete" within the same ainvoke, but is_paused stays True (inherited via {**state} spreads). In worker.py, the completion-recording branch guarded by if not is_paused never fires for a normal completed workflow, and a later stray webhook logs "Workflow is paused at complete" and sets should_resume=True based on stale state.
|
|
||
| # GitHub issue_comment events: detect /forge skip-gate and /forge unskip-gate | ||
| # commands posted as PR comments. | ||
| _CI_STAGES = ("ci_evaluator", "attempt_ci_fix", "human_review_gate") |
There was a problem hiding this comment.
A new local _CI_STAGES is defined inside _handle_resume_event, shadowing the module-level _CI_STAGES which still references the deleted wait_for_ci_gate node and omits human_review_gate.
Failure scenario: the module-level _CI_STAGES is now dead code diverging from reality. Any future refactor that removes the local shadow (e.g. extracting the skip-gate block into a helper) will silently fall back to the stale tuple, re-breaking /forge skip-gate at human_review_gate.
eshulman2
left a comment
eshulman2
left a comment
There was a problem hiding this comment.
Before merging, please provide one of the following for workflows currently checkpointed in the CI stage:
- Backward compatibility: retain a temporary route/alias from to , including migration of any values that still reference ; or
- A clear, tested migration path for affected tickets that preserves their existing PR mappings, per-PR CI/review state, and branches.
Simply sending another webhook will resume the existing checkpoint rather than restart it, while clearing the checkpoint can discard PR lifecycle state and cause work to be regenerated. The PR description currently says workflows must be manually restarted, but does not define a safe procedure or provide tooling to identify and migrate affected tickets.
Please document the exact operator steps, expected state transformations, rollback behavior, and tests covering an in-flight workflow at (including a multi-repository ticket).
eshulman2
left a comment
eshulman2
left a comment
There was a problem hiding this comment.
Correction/clarification to my previous review comment:
Before merging, please provide one of the following for workflows currently checkpointed in the CI stage:
- Backward compatibility: retain a temporary route or alias from wait_for_ci_gate to human_review_gate, including migration of any per-PR lifecycle_node values that still reference wait_for_ci_gate; or
- A clear, tested migration path for affected tickets that preserves their existing PR mappings, per-PR CI/review state, and branches.
Simply sending another webhook resumes the existing checkpoint rather than restarting it, while clearing the checkpoint can discard PR lifecycle state and cause work to be regenerated. The PR description currently says workflows must be manually restarted, but does not define a safe procedure or provide tooling to identify and migrate affected tickets.
Please document the exact operator steps, expected state transformations, rollback behavior, and tests covering an in-flight workflow at wait_for_ci_gate, including a multi-repository ticket.
2 participants
Summary
Reworks the post-PR lifecycle so CI evaluation and human review run concurrently. Previously, workflows blocked at
wait_for_ci_gateuntil all CI checks passed before proceeding to human review. Now, after PR creation the workflow goes directly tohuman_review_gateand accepts both CI and review webhooks in parallel — matching GitHub's natural PR workflow where merge is the final gate.Key changes:
wait_for_ci_gatenode; CI webhooks are routed tohuman_review_gateinsteadpending_ci_eventstate field for buffering CI webhooks athuman_review_gatepost_pr.py(reused by feature, bug, task_takeover graphs)current_nodeathuman_review_gate(prevents premature override toci_evaluator)This change is not backward compatible. In-flight workflows checkpointed at
wait_for_ci_gatewill need to be manually restarted.Test plan
human_review_gatecorrectly setspending_ci_eventand preservescurrent_nodeci_evaluatorstill routes directly (nopending_ci_event)pending_ci_eventCloses #137