Skip to content

Commit 306fe5c

Browse files
authored
feat: agentic 3-stage PR review pipeline (Investigator + Critic + Reporter) behind flag (#724)
* feat: add agentic 3-stage PR review pipeline (Investigator + Critic + Reporter) behind flag * fix: address review feedback (fail-closed prompts, mid-turn cost cap, repo_root) * refactor: remove cost cap, tighten code, decouple per-tool caps from agent_loop * fix: review feedback — critic-fail escalates, wall-clock cap, path canonicalization, schema normalization * fix: distinguish fatal vs bounded-completion, pipeline-wide wall-clock, canonical paths * refactor: simplify wall-clock, drop fatal flag, inline helpers; add regression test * fix: decouple agentic reporter prompt from legacy reporter prompt * fix: bound Reporter on wall-clock, split trust boundary, drop pointless cachePoint Addresses review feedback on the agentic pipeline. Six iterations of adversarial review converged on these fixes; 243 tests pass (up from 205). Reporter wall-clock bounds. A Reporter call inheriting the default 240s bedrock_timeout could blow the 600s GHA workflow cap when started near the pipeline_deadline. The helper now escalates pre-emptively if remaining budget < cfg.reporter_min_remaining_seconds (default 60s, sized for the 20-45s P95 of JSON-mode responses on slow Bedrock days), and the per-call read_timeout is capped at the remaining budget via a fresh boto client (BedrockClient._client_for_timeout). Single-attempt retry on the fresh client because retry storms aren't useful when wall-clock is tight. Reporter failure escalates. When invoke_with_usage returns None (Bedrock error / circuit-breaker / timeout) OR when the response fails JSON parsing, the helper now returns "reporter_failed" so the pipeline ESCALATEs with the investigator narrative preserved. Previously these paths would have RESPONDed with empty inline_comments, silently dropping confirmed findings. Trust boundary in agent_loop. agent_loop.run accepts an untrusted_user_prompt parameter. With a guardrail configured AND an untrusted segment, the user message is composed as [{text: trusted}, {guardContent: untrusted}] so the guardrail scans only the user-input part. Investigator notes paraphrasing a malicious PR comment can no longer false-trip the guardrail. _wrap_first_user_for_guardrail short-circuits when the caller has already set guardContent, with the ordering assumption documented inline. Investigator diff capped. Previously only the Critic's diff was truncated (BOT_CRITIC_MAX_DIFF_CHARS). Renamed to BOT_AGENT_MAX_DIFF_CHARS and applied to both stages. Non-positive values fall back to the 200_000 default; 0 would otherwise have silently dropped the diff entirely. Pipeline event taxonomy. Renamed the second tuple element of _run_critic_and_reporter from critic_skip_reason to pipeline_event. Added _CRITIC_STAGE_EVENTS / _ESCALATE_EVENTS / _KNOWN_PIPELINE_EVENTS frozensets. Critic-stage events tag metrics["critic"]["skip_reason"]; reporter-side events do not (they belong on metrics["reporter"]). Forward-compat guard: unknown event names fall back to "unknown_pipeline_event" (escalate-grade, distinct sentinel — never re-labeled as "critic_failed"). The reporter-deadline event uses "reporter_deadline_exceeded" in both the artifact's top-level reason and the reporter metrics row so dashboards filtering on either field don't drift. cachePoint cleanup. Removed from invoke_with_usage by the same logic that drove PR #728 for invoke() — Reporter (sole caller) embeds per-PR diff in the system prompt, so the cache prefix never repeats. Kept on converse_with_tools where Investigator-then-Critic shares the static prefix from _build_static_context; validate via the artifact's per-stage cache_read_tokens metric. Tests: 38 new (5 unit + integration). Every confirmed-finding-loss path (critic_failed, reporter_deadline_exceeded, reporter_failed via Bedrock None, reporter_failed via parse failure, unknown_pipeline_event) is exercised by a regression test.
1 parent 3878b26 commit 306fe5c

14 files changed

Lines changed: 3712 additions & 36 deletions

.github/workflows/issue-bot.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,16 @@ jobs:
7373
SM_ISSUE_RESPOND_PROMPT: deequ-bot/issue-respond-prompt
7474
SM_PR_FILE_REVIEW_PROMPT: deequ-bot/pr-file-review-prompt
7575
SM_PR_FILE_REVIEW_REPORT_PROMPT: deequ-bot/pr-file-review-report-prompt
76+
SM_PR_INVESTIGATOR_PROMPT: deequ-bot/pr-investigator-prompt
77+
SM_PR_CRITIC_PROMPT: deequ-bot/pr-critic-prompt
78+
SM_PR_REPORTER_PROMPT: deequ-bot/pr-reporter-prompt
7679
SM_FOLLOWUP_PROMPT: deequ-bot/followup-prompt
80+
# Flip BOT_AGENT_PIPELINE to "1" to enable the 3-agent (Investigator+Critic+Reporter) pipeline.
81+
# When unset/empty, the legacy two-phase flow runs unchanged.
82+
# NOTE: Set as a REPOSITORY VARIABLE (Settings → Secrets and variables → Actions → Variables tab),
83+
# NOT a secret. Misplacing it under "Secrets" leaves vars.BOT_AGENT_PIPELINE empty → legacy flow
84+
# runs silently. To verify: `gh api repos/awslabs/deequ/actions/variables` should list it.
85+
BOT_AGENT_PIPELINE: ${{ vars.BOT_AGENT_PIPELINE || '' }}
7786
CODEBASE_SRC_DIR: src/main/scala
7887
CODEBASE_FILE_EXT: .scala
7988
DRY_RUN: ${{ inputs.dry_run || 'false' }}
Lines changed: 289 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,289 @@
1+
"""Multi-turn tool-use loop for Bedrock Converse.
2+
3+
Each turn calls Bedrock with toolConfig; if stopReason is "tool_use" the
4+
loop executes the requested tools and continues, otherwise it returns the
5+
final text. Owns budget enforcement (turns, tool calls, tool output chars)
6+
and records each tool call in the result's tool_trace.
7+
"""
8+
import logging
9+
import time
10+
from collections import defaultdict
11+
from dataclasses import dataclass, field
12+
from typing import List, Dict, Any, Optional
13+
14+
from .tools import DEFAULT_PER_TOOL_CALL_CAPS
15+
16+
logger = logging.getLogger("issue_bot")
17+
18+
19+
@dataclass
20+
class AgentCaps:
21+
"""Per-agent budgets enforced by the loop. Defaults match the registered
22+
tool set in tools.py; pass per_tool_max_calls explicitly to override."""
23+
max_turns: int = 15
24+
max_tool_calls: int = 50
25+
max_tool_output_chars: int = 400_000
26+
per_tool_max_calls: Dict[str, int] = field(
27+
default_factory=lambda: dict(DEFAULT_PER_TOOL_CALL_CAPS)
28+
)
29+
max_tokens_per_call: int = 8000
30+
31+
32+
@dataclass
33+
class AgentResult:
34+
"""Outcome of an agent loop run. Empty `text` means the agent had no
35+
usable output and downstream stages should skip; `error` is a one-line
36+
label for logs."""
37+
text: str = ""
38+
turns: int = 0
39+
tool_calls: int = 0
40+
tool_output_chars: int = 0
41+
input_tokens: int = 0
42+
output_tokens: int = 0
43+
cache_read_tokens: int = 0
44+
cache_write_tokens: int = 0
45+
max_turns_reached: bool = False
46+
error: Optional[str] = None
47+
tool_trace: List[Dict[str, Any]] = field(default_factory=list)
48+
49+
50+
def run(
51+
bedrock_client,
52+
agent_name: str,
53+
system_prompt: str,
54+
user_prompt: str,
55+
tool_specs: List[Dict[str, Any]],
56+
tool_runner,
57+
caps: AgentCaps,
58+
pipeline_deadline: Optional[float] = None,
59+
untrusted_user_prompt: Optional[str] = None,
60+
):
61+
"""Execute an agent's tool-use loop. Returns an AgentResult.
62+
63+
user_prompt is trusted; untrusted_user_prompt (PR/issue user input)
64+
goes in a guardContent block when the client has a guardrail so model
65+
paraphrases of repo content don't false-trip the scanner.
66+
67+
Termination: caps.max_turns / max_tool_calls / max_tool_output_chars /
68+
per_tool_max_calls, or pipeline_deadline.
69+
"""
70+
result = AgentResult()
71+
messages = [{
72+
"role": "user",
73+
"content": _build_user_content(
74+
user_prompt, untrusted_user_prompt,
75+
has_guardrail=getattr(bedrock_client, "has_guardrail", False),
76+
),
77+
}]
78+
per_tool_calls = defaultdict(int)
79+
80+
for turn in range(max(0, caps.max_turns)):
81+
result.turns = turn + 1
82+
83+
if pipeline_deadline is not None and time.monotonic() >= pipeline_deadline:
84+
result.error = "pipeline wall-clock deadline reached"
85+
result.text = result.text or _recover_last_assistant_text(messages)
86+
logger.warning(
87+
"[%s turn=%d] pipeline wall-clock reached, terminating",
88+
agent_name, turn + 1,
89+
)
90+
return result
91+
92+
try:
93+
resp = bedrock_client.converse_with_tools(
94+
system_prompt=system_prompt,
95+
messages=messages,
96+
tool_specs=tool_specs,
97+
max_tokens=caps.max_tokens_per_call,
98+
)
99+
except Exception as e:
100+
logger.error("[%s turn=%d] bedrock error: %s", agent_name, turn + 1, type(e).__name__)
101+
result.error = f"bedrock error: {type(e).__name__}"
102+
return result
103+
104+
if resp is None:
105+
result.error = "bedrock returned None (circuit-breaker or transient failure)"
106+
return result
107+
108+
usage = resp.get("usage") or {}
109+
result.input_tokens += usage.get("inputTokens") or 0
110+
result.output_tokens += usage.get("outputTokens") or 0
111+
result.cache_read_tokens += usage.get("cacheReadInputTokens") or 0
112+
result.cache_write_tokens += usage.get("cacheWriteInputTokens") or 0
113+
logger.debug(
114+
"[%s turn=%d] in=%d out=%d cacheR=%d cacheW=%d",
115+
agent_name, turn + 1, result.input_tokens, result.output_tokens,
116+
result.cache_read_tokens, result.cache_write_tokens,
117+
)
118+
119+
msg = resp.get("output", {}).get("message", {})
120+
if not msg:
121+
result.error = "empty bedrock message"
122+
return result
123+
messages.append(msg)
124+
125+
if resp.get("stopReason") != "tool_use":
126+
result.text = _extract_text(msg)
127+
return result
128+
129+
tool_results = _run_tool_calls(
130+
msg, agent_name, turn + 1, tool_runner, caps, result, per_tool_calls,
131+
)
132+
if not tool_results:
133+
# Bedrock protocol violation: stopReason=tool_use with no toolUse
134+
# blocks. We capture any accompanying text and exit; if the text
135+
# happens to be parseable downstream the caller can use it.
136+
result.text = _extract_text(msg)
137+
result.error = "stopReason was tool_use but no toolUse blocks emitted"
138+
return result
139+
140+
# Stop before paying another Bedrock turn against exhausted budgets.
141+
if (result.tool_calls >= caps.max_tool_calls
142+
or result.tool_output_chars >= caps.max_tool_output_chars):
143+
result.error = "structural cap hit; terminating before next turn"
144+
result.text = _extract_text(msg) or (
145+
f"Investigation terminated on turn {turn + 1}: structural cap reached."
146+
)
147+
return result
148+
149+
messages.append({"role": "user", "content": tool_results})
150+
151+
result.max_turns_reached = True
152+
result.text = _recover_last_assistant_text(messages) or (
153+
f"Investigation hit max_turns ({caps.max_turns}) without conclusion. "
154+
f"Made {result.tool_calls} tool call(s)."
155+
)
156+
logger.warning("[%s] max_turns (%d) reached", agent_name, caps.max_turns)
157+
return result
158+
159+
160+
def _build_user_content(trusted, untrusted, *, has_guardrail):
161+
"""Two-block form only when an untrusted segment AND a guardrail are
162+
both present; otherwise concatenate into one text block."""
163+
if untrusted and has_guardrail:
164+
return [
165+
{"text": trusted},
166+
{"guardContent": {"text": {"text": untrusted}}},
167+
]
168+
if untrusted:
169+
return [{"text": f"{trusted}\n{untrusted}"}]
170+
return [{"text": trusted}]
171+
172+
173+
def _run_tool_calls(msg, agent_name, turn, tool_runner, caps, result, per_tool_calls):
174+
"""Execute every toolUse block in `msg` and return the toolResult blocks
175+
to append to the conversation. Mutates `result` and `per_tool_calls`."""
176+
tool_results = []
177+
for block in msg.get("content", []):
178+
if "toolUse" not in block:
179+
continue
180+
tu = block["toolUse"]
181+
tool_name = tu.get("name", "")
182+
tool_args = tu.get("input") or {}
183+
tool_use_id = tu.get("toolUseId", "")
184+
tool_text = _execute_or_budget(
185+
tool_name, tool_args, agent_name, turn, tool_runner,
186+
caps, result, per_tool_calls,
187+
)
188+
tool_results.append({
189+
"toolResult": {
190+
"toolUseId": tool_use_id,
191+
"content": [{"text": tool_text}],
192+
},
193+
})
194+
return tool_results
195+
196+
197+
def _execute_or_budget(tool_name, tool_args, agent_name, turn, tool_runner,
198+
caps, result, per_tool_calls):
199+
"""Run the tool, or return a budget-exhausted text the model will read."""
200+
per_tool_cap = caps.per_tool_max_calls.get(tool_name)
201+
if per_tool_cap is not None and per_tool_calls[tool_name] >= per_tool_cap:
202+
logger.info("[%s turn=%d] %s budget exhausted", agent_name, turn, tool_name)
203+
return (
204+
f"BUDGET EXHAUSTED: tool '{tool_name}' has been called {per_tool_cap} "
205+
"times. Investigate with information already gathered, or use a "
206+
"different tool."
207+
)
208+
if result.tool_calls >= caps.max_tool_calls:
209+
logger.info("[%s turn=%d] total tool budget exhausted", agent_name, turn)
210+
return (
211+
f"BUDGET EXHAUSTED: total tool calls reached {caps.max_tool_calls}. "
212+
"Conclude investigation with information already gathered."
213+
)
214+
if result.tool_output_chars >= caps.max_tool_output_chars:
215+
logger.info("[%s turn=%d] tool output budget exhausted", agent_name, turn)
216+
return (
217+
f"BUDGET EXHAUSTED: total tool output {result.tool_output_chars} chars "
218+
f"reached cap {caps.max_tool_output_chars}. Conclude investigation."
219+
)
220+
221+
t0 = time.monotonic()
222+
text = tool_runner.run(tool_name, tool_args)
223+
if not isinstance(text, str):
224+
text = str(text)
225+
latency_ms = int((time.monotonic() - t0) * 1000)
226+
per_tool_calls[tool_name] += 1
227+
result.tool_calls += 1
228+
result.tool_output_chars += len(text)
229+
logger.info(
230+
"[%s turn=%d] %s(%s) → %d chars in %dms",
231+
agent_name, turn, tool_name, _summarize_args(tool_args), len(text), latency_ms,
232+
)
233+
result.tool_trace.append({
234+
"agent": agent_name,
235+
"turn": turn,
236+
"tool": tool_name,
237+
"args": _capped_args(tool_args),
238+
"result_summary": text[:200] + ("..." if len(text) > 200 else ""),
239+
"result_chars": len(text),
240+
"latency_ms": latency_ms,
241+
})
242+
return text
243+
244+
245+
_MAX_TRACE_ARG_CHARS = 500
246+
247+
248+
def _capped_args(args):
249+
"""Cap each arg value so a single huge model-supplied string can't
250+
swamp the artifact's tool trace."""
251+
if not isinstance(args, dict):
252+
return repr(args)[:_MAX_TRACE_ARG_CHARS]
253+
out = {}
254+
for k, v in args.items():
255+
if isinstance(v, str) and len(v) > _MAX_TRACE_ARG_CHARS:
256+
out[k] = v[:_MAX_TRACE_ARG_CHARS] + "...[truncated]"
257+
else:
258+
out[k] = v
259+
return out
260+
261+
262+
def _extract_text(msg):
263+
"""Join text blocks from an assistant message; "" if none."""
264+
return "\n".join(
265+
b.get("text", "") for b in msg.get("content", []) if "text" in b
266+
).strip()
267+
268+
269+
def _recover_last_assistant_text(messages):
270+
"""Walk back to the most recent assistant message and extract its text.
271+
Used on max_turns / wall-clock exits so partial findings aren't dropped
272+
when the trailing message is the user-side toolResult."""
273+
for msg in reversed(messages):
274+
if msg.get("role") == "assistant":
275+
return _extract_text(msg)
276+
return ""
277+
278+
279+
def _summarize_args(args):
280+
"""One-line repr of tool args, truncated for log output."""
281+
if not isinstance(args, dict):
282+
return repr(args)[:80]
283+
parts = []
284+
for k, v in args.items():
285+
if isinstance(v, str):
286+
parts.append(f"{k}={v[:60]!r}")
287+
else:
288+
parts.append(f"{k}={v!r}")
289+
return ", ".join(parts)

0 commit comments

Comments
 (0)