Skip to content

Commit 3cc65b5

Browse files
krokokobgagent
andauthored
feat(agent): observable blocker signal + bounded self-remediation (#251) (#545)
* feat(agent): blocker taxonomy + agent_blocked event contract (#251) Phase 0 of #251 — the foundation contract, no behavior change. - agent/src/progress_writer.py: closed BLOCKER_KINDS set, format_blocker_reason() producing the canonical BLOCKED[<kind>]: <detail> (+ resource) terminal-reason string (single source of truth, decision D), and write_agent_blocked() emitting a distinct event_type=agent_blocked. - cdk error-classifier: new BLOCKED category + per-kind remedy builder that keys on the BLOCKED[<kind>] prefix and extracts the resource so remedies name the exact secret/host. - docs: CEDAR_HITL_GATES §13.16 taxonomy + event/reason contract; COMPUTE.md result-contract cross-link; regenerated Starlight mirrors. - tests: progress_writer event shape/kinds/truncation + reason round-trip; classifier per-kind + resource extraction + carry-path. * feat(agent): detect + surface + remediate environmental blockers (#251) Phases 1-3 of #251. Phase 1 — detection at the three fault points: - policy.py: structured fail_closed flag on PolicyDecision (decision E), set True only at engine-error/unavailable deny sites. - hooks.py: PreToolUse emits policy_fail_closed on fail-closed deny (branches on the flag, never a reason string; hard-denies never emit). PostToolUse scans tool output for egress-denial signatures and emits egress_denied with the host. A process-lifetime latch carries the canonical reason into the terminal error via pipeline._resolve_overall_task_status. - context-hydration.ts: resolveGitHubToken throws MissingSecretError with the canonical BLOCKED[missing_secret] reason on ResourceNotFoundException/empty secret; propagated (not degraded) so failTask + classifyError surface it. Phase 2 — bounded self-remediation (safe-by-default): - shell.py: run_cmd_with_backoff — capped exponential backoff, transient-only retry, injectable sleep + audit callback (no hooks/progress dep). - repo.py: clone + PR-branch fetch retry transient failures; on exhaustion report dependency_unreachable and raise. Scope-preserving by construction. Phase 3 — surface + consume: - CLI watch renders agent_blocked (⛔ + kind/resource/hint); status snapshot shows the latest blocker. Notifications flow via the existing classifier path. Tests across agent/cdk/cli; docs §13.16 updated + synced. * fix(agent): address PR review findings for #251 blocker signal - repo.py: reclassify a retry-exhausted setup failure that names a host (could not resolve host / name-resolution) as non-retryable egress_denied instead of retryable dependency_unreachable — the setup path and the PostToolUse egress detector now reach the same verdict for identical stderr, so users get the allowlist remedy rather than a fruitless 'retry the task'. - context-hydration.ts: add SecretUnreadableError (auth_failure) for a token secret that exists but can't be read (AccessDenied/throttling). Propagate it through hydration + issue/PR fetch instead of silently degrading to minimal context, which stranded the task with no token. - tests: egress-vs-transient reclassification, PR-branch fetch retry resource, exact retry+exhaustion event count, unreadable-secret propagation + cause, latest-blocker-wins in status snapshot; update preflight assertion. - docs: note the egress-over-transient precedence in CEDAR_HITL_GATES §13.16. Findings adjudicated not-a-defect: blocker latch cross-task leak (AgentCore runs one task per Firecracker MicroVM — COMPUTE.md; latch follows the existing per-process pattern) and host-less egress event (documented heuristic signal). * fix(agent,cdk,cli): address second-round PR review for #251 Major — repo setup no longer wastes backoff on named-host failures: run_cmd_with_backoff now bails immediately when stderr names a host (could not resolve host: <h>), so a firewalled/non-existent endpoint is not retried and emits no misleading dependency_unreachable events before _fail_setup_command reclassifies it to non-retryable egress_denied. A host-less name-resolution blip stays retryable. Minor fixes: - error-classifier: auth_failure remedy branches on a Secrets Manager ARN resource — IAM/GetSecretValue advice instead of PAT-scope advice. - preflight: token-secret failures now report GITHUB_TOKEN_SECRET_MISSING / GITHUB_TOKEN_SECRET_UNREADABLE instead of the misleading GITHUB_UNREACHABLE (classifyError still routes on the BLOCKED[...] detail; this fixes the failureReason label + priority ordering). - format.ts: suppress the historical Blocker: line on COMPLETED tasks so a self-remediated success is not shown as blocked. - workflow/runner: thread progress into setup_repo so clone/fetch backoff blocker events reach the live stream on the workflow path. Tests: named-host no-retry, host-less-still-retryable, SM-ARN auth remedy, new preflight reasons, COMPLETED blocker suppression, workflow stub arity. Docs: §13.16 updated for the bail-before-retry behavior + synced mirror. Nit (latch thread-safety) left as-is — safe under one-task-per-MicroVM. * fix(agent): narrow retry-bail to DNS-only + close review coverage gaps (#251) Third review round found one regression I introduced + two test gaps: - REGRESSION FIX (shell.py): the round-2 bail reused detect_egress_denial, which also matches 'Failed to connect to <host>' — a transient TCP timeout to an ALLOWLISTED host that git emits on 'Connection timed out'. That made backoff fast-fail a genuinely-retryable failure as egress_denied. Replaced with a DNS-only predicate (_UNRESOLVABLE_HOST_RE: could not resolve host / getaddrinfo ENOTFOUND|EAI_AGAIN / Failed to resolve). TCP timeouts now retry; a persistent one is still reclassified to egress_denied at exhaustion. Also removes the shell→hooks call-time import (F2: no detector can raise mid-retry). - COVERAGE (test_repo.py): add an end-to-end test driving the REAL run_cmd_with_backoff (patching shell.run_cmd, not the fake) — a named-host DNS failure bails on the first clone (no retry), emits zero dependency_unreachable events, exactly one non-retryable egress_denied. - COVERAGE (test_shell.py): add TCP-connect-timeout-still-retries (the regression guard) alongside the DNS-bail test. - COVERAGE (test_workflow_runner.py): assert progress is actually threaded through to setup_repo (was arity-only; a revert to setup_repo(config) would have passed silently). - COVERAGE (format-status-snapshot.test.ts): FAILED task still shows the Blocker line (guards the COMPLETED-only suppression against broadening). Docs §13.16 corrected: DNS-names-host bails, TCP-connect stays retryable. Not changed: GovCloud ARN partition in the auth_failure remedy (no producer today) and preflight priority-ordering test (token failure early-returns, so the ordering is defensive-only) — both noted as low/future by the review. --------- Co-authored-by: bgagent <bgagent@noreply.github.com>
1 parent 6340aba commit 3cc65b5

29 files changed

Lines changed: 1916 additions & 41 deletions

agent/src/hooks.py

Lines changed: 166 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,102 @@ def _tool_input_preview(tool_input: Any, max_len: int = TOOL_INPUT_PREVIEW_MAX)
9999
return _truncate(_strip_ansi(rendered), max_len)
100100

101101

102+
# ---------------------------------------------------------------------------
103+
# Egress-denial detection (#251, Phase 1)
104+
# ---------------------------------------------------------------------------
105+
#
106+
# Best-effort, heuristic scan of a tool's output for the signatures a blocked
107+
# outbound connection leaves behind — a non-allowlisted host hitting the DNS
108+
# Firewall, a refused connection, or a name-resolution failure. When one
109+
# matches we emit an ``egress_denied`` blocker naming the host to allowlist.
110+
# This is intentionally a string-signature scan (not a network probe): the
111+
# model's Bash tool calls (``npm install``, ``git clone``, ``curl``) surface
112+
# their stderr here, and those tools use stable, well-known phrasings. Patterns
113+
# that capture a host use a named ``host`` group; detection-only patterns omit
114+
# it (host stays None → the event still fires, just without ``resource``).
115+
_EGRESS_DENIAL_PATTERNS: tuple[re.Pattern[str], ...] = (
116+
# curl / libcurl, git-over-https
117+
re.compile(r"[Cc]ould not resolve host:?\s+(?P<host>[A-Za-z0-9._-]+)"),
118+
re.compile(r"[Ff]ailed to connect to\s+(?P<host>[A-Za-z0-9._-]+)"),
119+
# Node (npm/yarn), python (requests/urllib), getaddrinfo
120+
re.compile(r"getaddrinfo (?:ENOTFOUND|EAI_AGAIN)\s+(?P<host>[A-Za-z0-9._-]+)"),
121+
# requests/urllib3: the real host appears in ``HTTPSConnectionPool(host='x')``
122+
# or after ``Failed to resolve 'x'``. Match those anchors specifically —
123+
# a bare ``NameResolutionError.*?<fqdn>`` lazy-match would instead capture
124+
# the ``urllib3.connection.HTTPSConnection`` class path that precedes the host.
125+
re.compile(r"HTTPS?ConnectionPool\(host='(?P<host>[A-Za-z0-9._-]+)'"),
126+
re.compile(r"Failed to resolve '(?P<host>[A-Za-z0-9._-]+)'"),
127+
re.compile(
128+
r"nodename nor servname provided|Temporary failure in name resolution"
129+
r"|Name or service not known|Connection refused|Network is unreachable"
130+
r"|NameResolutionError|Failed to establish a new connection",
131+
),
132+
)
133+
134+
135+
# #251 carry-path: the most recent agent-detected blocker, as a canonical
136+
# ``BLOCKED[<kind>]: …`` reason string (see ``format_blocker_reason``). Blockers
137+
# are detected mid-turn in the hooks (egress in PostToolUse, fail-closed in
138+
# PreToolUse) but the terminal ``error_message`` is assembled later in the
139+
# pipeline. This latch bridges the two: the pipeline promotes it into
140+
# ``TaskResult.error`` ONLY when the task errored without a more specific
141+
# reason, so the CDK classifier surfaces a precise remedy. Process-lifetime
142+
# (== one task) like ``_INJECTED_NUDGES``; last-writer-wins is fine — the most
143+
# recent blocker is the most relevant to a terminal failure.
144+
_LAST_BLOCKER_REASON: str | None = None
145+
146+
147+
def _record_blocker_reason(kind: str, detail: str, resource: str | None = None) -> None:
148+
"""Latch the canonical reason for the most recent agent-detected blocker."""
149+
global _LAST_BLOCKER_REASON
150+
from progress_writer import format_blocker_reason
151+
152+
_LAST_BLOCKER_REASON = format_blocker_reason(kind, detail, resource)
153+
154+
155+
def last_blocker_reason() -> str | None:
156+
"""Return the latched canonical blocker reason for terminal promotion (#251)."""
157+
return _LAST_BLOCKER_REASON
158+
159+
160+
def reset_blocker_reason() -> None:
161+
"""Clear the in-process blocker latch.
162+
163+
Called at the start of each ``run_task`` (the latch is a process-scalar,
164+
not task_id-keyed, so it must be reset per task to avoid leaking a prior
165+
task's blocker) and by test fixtures for isolation.
166+
"""
167+
global _LAST_BLOCKER_REASON
168+
_LAST_BLOCKER_REASON = None
169+
170+
171+
# Back-compat alias for test fixtures that import the old name.
172+
_reset_blocker_reason_for_tests = reset_blocker_reason
173+
174+
175+
def detect_egress_denial(text: str) -> tuple[bool, str | None]:
176+
"""Scan tool output for an egress-denial signature (#251).
177+
178+
Returns ``(detected, host_or_None)``: ``(True, host)`` on the first matching
179+
signature (``host`` populated when a pattern captured one so the emitted
180+
``egress_denied`` event can name the exact domain to allowlist; ``None`` for
181+
detection-only signatures), or ``(False, None)`` when nothing matches.
182+
183+
Best-effort and deliberately conservative: it only fires on the
184+
well-known phrasings the common CLIs (curl/git/npm/pip) emit, so a task
185+
log that merely *mentions* "connection refused" in prose could produce a
186+
false positive — acceptable for an observability signal that never blocks.
187+
"""
188+
if not text:
189+
return False, None
190+
for pattern in _EGRESS_DENIAL_PATTERNS:
191+
match = pattern.search(text)
192+
if match:
193+
host = match.groupdict().get("host") if match.groupdict() else None
194+
return True, host
195+
return False, None
196+
197+
102198
def _deny_response(reason: str) -> dict:
103199
"""Build a PreToolUse DENY response with a sanitized reason.
104200
@@ -214,6 +310,30 @@ async def pre_tool_use_hook(
214310
"write_policy_decision_cached",
215311
**decision.cache_hit_metadata,
216312
)
313+
# #251 (decision E): a fail-closed deny means the Cedar engine errored
314+
# or was unavailable — an ENVIRONMENTAL fault (misconfiguration), NOT
315+
# an intentional hard-deny. Emit a ``policy_fail_closed`` blocker event
316+
# so it renders distinctly and carries a remediation hint. We branch on
317+
# the structured ``decision.fail_closed`` flag, never a reason-string
318+
# prefix. Intentional hard-denies / cache-denies (fail_closed=False)
319+
# never emit. Emitted strictly BEFORE the deny; the deny itself is
320+
# unchanged, so this adds observability without altering the outcome.
321+
if getattr(decision, "fail_closed", False):
322+
_record_blocker_reason("policy_fail_closed", decision.reason)
323+
if progress is not None:
324+
_try_progress(
325+
progress,
326+
"write_agent_blocked",
327+
kind="policy_fail_closed",
328+
detail=decision.reason,
329+
remediation_hint=(
330+
"The Cedar policy engine errored or was unavailable "
331+
"(fail-closed). Check the agent policy-engine logs and the "
332+
"policy bundle; this is a misconfiguration, not an "
333+
"intentional deny."
334+
),
335+
retryable=False,
336+
)
217337
log("POLICY", f"DENIED: {tool_name}{decision.reason}")
218338
return _deny_response(decision.reason)
219339

@@ -918,13 +1038,19 @@ async def post_tool_use_hook(
9181038
hook_context: Any,
9191039
*,
9201040
trajectory: _TrajectoryWriter | None = None,
1041+
progress: Any = None,
9211042
) -> dict:
9221043
"""PostToolUse hook: screen tool output for secrets/PII.
9231044
9241045
Returns a dict with hookSpecificOutput. When sensitive content is
9251046
detected the response includes ``updatedMCPToolOutput`` containing the
9261047
redacted version (steered enforcement — content is sanitized, not
9271048
blocked).
1049+
1050+
``progress`` is optional (preserves the Phase 1 test call shape). When
1051+
present, an egress-denial signature in the tool output emits an
1052+
``egress_denied`` blocker event (#251) — best-effort observability,
1053+
never alters the screening decision.
9281054
"""
9291055
_PASS_THROUGH: dict = {"hookSpecificOutput": {"hookEventName": "PostToolUse"}}
9301056
_FAIL_CLOSED: dict = {
@@ -950,6 +1076,43 @@ async def post_tool_use_hook(
9501076
if not isinstance(tool_response, str):
9511077
tool_response = str(tool_response)
9521078

1079+
# #251: best-effort egress-denial detection. A blocked outbound connection
1080+
# (non-allowlisted host hitting the DNS Firewall, refused connection, name
1081+
# resolution failure) surfaces in the tool's stderr here. Emit an
1082+
# ``egress_denied`` blocker naming the host to allowlist. Observability
1083+
# only — never changes the pass-through / redaction decision below.
1084+
egress_detected, host = detect_egress_denial(tool_response)
1085+
if egress_detected:
1086+
detail = (
1087+
f"A connection to {host!r} was blocked"
1088+
if host
1089+
else "An outbound connection was blocked"
1090+
) + f" during {tool_name}."
1091+
# Latch for terminal promotion ONLY when a host was captured. The latch
1092+
# feeds the authoritative TaskResult.error carry-path, so a host-less,
1093+
# detection-only signature ("Connection refused" / "Network is
1094+
# unreachable") — which commonly fires for an internal localhost race,
1095+
# not an egress denial — must not masquerade as the terminal cause. The
1096+
# observability event below still fires unconditionally (it never blocks
1097+
# and is clearly a heuristic signal in the live stream).
1098+
if host:
1099+
_record_blocker_reason("egress_denied", detail, host)
1100+
if progress is not None:
1101+
_try_progress(
1102+
progress,
1103+
"write_agent_blocked",
1104+
kind="egress_denied",
1105+
detail=detail,
1106+
remediation_hint=(
1107+
f"Allowlist {host!r} in the DNS Firewall rule group"
1108+
if host
1109+
else "Allowlist the required host in the DNS Firewall rule group"
1110+
)
1111+
+ " if it is a legitimate dependency. The agent never widens egress itself.",
1112+
retryable=False,
1113+
resource=host,
1114+
)
1115+
9531116
try:
9541117
result = scan_tool_output(tool_response)
9551118
except Exception as exc:
@@ -1404,7 +1567,9 @@ async def _post(
14041567
hook_input: HookInput, tool_use_id: str | None, ctx: HookContext
14051568
) -> HookJSONOutput:
14061569
try:
1407-
result = await post_tool_use_hook(hook_input, tool_use_id, ctx, trajectory=trajectory)
1570+
result = await post_tool_use_hook(
1571+
hook_input, tool_use_id, ctx, trajectory=trajectory, progress=progress
1572+
)
14081573
return SyncHookJSONOutput(**result)
14091574
except Exception as exc:
14101575
log("ERROR", f"PostToolUse wrapper crashed: {type(exc).__name__}: {exc}")

agent/src/pipeline.py

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,16 @@ def _resolve_overall_task_status(
469469
if agent_status in ("success", "end_turn") and build_ok:
470470
return "success", err
471471

472+
# #251 carry-path: a hook may have detected an environmental blocker mid-run
473+
# (egress denial, policy fail-closed) that the SDK surfaced only as a generic
474+
# failure or as a missing ResultMessage. Promote the canonical
475+
# ``BLOCKED[<kind>]: …`` reason so the CDK classifier attaches a precise
476+
# remedy. Import locally to avoid a module-load cycle (hooks imports
477+
# pipeline-adjacent modules).
478+
from hooks import last_blocker_reason
479+
480+
blocker = last_blocker_reason()
481+
472482
if agent_status == "unknown":
473483
if pr_url:
474484
log(
@@ -480,10 +490,17 @@ def _resolve_overall_task_status(
480490
"INFO",
481491
"No ResultMessage from SDK; build_ok=True (informational; task still failed)",
482492
)
493+
# An egress denial that kills the agent's outbound calls is a likely
494+
# cause of a missing ResultMessage — prefer the specific blocker reason
495+
# over the generic SDK-no-result message when both are present.
496+
if blocker and not err:
497+
return "error", blocker
483498
merged = f"{err}; {_SDK_NO_RESULT_MESSAGE}" if err else _SDK_NO_RESULT_MESSAGE
484499
return "error", merged
485500

486501
if not err:
502+
if blocker:
503+
return "error", blocker
487504
err = f"Task did not succeed (agent_status={agent_status!r}, build_ok={build_ok})"
488505
return "error", err
489506

@@ -669,6 +686,14 @@ def run_task(
669686

670687
agent_result: AgentResult | None = None
671688
progress = _ProgressWriter(config.task_id, trace=trace)
689+
# #251: clear any blocker latched by a prior task. The agent container
690+
# is one-task-per-process today, but the FastAPI server thread-pool can
691+
# in principle dispatch a second run_task in the same process — reset
692+
# here so a stale BLOCKED[...] reason can never leak into this task's
693+
# terminal error_message (the latch is a scalar, not task_id-keyed).
694+
from hooks import reset_blocker_reason
695+
696+
reset_blocker_reason()
672697
# --trace accumulator (design §10.1): when the task opted into
673698
# trace, ``_TrajectoryWriter`` keeps an in-memory copy of each
674699
# event so the pipeline can gzip+upload the full trajectory to
@@ -794,7 +819,7 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
794819

795820
# Setup repo (deterministic pre-hooks)
796821
with task_span("task.repo_setup") as setup_span:
797-
setup = setup_repo(config)
822+
setup = setup_repo(config, progress=progress)
798823
setup_span.set_attribute("build.before", setup.build_before)
799824
progress.write_agent_milestone(
800825
"repo_setup_complete",

agent/src/policy.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ class PolicyDecision:
205205
__slots__ = (
206206
"cache_hit_metadata",
207207
"duration_ms",
208+
"fail_closed",
208209
"matching_rule_ids",
209210
"outcome",
210211
"reason",
@@ -223,6 +224,7 @@ def __init__(
223224
matching_rule_ids: tuple[str, ...] = (),
224225
duration_ms: float = 0.0,
225226
cache_hit_metadata: dict | None = None,
227+
fail_closed: bool = False,
226228
) -> None:
227229
if outcome is None and allowed is None:
228230
raise TypeError("PolicyDecision requires either outcome= or allowed=")
@@ -244,6 +246,16 @@ def __init__(
244246
# decisions with different original_decision_ts values still
245247
# represent the same deny outcome.
246248
self.cache_hit_metadata = cache_hit_metadata
249+
# #251 (decision E): structured discriminator for environmental
250+
# fail-closed denies (Cedar engine errored / unavailable) vs.
251+
# intentional hard-denies and cache-driven denies. The hook branches
252+
# on THIS flag — not a brittle ``reason.startswith("fail-closed:")``
253+
# string match — to decide whether to emit a ``policy_fail_closed``
254+
# blocker event. Set True ONLY at engine-error/unavailable deny sites;
255+
# hard-deny and cache-deny leave it False. Like ``cache_hit_metadata``,
256+
# NOT part of __eq__/__hash__ (it is derived from outcome+reason, and
257+
# legacy equality-based tests predate the field).
258+
self.fail_closed = fail_closed
247259

248260
@property
249261
def allowed(self) -> bool:
@@ -296,8 +308,15 @@ def allow(cls, reason: str = "permitted", duration_ms: float = 0.0) -> PolicyDec
296308
return cls(outcome=Outcome.ALLOW, reason=reason, duration_ms=duration_ms)
297309

298310
@classmethod
299-
def deny(cls, reason: str, duration_ms: float = 0.0) -> PolicyDecision:
300-
return cls(outcome=Outcome.DENY, reason=reason, duration_ms=duration_ms)
311+
def deny(
312+
cls, reason: str, duration_ms: float = 0.0, fail_closed: bool = False
313+
) -> PolicyDecision:
314+
return cls(
315+
outcome=Outcome.DENY,
316+
reason=reason,
317+
duration_ms=duration_ms,
318+
fail_closed=fail_closed,
319+
)
301320

302321
@classmethod
303322
def require_approval(
@@ -1153,6 +1172,7 @@ def evaluate_tool_use(self, tool_name: str, tool_input: dict) -> PolicyDecision:
11531172
return PolicyDecision.deny(
11541173
reason="policy engine unavailable",
11551174
duration_ms=(time.monotonic() - start) * 1000,
1175+
fail_closed=True,
11561176
)
11571177

11581178
base_context = {
@@ -1174,6 +1194,7 @@ def evaluate_tool_use(self, tool_name: str, tool_input: dict) -> PolicyDecision:
11741194
return PolicyDecision.deny(
11751195
reason="fail-closed: unhashable_tool_input",
11761196
duration_ms=(time.monotonic() - start) * 1000,
1197+
fail_closed=True,
11771198
)
11781199

11791200
try:
@@ -1306,6 +1327,7 @@ def evaluate_tool_use(self, tool_name: str, tool_input: dict) -> PolicyDecision:
13061327
return PolicyDecision.deny(
13071328
reason=f"fail-closed: {type(exc).__name__}",
13081329
duration_ms=(time.monotonic() - start) * 1000,
1330+
fail_closed=True,
13091331
)
13101332

13111333
# ---- Per-action routing ------------------------------------------------

0 commit comments

Comments
 (0)