mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[1f6a06a2] PR-review gate: verify ACs literally and require green CI before pr_pass (#428)
* [a1bde3b9] Add CI-status guard to pr_pass + update pr_reviewer prompt (#417) (#420) * [a1bde3b9] feat(gateway): CI-status guard on pr_pass + reviewer prompt update * [a1bde3b9] docs(pr-gate-review, worksession-git): document CI-status guard on pr_pass Updated two architecture documentation files to reflect the new CI-status guard: **pr-gate-review.md:** - Documented _ci_status_guard method: blocks pr_pass on failing/pending/unscheduled/error CI with reviewer-aware pr_fail remediation - Documented _resolve_ci_status: best-effort GitHub check-runs lookup with fail-open behavior - Updated _pr_pass_blocked description: now returns (rejection_envelope, ci_note) tuple - Updated _record_gate_verdict_for/verdict to note ci_status field stamping on pr_pass - Added ci_note parameter documentation for evidence tracking when no CI is configured - Updated Logical Tree to show new methods - Added Config Flags note: CI guard is always armed, fails open on config gaps - Added two regression risks: check-runs-only limitation, fail-open design **worksession-git.md:** - Documented GitService.get_pr_ci_status(project_slug, pr_number): CI status lookup with state classification - Documented supporting methods: _ci_status_prereqs, _fetch_check_runs, _classify_check_runs, _classify_zero_check_runs - Each method notes its fail-open behavior and configuration gap handling --------- Co-authored-by: Backend Developer 1 <be-dev-1@roboco.tech> Co-authored-by: Backend Documenter <be-doc@roboco.tech> * [e8f275d7] test(gateway): lock the 7-AC-to-test map + assert pr_reviewer prompt content (#425) (#426) Co-authored-by: Backend Developer 1 <be-dev-1@roboco.tech> * [24b4237e] Fix reflow-check, CI-status classification, and noqa suppression (#440) (#443) * [24b4237e] fix(gateway): classify unreachable/nonexistent CI-status repo as no_ci_configured, remove test noqa, reflow pr_reviewer.md Split GitService.get_pr_ci_status's PR-head-sha lookup into a dedicated helper so a config gap (missing project/git_url/token) or an unreachable/ nonexistent repo/PR (network error or 404) classifies as no_ci_configured (pr_pass passes through and stamps the evidence note) while a genuine GitHub API failure on a real, reachable repo (any other non-2xx, or an unparseable body) stays the fail-closed error state. Replaced the `# noqa: PLR2004` in test_git_pr_ci_status.py with a named HTTP-status range constant, updated the config-gap tests to assert the new classification, and added tests for the unreachable-repo and real-repo- API-error branches. Reflowed agents/prompts/roles/pr_reviewer.md's one hard-wrapped continuation line so it passes make reflow-check. * [24b4237e] docs(gateway): update pr-gate-review.md for CI-status classification refactor Updated the internal architectural map to reflect the new CI-status classification scheme introduced in PR #440. Configuration gaps (missing project/git_url/token) and unreachable/nonexistent repos (404 or network error) now explicitly classify as no_ci_configured and pass through with evidence stamps. Genuine GitHub API failures on reachable repos classify as error and stay fail-closed (retryable). - Clarified _ci_status_guard behavior: config gaps/unreachable repos pass through with distinct classification; only real API failures stay fail-closed - Updated Config Flags section to describe the new three-way classification - Updated Regression Risks section to document the new explicit classification scheme - Noted that _resolve_ci_status now wraps git.get_pr_ci_status and interprets its result dict --------- Co-authored-by: Backend Developer 1 <be-dev-1@roboco.tech> Co-authored-by: Backend Documenter <be-doc@roboco.tech> * [1f6a06a2] round-3 fixes: pr_gate back to xenon rank A; 404 means no CI, not error Eight extracted helpers bring the module average from B(5.05) to A(4.04) with every external contract untouched (170 gate tests byte-identical). The CI-status guard now classifies a 404 on the check-runs or workflows endpoints as no_ci_configured (pass-through with evidence note) — a repo without Actions is not a transport failure — reserving the fail-closed error state for network/5xx/auth failures, with pinning tests for all four shapes. The e2e fake-GitHub router gains check-runs and workflows routes so the scripted lifecycle exercises the guard's green-CI success branch end to end. * [1f6a06a2] merge master; align gate-diff-base tests with the tuple contract The merged tree is the first integration of the CI-status guard with the preferred-parent diff-base guard: _pr_pass_blocked now returns (rejection, ci_note), so the diff-base tests unpack it instead of asserting on a bare result. Both guards verified live in the merged pr_gate (preferred_parent threading and _ci_status_guard present). --------- Co-authored-by: Backend Developer 1 <be-dev-1@roboco.tech> Co-authored-by: Backend Documenter <be-doc@roboco.tech> Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
co-authored by
Backend Developer 1
Backend Documenter
Renn F
Backend Developer 1
Backend Documenter
Renn F
parent
15a3e87a2f
commit
4d52f6ff59
@@ -58,7 +58,13 @@ You have a second, distinct surface: the **in-path PR-review gate**. After a Cel
|
||||
3. Review the assembled diff against the parent objective + full acceptance criteria + the cross-cell contract, with the same adversarial bar as an external PR (a block-level convention violation — a misplaced definition, a lint/type suppression — is an automatic `pr_fail`; the gate already refuses `pr_pass`).
|
||||
4. `pr_pass(task_id, notes='<>=20 chars')` to send it on to `awaiting_pm_review` for the PM merge, or `pr_fail(task_id, issues=[...])` to route it back to `needs_revision` (the owning dev/cell PM re-claims and revises — for a Main-PM branch-bearing root, `pr_fail`'s `remediate` tells the Main PM to re-delegate the fixes to the owning cell PM(s) and wait for re-assembly, NOT to re-submit the unchanged root).
|
||||
|
||||
**On a blocked `pr_pass`:** if the toolchain or conventions validator cannot run in your workspace (interpreter mismatch, validator hang), the gate refuses `pr_pass` and its `remediate` points at `pr_fail(issues=['toolchain: ...'])` — your reject lever, since you have no `i_am_blocked` verb. Do NOT chase `i_am_blocked`; send the PR back with `pr_fail` so the dev rebuilds the environment.
|
||||
**The per-AC evidence-walk (non-negotiable).** Do not assert "criteria met" from a skim. Walk every acceptance criterion on the parent task ONE AT A TIME and pin it to a concrete `file:line` in the assembled diff that satisfies it. A criterion you cannot pin to a line is not satisfied — treat it exactly like a missing deliverable (see next rule), not a maybe.
|
||||
|
||||
**The named-deliverable/silent-drop rule.** When a criterion, the parent objective, or a dev's own notes name a specific deliverable (an endpoint, a migration, a test file, a doc update, a UI element), confirm it actually landed in the diff at the file you'd expect. A deliverable that is missing, stubbed, or silently dropped between what was claimed and what the diff contains is an automatic `pr_fail` — never a `pr_pass` with a "note for later"; a passed gate merges, so a silent drop that slips through here ships silently.
|
||||
|
||||
**On a blocked `pr_pass`:** three guards can refuse the transition, each with a reviewer-aware `remediate` pointing at `pr_fail` (never `i_am_blocked` — you have no such verb):
|
||||
- **Toolchain / conventions:** if the toolchain or conventions validator cannot run in your workspace (interpreter mismatch, validator hang), `remediate` points at `pr_fail(issues=['toolchain: ...'])` so the dev rebuilds the environment.
|
||||
- **CI status:** `pr_pass` also refuses when CI on the assembled PR's head commit is not resolvably green. Failing CI names the check(s) and `remediate` points at `pr_fail(issues=['CI failing: ...'])`; pending / not-yet-scheduled / a GitHub API error are framed as retryable — wait and call `pr_pass` again once CI resolves, do not treat any of these as a defect to route back to the dev via `pr_fail` unless the diff itself is also bad. A project with no CI configured at all passes through cleanly (the verdict note is stamped `ci_status: "no CI configured on this project"` so the PM sees the guard ran and deliberately did not block). Do NOT chase `i_am_blocked` for any of these; the reject lever is always `pr_fail`.
|
||||
|
||||
**Single-claimant:** a gate task already actively claimed by a different reviewer returns `invalid_state` ("it may already be claimed; `give_me_work` for the next") — call `give_me_work()` for the next review. A re-claim by the same reviewer is idempotent.
|
||||
|
||||
|
||||
@@ -17,12 +17,14 @@ The two choreographer mixins that implement the PR-reviewer's two distinct surfa
|
||||
| PRGateMixin.pr_pass | method | roboco/services/gateway/choreographer/pr_gate.py:115 | Pass the assembled PR: awaiting_pr_review → awaiting_pm_review; delegates to _gate_decision |
|
||||
| PRGateMixin.pr_fail | method | roboco/services/gateway/choreographer/pr_gate.py:123 | Fail the assembled PR with concrete issues → needs_revision; rejects empty issues list, formats issues into notes, delegates to _gate_decision |
|
||||
| PRGateMixin._gate_preflight | method | roboco/services/gateway/choreographer/pr_gate.py:148 | Ownership + role + spec gate (with self_review_block via actor_slug/original_developer_slug) + free-text soup guard for pr_pass/pr_fail; returns rejection Envelope or (t,agent,role_str,briefing,spec_ctx) |
|
||||
| PRGateMixin._record_gate_verdict_for | method | roboco/services/gateway/choreographer/pr_gate.py:229 | Author the canonical pr_review verdict note before the transition; on pr_fail also capture the assembled PR head SHA for the unchanged-PR gate |
|
||||
| PRGateMixin._record_gate_verdict_for | method | roboco/services/gateway/choreographer/pr_gate.py:229 | Author the canonical pr_review verdict note before the transition; on pr_fail also capture the assembled PR head SHA for the unchanged-PR gate; on pr_pass with ci_note, stamp the ci_status field into the verdict with evidence the CI guard ran |
|
||||
| PRGateMixin._post_gate_review | method | roboco/services/gateway/choreographer/pr_gate.py:245 | Post the gate verdict to the PR itself (best-effort, after the DB transition); resolves reviewer slug |
|
||||
| PRGateMixin._deliver_pr_fail_to_owner | method | roboco/services/gateway/choreographer/pr_gate.py:253 | a2a the pr_fail change-requests to the owning PM (best-effort) with a Main-PM-root steer to re-delegate not re-submit; closes the blind re-submit loop |
|
||||
| PRGateMixin._gate_decision | method | roboco/services/gateway/choreographer/pr_gate.py:292 | Shared body for pr_pass/pr_fail: preflight + tracing + pr_pass blocked guards + record verdict + run_intent + None-guard for concurrent transition + post-PR + a2a on fail |
|
||||
| PRGateMixin._pr_pass_blocked | method | roboco/services/gateway/choreographer/pr_gate.py:373 | Refuse pr_pass on a broken toolchain or block-level convention violation; pr_fail stays available; both guards inert when their flag is off |
|
||||
| PRGateMixin._record_gate_verdict | method | roboco/services/gateway/choreographer/pr_gate.py:403 | Persist the gate verdict as the canonical pr_review structured note (passed/failed), with issues slot for pr_fail and head_sha stamp; best-effort (ContentValidationError logged not raised) |
|
||||
| PRGateMixin._pr_pass_blocked | method | roboco/services/gateway/choreographer/pr_gate.py:373 | Refuse pr_pass on a broken toolchain, block-level convention violation, or non-green CI on the assembled PR's head commit; returns (rejection_envelope, ci_note). Both toolchain and conventions guards inert when their flags are off; CI guard fails open on configuration gaps |
|
||||
| PRGateMixin._ci_status_guard | method | roboco/services/gateway/choreographer/pr_gate.py:520 | Refuse pr_pass unless CI on the assembled PR's head commit is green. Failing/pending/unscheduled CI blocks with reviewer-aware remediation pointing at pr_fail; configuration gaps, unreachable/nonexistent repos, or real API failures on reachable repos each pass through with distinct classifications (no_ci_configured vs error) via git.get_pr_ci_status |
|
||||
| PRGateMixin._resolve_ci_status | method | roboco/services/gateway/choreographer/pr_gate.py:480 | Thin wrapper: calls git.get_pr_ci_status, interprets the returned dict (no_ci_configured/pending/failure/error/success), and composes a rejection Envelope if CI must block pr_pass |
|
||||
| PRGateMixin._record_gate_verdict | method | roboco/services/gateway/choreographer/pr_gate.py:403 | Persist the gate verdict as the canonical pr_review structured note (passed/failed), with issues slot for pr_fail, head_sha stamp for pr_fail, and ci_status evidence on pr_pass; best-effort (ContentValidationError logged not raised) |
|
||||
| PRGateMixin._capture_pr_head_sha | method | roboco/services/gateway/choreographer/pr_gate.py:468 | Best-effort capture of the assembled PR head SHA at pr_fail time via _project_slug_for + git.get_pr_head_sha; returns None on any failure (fail-open) |
|
||||
| PRGateMixin._post_gate_review_to_pr | method | roboco/services/gateway/choreographer/pr_gate.py:502 | Post APPROVE/REQUEST_CHANGES on cell→root PRs; always COMMENT on root→master (only CEO merges master); best-effort |
|
||||
| PRGateMixin._gate_role_or_rejection | method | roboco/services/gateway/choreographer/pr_gate.py:545 | Parse the role enum from role_str or return a not_authorized rejection Envelope |
|
||||
@@ -85,8 +87,10 @@ pr-gate-review slice
|
||||
│ ├── _gate_decision — shared body (preflight→tracing→blocked→record→run_intent→None-guard→post→a2a)
|
||||
│ ├── _gate_preflight — ownership/role/spec-gate (self_review_block) + soup guard
|
||||
│ ├── _gate_tracing — journal:learning + pr_reviewer_notes min chars
|
||||
│ ├── _pr_pass_blocked — toolchain-broken + conventions block guards
|
||||
│ ├── _record_gate_verdict_for / _record_gate_verdict — structured pr_review note (+ issues + head_sha)
|
||||
│ ├── _pr_pass_blocked — toolchain-broken + conventions block + CI-status guards (returns rejection, ci_note)
|
||||
│ ├── _ci_status_guard — refuse pr_pass on failing/pending/unscheduled CI; config gaps + unreachable repos pass through with evidence stamp; real API failures stay fail-closed
|
||||
│ ├── _resolve_ci_status — thin wrapper calling git.get_pr_ci_status, interprets result dict, returns rejection Envelope if CI must block
|
||||
│ ├── _record_gate_verdict_for / _record_gate_verdict — structured pr_review note (+ issues + head_sha + ci_status)
|
||||
│ ├── _re_stamp_pr_fail_head_sha_if_advanced — re-capture head SHA post-transition and re-stamp verdict note if advanced (#189)
|
||||
│ ├── _capture_pr_head_sha — best-effort PR head SHA for unchanged-PR gate
|
||||
│ ├── _post_gate_review / _post_gate_review_to_pr — PR review post (COMMENT on root→master or MegaTask root-subtask)
|
||||
@@ -125,6 +129,7 @@ pr-gate-review slice
|
||||
- ROBOCO_TOOLCHAIN_MATCH_ENABLED (gates _toolchain_broken_guard in _pr_pass_blocked — inert when off)
|
||||
- ROBOCO_CONVENTIONS_ENABLED (gates _conventions_guard in _pr_pass_blocked — inert when off)
|
||||
- ROBOCO_PR_REVIEWER_NOTES_MIN_CHARS / settings.pr_reviewer_notes_min_chars (tracing gate substantive-note threshold for pr_pass/pr_fail/post_pr_review)
|
||||
- CI-status guard is always armed when the toolchain can reach get_pr_ci_status via git service. Configuration gaps (missing project/git_url/token) and unreachable/nonexistent repos (404 or network error) classify as no_ci_configured and pass through with evidence stamp. Genuine API failures on reachable repos classify as error and stay fail-closed (retryable). A project with no CI configured at all also passes through cleanly (no_ci_configured). The guard never blocks pr_pass on a misconfigured project.
|
||||
|
||||
|
||||
## Gotchas
|
||||
@@ -168,6 +173,9 @@ pr-gate-review slice
|
||||
|
||||
| Title | File:Line | Claim | Severity |
|
||||
|---|---|---|---|
|
||||
| CI-status guard reads GitHub check-runs only, not legacy commit-status API | roboco/services/gateway/choreographer/pr_gate.py:520 | _ci_status_guard and get_pr_ci_status read only the check-runs API endpoint. A repo whose only CI signal is the legacy commit-status API would show zero check-runs and be classified as no_ci_configured (passes through). Noted as ponytail-comment in git.py with the upgrade path if a project ever needs it. | low |
|
||||
| CI-status classification: config gaps and unreachable repos now explicitly no_ci_configured | roboco/services/git.py:_resolve_ci_head_sha | Missing project/git_url/git-token, or unreachable/nonexistent repo (404 on PR head lookup or network error), all classify as no_ci_configured → pr_pass passes through with evidence stamp. Only genuine GitHub API failures on reachable repos classify as error → pr_pass stays fail-closed (retryable). By design: configuration gaps should not block the gate, but real API failures should fail-closed to avoid false-green verdicts. A deliberately misconfigured project's CI is silently not enforced, with clear evidence in the verdict note. | low |
|
||||
|---|---|---|---|
|
||||
| self_review_block could fire if a reviewer is also the original developer | roboco/services/gateway/choreographer/pr_gate.py:202 | actor_slug=str(reviewer_agent_id) + original_developer_slug=markers.get_original_developer(t). The comment asserts dormancy because the marker is never set on assembled coordination tasks. If a future change sets the marker on an assembled task (or a reviewer UUID coincides with the recorded dev UUID), pr_pass/pr_fail would be refused as self-review with no remediate path. The defense is correctly wired but unguarded by a test asserting dormancy. | low |
|
||||
| ~~resolve_task_project_slug cell_projects branch can raise AttributeError on malformed mapping~~ **FIXED 536bbb64 #82** | roboco/services/gateway/choreographer/pr_review.py:594 | ~~sorted(cell_map, key=lambda m: m.team.value) assumes every mapping has a non-None team with .value. _capture_pr_head_sha wraps the slug call in try/except (fail-open), but _post_gate_review_to_pr calls self._project_slug_for(t) WITHOUT a try/except — a malformed cell_map mapping would raise and abort the verdict PR post (best-effort but the exception escapes the helper, caught only by the outer try in _post_gate_review_to_pr's git.post_pr_review call, NOT the slug resolution).~~ _post_gate_review_to_pr now wraps the slug-resolve call in its own try/except (mirrors _capture_pr_head_sha) — a malformed mapping logs and returns, no longer 500s the reviewer after the committed gate transition. The underlying AttributeError possibility in resolve_task_project_slug remains but is contained. | medium |
|
||||
| ~~_is_hand_formatted_verdict false-positive on summaries quoting PR-added headers~~ **FIXED 536bbb64 #188** | roboco/services/gateway/choreographer/pr_review.py:174 | ~~Substring match on '## summary'/'## issues'/'## verdict'/'## findings' in lowercased body. A reviewer summarizing a PR that itself adds a '## Summary' section (quoting it in the body) with findings=[] would be falsely refused.~~ Regex now anchored to line-start (^[ \t]*## ..., re.MULTILINE) — quoted headers (> ## Summary) and mid-prose mentions no longer trip the guard. | low |
|
||||
|
||||
@@ -64,6 +64,11 @@ This slice is the git substrate every delivery agent works on. `GitService` runs
|
||||
| `GitService.update_pr_for_task` | method | git.py:2506 | Patch PR title/body; 404→typed GitError |
|
||||
| `GitService.get_pr_head_sha` | method | git.py:2451 | PR head SHA for pr_fail re-submit loop guard (fail-open) |
|
||||
| `GitService.get_latest_ci_conclusion` | method | git.py:1929 | Per-project CI signal (unknown never false-green) |
|
||||
| `GitService.get_pr_ci_status` | method | git.py:2662 | CI status of a PR's current head commit for the in-path pr_pass gate; returns {state, failing_checks?, head_sha} or None on config gaps (fail-open) |
|
||||
| `GitService._ci_status_prereqs` | method | git.py:2698 | Resolve (owner, repo, auth headers, head_sha) for CI-status lookup or None on any gap |
|
||||
| `GitService._fetch_check_runs` | method | git.py:2730 | GET check-runs for head_sha; None on any API failure |
|
||||
| `GitService._classify_check_runs` | method | git.py:2770 | State classification: success/failure/pending from check-run conclusions list |
|
||||
| `GitService._classify_zero_check_runs` | method | git.py:2790 | State classification when zero check-runs exist: pending_not_scheduled or no_ci_configured (depends on workflow count) |
|
||||
| `GitService.list_open_prs` | method | git.py:1844 | Normalized open-PR list |
|
||||
| `GitService.post_pr_review` | method | git.py:2329 | Post reviewer comments via GitHub API |
|
||||
| `GitService.merge_pull_request` | method | git.py:2914 | GitHub merge API + method fallback + already-merged disambiguation |
|
||||
|
||||
@@ -53,37 +53,10 @@ class PRGateMixin(_Base):
|
||||
pr_pass / pr_fail source-status still matches. The assembled PR's diff is
|
||||
returned inline (read-only) so the reviewer inspects it before deciding.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
agent = await self.task.agent_for(reviewer_agent_id)
|
||||
role_str = str(agent.role) if agent is not None else "pr_reviewer"
|
||||
briefing = await self._briefing_for(reviewer_agent_id, task_id, full=True)
|
||||
role = await self._gate_role_or_rejection(
|
||||
t, role_str, briefing, reviewer_agent_id, task_id, "claim_gate_review"
|
||||
)
|
||||
if isinstance(role, Envelope):
|
||||
return role
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
agent_team=str(agent.team) if agent is not None and agent.team else None,
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, "claim_gate_review", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
pre = await self._claim_gate_preflight(reviewer_agent_id, task_id)
|
||||
if isinstance(pre, Envelope):
|
||||
return pre
|
||||
t, role_str, briefing = pre
|
||||
guard = await self._run_claim_guards(
|
||||
agent_id=reviewer_agent_id,
|
||||
task=t,
|
||||
@@ -119,6 +92,52 @@ class PRGateMixin(_Base):
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str)
|
||||
|
||||
async def _claim_gate_preflight(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID
|
||||
) -> Any:
|
||||
"""Task fetch + role + spec gate for ``claim_gate_review``.
|
||||
|
||||
Returns a rejection ``Envelope`` or the ``(t, role_str, briefing)``
|
||||
tuple on pass.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
agent = await self.task.agent_for(reviewer_agent_id)
|
||||
role_str = self._role_str_for_agent(agent)
|
||||
briefing = await self._briefing_for(reviewer_agent_id, task_id, full=True)
|
||||
role = await self._gate_role_or_rejection(
|
||||
t, role_str, briefing, reviewer_agent_id, task_id, "claim_gate_review"
|
||||
)
|
||||
if isinstance(role, Envelope):
|
||||
return role
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
agent_team=str(agent.team) if agent is not None and agent.team else None,
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, "claim_gate_review", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
return (t, role_str, briefing)
|
||||
|
||||
@staticmethod
|
||||
def _role_str_for_agent(agent: Any) -> str:
|
||||
"""Reviewer role string off the agent view, defaulting to pr_reviewer."""
|
||||
return str(agent.role) if agent is not None else "pr_reviewer"
|
||||
|
||||
async def pr_pass(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID, notes: str
|
||||
) -> Envelope:
|
||||
@@ -166,53 +185,19 @@ class PRGateMixin(_Base):
|
||||
Returns a rejection ``Envelope`` or the
|
||||
``(t, agent, role_str, briefing, spec_ctx)`` tuple on pass.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
if t.assigned_to != reviewer_agent_id:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_gate_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(
|
||||
reviewer_agent_id, task_id
|
||||
),
|
||||
).with_introspection(task=t, role="pr_reviewer"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
t = await self._gate_ownership_or_rejection(reviewer_agent_id, task_id, verb)
|
||||
if isinstance(t, Envelope):
|
||||
return t
|
||||
agent = await self.task.agent_for(reviewer_agent_id)
|
||||
role_str = str(agent.role) if agent is not None else "pr_reviewer"
|
||||
role_str = self._role_str_for_agent(agent)
|
||||
briefing = await self._briefing_for(reviewer_agent_id, task_id)
|
||||
role = await self._gate_role_or_rejection(
|
||||
t, role_str, briefing, reviewer_agent_id, task_id, verb
|
||||
)
|
||||
if isinstance(role, Envelope):
|
||||
return role
|
||||
# The spec gate's ``self_review_block`` is the only self-review defense
|
||||
# for pr_pass / pr_fail: the service-layer ``_validate_not_self_review``
|
||||
# backstop covers qa/documenter but skips pr_reviewer. For the comparison
|
||||
# to fire, both sides must be populated. ``GatewayAgentView`` carries no
|
||||
# ``slug`` field (so ``getattr(agent, "slug", None)`` is always None in
|
||||
# production), and the ``original_developer`` marker stores the dev's
|
||||
# UUID — so resolve both as UUID strings and let the spec's string
|
||||
# equality do the rest. The marker is never set on assembled coordination
|
||||
# tasks (only on dev-leaf tasks at QA/doc claim), so the block is dormant
|
||||
# by design in production — but the gate is now correctly wired to fire
|
||||
# if the marker were ever set to the reviewer.
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=str(reviewer_agent_id),
|
||||
agent_team=str(agent.team) if agent is not None and agent.team else None,
|
||||
original_developer_slug=markers.get_original_developer(t),
|
||||
notes=notes,
|
||||
issues=issues,
|
||||
spec_ctx = self._gate_preflight_spec_ctx(
|
||||
reviewer_agent_id, agent, t, notes, issues
|
||||
)
|
||||
if soup := await self._guard_free_text(
|
||||
checks=(("notes", notes, 8), ("issues", list(issues), 8)),
|
||||
@@ -234,8 +219,73 @@ class PRGateMixin(_Base):
|
||||
)
|
||||
return (t, agent, role_str, briefing, spec_ctx)
|
||||
|
||||
async def _gate_ownership_or_rejection(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID, verb: str
|
||||
) -> Any:
|
||||
"""Fetch the task and verify it is assigned to the reviewer.
|
||||
|
||||
Returns the task on success, or a ``not_found`` / ``not_authorized``
|
||||
rejection ``Envelope`` on failure.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
if t.assigned_to != reviewer_agent_id:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_gate_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(
|
||||
reviewer_agent_id, task_id
|
||||
),
|
||||
).with_introspection(task=t, role="pr_reviewer"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
return t
|
||||
|
||||
@staticmethod
|
||||
def _gate_preflight_spec_ctx(
|
||||
reviewer_agent_id: UUID, agent: Any, t: Any, notes: str, issues: tuple[str, ...]
|
||||
) -> spec_module.Context:
|
||||
"""Build the pr_pass / pr_fail spec ``Context``, including the
|
||||
self-review wiring.
|
||||
|
||||
The spec gate's ``self_review_block`` is the only self-review defense
|
||||
for pr_pass / pr_fail: the service-layer ``_validate_not_self_review``
|
||||
backstop covers qa/documenter but skips pr_reviewer. For the comparison
|
||||
to fire, both sides must be populated. ``GatewayAgentView`` carries no
|
||||
``slug`` field (so ``getattr(agent, "slug", None)`` is always None in
|
||||
production), and the ``original_developer`` marker stores the dev's
|
||||
UUID — so resolve both as UUID strings and let the spec's string
|
||||
equality do the rest. The marker is never set on assembled coordination
|
||||
tasks (only on dev-leaf tasks at QA/doc claim), so the block is dormant
|
||||
by design in production — but the gate is now correctly wired to fire
|
||||
if the marker were ever set to the reviewer.
|
||||
"""
|
||||
return spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=str(reviewer_agent_id),
|
||||
agent_team=str(agent.team) if agent is not None and agent.team else None,
|
||||
original_developer_slug=markers.get_original_developer(t),
|
||||
notes=notes,
|
||||
issues=issues,
|
||||
)
|
||||
|
||||
async def _record_gate_verdict_for(
|
||||
self, verb: str, t: Any, notes: str, *, issues: tuple[str, ...]
|
||||
self,
|
||||
verb: str,
|
||||
t: Any,
|
||||
notes: str,
|
||||
*,
|
||||
issues: tuple[str, ...],
|
||||
ci_note: str | None = None,
|
||||
) -> str | None:
|
||||
"""Author the canonical pr_review verdict note before the transition.
|
||||
|
||||
@@ -243,6 +293,9 @@ class PRGateMixin(_Base):
|
||||
can structurally refuse to re-submit the unchanged root (the 2026-06-27
|
||||
infinite pr_fail re-submit loop). Best-effort: a capture failure leaves
|
||||
head_sha absent and submit_root fails open rather than wedging the PM.
|
||||
On pr_pass, ``ci_note`` (set by ``_pr_pass_blocked`` when the CI-status
|
||||
guard passed through a project with no CI configured) is stamped into
|
||||
the verdict's ``ci_status`` field as evidence the guard actually ran.
|
||||
|
||||
Returns the captured head_sha for pr_fail (None for pr_pass) so the caller
|
||||
can re-capture after the transition commits and re-stamp if the PR head
|
||||
@@ -252,7 +305,7 @@ class PRGateMixin(_Base):
|
||||
head_sha = await self._capture_pr_head_sha(t)
|
||||
self._record_gate_verdict(t, verb, notes, issues=issues, head_sha=head_sha)
|
||||
return head_sha
|
||||
self._record_gate_verdict(t, verb, notes, issues=issues)
|
||||
self._record_gate_verdict(t, verb, notes, issues=issues, ci_note=ci_note)
|
||||
return None
|
||||
|
||||
async def _re_stamp_pr_fail_head_sha_if_advanced(
|
||||
@@ -354,8 +407,9 @@ class PRGateMixin(_Base):
|
||||
)
|
||||
if gate is not None:
|
||||
return gate
|
||||
ci_note: str | None = None
|
||||
if verb == "pr_pass":
|
||||
blocked = await self._pr_pass_blocked(
|
||||
blocked, ci_note = await self._pr_pass_blocked(
|
||||
reviewer_agent_id, task_id, t, role_str, briefing
|
||||
)
|
||||
if blocked is not None:
|
||||
@@ -363,7 +417,9 @@ class PRGateMixin(_Base):
|
||||
# Author the canonical pr_review verdict note BEFORE the transition so it
|
||||
# is persisted by the same commit (mirrors post_pr_review) and stays in
|
||||
# lock-step with the decision (pr_fail overwrites an earlier pr_pass).
|
||||
pre_sha = await self._record_gate_verdict_for(verb, t, notes, issues=issues)
|
||||
pre_sha = await self._record_gate_verdict_for(
|
||||
verb, t, notes, issues=issues, ci_note=ci_note
|
||||
)
|
||||
runner = self._verb_runner()
|
||||
try:
|
||||
t = await runner.run_intent(verb, t, agent, spec_ctx)
|
||||
@@ -433,13 +489,23 @@ class PRGateMixin(_Base):
|
||||
t: Any,
|
||||
role_str: str,
|
||||
briefing: dict[str, Any],
|
||||
) -> Envelope | None:
|
||||
"""Refuse pr_pass on a broken toolchain or a block-level violation.
|
||||
) -> tuple[Envelope | None, str | None]:
|
||||
"""Refuse pr_pass on a broken toolchain, a block-level violation, or
|
||||
non-green CI on the assembled PR's head commit.
|
||||
|
||||
A reviewer must not PASS an assembled PR whose suite can't run in the
|
||||
workspace, or that carries unresolved architectural-convention
|
||||
violations; pr_fail stays available. Returns the emitted rejection or
|
||||
None to proceed. Both guards are inert when their flag is off.
|
||||
workspace, that carries unresolved architectural-convention
|
||||
violations, or whose CI is red/pending/unscheduled/unresolvable;
|
||||
pr_fail stays available for all three. Returns ``(rejection, None)``
|
||||
to block, or ``(None, ci_note)`` to proceed — ``ci_note`` is a
|
||||
non-None evidence stamp only when the CI guard passed through a
|
||||
project with no CI configured at all. The toolchain/conventions
|
||||
guards are inert when their flag is off; the CI guard fails open on
|
||||
an unresolvable gate-level slug/PR number (``None`` from
|
||||
``_resolve_ci_status``) and also passes through — with an evidence
|
||||
stamp — when ``get_pr_ci_status`` itself classifies a missing
|
||||
project/git_url/token or an unreachable/nonexistent repo as
|
||||
``no_ci_configured``.
|
||||
"""
|
||||
from roboco.config import settings as _settings
|
||||
|
||||
@@ -457,13 +523,128 @@ class PRGateMixin(_Base):
|
||||
for guard in guards:
|
||||
rejection = await guard()
|
||||
if rejection is not None:
|
||||
return await self._emit_rejection(
|
||||
rejection.with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pr_pass",
|
||||
return (
|
||||
await self._emit_rejection(
|
||||
rejection.with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pr_pass",
|
||||
),
|
||||
None,
|
||||
)
|
||||
return None
|
||||
return await self._ci_status_guard(
|
||||
reviewer_agent_id, task_id, t, role_str, briefing
|
||||
)
|
||||
|
||||
async def _resolve_ci_status(self, task_id: UUID, t: Any) -> dict[str, Any] | None:
|
||||
"""Best-effort CI-status lookup for the assembled PR's head commit.
|
||||
|
||||
Returns ``None`` on ANY configuration gap or lookup failure (no
|
||||
resolvable slug/PR number, a raised exception, or a caller returning
|
||||
something other than the documented ``dict[str, Any]`` shape) so
|
||||
``_ci_status_guard`` fails open on every one of them uniformly.
|
||||
"""
|
||||
pr_number = getattr(t, "pr_number", None)
|
||||
try:
|
||||
slug = await self._project_slug_for(t)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"ci status guard: slug resolve failed", task_id=str(task_id)
|
||||
)
|
||||
return None
|
||||
if not slug or not pr_number:
|
||||
return None
|
||||
try:
|
||||
status = await self.git.get_pr_ci_status(slug, int(pr_number))
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"ci status guard: get_pr_ci_status raised", task_id=str(task_id)
|
||||
)
|
||||
return None
|
||||
return status if isinstance(status, dict) else None
|
||||
|
||||
async def _ci_status_guard(
|
||||
self,
|
||||
reviewer_agent_id: UUID,
|
||||
task_id: UUID,
|
||||
t: Any,
|
||||
role_str: str,
|
||||
briefing: dict[str, Any],
|
||||
) -> tuple[Envelope | None, str | None]:
|
||||
"""Refuse pr_pass unless CI on the assembled PR's head commit is green.
|
||||
|
||||
Failing, pending, unscheduled, or unresolvable-via-API CI states all
|
||||
block with a reviewer-aware remediation pointing at ``pr_fail`` (a
|
||||
reviewer has no ``i_am_blocked``) — pending/unscheduled/error are
|
||||
framed as retryable (wait and call pr_pass again), never as a defect
|
||||
to route back to the dev. A project with no CI configured at all
|
||||
passes through cleanly, returning an evidence note so the caller can
|
||||
stamp the verdict with why the guard did not block.
|
||||
"""
|
||||
status = await self._resolve_ci_status(task_id, t)
|
||||
if status is None:
|
||||
# A configuration gap or lookup failure — never mistaken for a CI
|
||||
# signal, so the guard fails open rather than blocking.
|
||||
return None, None
|
||||
state = status.get("state")
|
||||
if state == "success":
|
||||
return None, None
|
||||
if state == "no_ci_configured":
|
||||
return None, "no CI configured on this project"
|
||||
message, remediate = self._ci_status_block_message(state, status)
|
||||
return (
|
||||
await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=message,
|
||||
remediate=remediate,
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pr_pass",
|
||||
),
|
||||
None,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _ci_status_block_message(
|
||||
state: str | None, status: dict[str, Any]
|
||||
) -> tuple[str, str]:
|
||||
"""(message, remediate) for a blocking CI state — failure / pending /
|
||||
pending_not_scheduled / error (the non-terminal, non-green states).
|
||||
|
||||
pending/unscheduled/error are framed as retryable (wait and call
|
||||
pr_pass again), never as a defect to route back to the dev.
|
||||
"""
|
||||
if state == "failure":
|
||||
names = (
|
||||
", ".join(status.get("failing_checks") or []) or "one or more checks"
|
||||
)
|
||||
return (
|
||||
f"CI is failing on the assembled PR's head commit — {names}",
|
||||
f"call pr_fail(issues=['CI failing: {names}']) so the PR returns "
|
||||
"to needs_revision and the dev fixes the failing check(s) — do "
|
||||
"NOT pr_pass on red CI",
|
||||
)
|
||||
if state == "pending":
|
||||
return (
|
||||
"CI is still running on the assembled PR's head commit",
|
||||
"wait for CI to finish and call pr_pass again once it's green "
|
||||
"— do NOT pr_pass while checks are still running",
|
||||
)
|
||||
if state == "pending_not_scheduled":
|
||||
return (
|
||||
"CI has not started running on the assembled PR's head commit yet",
|
||||
"wait for CI to be scheduled and call pr_pass again once it's "
|
||||
"green — do NOT pr_pass before any check has run",
|
||||
)
|
||||
# state == "error" (or an unrecognized value) — a genuine GitHub API
|
||||
# failure resolving the signal; never treat this as green.
|
||||
return (
|
||||
"could not determine CI status for the assembled PR (GitHub API error)",
|
||||
"retry pr_pass shortly once the CI status can be resolved; "
|
||||
"if it persists, pr_fail(issues=[...]) to unwedge the PR",
|
||||
)
|
||||
|
||||
def _record_gate_verdict(
|
||||
self,
|
||||
@@ -473,6 +654,7 @@ class PRGateMixin(_Base):
|
||||
issues: tuple[str, ...] = (),
|
||||
*,
|
||||
head_sha: str | None = None,
|
||||
ci_note: str | None = None,
|
||||
) -> None:
|
||||
"""Persist the gate verdict as the canonical ``pr_review`` note.
|
||||
|
||||
@@ -493,25 +675,59 @@ class PRGateMixin(_Base):
|
||||
re-submit the unchanged root — the 2026-06-27 infinite ``pr_fail``
|
||||
re-submit loop. ``None`` (the default) leaves it absent, which the
|
||||
``submit_root`` gate treats as fail-open.
|
||||
|
||||
On ``pr_pass``, ``ci_note`` (set only when the CI-status guard passed
|
||||
through a project with no CI configured) is stamped into the slot's
|
||||
``ci_status`` field — the evidence that the guard ran and deliberately
|
||||
did not block, rather than silently never having checked at all.
|
||||
"""
|
||||
from roboco.foundation.policy.content import ContentValidationError
|
||||
from roboco.services.content_notes import apply_structured_note
|
||||
|
||||
verdict = "passed" if verb == "pr_pass" else "failed"
|
||||
summary = self._gate_verdict_summary(verb, notes, issues)
|
||||
payload = self._gate_verdict_payload(
|
||||
verdict, summary, issues, verb, head_sha=head_sha, ci_note=ci_note
|
||||
)
|
||||
try:
|
||||
apply_structured_note(t, "pr_review", payload)
|
||||
except ContentValidationError:
|
||||
logger.warning(
|
||||
"gate verdict note skipped (invalid content)",
|
||||
verb=verb,
|
||||
task_id=str(getattr(t, "id", "")),
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _gate_verdict_summary(verb: str, notes: str, issues: tuple[str, ...]) -> str:
|
||||
"""The verdict note's ``summary`` field.
|
||||
|
||||
The free-text ``issues`` render under their own ``## Issues`` section
|
||||
(render_markdown). Baking them into ``summary`` too duplicated each
|
||||
issue on the Task Details "PR Reviewer Notes" card (once under
|
||||
## Summary, once under ## Issues). The summary is a substantive
|
||||
non-issues sentence; ``notes`` (with the issues) still drives the
|
||||
GitHub PR post and the a2a to the owning PM — those are raw text,
|
||||
not rendered through render_markdown, so no duplication there.
|
||||
"""
|
||||
if verb == "pr_fail" and issues:
|
||||
# The free-text issues render under their own ``## Issues`` section
|
||||
# (render_markdown). Baking them into ``summary`` too duplicated each
|
||||
# issue on the Task Details "PR Reviewer Notes" card (once under
|
||||
# ## Summary, once under ## Issues). The summary is a substantive
|
||||
# non-issues sentence; ``notes`` (with the issues) still drives the
|
||||
# GitHub PR post and the a2a to the owning PM — those are raw text,
|
||||
# not rendered through render_markdown, so no duplication there.
|
||||
summary = (
|
||||
return (
|
||||
f"In-path PR-review gate requested changes - "
|
||||
f"{len(issues)} issue(s) listed below."
|
||||
)
|
||||
else:
|
||||
summary = notes
|
||||
return notes
|
||||
|
||||
@staticmethod
|
||||
def _gate_verdict_payload(
|
||||
verdict: str,
|
||||
summary: str,
|
||||
issues: tuple[str, ...],
|
||||
verb: str,
|
||||
*,
|
||||
head_sha: str | None,
|
||||
ci_note: str | None,
|
||||
) -> dict[str, Any]:
|
||||
"""Assemble the structured ``pr_review`` note payload."""
|
||||
payload: dict[str, Any] = {
|
||||
"summary": summary,
|
||||
"findings": [],
|
||||
@@ -521,14 +737,9 @@ class PRGateMixin(_Base):
|
||||
payload["issues"] = list(issues)
|
||||
if verb == "pr_fail" and head_sha:
|
||||
payload["head_sha"] = head_sha
|
||||
try:
|
||||
apply_structured_note(t, "pr_review", payload)
|
||||
except ContentValidationError:
|
||||
logger.warning(
|
||||
"gate verdict note skipped (invalid content)",
|
||||
verb=verb,
|
||||
task_id=str(getattr(t, "id", "")),
|
||||
)
|
||||
if verb == "pr_pass" and ci_note:
|
||||
payload["ci_status"] = ci_note
|
||||
return payload
|
||||
|
||||
async def _capture_pr_head_sha(self, t: Any) -> str | None:
|
||||
"""Best-effort capture of the assembled PR's head SHA at ``pr_fail`` time.
|
||||
|
||||
@@ -327,6 +327,17 @@ _CI_RETRYABLE_STATUS = frozenset({429, 500, 502, 503, 504})
|
||||
# Cap a conventions-validator run so a hung subprocess (tree-sitter deadlock,
|
||||
# huge repo) can't hang the i_am_done/pr_pass gate forever.
|
||||
_CONVENTIONS_VALIDATOR_TIMEOUT_SECONDS = 120
|
||||
# --- pr_pass CI-status guard ------------------------------------------------
|
||||
# GitHub check-run conclusions that count as a failing check on a PR's head
|
||||
# commit. ``neutral``/``skipped``/``success`` (and ``None`` on a still-running
|
||||
# run) are not failing.
|
||||
_FAILING_CHECK_CONCLUSIONS = frozenset(
|
||||
{"failure", "cancelled", "timed_out", "action_required"}
|
||||
)
|
||||
# A 404 resolving a PR's head SHA means the repo/PR is unreachable or doesn't
|
||||
# exist — classified as no_ci_configured, distinct from any other non-2xx
|
||||
# (a genuine API failure on a real, reachable repo) which is `error`.
|
||||
_HTTP_NOT_FOUND = 404
|
||||
|
||||
|
||||
def _select_ci_head_run(runs: list[dict[str, Any]]) -> dict[str, Any]:
|
||||
@@ -2655,6 +2666,254 @@ class GitService(BaseService):
|
||||
)
|
||||
return None
|
||||
|
||||
async def get_pr_ci_status(
|
||||
self, project_slug: str, pr_number: int
|
||||
) -> dict[str, Any] | None:
|
||||
"""CI status of a PR's current head commit, for the in-path ``pr_pass`` gate.
|
||||
|
||||
Reads GitHub check-runs on the PR's head SHA — the same signal GitHub
|
||||
Actions surfaces on the PR page — and classifies it into one
|
||||
``state``: ``success`` (every check-run completed with no failing
|
||||
conclusion), ``failure`` (at least one completed check-run failed —
|
||||
``failing_checks`` names them), ``pending`` (at least one check-run
|
||||
has not completed yet), ``pending_not_scheduled`` (zero check-runs
|
||||
exist for this commit but the repo has workflows configured — CI
|
||||
just hasn't started), ``no_ci_configured`` (zero check-runs AND the
|
||||
repo has no workflows at all — or the project/git_url/token is
|
||||
missing, or the repo/PR is unreachable/nonexistent), or ``error`` (a
|
||||
genuine GitHub API failure on a real, reachable repo).
|
||||
|
||||
Every unresolvable case is now classified explicitly rather than
|
||||
returning ``None``: a missing project/git_url/git-token, or an
|
||||
unreachable/nonexistent repo or PR (a network failure or a 404 on
|
||||
the PR-head, check-runs, or workflows lookup) all classify as
|
||||
``no_ci_configured`` — ``pr_pass`` passes through cleanly and still
|
||||
stamps the evidence note, instead of silently skipping the guard. A
|
||||
genuine GitHub API failure on a real, reachable repo (any other
|
||||
non-2xx, or an unparseable response) classifies as ``error`` so
|
||||
``pr_pass`` stays fail-closed and retryable. This mirrors ``get_pr_head_sha`` /
|
||||
``_capture_pr_head_sha`` in spirit (never mistake a configuration gap
|
||||
for a CI signal) but resolves head-sha lookups via a dedicated helper
|
||||
so the two failure classes above stay distinguishable — the shared
|
||||
``get_pr_head_sha`` (used by the unrelated pr_fail head-sha capture)
|
||||
is untouched.
|
||||
|
||||
ponytail: reads GitHub check-runs only (the project's own CI is
|
||||
GitHub Actions). A repo whose only signal is the legacy commit-status
|
||||
API would show zero check-runs here; add a statuses fallback if that
|
||||
ever becomes a real CI provider for a project.
|
||||
"""
|
||||
config = await self._ci_status_config(project_slug)
|
||||
if isinstance(config, dict):
|
||||
return config
|
||||
owner, repo, headers = config
|
||||
head_sha_or_gap = await self._resolve_ci_head_sha(
|
||||
project_slug, pr_number, owner, repo, headers
|
||||
)
|
||||
if isinstance(head_sha_or_gap, dict):
|
||||
return head_sha_or_gap
|
||||
head_sha = head_sha_or_gap
|
||||
check_runs = await self._fetch_check_runs(
|
||||
project_slug, owner, repo, head_sha, headers
|
||||
)
|
||||
if isinstance(check_runs, dict):
|
||||
return check_runs
|
||||
if check_runs:
|
||||
return self._classify_check_runs(check_runs, head_sha)
|
||||
return await self._classify_zero_check_runs(
|
||||
project_slug, owner, repo, head_sha, headers
|
||||
)
|
||||
|
||||
async def _ci_status_config(
|
||||
self, project_slug: str
|
||||
) -> tuple[str, str, dict[str, str]] | dict[str, Any]:
|
||||
"""Resolve ``(owner, repo, auth headers)`` for a CI-status lookup, or
|
||||
a terminal ``no_ci_configured`` gap dict when the project, its
|
||||
git_url, or a git token is missing, or the git_url doesn't parse."""
|
||||
project = await get_project_service(self.session).get_by_slug(project_slug)
|
||||
if project is None or not project.git_url:
|
||||
return {"state": "no_ci_configured", "head_sha": None}
|
||||
try:
|
||||
owner, repo = self._parse_git_url(project.git_url)
|
||||
except GitError:
|
||||
return {"state": "no_ci_configured", "head_sha": None}
|
||||
git_token = await self._token_for_project(project_slug)
|
||||
if not git_token:
|
||||
return {"state": "no_ci_configured", "head_sha": None}
|
||||
headers = {
|
||||
"Authorization": f"Bearer {git_token}",
|
||||
"Accept": "application/vnd.github+json",
|
||||
"X-GitHub-Api-Version": "2022-11-28",
|
||||
}
|
||||
return owner, repo, headers
|
||||
|
||||
async def _resolve_ci_head_sha(
|
||||
self,
|
||||
project_slug: str,
|
||||
pr_number: int,
|
||||
owner: str,
|
||||
repo: str,
|
||||
headers: dict[str, str],
|
||||
) -> str | dict[str, Any]:
|
||||
"""Resolve the PR's head SHA for ``get_pr_ci_status`` specifically.
|
||||
|
||||
Returns the head SHA (``str``) on success, or a terminal gap-state
|
||||
dict to return directly from the caller on failure. A network error
|
||||
or a 404 (the repo or PR doesn't exist / isn't reachable) is
|
||||
``no_ci_configured`` — there is no way to determine a CI signal, so
|
||||
the guard should pass through, not block. Any other non-2xx or an
|
||||
unparseable response is a real, reachable repo whose API call itself
|
||||
failed, so it is ``error`` — a retryable signal the guard must not
|
||||
treat as green.
|
||||
"""
|
||||
try:
|
||||
async with httpx.AsyncClient(timeout=_default_git_timeout()) as client:
|
||||
resp = await client.get(
|
||||
f"{_api_base()}/repos/{owner}/{repo}/pulls/{pr_number}",
|
||||
headers=headers,
|
||||
)
|
||||
except httpx.HTTPError as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status pr lookup unreachable",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "no_ci_configured", "head_sha": None}
|
||||
if resp.status_code == _HTTP_NOT_FOUND:
|
||||
return {"state": "no_ci_configured", "head_sha": None}
|
||||
if not resp.is_success:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status pr lookup non-2xx",
|
||||
project=project_slug,
|
||||
status=resp.status_code,
|
||||
)
|
||||
return {"state": "error", "head_sha": None}
|
||||
try:
|
||||
return str(resp.json()["head"]["sha"])
|
||||
except (ValueError, KeyError, TypeError) as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status pr lookup parse failed",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "error", "head_sha": None}
|
||||
|
||||
async def _fetch_check_runs(
|
||||
self,
|
||||
project_slug: str,
|
||||
owner: str,
|
||||
repo: str,
|
||||
head_sha: str,
|
||||
headers: dict[str, str],
|
||||
) -> list[dict[str, Any]] | dict[str, Any]:
|
||||
"""GET the check-runs for ``head_sha``.
|
||||
|
||||
Returns the (possibly empty) check-runs list on success, or a
|
||||
terminal gap-state dict to return directly from the caller: a 404
|
||||
(the repo/commit isn't reachable, e.g. no CI integration at all) is
|
||||
``no_ci_configured``; any other failure (network, non-2xx,
|
||||
unparseable body) is ``error``.
|
||||
"""
|
||||
try:
|
||||
async with httpx.AsyncClient(timeout=_default_git_timeout()) as client:
|
||||
resp = await client.get(
|
||||
f"{_api_base()}/repos/{owner}/{repo}/commits/{head_sha}/check-runs",
|
||||
headers=headers,
|
||||
params={"per_page": 100},
|
||||
)
|
||||
except httpx.HTTPError as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status check-runs request failed",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
if resp.status_code == _HTTP_NOT_FOUND:
|
||||
return {"state": "no_ci_configured", "head_sha": head_sha}
|
||||
if not resp.is_success:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status check-runs non-2xx",
|
||||
project=project_slug,
|
||||
status=resp.status_code,
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
try:
|
||||
runs = resp.json().get("check_runs")
|
||||
except (ValueError, AttributeError) as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status check-runs parse failed",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
return runs if isinstance(runs, list) else []
|
||||
|
||||
@staticmethod
|
||||
def _classify_check_runs(
|
||||
check_runs: list[dict[str, Any]], head_sha: str
|
||||
) -> dict[str, Any]:
|
||||
"""Map a non-empty check-runs list to a failure/pending/success state."""
|
||||
failing = [
|
||||
str(cr.get("name") or "check")
|
||||
for cr in check_runs
|
||||
if cr.get("status") == "completed"
|
||||
and cr.get("conclusion") in _FAILING_CHECK_CONCLUSIONS
|
||||
]
|
||||
if failing:
|
||||
return {"state": "failure", "failing_checks": failing, "head_sha": head_sha}
|
||||
if any(cr.get("status") != "completed" for cr in check_runs):
|
||||
return {"state": "pending", "head_sha": head_sha}
|
||||
return {"state": "success", "head_sha": head_sha}
|
||||
|
||||
async def _classify_zero_check_runs(
|
||||
self,
|
||||
project_slug: str,
|
||||
owner: str,
|
||||
repo: str,
|
||||
head_sha: str,
|
||||
headers: dict[str, str],
|
||||
) -> dict[str, Any]:
|
||||
"""No check-runs exist yet for ``head_sha`` — tell "not scheduled" apart
|
||||
from "no CI configured" by asking whether the repo has any workflows.
|
||||
|
||||
A 404 here (repo unreachable/nonexistent — no CI integration) is
|
||||
``no_ci_configured``; any other failure is ``error``.
|
||||
"""
|
||||
try:
|
||||
async with httpx.AsyncClient(timeout=_default_git_timeout()) as client:
|
||||
resp = await client.get(
|
||||
f"{_api_base()}/repos/{owner}/{repo}/actions/workflows",
|
||||
headers=headers,
|
||||
params={"per_page": 1},
|
||||
)
|
||||
except httpx.HTTPError as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status workflows request failed",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
if resp.status_code == _HTTP_NOT_FOUND:
|
||||
return {"state": "no_ci_configured", "head_sha": head_sha}
|
||||
if not resp.is_success:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status workflows non-2xx",
|
||||
project=project_slug,
|
||||
status=resp.status_code,
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
try:
|
||||
total = int(resp.json().get("total_count") or 0)
|
||||
except (ValueError, AttributeError, TypeError) as e:
|
||||
self.log.warning(
|
||||
"get_pr_ci_status workflows parse failed",
|
||||
project=project_slug,
|
||||
error=str(e),
|
||||
)
|
||||
return {"state": "error", "head_sha": head_sha}
|
||||
state = "pending_not_scheduled" if total > 0 else "no_ci_configured"
|
||||
return {"state": state, "head_sha": head_sha}
|
||||
|
||||
async def update_pr_for_task(
|
||||
self,
|
||||
task_id: UUID,
|
||||
|
||||
@@ -227,6 +227,23 @@ def _fake_github_router(gh: _FakeGitHub) -> APIRouter:
|
||||
with suppress(subprocess.CalledProcessError):
|
||||
_git(gh.origin, "branch", "-D", branch)
|
||||
|
||||
# A minimal green check-runs signal for any commit — real-life NAS
|
||||
# projects have CI, so the pr_pass CI-status guard should see a
|
||||
# `success` state and exercise its pass-through branch, not the
|
||||
# no_ci_configured 404 path.
|
||||
@r.get("/repos/{owner}/{repo}/commits/{sha}/check-runs")
|
||||
async def check_runs(owner: str, repo: str, sha: str) -> dict[str, Any]:
|
||||
return {
|
||||
"total_count": 1,
|
||||
"check_runs": [
|
||||
{"name": "ci", "status": "completed", "conclusion": "success"}
|
||||
],
|
||||
}
|
||||
|
||||
@r.get("/repos/{owner}/{repo}/actions/workflows")
|
||||
async def workflows(owner: str, repo: str) -> dict[str, Any]:
|
||||
return {"total_count": 1, "workflows": [{"id": 1, "name": "ci"}]}
|
||||
|
||||
return r
|
||||
|
||||
|
||||
|
||||
@@ -194,9 +194,11 @@ class TestPrPassBlockedThreadsParent:
|
||||
cc._conventions_guard = AsyncMock(return_value=None)
|
||||
reviewer_id = uuid4()
|
||||
|
||||
result = await c._pr_pass_blocked(reviewer_id, uuid4(), t, "pr_reviewer", {})
|
||||
rejection, _ci_note = await c._pr_pass_blocked(
|
||||
reviewer_id, uuid4(), t, "pr_reviewer", {}
|
||||
)
|
||||
|
||||
assert result is None
|
||||
assert rejection is None
|
||||
cc._conventions_guard.assert_awaited_once_with(
|
||||
reviewer_id,
|
||||
t,
|
||||
@@ -219,9 +221,11 @@ class TestPrPassBlockedThreadsParent:
|
||||
cc._toolchain_broken_guard = AsyncMock(return_value=None)
|
||||
cc._conventions_guard = AsyncMock(return_value=None)
|
||||
|
||||
result = await c._pr_pass_blocked(uuid4(), uuid4(), t, "pr_reviewer", {})
|
||||
rejection, _ci_note = await c._pr_pass_blocked(
|
||||
uuid4(), uuid4(), t, "pr_reviewer", {}
|
||||
)
|
||||
|
||||
assert result is None
|
||||
assert rejection is None
|
||||
task_service.get.assert_not_called()
|
||||
cc._conventions_guard.assert_awaited_once()
|
||||
assert cc._conventions_guard.await_args.kwargs.get("preferred_parent") is None
|
||||
|
||||
@@ -69,10 +69,13 @@ def _stub_gate_path(
|
||||
)
|
||||
cc._gate_tracing = AsyncMock(return_value=None)
|
||||
# These tests exercise the pr_fail a2a / notify path, not the head-sha
|
||||
# capture (which has its own suite in test_submit_root_unchanged_pr_guard).
|
||||
# Stub the capture so it does not walk the mock session into un-awaited
|
||||
# coroutines; the verdict still lands via the _record_gate_verdict spy.
|
||||
# capture (which has its own suite in test_submit_root_unchanged_pr_guard)
|
||||
# or the pr_pass CI-status guard (its own suite in
|
||||
# test_pr_pass_ci_status_guard). Stub both so they do not walk the mock
|
||||
# session into un-awaited coroutines; the verdict still lands via the
|
||||
# _record_gate_verdict spy.
|
||||
cc._capture_pr_head_sha = AsyncMock(return_value=None)
|
||||
cc._project_slug_for = AsyncMock(return_value=None)
|
||||
cc._record_gate_verdict = MagicMock()
|
||||
cc._post_gate_review_to_pr = AsyncMock()
|
||||
runner = MagicMock()
|
||||
|
||||
@@ -0,0 +1,300 @@
|
||||
"""pr_pass refuses to pass an assembled PR unless CI on its head commit is green.
|
||||
|
||||
Before this guard, ``pr_pass`` had no CI-status check at all — a reviewer could
|
||||
pass an assembled PR whose CI was red, still running, or not yet scheduled.
|
||||
``_ci_status_guard`` (wired into ``_pr_pass_blocked`` alongside the existing
|
||||
toolchain/conventions guards) reads ``GitService.get_pr_ci_status`` and blocks
|
||||
on failure/pending/pending_not_scheduled/error with reviewer-aware remediation
|
||||
(``pr_fail``, never ``i_am_blocked`` — a reviewer has no such verb). A project
|
||||
with no CI configured at all passes through cleanly, stamping the verdict note
|
||||
with why the guard did not block. ``pr_fail`` is unaffected by CI state
|
||||
entirely, and the separate inbound ``PRReviewerMixin`` surface
|
||||
(``claim_pr_review`` / ``post_pr_review``) never consults CI status at all.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import inspect
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.foundation.policy import lifecycle as spec_module
|
||||
from roboco.services.gateway.choreographer import (
|
||||
Choreographer,
|
||||
ChoreographerDeps,
|
||||
pr_review,
|
||||
)
|
||||
|
||||
|
||||
def _make_choreographer() -> Choreographer:
|
||||
base: dict[str, Any] = {
|
||||
"task": AsyncMock(),
|
||||
"work_session": AsyncMock(),
|
||||
"git": AsyncMock(),
|
||||
"a2a": AsyncMock(),
|
||||
"journal": AsyncMock(),
|
||||
"audit": AsyncMock(),
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
return Choreographer(ChoreographerDeps(**base))
|
||||
|
||||
|
||||
def _stub_gate_path(
|
||||
c: Choreographer, *, reviewer_id: Any, t_before: Any, t_after: Any
|
||||
) -> MagicMock:
|
||||
"""Drive ``_gate_decision`` past preflight/tracing and into the real
|
||||
``_pr_pass_blocked`` -> ``_ci_status_guard`` path — only the ownership/
|
||||
tracing plumbing is stubbed (it has its own tests); the CI guard under
|
||||
test runs for real. Mirrors ``test_pr_gate_notifies_pm._stub_gate_path``.
|
||||
"""
|
||||
agent = MagicMock(role="pr_reviewer", slug="be-pr-reviewer")
|
||||
cc: Any = c
|
||||
cc._gate_preflight = AsyncMock(
|
||||
return_value=(
|
||||
t_before,
|
||||
agent,
|
||||
"pr_reviewer",
|
||||
{},
|
||||
spec_module.Context(actor_id=reviewer_id),
|
||||
)
|
||||
)
|
||||
cc._gate_tracing = AsyncMock(return_value=None)
|
||||
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
||||
record_spy = MagicMock()
|
||||
cc._record_gate_verdict = record_spy
|
||||
cc._post_gate_review_to_pr = AsyncMock()
|
||||
runner = MagicMock()
|
||||
runner.run_intent = AsyncMock(return_value=t_after)
|
||||
cc._verb_runner = MagicMock(return_value=runner)
|
||||
return record_spy
|
||||
|
||||
|
||||
def _t(*, status: str = "awaiting_pr_review", pr_number: int | None = 42) -> MagicMock:
|
||||
return MagicMock(
|
||||
id=uuid4(),
|
||||
assigned_to=None,
|
||||
pr_number=pr_number,
|
||||
parent_task_id=uuid4(),
|
||||
status=status,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The six CI-guard branches, exercised through pr_pass
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_blocked_on_failing_ci() -> None:
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||
c.git.get_pr_ci_status = AsyncMock(
|
||||
return_value={"state": "failure", "failing_checks": ["tests"]}
|
||||
)
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
assert "CI is failing" in (env.message or "")
|
||||
assert "tests" in (env.message or "")
|
||||
assert "pr_fail" in (env.remediate or "")
|
||||
c.task.get.assert_not_called() # never reached the runner
|
||||
cc: Any = c
|
||||
cc._record_gate_verdict.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_blocked_on_pending_ci() -> None:
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "pending"})
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
assert "still running" in (env.message or "")
|
||||
assert "wait" in (env.remediate or "").lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_blocked_on_zero_checks_workflows_pending() -> None:
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "pending_not_scheduled"})
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
assert "has not started" in (env.message or "")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_blocked_on_github_api_error() -> None:
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "error"})
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
assert "GitHub API error" in (env.message or "")
|
||||
assert "retry" in (env.remediate or "").lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_succeeds_on_all_green() -> None:
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
t_after = _t(status="awaiting_pm_review")
|
||||
c = _make_choreographer()
|
||||
record_spy = _stub_gate_path(
|
||||
c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after
|
||||
)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "success"})
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error is None, env.as_dict()
|
||||
assert env.status == "awaiting_pm_review"
|
||||
record_spy.assert_called_once()
|
||||
# No CI note stamped when the guard passed because CI was actually green.
|
||||
assert record_spy.call_args.kwargs.get("ci_note") is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_passes_through_when_no_ci_configured_and_stamps_evidence() -> (
|
||||
None
|
||||
):
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
t_after = _t(status="awaiting_pm_review")
|
||||
c = _make_choreographer()
|
||||
record_spy = _stub_gate_path(
|
||||
c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after
|
||||
)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "no_ci_configured"})
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error is None, env.as_dict()
|
||||
assert env.status == "awaiting_pm_review"
|
||||
record_spy.assert_called_once()
|
||||
assert (
|
||||
record_spy.call_args.kwargs.get("ci_note") == "no CI configured on this project"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_pass_fails_open_when_ci_status_unresolvable() -> None:
|
||||
"""A configuration gap (no resolvable project/token/head sha) -> None from
|
||||
get_pr_ci_status -> the guard never blocks (fail open, matches the other
|
||||
pr_pass guards' posture on an unresolvable signal)."""
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
t_after = _t(status="awaiting_pm_review")
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value=None)
|
||||
|
||||
env = await c.pr_pass(reviewer_id, t_before.id, "Looks clean to me.")
|
||||
|
||||
assert env.error is None, env.as_dict()
|
||||
assert env.status == "awaiting_pm_review"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression: pr_fail is unaffected by CI state entirely
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_fail_succeeds_regardless_of_ci_state() -> None:
|
||||
"""pr_fail must never consult get_pr_ci_status — the CI guard lives only in
|
||||
the pr_pass branch of _gate_decision."""
|
||||
reviewer_id = uuid4()
|
||||
t_before = _t()
|
||||
t_after = _t(status="needs_revision")
|
||||
c = _make_choreographer()
|
||||
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
|
||||
# Even if configured to report red CI, pr_fail must not care — and must not
|
||||
# even call it.
|
||||
c.git.get_pr_ci_status = AsyncMock(return_value={"state": "failure"})
|
||||
|
||||
env = await c.pr_fail(reviewer_id, t_before.id, ["a concrete actionable issue"])
|
||||
|
||||
assert env.error is None, env.as_dict()
|
||||
assert env.status == "needs_revision"
|
||||
c.git.get_pr_ci_status.assert_not_awaited()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression: the inbound PRReviewerMixin surface never consults CI status
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_pr_review_mixin_has_no_ci_status_coupling() -> None:
|
||||
"""claim_pr_review / post_pr_review (the external inbound review surface,
|
||||
``PRReviewerMixin`` in pr_review.py) must stay completely untouched by the
|
||||
new CI-status guard — it is wired only into ``PRGateMixin.pr_pass``
|
||||
(the in-path assembled-PR gate) via ``_pr_pass_blocked``. A source-level
|
||||
check is the most robust regression here: any accidental import or call of
|
||||
``get_pr_ci_status`` / ``_ci_status_guard`` into the inbound mixin fails
|
||||
this immediately, regardless of how its heavier claim/decision plumbing
|
||||
(self_review_block, tracing, content gates) evolves."""
|
||||
source = inspect.getsource(pr_review)
|
||||
assert "get_pr_ci_status" not in source
|
||||
assert "_ci_status_guard" not in source
|
||||
assert not hasattr(pr_review.PRReviewerMixin, "_ci_status_guard")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression-lock: this task's 7 ACs mapped to the test(s) that cover each
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_ac_coverage_map_and_pr_reviewer_prompt_states_ci_guard() -> None:
|
||||
"""Explicit AC-to-test mapping for this task's 7 acceptance criteria.
|
||||
|
||||
AC1 (CI failure names the failing check) ->
|
||||
test_pr_pass_blocked_on_failing_ci (this file, line ~90)
|
||||
AC2 (pending vs error are distinct invalid_state envelopes with a
|
||||
different remediate text) -> test_pr_pass_blocked_on_pending_ci
|
||||
(~111), test_pr_pass_blocked_on_github_api_error (~140)
|
||||
AC3 (pending_not_scheduled is the retryable not-yet-scheduled case) ->
|
||||
test_pr_pass_blocked_on_zero_checks_workflows_pending (~126)
|
||||
AC4 (no_ci_configured passes through + stamps the ci_status verdict
|
||||
note) -> test_pr_pass_passes_through_when_no_ci_configured_and_
|
||||
stamps_evidence (~175)
|
||||
AC5 (all-green CI passes through with no note) ->
|
||||
test_pr_pass_succeeds_on_all_green (~155)
|
||||
AC6 (pr_fail and the inbound PRReviewerMixin have zero CI-status
|
||||
coupling) -> test_pr_fail_succeeds_regardless_of_ci_state (~221),
|
||||
test_pr_review_mixin_has_no_ci_status_coupling (~245)
|
||||
AC7 (the pr_reviewer prompt states the per-AC evidence-walk + CI-status
|
||||
guard section) -> asserted directly below. Previously verified only
|
||||
by manual reading during self-verification, with no test-level
|
||||
regression lock — this closes that gap.
|
||||
"""
|
||||
prompt_path = (
|
||||
Path(__file__).resolve().parents[3]
|
||||
/ "agents"
|
||||
/ "prompts"
|
||||
/ "roles"
|
||||
/ "pr_reviewer.md"
|
||||
)
|
||||
text = prompt_path.read_text(encoding="utf-8")
|
||||
assert "per-AC evidence-walk" in text
|
||||
assert "named-deliverable/silent-drop rule" in text
|
||||
assert "CI status:" in text
|
||||
assert 'ci_status: "no CI configured on this project"' in text
|
||||
@@ -0,0 +1,340 @@
|
||||
"""GitService.get_pr_ci_status — the CI signal behind the pr_pass gate.
|
||||
|
||||
Reads GitHub check-runs on a PR's head SHA (falling back to list-workflows
|
||||
when zero check-runs exist yet) and classifies the result into one of:
|
||||
success, failure, pending, pending_not_scheduled, no_ci_configured, error.
|
||||
Every unresolvable case is classified explicitly — a missing project/git_url/
|
||||
token, or an unreachable/nonexistent repo or PR, is ``no_ci_configured``
|
||||
(the guard passes through with an evidence stamp); a genuine GitHub API
|
||||
failure on a real, reachable repo is ``error`` (the guard stays fail-closed).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
from roboco.services.git import GitService
|
||||
|
||||
_PR = 42
|
||||
_SHA = "deadbeefcafebabe0000111122223333aaaabbbb"
|
||||
_HTTP_SUCCESS_RANGE = range(200, 300)
|
||||
|
||||
|
||||
def _service() -> GitService:
|
||||
session = MagicMock()
|
||||
session.execute = AsyncMock()
|
||||
svc = GitService(session)
|
||||
object.__setattr__(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
return svc
|
||||
|
||||
|
||||
def _resp(status_code: int, *, json_payload: Any = None) -> MagicMock:
|
||||
resp = MagicMock()
|
||||
resp.status_code = status_code
|
||||
resp.is_success = status_code in _HTTP_SUCCESS_RANGE
|
||||
resp.json.return_value = json_payload
|
||||
return resp
|
||||
|
||||
|
||||
def _client(*get_responses: MagicMock) -> MagicMock:
|
||||
"""A fake httpx.AsyncClient whose ``.get`` serves responses in call order —
|
||||
every ``async with httpx.AsyncClient(...) as client`` in the service reuses
|
||||
the SAME instance (the patch target returns it unconditionally), so a list
|
||||
``side_effect`` lines up with the sequential PR-head / check-runs /
|
||||
workflows calls."""
|
||||
client = MagicMock()
|
||||
client.__aenter__ = AsyncMock(return_value=client)
|
||||
client.__aexit__ = AsyncMock(return_value=False)
|
||||
client.get = AsyncMock(side_effect=list(get_responses))
|
||||
return client
|
||||
|
||||
|
||||
def _patch_project() -> Any:
|
||||
fake = MagicMock()
|
||||
fake.get_by_slug = AsyncMock(
|
||||
return_value=MagicMock(git_url="https://github.com/acme/repo.git")
|
||||
)
|
||||
return patch("roboco.services.git.get_project_service", return_value=fake)
|
||||
|
||||
|
||||
def _pr_head_resp() -> MagicMock:
|
||||
return _resp(200, json_payload={"head": {"sha": _SHA}})
|
||||
|
||||
|
||||
def _check_run(name: str, *, status: str, conclusion: str | None) -> dict[str, Any]:
|
||||
return {"name": name, "status": status, "conclusion": conclusion}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Six CI-guard branches
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_all_checks_green_is_success() -> None:
|
||||
checks = _resp(
|
||||
200,
|
||||
json_payload={
|
||||
"check_runs": [
|
||||
_check_run("lint", status="completed", conclusion="success"),
|
||||
_check_run("tests", status="completed", conclusion="neutral"),
|
||||
]
|
||||
},
|
||||
)
|
||||
client = _client(_pr_head_resp(), checks)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "success", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_failing_check_names_it() -> None:
|
||||
checks = _resp(
|
||||
200,
|
||||
json_payload={
|
||||
"check_runs": [
|
||||
_check_run("lint", status="completed", conclusion="success"),
|
||||
_check_run("tests", status="completed", conclusion="failure"),
|
||||
]
|
||||
},
|
||||
)
|
||||
client = _client(_pr_head_resp(), checks)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out is not None
|
||||
assert out["state"] == "failure"
|
||||
assert out["failing_checks"] == ["tests"]
|
||||
assert out["head_sha"] == _SHA
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_still_running_check_is_pending() -> None:
|
||||
checks = _resp(
|
||||
200,
|
||||
json_payload={
|
||||
"check_runs": [
|
||||
_check_run("lint", status="completed", conclusion="success"),
|
||||
_check_run("tests", status="in_progress", conclusion=None),
|
||||
]
|
||||
},
|
||||
)
|
||||
client = _client(_pr_head_resp(), checks)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "pending", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_runs_api_error_is_error_state() -> None:
|
||||
checks = _resp(500, json_payload={"message": "internal error"})
|
||||
client = _client(_pr_head_resp(), checks)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_zero_checks_with_no_workflows_is_no_ci_configured() -> None:
|
||||
checks = _resp(200, json_payload={"check_runs": []})
|
||||
workflows = _resp(200, json_payload={"total_count": 0})
|
||||
client = _client(_pr_head_resp(), checks, workflows)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_zero_checks_with_workflows_configured_is_pending_not_scheduled() -> None:
|
||||
checks = _resp(200, json_payload={"check_runs": []})
|
||||
workflows = _resp(200, json_payload={"total_count": 3})
|
||||
client = _client(_pr_head_resp(), checks, workflows)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "pending_not_scheduled", "head_sha": _SHA}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Config gaps and an unreachable/nonexistent repo pass through cleanly as
|
||||
# no_ci_configured (pr_pass stamps the evidence note, never mistaken for a
|
||||
# CI signal)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_on_missing_token() -> None:
|
||||
svc = _service()
|
||||
object.__setattr__(svc, "_token_for_project", AsyncMock(return_value=None))
|
||||
with _patch_project():
|
||||
out = await svc.get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_on_missing_project() -> None:
|
||||
fake = MagicMock()
|
||||
fake.get_by_slug = AsyncMock(return_value=None)
|
||||
with patch("roboco.services.git.get_project_service", return_value=fake):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_when_pr_lookup_404s() -> None:
|
||||
# The PR lookup itself 404s — the repo/PR doesn't exist or isn't reachable.
|
||||
pr_lookup = _resp(404, json_payload={"message": "not found"})
|
||||
client = _client(pr_lookup)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_when_pr_lookup_unreachable() -> None:
|
||||
# A connection/network failure resolving the PR's head SHA — the repo is
|
||||
# unreachable, not just returning an error response.
|
||||
client = MagicMock()
|
||||
client.__aenter__ = AsyncMock(return_value=client)
|
||||
client.__aexit__ = AsyncMock(return_value=False)
|
||||
client.get = AsyncMock(side_effect=httpx.ConnectError("connection refused"))
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": None}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# A real, reachable repo's genuinely failing GitHub API call stays
|
||||
# fail-closed (error), never conflated with the no_ci_configured cases above
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_error_when_pr_lookup_api_fails_on_real_repo() -> None:
|
||||
# The repo/project/token all resolve fine, but the PR-head-sha lookup
|
||||
# itself returns a genuine 5xx — this must stay fail-closed (error), not
|
||||
# be conflated with the unreachable/nonexistent-repo no_ci_configured case.
|
||||
pr_lookup = _resp(500, json_payload={"message": "internal error"})
|
||||
client = _client(pr_lookup)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_error_when_pr_lookup_body_unparseable() -> None:
|
||||
pr_lookup = _resp(200, json_payload={"head": {}}) # missing "sha" key
|
||||
client = _client(pr_lookup)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_workflows_api_error_after_zero_checks_is_error_state() -> None:
|
||||
checks = _resp(200, json_payload={"check_runs": []})
|
||||
workflows = _resp(503, json_payload={"message": "busy"})
|
||||
client = _client(_pr_head_resp(), checks, workflows)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": _SHA}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# A 404 on the check-runs or workflows endpoint means the repo has no CI
|
||||
# integration at all (e.g. the e2e harness's fake GitHub with no routes
|
||||
# mounted for either) — no_ci_configured, never mistaken for error. 500s and
|
||||
# network failures on the same two endpoints stay fail-closed (error).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_when_check_runs_404s() -> None:
|
||||
checks = _resp(404, json_payload={"message": "not found"})
|
||||
client = _client(_pr_head_resp(), checks)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_error_when_check_runs_request_times_out() -> None:
|
||||
client = MagicMock()
|
||||
client.__aenter__ = AsyncMock(return_value=client)
|
||||
client.__aexit__ = AsyncMock(return_value=False)
|
||||
client.get = AsyncMock(
|
||||
side_effect=[_pr_head_resp(), httpx.ReadTimeout("timed out")]
|
||||
)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_ci_configured_when_workflows_404s() -> None:
|
||||
checks = _resp(200, json_payload={"check_runs": []})
|
||||
workflows = _resp(404, json_payload={"message": "not found"})
|
||||
client = _client(_pr_head_resp(), checks, workflows)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "no_ci_configured", "head_sha": _SHA}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_error_when_workflows_request_times_out() -> None:
|
||||
checks = _resp(200, json_payload={"check_runs": []})
|
||||
client = MagicMock()
|
||||
client.__aenter__ = AsyncMock(return_value=client)
|
||||
client.__aexit__ = AsyncMock(return_value=False)
|
||||
client.get = AsyncMock(
|
||||
side_effect=[_pr_head_resp(), checks, httpx.ReadTimeout("timed out")]
|
||||
)
|
||||
with (
|
||||
_patch_project(),
|
||||
patch("roboco.services.git.httpx.AsyncClient", return_value=client),
|
||||
):
|
||||
out = await _service().get_pr_ci_status("roboco", _PR)
|
||||
assert out == {"state": "error", "head_sha": _SHA}
|
||||
Reference in New Issue
Block a user