diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 00000000..9402c7e7 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,58 @@ +# Build context is repo root (see cdk/src/stacks/agent.ts) so the +# Dockerfile can COPY contracts/ alongside agent/. Exclusions below +# keep the context lean — without them the entire monorepo (CDK +# cdk.out/, node_modules/, docs/dist/, etc.) gets uploaded on every +# AgentCore deploy. + +# CDK output (recursive include if not excluded) +cdk/cdk.out/ +cdk/lib/ +cdk/node_modules/ + +# CLI and docs build artifacts +cli/lib/ +cli/node_modules/ +docs/dist/ +docs/node_modules/ +docs/.astro/ + +# Shared node_modules +node_modules/ + +# Agent venv and cache (rebuilt inside image via uv) +agent/.venv/ +agent/__pycache__/ +agent/**/__pycache__/ +agent/**/*.pyc + +# Git and tooling +.git/ +.prek/ +.claude/ +**/.DS_Store + +# Docs and assets not needed in image +*.md +*.png +*.drawio +*.html +*.gif +*.tape + +# Worktrees + scratch +abca-worktrees/ +.next-session-prompt.md +.e2e-test-plan.md + +# Test/coverage output +coverage/ +**/coverage/ +.pytest_cache/ +**/.pytest_cache/ + +# IDE / OS +.idea/ +.vscode/ +yarn-error.log +yarn-debug.log +npm-debug.log* diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 613657fd..fe6adbb5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -71,6 +71,22 @@ repos: exclude: ^docs/node_modules/ stages: [pre-commit] + - id: types-sync-cdk-cli + name: type sync drift (CDK ↔ CLI) + entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run check:types-sync' + language: system + pass_filenames: false + files: ^(cdk/src/handlers/shared/types\.ts$|cli/src/types\.ts$|scripts/check-types-sync\.ts$) + stages: [pre-commit] + + - id: constants-sync + name: cross-language constants drift (contracts/constants.json) + entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run check:constants-sync' + language: system + pass_filenames: false + files: ^(contracts/constants\.json$|agent/src/policy\.py$|cdk/src/handlers/shared/types\.ts$|cdk/src/constructs/blueprint\.ts$|scripts/check-constants-sync\.ts$) + stages: [pre-commit] + - id: monorepo-security-pre-push name: security scans (pre-push) entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run hooks:pre-push:security' diff --git a/agent/.dockerignore b/agent/.dockerignore deleted file mode 100644 index 3beeb2c6..00000000 --- a/agent/.dockerignore +++ /dev/null @@ -1,6 +0,0 @@ -*.md -*.png -.claude/ -.git/ -__pycache__/ -*.pyc diff --git a/agent/Dockerfile b/agent/Dockerfile index 131e47de..2eeeb17b 100644 --- a/agent/Dockerfile +++ b/agent/Dockerfile @@ -20,14 +20,19 @@ COPY --from=gh-builder /out/gh /usr/local/bin/gh # - build-essential (native compilation for some repos) # - curl (downloads) RUN apt-get update && \ + # Patch any base-image CVEs that have a fix available in the + # current Debian point release. Without this, transitive system- + # library CVEs (e.g. libnghttp2 CVE-2026-27135) ride the base + # ``python:3.13-slim`` tag until upstream rebuilds, which can be + # weeks. ``--no-install-recommends`` keeps the upgrade narrow and + # reproducible — only already-installed packages get bumped. + apt-get upgrade -y --no-install-recommends && \ apt-get install -y --no-install-recommends \ curl \ git \ build-essential \ ca-certificates \ gnupg && \ - # Upgrade base image's CVE-2026-27135 vulnerability - apt-get upgrade -y --no-install-recommends libnghttp2-14 && \ # Cleanup early to keep peak disk usage low during builds. apt-get clean && \ rm -rf /var/lib/apt/lists/* /var/cache/apt/archives/* @@ -49,15 +54,28 @@ RUN npm install -g npm@latest && \ # Install uv (fast Python package manager) COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv -# Install Python dependencies via uv -COPY pyproject.toml uv.lock /app/ +# Install Python dependencies via uv. Build context is repo root (set in +# ``cdk/src/stacks/agent.ts``) so source paths are prefixed with ``agent/``. +COPY agent/pyproject.toml agent/uv.lock /app/ RUN uv sync --frozen --no-dev --directory /app # Copy agent code (ARG busts cache so file edits are always picked up) ARG CACHE_BUST=0 -COPY src/ /app/src/ -COPY prepare-commit-msg.sh /app/ -COPY test_sdk_smoke.py test_subprocess_threading.py /app/ +COPY agent/src/ /app/src/ +# Cedar HITL built-in policy files (hard_deny.cedar + soft_deny.cedar). +# ``agent/src/policy.py::_POLICIES_DIR`` resolves to ``/app/policies`` +# at import time; without these files the PolicyEngine init raises +# ``missing built-in hard-deny policies`` and every task fails at 0 +# turns before the agent even connects to the CLI. Discovered during +# Chunk 10 E2E T2.2 — the Dockerfile previously only copied ``src/``. +COPY agent/policies/ /app/policies/ +# Cross-language constants (S9). ``agent/src/policy.py`` reads +# ``/app/contracts/constants.json`` at import; the same file is consumed +# by ``cdk/src/handlers/shared/types.ts`` at synth time. See +# ``contracts/README.md`` for the contract. +COPY contracts/ /app/contracts/ +COPY agent/prepare-commit-msg.sh /app/ +COPY agent/test_sdk_smoke.py agent/test_subprocess_threading.py /app/ # Create non-root user (Claude Code CLI refuses bypassPermissions as root) RUN useradd -m -s /bin/bash agent && \ diff --git a/agent/policies/hard_deny.cedar b/agent/policies/hard_deny.cedar new file mode 100644 index 00000000..97146ac9 --- /dev/null +++ b/agent/policies/hard_deny.cedar @@ -0,0 +1,59 @@ +// Built-in hard-deny policy set for Cedar HITL engine. +// +// Hard-deny is ABSOLUTE: no --pre-approve scope and no blueprint `disable:` +// directive can bypass these rules. See docs/design/CEDAR_HITL_GATES.md +// §12.5 and decision #8. +// +// Every rule in this file MUST carry @tier("hard") + @rule_id annotations. +// Adding a rule here expands the set of categorically-forbidden agent +// actions; removing a rule requires a security review. + +// Base catch-all permit. Specific forbid rules below override. +@rule_id("base_permit") +permit (principal, action, resource); + +// pr_review tasks may never invoke Write. Absolute; cannot be overridden +// by per-blueprint customization or --pre-approve. +@tier("hard") +@rule_id("pr_review_forbid_write") +forbid ( + principal == Agent::TaskAgent::"pr_review", + action == Agent::Action::"invoke_tool", + resource == Agent::Tool::"Write" +); + +// pr_review tasks may never invoke Edit. +@tier("hard") +@rule_id("pr_review_forbid_edit") +forbid ( + principal == Agent::TaskAgent::"pr_review", + action == Agent::Action::"invoke_tool", + resource == Agent::Tool::"Edit" +); + +// Reject `rm -rf /` and similar absolute-root destructive commands. +@tier("hard") +@rule_id("rm_slash") +forbid (principal, action == Agent::Action::"execute_bash", resource) +when { context.command like "*rm -rf /*" }; + +// Reject writes into `.git/` at the repo root (breaks local git state). +@tier("hard") +@rule_id("write_git_internals") +forbid (principal, action == Agent::Action::"write_file", resource) +when { context.file_path like ".git/*" }; + +// Reject writes into nested `.git/` directories (submodules, worktrees). +@tier("hard") +@rule_id("write_git_internals_nested") +forbid (principal, action == Agent::Action::"write_file", resource) +when { context.file_path like "*/.git/*" }; + +// Reject any SQL DROP TABLE through Bash — agents should not be running +// destructive DDL against production or dev databases without a human +// in the loop. Hard-deny because even "just testing locally" is a common +// vector for data loss (wrong DB connected via saved credentials). +@tier("hard") +@rule_id("drop_table") +forbid (principal, action == Agent::Action::"execute_bash", resource) +when { context.command like "*DROP TABLE*" }; diff --git a/agent/policies/soft_deny.cedar b/agent/policies/soft_deny.cedar new file mode 100644 index 00000000..fd04ae7f --- /dev/null +++ b/agent/policies/soft_deny.cedar @@ -0,0 +1,84 @@ +// Base catch-all permit. Without it, cedarpy's default-deny would turn +// every non-matching Cedar evaluation on this tier into a DENY decision, +// making the soft tier indistinguishable from hard-deny. With it, Cedar +// returns ALLOW (no matching forbid) and our engine's STEP 3 sees only +// the genuine forbid hits as REQUIRE_APPROVAL. +@rule_id("base_permit") +permit (principal, action, resource); + +// Built-in soft-deny policy set for Cedar HITL engine. +// +// Soft-deny is the HUMAN-IN-THE-LOOP surface: matching rules pause the +// tool call, write an approval request to DynamoDB, and await a human +// response via `bgagent approve` / `bgagent deny`. See +// docs/design/CEDAR_HITL_GATES.md §§2, 6, 15.4. +// +// Every rule in this file MUST carry: +// @tier("soft") +// @rule_id("...") — stable ID for --pre-approve rule:X +// @approval_timeout_s — integer seconds >= 30 (<120 emits WARN per IMPL-25) +// @severity — "low" | "medium" | "high" +// @category — optional free-form UX grouping +// +// Blueprints may OPT OUT of specific rules here via +// `security.cedarPolicies.disable: [rule_id]`. They may NOT disable any +// rule in hard_deny.cedar (blueprint loader rejects those at task start). + +// Gate any git --force / -f push. 300s default approval window, medium severity. +// Covers both long-form (--force) and short-form (-f) variants, including +// the bare `git push -f` invocation with no branch argument. +@tier("soft") +@rule_id("force_push_any") +@approval_timeout_s("300") +@severity("medium") +@category("destructive") +forbid (principal, action == Agent::Action::"execute_bash", resource) +when { context.command like "*git push --force*" + || context.command like "*git push -f *" + || context.command like "*git push -f" }; + +// Force-push to main/prod specifically — longer window, higher severity. +// Multi-match with force_push_any is expected: the engine's annotation +// merging picks min(300, 600)=300s and max(medium, high)=high. +@tier("soft") +@rule_id("force_push_main") +@approval_timeout_s("600") +@severity("high") +@category("destructive") +forbid (principal, action == Agent::Action::"execute_bash", resource) +when { context.command like "*git push --force origin main*" + || context.command like "*git push --force origin prod*" + || context.command like "*git push -f origin main*" + || context.command like "*git push -f origin prod*" }; + +// Non-force pushes to protected branches — catches the case where an +// agent bypasses PR workflow by pushing directly. +@tier("soft") +@rule_id("push_to_protected_branch") +@approval_timeout_s("300") +@severity("medium") +@category("destructive") +forbid (principal, action == Agent::Action::"execute_bash", resource) +when { context.command like "*git push origin main*" + || context.command like "*git push origin master*" + || context.command like "*git push origin prod*" + || context.command like "*git push origin release/*" }; + +// Writes to `.env` files typically contain secrets. 600s window, high severity. +@tier("soft") +@rule_id("write_env_files") +@approval_timeout_s("600") +@severity("high") +@category("filesystem") +forbid (principal, action == Agent::Action::"write_file", resource) +when { context.file_path like "*.env" }; + +// Writes to any path containing "credentials" — SSH keys, AWS creds, +// service-account JSON, etc. 300s window, high severity. +@tier("soft") +@rule_id("write_credentials") +@approval_timeout_s("300") +@severity("high") +@category("auth") +forbid (principal, action == Agent::Action::"write_file", resource) +when { context.file_path like "*credentials*" }; diff --git a/agent/pyproject.toml b/agent/pyproject.toml index bc845271..70d660b5 100644 --- a/agent/pyproject.toml +++ b/agent/pyproject.toml @@ -11,7 +11,7 @@ dependencies = [ "uvicorn==0.46.0", #https://pypi.org/project/uvicorn/ "aws-opentelemetry-distro~=0.17.0", #https://pypi.org/project/aws-opentelemetry-distro/ "mcp==1.27.1", #https://pypi.org/project/mcp/ - "cedarpy>=4.8.1", #https://github.com/k9securityio/cedar-py + "cedarpy==4.8.0", #https://github.com/k9securityio/cedar-py — EXACT pin per mise.toml parity contract with @cedar-policy/cedar-wasm@4.10.0 ] [tool.bandit] diff --git a/agent/src/config.py b/agent/src/config.py index 0e9e4958..34d9f9be 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -92,6 +92,10 @@ def build_config( channel_metadata: dict[str, str] | None = None, trace: bool = False, user_id: str = "", + approval_timeout_s: int | None = None, + initial_approvals: list[str] | None = None, + initial_approval_gate_count: int = 0, + approval_gate_cap: int | None = None, ) -> TaskConfig: """Build and validate configuration from explicit parameters. @@ -146,6 +150,10 @@ def build_config( channel_metadata=channel_metadata or {}, trace=trace, user_id=user_id, + approval_timeout_s=approval_timeout_s, + initial_approvals=initial_approvals or [], + initial_approval_gate_count=initial_approval_gate_count, + approval_gate_cap=approval_gate_cap, ) diff --git a/agent/src/hooks.py b/agent/src/hooks.py index 634b7f83..77c0ebe7 100644 --- a/agent/src/hooks.py +++ b/agent/src/hooks.py @@ -1,34 +1,124 @@ """PreToolUse, PostToolUse, and Stop hook callbacks. -- PreToolUse / PostToolUse: policy enforcement (Cedar policy engine and the - output scanner for secrets/PII). -- Stop: between-turns nudge injection (Phase 2). When the agent is about to - stop a turn we check the TaskNudgesTable for pending user nudges and, if - any are present, inject them as authoritative ```` blocks via - the SDK's ``decision: "block"`` / ``reason: ...`` mechanism, which tells - the CLI to continue with that text as the next user message. - -A module-level registry ``between_turns_hooks`` lets future phases (e.g. -Phase 3 approval gates) append additional synthetic-message producers -without touching the Stop hook callback itself. +- PreToolUse: three-outcome Cedar policy enforcement (ALLOW / DENY / + REQUIRE_APPROVAL). The REQUIRE_APPROVAL path writes a pending approval + row + transitions the task to AWAITING_APPROVAL atomically, polls for a + human decision, then resumes / denies per the user's input. See + ``docs/design/CEDAR_HITL_GATES.md`` §6.5. +- PostToolUse: output scanner for secrets/PII. +- Stop: between-turns hook dispatcher. Cancel → nudge → denial injection + in that order (cancel-wins semantics, finding #2). Each producer + appends synthetic user-message strings that get reinjected via the + SDK's ``decision: "block"`` mechanism. + +A module-level registry ``between_turns_hooks`` lets phases (Phase 2 +nudges, Phase 3 denial injections) append additional synthetic-message +producers without touching the Stop hook callback itself. """ from __future__ import annotations import asyncio import json +import os +import re +import time from collections.abc import Callable from typing import TYPE_CHECKING, Any import nudge_reader import task_state +from nudge_reader import _xml_escape from output_scanner import scan_tool_output -from shell import log +from policy import APPROVAL_RATE_LIMIT, Outcome +from progress_writer import _generate_ulid +from shell import log, log_error_cw if TYPE_CHECKING: from policy import PolicyEngine from telemetry import _TrajectoryWriter +# --------------------------------------------------------------------------- +# Chunk 3 constants (§6.5 pseudocode) +# --------------------------------------------------------------------------- + +FLOOR_30S: int = 30 # §6 decision #6: approval floor on effective timeout +CLEANUP_MARGIN_120S: int = 120 # §6.5 lifetime-margin reserve for cleanup +# Poll cadence per §3 decision #3 and IMPL-12: 2s for the first 30s, 5s +# thereafter. Exact counts vary with ``timeout_s``; these pin the +# user-observable "fast for a bit, then slack off" behavior. +POLL_FAST_INTERVAL_S: float = 2.0 +POLL_FAST_DURATION_S: float = 30.0 +POLL_SLOW_INTERVAL_S: float = 5.0 +POLL_DEGRADED_FAILS: int = 3 # emit approval_poll_degraded at this count (§13.2) +POLL_MAX_CONSECUTIVE_FAILS: int = 10 # treat as TIMED_OUT at this count (§13.2) +TOOL_INPUT_PREVIEW_MAX: int = 256 # §6.5: strip-ANSI, truncate + +# ANSI CSI / OSC escape sequence stripper for ``tool_input_preview`` + +# ``permissionDecisionReason`` fields (§12.7). Re-derives the pattern from +# the canonical definition; kept local to avoid adding a cross-module +# dependency for one regex. +_ANSI_ESCAPE_RE = re.compile(r"\x1B(?:\[[0-?]*[ -/]*[@-~]|\][^\x07\x1B]*(?:\x07|\x1B\\)|[@-Z\\-_])") + + +def _strip_ansi(text: str) -> str: + """Remove ANSI CSI / OSC sequences to prevent terminal injection (§12.7).""" + return _ANSI_ESCAPE_RE.sub("", text) + + +def _truncate(text: str, max_len: int) -> str: + """Truncate ``text`` to ``max_len`` chars with an ellipsis marker.""" + if text is None: + return "" + if len(text) <= max_len: + return text + # Reserve 3 chars for the ellipsis so the returned string never + # exceeds ``max_len``. + return text[: max_len - 3] + "..." + + +def _tool_input_preview(tool_input: Any, max_len: int = TOOL_INPUT_PREVIEW_MAX) -> str: + """Return an ANSI-stripped, truncated preview of ``tool_input`` for DDB. + + Uses ``json.dumps`` so dict/list inputs render as stable JSON rather + than Python repr (avoids leaking ``OrderedDict(...)`` wrappers etc.). + Falls back to ``str()`` on any serialization error; never raises. + """ + try: + rendered = ( + tool_input if isinstance(tool_input, str) else json.dumps(tool_input, default=str) + ) + except (TypeError, ValueError): + rendered = str(tool_input) + return _truncate(_strip_ansi(rendered), max_len) + + +def _deny_response(reason: str) -> dict: + """Build a PreToolUse DENY response with a sanitized reason. + + Guaranteed surface: ``permissionDecisionReason`` is ANSI-stripped and + truncated to 500 chars so it can never carry terminal-escape injection + or overflow a log line (§6.5, §12.7). + """ + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": _truncate(_strip_ansi(reason or "denied"), 500), + } + } + + +def _allow_response(reason: str = "permitted") -> dict: + """Build a PreToolUse ALLOW response.""" + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "allow", + "permissionDecisionReason": reason, + } + } + async def pre_tool_use_hook( hook_input: Any, @@ -37,22 +127,37 @@ async def pre_tool_use_hook( *, engine: PolicyEngine, trajectory: _TrajectoryWriter | None = None, + task_id: str | None = None, + user_id: str | None = None, + progress: Any = None, + task_state_module: Any = None, ) -> dict: - """PreToolUse hook: evaluate tool call against Cedar policies. + """PreToolUse hook: three-outcome Cedar policy enforcement (§6.5). Returns a dict with hookSpecificOutput containing: - permissionDecision: "allow" or "deny" - permissionDecisionReason: explanation string + + The REQUIRE_APPROVAL path (Chunk 3, §6.5) pauses here: writes a + pending approval row + transitions the task to AWAITING_APPROVAL + atomically, polls for a human decision with 2s→5s backoff, then + returns allow / deny based on the decision. On TIMED_OUT a + ConditionCheckFailed from the best-effort status write triggers the + IMPL-24 re-read — if the user's decision landed between our poll + and write, we honor it instead of falsely denying. + + ``task_id`` / ``user_id`` / ``progress`` are optional to preserve + the Phase 1 test call shape. Without them the REQUIRE_APPROVAL path + falls through to fail-closed DENY (state-write infrastructure is + missing), so legacy callers still see coherent behaviour. + ``task_state_module`` is a test seam for injecting a mocked + ``task_state`` namespace; production callers rely on the default. """ + ts_module = task_state_module if task_state_module is not None else task_state + if not isinstance(hook_input, dict): log("WARN", "PreToolUse hook received non-dict input — denying") - return { - "hookSpecificOutput": { - "hookEventName": "PreToolUse", - "permissionDecision": "deny", - "permissionDecisionReason": "invalid hook input", - } - } + return _deny_response("invalid hook input") tool_name = hook_input.get("tool_name", "unknown") tool_input = hook_input.get("tool_input", {}) @@ -61,39 +166,728 @@ async def pre_tool_use_hook( tool_input = json.loads(tool_input) except (json.JSONDecodeError, TypeError): log("WARN", f"PreToolUse hook failed to parse tool_input — denying {tool_name}") - return { - "hookSpecificOutput": { - "hookEventName": "PreToolUse", - "permissionDecision": "deny", - "permissionDecisionReason": "unparseable tool input", - } - } + return _deny_response("unparseable tool input") decision = engine.evaluate_tool_use(tool_name, tool_input) - # Emit telemetry for all non-permitted decisions (including fail-closed) + # Telemetry: ALLOW "permitted" is the quiet happy path; everything else + # is worth a trajectory event. Treat REQUIRE_APPROVAL as "not allowed" + # for the legacy ``allowed=False`` field so the Phase 2 trajectory + # schema stays coherent — the specific outcome is already on the + # ``reason`` string. if trajectory and decision.reason != "permitted": trajectory.write_policy_decision( tool_name, decision.allowed, decision.reason, decision.duration_ms ) - if not decision.allowed: + if decision.outcome == Outcome.ALLOW: + return _allow_response(decision.reason or "permitted") + + if decision.outcome == Outcome.DENY: + # IMPL-23: when the DENY arrived from the recent-decision cache + # (evaluate_tool_use Step 2.5), emit a ``policy_decision`` + # milestone with ``decision_source="recent_decision_cache"`` to + # TaskEventsTable so cache-driven denies are visible in the live + # stream + 90d audit record (§12.8). No new approval row is + # written and the gate counter is NOT bumped — the original + # gate already accounted for the decision. + if progress is not None and decision.cache_hit_metadata is not None: + _try_progress( + progress, + "write_policy_decision_cached", + **decision.cache_hit_metadata, + ) log("POLICY", f"DENIED: {tool_name} — {decision.reason}") + return _deny_response(decision.reason) + + # -- REQUIRE_APPROVAL path (§6.5) --------------------------------------- + return await _handle_require_approval( + decision=decision, + tool_name=tool_name, + tool_input=tool_input, + engine=engine, + task_id=task_id, + user_id=user_id, + progress=progress, + ts=ts_module, + ) + + +async def _handle_require_approval( + *, + decision: Any, + tool_name: str, + tool_input: dict, + engine: PolicyEngine, + task_id: str | None, + user_id: str | None, + progress: Any, + ts: Any, +) -> dict: + """REQUIRE_APPROVAL branch of ``pre_tool_use_hook``. + + Split out of the main hook for readability — the control-flow in + §6.5 is long enough that inlining it obscures the top-level three-way + branch. + """ + # Missing task infrastructure → fail closed with a clear reason. This + # lines up with §13.15: every exceptional branch ends in DENY. + if not task_id: + log("WARN", "REQUIRE_APPROVAL hit without task_id — fail-closed deny") + return _deny_response("approval system unavailable (no task_id)") + + request_id = _generate_ulid() + + # Step 1 — per-task cap. §12.9: cap exceeded fails closed and the + # cap_exceeded milestone carries the configured cap so dashboards + # reflect the blueprint override. + if engine.approval_gate_count >= engine.approval_gate_cap: + if progress is not None: + _try_progress( + progress, + "write_approval_cap_exceeded", + request_id=request_id, + count=engine.approval_gate_count, + cap=engine.approval_gate_cap, + ) + return _deny_response(f"approval-gate cap exceeded ({engine.approval_gate_cap}/task)") + + # Step 2 — per-minute rate limit. ``approvals_in_last_minute`` prunes + # on read so the comparison is against the current sliding window. + rate = engine.approvals_in_last_minute + if rate >= APPROVAL_RATE_LIMIT: + if progress is not None: + _try_progress( + progress, + "write_approval_rate_limit_exceeded", + request_id=request_id, + rate=rate, + limit=APPROVAL_RATE_LIMIT, + ) + return _deny_response(f"approval-gate rate limit exceeded ({APPROVAL_RATE_LIMIT}/min)") + + # Step 3 — effective timeout with floor/ceiling math (§6.5). Emit + # ``approval_timeout_capped`` when the caller's ask is clipped so the + # user can see why (IMPL-26). + remaining_lifetime = _remaining_maxlifetime_s() + effective_timeout, clip_reason, requested_timeout = _compute_effective_timeout( + decision_timeout_s=decision.timeout_s, + task_default_timeout_s=engine.task_default_timeout_s, + remaining_lifetime_s=remaining_lifetime, + ) + if clip_reason is not None and progress is not None: + _try_progress( + progress, + "write_approval_timeout_capped", + request_id=request_id, + requested_timeout_s=requested_timeout, + effective_timeout_s=effective_timeout, + reason=clip_reason, + matching_rule_ids=( + list(decision.matching_rule_ids) if clip_reason == "rule_annotation" else None + ), + ) + # IMPL-26: once per task, surface the "gates will have small windows + # from here on" ceiling-shrinking milestone when the remaining + # lifetime is approaching the 2x-task-default threshold. + if ( + remaining_lifetime is not None + and remaining_lifetime - CLEANUP_MARGIN_120S < 2 * engine.task_default_timeout_s + and engine.mark_ceiling_shrinking_emitted() + and progress is not None + ): + _try_progress( + progress, + "write_approval_ceiling_shrinking", + request_id=request_id, + max_lifetime_remaining_s=remaining_lifetime, + cleanup_margin_s=CLEANUP_MARGIN_120S, + task_default_timeout_s=engine.task_default_timeout_s, + ) + + # Step 4 — insufficient lifetime remaining for a valid approval + # (§13.7). Below the floor we DENY immediately without writing the + # approval row; no point waking the user to a guaranteed-dead gate. + if remaining_lifetime is not None and remaining_lifetime - CLEANUP_MARGIN_120S < FLOOR_30S: + return _deny_response( + f"insufficient maxLifetime remaining ({remaining_lifetime}s) for approval" + ) + + # Step 5 — build the approval row per §10.1 schema. + tool_input_sha256 = _sha256_tool_input_for_row(tool_input) + row = { + "task_id": task_id, + "request_id": request_id, + "tool_name": tool_name, + "tool_input_preview": _tool_input_preview(tool_input), + "tool_input_sha256": tool_input_sha256, + "reason": decision.reason, + "severity": decision.severity or "medium", + "matching_rule_ids": list(decision.matching_rule_ids), + "status": "PENDING", + "created_at": _iso_now(), + "timeout_s": effective_timeout, + "ttl": int(time.time()) + effective_timeout + CLEANUP_MARGIN_120S, + "user_id": user_id or "", + "repo": engine.repo, + } + + # Step 6 — bump counters BEFORE the write so cap/rate checks on + # subsequent gates reflect the attempt even if the DDB write itself + # fails. The session counter survives within the task; the failure + # path below emits ``approval_write_failed`` so the lost row is + # visible. + engine.increment_approval_gate_count() + engine.record_approval_gate_timestamp() + + # Chunk 7 (§13.6): best-effort atomic increment of the persisted + # ``approval_gate_count`` on TaskTable. The session counter + # enforces the cap within THIS container; the persisted counter + # exists so a restarted container re-seeds from a non-zero value + # instead of re-exposing the user to another ``approval_gate_cap`` + # worth of gates. Failure is best-effort per §13.6 — "counter is + # a safety bound, not a correctness bound" — so we keep going on + # error and accept the (bounded) restart-retry amplification. + if task_id: + await asyncio.to_thread(ts.increment_approval_gate_count_in_ddb, task_id) + + # Step 7 — cross-table atomic transition. + try: + await asyncio.to_thread(ts.transact_write_approval_request, task_id, request_id, row) + except ts.ApprovalWriteError as exc: + if progress is not None: + _try_progress( + progress, + "write_approval_write_failed", + request_id=request_id, + error=f"cancelled: {exc.cancellation_reasons}", + ) + return _deny_response("approval system unavailable (write cancelled)") + except ts.ApprovalTablesUnavailable as exc: + if progress is not None: + _try_progress( + progress, + "write_approval_write_failed", + request_id=request_id, + error=f"tables unavailable: {exc}", + ) + return _deny_response("approval system unavailable (tables unconfigured)") + except Exception as exc: + log( + "ERROR", + f"approval request write failed: {type(exc).__name__}: {exc}", + ) + if progress is not None: + _try_progress( + progress, + "write_approval_write_failed", + request_id=request_id, + error=f"{type(exc).__name__}: {exc}", + ) + return _deny_response("approval system unavailable") + + # Step 8 — ``approval_requested`` milestone so the user's stream + # shows the gate immediately. + if progress is not None: + _try_progress( + progress, + "write_approval_requested", + request_id=request_id, + tool_name=tool_name, + input_preview=row["tool_input_preview"], + reason=decision.reason, + severity=row["severity"], + timeout_s=effective_timeout, + matching_rule_ids=list(decision.matching_rule_ids), + ) + + # Step 9 — poll for a decision. + outcome = await _poll_for_decision( + task_id=task_id, + request_id=request_id, + timeout_s=effective_timeout, + progress=progress, + ts=ts, + ) + + # Step 10 — IMPL-24 VM-throttle + late-approval race. Best-effort + # flip to TIMED_OUT; if ConditionCheckFailed, the user beat us — read + # and honor. + if outcome["status"] == "TIMED_OUT": + try: + wrote = await asyncio.to_thread( + ts.best_effort_update_approval_status, + task_id, + request_id, + "TIMED_OUT", + reason=outcome.get("reason"), + ) + except Exception as exc: + log("WARN", f"approval TIMED_OUT write raised: {type(exc).__name__}: {exc}") + # Fall into the IMPL-24 re-read path. A transient DDB write + # error MUST NOT bypass the late-approval check — the user's + # APPROVED decision may already be on the row, and skipping + # the re-read would falsely deny their tool call. Setting + # ``wrote = False`` triggers the ConsistentRead below; if + # the row still says PENDING the re-read is a no-op and we + # keep the TIMED_OUT outcome. If it says APPROVED/DENIED, + # ``_reconcile_late_decision`` honors the user's choice. + wrote = False + if not wrote: + # User's decision beat our timer; re-read with ConsistentRead. + try: + row_reread = await asyncio.to_thread( + ts.get_approval_row, + task_id, + request_id, + consistent_read=True, + ) + except Exception as exc: + log("WARN", f"approval re-read raised: {type(exc).__name__}: {exc}") + row_reread = None + outcome = _reconcile_late_decision(outcome, row_reread, progress, request_id) + + # Step 11 — resume transition (RUNNING). The ``awaiting_approval_request_id`` + # condition prevents resuming a cancelled task or racing with another + # approval. + try: + await asyncio.to_thread(ts.transact_resume_from_approval, task_id, request_id) + except ts.ApprovalResumeError as exc: + if progress is not None: + _try_progress( + progress, + "write_approval_resume_failed", + request_id=request_id, + error=f"cancelled: {exc.cancellation_reasons}", + ) + return _deny_response("task no longer awaiting approval") + except Exception as exc: + log("WARN", f"approval resume raised: {type(exc).__name__}: {exc}") + if progress is not None: + _try_progress( + progress, + "write_approval_resume_failed", + request_id=request_id, + error=f"{type(exc).__name__}: {exc}", + ) + return _deny_response("approval resume failed") + + # Step 12 — terminal branches. + status = outcome.get("status") + if status == "APPROVED": + scope = outcome.get("scope") or "this_call" + if scope != "this_call": + try: + engine.allowlist.add(scope) + except ValueError as exc: + # Malformed scope from the API — log loudly but still + # allow this one call (the user did approve it). + log("WARN", f"invalid approved scope {scope!r}: {exc}") + if progress is not None: + _try_progress( + progress, + "write_approval_granted", + request_id=request_id, + scope=scope, + decided_at=outcome.get("decided_at"), + # Chunk 8a: propagate the row's ``created_at`` so the + # ApprovalMetricsPublisher can compute decision latency. + created_at=row.get("created_at"), + ) + return _allow_response(f"User approved ({scope})") + + # DENIED or TIMED_OUT — cache + queue injection. + cache_decision = "DENIED" if status == "DENIED" else "TIMED_OUT" + # IMPL-23: thread the user's ``decided_at`` into the cache entry so + # subsequent cache-hit events surface the ORIGINAL decision timestamp, + # not the wall-clock time the cache was populated (which is ~the same + # but technically wrong). ``decided_at`` for TIMED_OUT is the + # agent-side clock moment the timeout fired; for DENIED it's the + # user's deny timestamp from the Lambda audit row. + engine.recent_decisions.record( + tool_name, + tool_input_sha256, + decision=cache_decision, + reason=outcome.get("reason", ""), + original_decision_ts=outcome.get("decided_at"), + ) + + # Rule-level cache (§12.8 extension): on DENIED, record an entry + # per matching_rule_id so semantic retries — same rule, different + # input — get fast-denied without a new approval round-trip. Only + # populate on DENIED because TIMED_OUT is ambiguous (user was + # away, not actively refusing); TIMED_OUT cache entries stay + # input-hash-scoped. + if status == "DENIED": + for rule_id in decision.matching_rule_ids: + engine.recent_decisions.record_rule_decision( + tool_name, + rule_id, + decision="DENIED", + reason=outcome.get("reason", ""), + original_decision_ts=outcome.get("decided_at"), + ) + + if status == "DENIED": + engine.queue_denial_injection( + request_id=request_id, + reason=outcome.get("reason", ""), + decided_at=outcome.get("decided_at"), + ) + if progress is not None: + _try_progress( + progress, + "write_approval_denied", + request_id=request_id, + reason=outcome.get("reason", ""), + decided_at=outcome.get("decided_at"), + # Chunk 8a: propagate the row's ``created_at`` so the + # ApprovalMetricsPublisher can compute decision latency. + created_at=row.get("created_at"), + ) + elif progress is not None: + _try_progress( + progress, + "write_approval_timed_out", + request_id=request_id, + timeout_s=effective_timeout, + # Chunk 8a: propagate the row's ``created_at`` + + # ``matching_rule_ids`` + the post-clip effective timeout + # so the ApprovalMetricsPublisher can emit the decision + # latency + the ``ApprovalTimeoutBreakdown`` histogram + # with a normalized ``rule_id`` dimension. + created_at=row.get("created_at"), + effective_timeout_s=effective_timeout, + matching_rule_ids=list(decision.matching_rule_ids), + ) + + # Guaranteed surface (§6.5): truncated reason even when denial + # injection is pre-empted by a concurrent cancel. Wrap the user's + # reason in authoritative stop-language — E2E Phase 4 observed + # the agent treating bare "User denied" as "try a different + # approach" and burning through max_turns retrying the same rule + # with trivial variations. The explicit AUTHORITATIVE-prefixed + # wording, combined with the rule-level recent-deny cache (§12.8), + # makes retries fail fast with clear feedback. + raw_reason = outcome.get("reason") or f"User {status.lower() if status else 'denied'}" + if status == "DENIED": + rule_hint = ( + f" (matching rule{'s' if len(decision.matching_rule_ids) != 1 else ''}: " + f"{', '.join(decision.matching_rule_ids)})" + if decision.matching_rule_ids + else "" + ) + reason_text = ( + f"AUTHORITATIVE DENY from human reviewer: {raw_reason}{rule_hint}. " + "Do NOT retry this class of action with trivial variations; " + "the same rule will fast-deny subsequent attempts. Find an " + "alternative task strategy or report back to the user explaining " + "why progress is blocked." + ) + else: + reason_text = raw_reason + return _deny_response(reason_text) + + +def _reconcile_late_decision( + outcome: dict, + row: dict | None, + progress: Any, + request_id: str, +) -> dict: + """IMPL-24: rebuild outcome from a re-read row after a TIMED_OUT race. + + - ``row["status"] == "APPROVED"`` → rebuild as APPROVED (allow flow). + - ``row["status"] == "DENIED"`` → rebuild as DENIED (deny flow). + - Anything else (row gone, still PENDING) → fall through with the + original TIMED_OUT (§13.12 fail-closed branch). + + Emits ``approval_late_win`` for APPROVED or DENIED races so operator + telemetry can count them. + """ + if row is None: + return outcome + status = row.get("status") + if status == "APPROVED": + if progress is not None: + _try_progress( + progress, + "write_approval_late_win", + request_id=request_id, + outcome="APPROVED", + reason="user decision landed during TIMED_OUT write", + ) return { - "hookSpecificOutput": { - "hookEventName": "PreToolUse", - "permissionDecision": "deny", - "permissionDecisionReason": decision.reason, - } + "status": "APPROVED", + "scope": row.get("scope"), + "decided_at": row.get("decided_at"), + "decided_by": row.get("user_id"), } - - return { - "hookSpecificOutput": { - "hookEventName": "PreToolUse", - "permissionDecision": "allow", - "permissionDecisionReason": "permitted", + if status == "DENIED": + if progress is not None: + _try_progress( + progress, + "write_approval_late_win", + request_id=request_id, + outcome="DENIED", + reason="user decision landed during TIMED_OUT write", + ) + return { + "status": "DENIED", + "reason": row.get("deny_reason") or "denied", + "decided_at": row.get("decided_at"), } - } + return outcome + + +async def _poll_for_decision( + *, + task_id: str, + request_id: str, + timeout_s: int, + progress: Any, + ts: Any, +) -> dict: + """Poll the approval row until terminal or timeout. + + Cadence: ``POLL_FAST_INTERVAL_S`` for ``POLL_FAST_DURATION_S``, then + ``POLL_SLOW_INTERVAL_S`` (IMPL-12). Each iteration uses + ConsistentRead; after ``POLL_DEGRADED_FAILS`` consecutive failures we + emit ``approval_poll_degraded``; at ``POLL_MAX_CONSECUTIVE_FAILS`` we + fall through as TIMED_OUT with a distinct reason (§13.2). + + Returns an outcome dict mirroring the approval row's terminal fields. + """ + deadline = time.monotonic() + timeout_s + start = time.monotonic() + consecutive_fails = 0 + degraded_emitted = False + + while True: + now = time.monotonic() + if now >= deadline: + return {"status": "TIMED_OUT", "reason": None} + + try: + row = await asyncio.to_thread( + ts.get_approval_row, + task_id, + request_id, + consistent_read=True, + ) + consecutive_fails = 0 + except Exception as exc: + consecutive_fails += 1 + log( + "WARN", + f"approval poll get_item raised ({consecutive_fails}/" + f"{POLL_MAX_CONSECUTIVE_FAILS}): {type(exc).__name__}: {exc}", + ) + if consecutive_fails >= POLL_DEGRADED_FAILS and not degraded_emitted: + if progress is not None: + _try_progress( + progress, + "write_approval_poll_degraded", + request_id=request_id, + consecutive_failures=consecutive_fails, + ) + degraded_emitted = True + if consecutive_fails >= POLL_MAX_CONSECUTIVE_FAILS: + return { + "status": "TIMED_OUT", + "reason": f"poll failed {consecutive_fails} consecutive times", + } + row = None # force sleep below + + if row is not None: + status = row.get("status") + if status == "APPROVED": + return { + "status": "APPROVED", + "scope": row.get("scope"), + "decided_at": row.get("decided_at"), + "decided_by": row.get("user_id"), + } + if status == "DENIED": + return { + "status": "DENIED", + "reason": row.get("deny_reason") or "denied", + "decided_at": row.get("decided_at"), + } + + # Compute sleep interval based on elapsed since poll started. + elapsed = time.monotonic() - start + interval = POLL_FAST_INTERVAL_S if elapsed < POLL_FAST_DURATION_S else POLL_SLOW_INTERVAL_S + # Clamp sleep against remaining deadline so we don't oversleep. + sleep_for = min(interval, max(0.0, deadline - time.monotonic())) + if sleep_for <= 0: + return {"status": "TIMED_OUT", "reason": None} + await asyncio.sleep(sleep_for) + + +def _compute_effective_timeout( + *, + decision_timeout_s: int | None, + task_default_timeout_s: int, + remaining_lifetime_s: int | None, +) -> tuple[int, str | None, int]: + """Compute the effective timeout per §6.5. + + ``min(rule-annotation timeout, task default, remaining lifetime - + cleanup margin)``, floored at FLOOR_30S. The engine's + ``_merge_annotations`` already applies ``min(rule_annotation, + task_default)`` — decision.timeout_s reaches us pre-clipped against + those two. Here we apply the remaining-lifetime ceiling and report + whichever source pulled the effective timeout below the task + default, so the user sees "your gate was clipped because ..." rather + than silent clipping. + + Returns ``(effective, clip_reason, requested)``: + - ``requested`` — the user-visible "would have liked" value (task + default for display purposes; the rule's ask was already merged). + - ``clip_reason`` — ``"rule_annotation"`` when the rule's annotation + pulled the decision below the task default; ``"maxLifetime_ceiling"`` + when the remaining-lifetime ceiling is the tightest bound; ``None`` + when nothing clipped. + """ + requested = task_default_timeout_s + decision_value = ( + decision_timeout_s if decision_timeout_s is not None else task_default_timeout_s + ) + + # Start with the decision value (already clipped by rule vs task default + # in the engine) and apply the remaining-lifetime ceiling here. + effective = decision_value + clip_reason: str | None = None + + # Rule annotation clipped below task default — surface that first so a + # later lifetime-ceiling clip can override with a more specific reason. + if decision_value < requested: + clip_reason = "rule_annotation" + + if remaining_lifetime_s is not None: + ceiling = remaining_lifetime_s - CLEANUP_MARGIN_120S + if ceiling < effective: + effective = ceiling + clip_reason = "maxLifetime_ceiling" + + # Floor: if clipping pushed below the hard floor, the effective value + # is floored (so the user can still respond) but the clip reason is + # still the tightest-binding input. Floor is a safety net, not a user + # concept. + if effective < FLOOR_30S: + effective = FLOOR_30S + + return effective, clip_reason, requested + + +def _remaining_maxlifetime_s() -> int | None: + """Compute remaining AgentCore maxLifetime seconds. + + Reads ``AGENTCORE_MAX_LIFETIME_S`` (default 8h) and ``TASK_STARTED_AT`` + (ISO 8601, optional). Returns ``None`` if the start timestamp is + unavailable; the hook treats this as "unknown, don't clip" so the + gate still fires with the task default (fail-open on the optional + signal rather than pre-DENY when unknown). A future Chunk wires + these from the task launch path; for now they are optional hints. + """ + try: + max_lifetime = int(os.environ.get("AGENTCORE_MAX_LIFETIME_S", "28800")) + except ValueError: + max_lifetime = 28800 + started_at = os.environ.get("TASK_STARTED_AT") + if not started_at: + return None + try: + # Support both ISO 8601 (YYYY-MM-DDTHH:MM:SSZ) and raw epoch seconds. + if started_at.isdigit(): + started_epoch = int(started_at) + else: + from datetime import datetime + + started_epoch = int(datetime.strptime(started_at, "%Y-%m-%dT%H:%M:%SZ").timestamp()) + except (ValueError, AttributeError): + return None + elapsed = int(time.time()) - started_epoch + remaining = max_lifetime - elapsed + return max(0, remaining) + + +def _sha256_tool_input_for_row(tool_input: Any) -> str: + """Stable SHA-256 of ``tool_input`` for the approval row + cache key. + + Re-derives hashing here (rather than importing ``policy._sha256_tool_input``) + so the hook's failure-mode is independent of the engine's internals and + to keep import graphs shallow. The engine's own cache uses the same + algorithm; §6.5 row + ``RecentDecisionCache`` need the same key shape. + """ + import hashlib + + try: + serialized = ( + tool_input if isinstance(tool_input, str) else json.dumps(tool_input, sort_keys=True) + ) + except (TypeError, ValueError): + serialized = str(tool_input) + return hashlib.sha256(serialized.encode("utf-8")).hexdigest() + + +def _iso_now() -> str: + """ISO 8601 UTC timestamp in the ``YYYY-MM-DDTHH:MM:SSZ`` form.""" + return time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) + + +# S10: per-method consecutive-failure counters for ``_try_progress``. +# Progress writes are best-effort, but a hard infrastructure failure +# (DDB throttle, IAM regression, missing table) would otherwise be +# masked as a stream of WARN lines with no escalation. After +# ``_TRY_PROGRESS_ESCALATE_AFTER`` consecutive failures of the same +# method the next failure is logged at ERROR via ``log_error_cw`` so +# it lands in APPLICATION_LOGS / TaskDashboard, and the counter +# resets so escalations don't spam. Per-method state because a +# single failing method shouldn't suppress visibility into other +# kinds of progress writes that might still be working. +_TRY_PROGRESS_ESCALATE_AFTER = 5 +_try_progress_consecutive_failures: dict[str, int] = {} + + +def _try_progress(progress: Any, method_name: str, /, **kwargs: Any) -> None: + """Call a progress-writer method, swallowing errors. + + Progress is best-effort observability; a throttled DDB write must not + break the approval flow. ``_emit_nudge_milestone`` uses a similar + pattern for the Phase 2 nudges. + + S10 escalation: repeated consecutive failures of the same + ``method_name`` are tracked per-process and escalated to ERROR + after ``_TRY_PROGRESS_ESCALATE_AFTER`` so a silent live-stream + outage becomes operator-visible. + """ + if getattr(progress, "_disabled", False) is True: + log("WARN", f"progress {method_name!r} skipped: circuit breaker open") + return + method = getattr(progress, method_name, None) + if method is None: + log("DEBUG", f"progress missing {method_name!r}; skipping") + return + try: + method(**kwargs) + except Exception as exc: # pragma: no cover — defensive + prev = _try_progress_consecutive_failures.get(method_name, 0) + count = prev + 1 + if count >= _TRY_PROGRESS_ESCALATE_AFTER: + log_error_cw( + f"progress {method_name!r} has failed {count} consecutive times; " + f"latest: {type(exc).__name__}: {exc}", + ) + _try_progress_consecutive_failures[method_name] = 0 # reset to avoid spam + else: + log("WARN", f"progress {method_name!r} raised: {type(exc).__name__}: {exc}") + _try_progress_consecutive_failures[method_name] = count + return + # Success path — reset the consecutive-failure count. + if _try_progress_consecutive_failures.get(method_name): + _try_progress_consecutive_failures[method_name] = 0 async def post_tool_use_hook( @@ -315,6 +1109,64 @@ def _nudge_between_turns_hook(ctx: dict) -> list[str]: return [formatted] if formatted else [] +def _denial_between_turns_hook(ctx: dict) -> list[str]: + """Drain queued denial injections into ```` XML blocks. + + Registered AFTER ``_cancel_between_turns_hook`` and + ``_nudge_between_turns_hook`` (cancel-wins semantics, finding #2). + If cancel flagged this turn we early-return — denial injection is + explicitly best-effort on cancelled tasks, and the guaranteed + surface is the ``permissionDecisionReason`` that + ``pre_tool_use_hook`` already returned on the deny response + (§6.5 line 922). + + ``ctx["engine"]`` is expected when the pipeline wires the approval + engine through; absent it this hook is a no-op (Phase 1 call sites + that don't thread an engine ref through still work). + """ + if ctx.get("_cancel_requested"): + return [] + engine = ctx.get("engine") + if engine is None: + return [] + # ``drain_denial_injections`` clears the queue; this is deliberate + # so a transient SDK failure that drops the injection does not + # re-inject the same denial on every subsequent Stop seam. + try: + pending = engine.drain_denial_injections() + except Exception as exc: # pragma: no cover — defensive + log("WARN", f"denial drain raised: {type(exc).__name__}: {exc}") + return [] + if not pending: + return [] + + blocks: list[str] = [] + request_ids: list[str] = [] + for entry in pending: + rid = _xml_escape(str(entry.get("request_id", ""))) + reason = _xml_escape(str(entry.get("reason", ""))) + decided_at = _xml_escape(str(entry.get("decided_at", "") or "")) + blocks.append( + f'\n{reason}\n' + ) + if entry.get("request_id"): + request_ids.append(str(entry["request_id"])) + + count = len(pending) + log("POLICY", f"Injecting {count} denial(s) for task {ctx.get('task_id', '')}") + # Emit a single ``user_denial_injected`` milestone per Stop seam so + # the durable event stream records the ack. Mirrors + # ``nudge_acknowledged`` (§AD-5); this is an additive milestone not + # in the §11.1 enumerated list, so keep the name distinct from the + # enumerated ``approval_*`` prefix. + _emit_nudge_milestone( + ctx, + "user_denial_injected", + f"{count} denial(s) injected (ids={','.join(request_ids)})", + ) + return ["\n".join(blocks)] if blocks else [] + + def _cancel_between_turns_hook(ctx: dict) -> list[str]: """Detect user-initiated cancellation and signal the Stop hook to halt. @@ -361,6 +1213,11 @@ def _cancel_between_turns_hook(ctx: dict) -> list[str]: between_turns_hooks: list[BetweenTurnsHook] = [ _cancel_between_turns_hook, _nudge_between_turns_hook, + # Chunk 3 (finding #2): denial injection runs LAST so both cancel and + # nudge short-circuits pre-empt it. The hook explicitly re-checks + # ``_cancel_requested`` so re-ordering doesn't silently break cancel + # semantics (belt-and-braces, matching ``_nudge_between_turns_hook``). + _denial_between_turns_hook, ] @@ -371,6 +1228,7 @@ async def stop_hook( *, task_id: str, progress: Any = None, + engine: Any = None, ) -> dict: """Stop hook: run registered between-turns hooks; block if they produce text. @@ -385,11 +1243,14 @@ async def stop_hook( ``progress`` is an optional writer ref threaded into each hook's ``ctx`` so hooks can emit their own milestone / progress events without holding - a module-global reference to it. + a module-global reference to it. ``engine`` is threaded through for + ``_denial_between_turns_hook`` which needs to drain queued denials; + absent it the denial hook is a no-op (Phase 1 / Phase 2 call paths). """ ctx = { "task_id": task_id, "progress": progress, + "engine": engine, } # Cancel-before-nudge short-circuit (krokoko PR #52 review finding #3). @@ -447,6 +1308,7 @@ def build_hook_matchers( trajectory: _TrajectoryWriter | None = None, task_id: str = "", progress: Any = None, + user_id: str = "", ) -> dict: """Build hook matchers dict for ClaudeAgentOptions. @@ -456,9 +1318,10 @@ def build_hook_matchers( The SDK expects ``dict[HookEvent, list[HookMatcher]]`` where HookMatcher has ``matcher: str | None`` and ``hooks: list[HookCallback]``. - ``progress`` is forwarded to the Stop hook so that between-turns hooks - can emit milestones (e.g. ``nudge_acknowledged``) that show up in the - durable progress stream as a visible marker of Phase 2 nudge activity. + ``progress`` is forwarded to both the PreToolUse hook (approval gate + milestones) and the Stop hook (nudge/denial acks). ``user_id`` is + written onto the approval row so ownership checks on the REST side + can enforce §12.2 (user can only approve their own gates). """ from claude_agent_sdk.types import ( HookContext, @@ -474,9 +1337,36 @@ def build_hook_matchers( async def _pre( hook_input: HookInput, tool_use_id: str | None, ctx: HookContext ) -> HookJSONOutput: - result = await pre_tool_use_hook( - hook_input, tool_use_id, ctx, engine=engine, trajectory=trajectory - ) + # Fail-closed wrapper (mirrors _post and _stop). If the inner hook + # or its dispatch path raises an unexpected exception (asyncio + # cancellation, TypeError from a malformed payload, etc.), the + # SDK's default behaviour for an unhandled hook exception is + # undefined — we MUST NOT trust it to fail closed. Mapping every + # uncaught exception to a DENY here makes the security posture + # explicit at the SDK boundary. + try: + result = await pre_tool_use_hook( + hook_input, + tool_use_id, + ctx, + engine=engine, + trajectory=trajectory, + task_id=task_id or None, + user_id=user_id or None, + progress=progress, + ) + except Exception as exc: + log( + "ERROR", + f"PreToolUse wrapper crashed (task_id={task_id}): {type(exc).__name__}: {exc}", + ) + log_error_cw( + f"PreToolUse wrapper crashed: {type(exc).__name__}: {exc}", + task_id=task_id or None, + ) + return SyncHookJSONOutput( + **_deny_response("Hook error — fail-closed deny"), + ) return SyncHookJSONOutput(**result) async def _post( @@ -506,6 +1396,7 @@ async def _stop( ctx, task_id=stop_task_id, progress=progress, + engine=engine, ) except Exception as exc: log( diff --git a/agent/src/models.py b/agent/src/models.py index 255c18f9..bda8fe13 100644 --- a/agent/src/models.py +++ b/agent/src/models.py @@ -129,6 +129,25 @@ class TaskConfig(BaseModel): trace: bool = False # Enriched mid-flight by pipeline.py: cedar_policies: list[str] = [] + # Cedar HITL (§7.3, §10.2). Per-task approval defaults threaded + # from the orchestrator payload; consumed by PolicyEngine at + # construction so the engine seeds ApprovalAllowlist and adopts + # the per-task timeout default. + approval_timeout_s: int | None = None + initial_approvals: list[str] = [] + # Chunk 7: TaskTable-persisted ``approval_gate_count`` seeded into + # the session counter so container restarts (§13.6) resume the + # cumulative gate budget without resetting to 0. Threaded from the + # orchestrator payload; zero default preserves legacy callers. + initial_approval_gate_count: int = 0 + # Chunk 7b (§4 step 5, decision #13): per-task approval-gate cap + # resolved at task submit-time from ``Blueprint.security.approvalGateCap`` + # (or the platform default of 50). Persisted on the TaskRecord so + # it survives container restarts and mid-task blueprint edits do + # not shift the cap beneath a running task. ``None`` when the + # orchestrator payload did not include the field (legacy tasks); + # PolicyEngine falls back to its own default of 50 in that case. + approval_gate_cap: int | None = None issue: GitHubIssue | None = None base_branch: str | None = None diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index 9a20afe9..10f930ab 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -29,7 +29,7 @@ from progress_writer import _ProgressWriter from prompt_builder import build_system_prompt, discover_project_config from runner import run_agent -from shell import log +from shell import log, log_error_cw from system_prompt import SYSTEM_PROMPT from telemetry import ( _TrajectoryWriter, @@ -244,6 +244,10 @@ def run_task( branch_name: str = "", pr_number: str = "", cedar_policies: list[str] | None = None, + approval_timeout_s: int | None = None, + initial_approvals: list[str] | None = None, + initial_approval_gate_count: int = 0, + approval_gate_cap: int | None = None, channel_source: str = "", channel_metadata: dict[str, str] | None = None, trace: bool = False, @@ -282,6 +286,10 @@ def run_task( channel_metadata=channel_metadata, trace=trace, user_id=user_id, + approval_timeout_s=approval_timeout_s, + initial_approvals=initial_approvals, + initial_approval_gate_count=initial_approval_gate_count, + approval_gate_cap=approval_gate_cap, ) # Inject Cedar policies into config for the PolicyEngine in runner.py @@ -464,7 +472,13 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: ) ) except Exception as e: - log("ERROR", f"Agent failed: {e}") + # Fatal agent error: mirror to APPLICATION_LOGS so + # TaskDashboard widgets + ``bgagent status`` can see + # the real failure text instead of stopping at + # ``error_classification.UNKNOWN``. Local stdout + # path is preserved for docker-compose / unit-test + # capture. + log_error_cw(f"Agent failed: {e}", task_id=config.task_id or None) agent_span.set_status(StatusCode.ERROR, str(e)) agent_span.record_exception(e) agent_result = AgentResult(status="error", error=str(e)) diff --git a/agent/src/policy.py b/agent/src/policy.py index bbda2928..64fb419e 100644 --- a/agent/src/policy.py +++ b/agent/src/policy.py @@ -1,93 +1,790 @@ -"""Cedar policy engine for tool-call governance. - -Uses cedarpy (in-process Cedar evaluation) to enforce per-task-type -tool restrictions. No network calls, no AWS Verified Permissions. - -Custom Cedar policies (via Blueprint ``security.cedarPolicies``) must -use ``context`` conditions in ``when`` clauses — not ``resource ==`` -matching — for ``write_file`` and ``execute_bash`` actions. The engine -passes fixed sentinel resource IDs (``Agent::File::"file"``, -``Agent::BashCommand::"command"``) because Cedar entity UIDs cannot -contain the special characters found in file paths and bash commands. -The actual values are available in ``context.file_path`` and -``context.command`` respectively. For ``invoke_tool`` actions, the -resource ID is the real tool name (e.g. ``Agent::Tool::"Write"``), so -``resource ==`` matching works normally there. +"""Cedar policy engine — three-outcome (allow / deny / require-approval). + +Uses cedarpy (in-process Cedar evaluation) to enforce per-task-type tool +restrictions. See ``docs/design/CEDAR_HITL_GATES.md`` for the full design; +short summary below. + +**Three outcomes** (§2, §6.2). Each ``evaluate_tool_use`` call walks up to +two Cedar evaluations interleaved with in-process caches: + + 1. Hard-deny Cedar eval (agent/policies/hard_deny.cedar + blueprint hard). + Absolute; no --pre-approve scope or blueprint disable can bypass. + 2. Approval allowlist fast-path (tool_type, tool_group, bash_pattern, + write_path, all_session scopes). Skips human prompt for pre-approved + patterns. + 2.5 Recent-decision cache: same (tool_name, input_sha256) within 60s of a + DENIED/TIMED_OUT outcome auto-denies. Session-scoped, cleared on + container restart (§12.8). + 3. Soft-deny Cedar eval (agent/policies/soft_deny.cedar + blueprint soft). + Match → REQUIRE_APPROVAL with merged annotations; rule-scope allowlist + match → ALLOW; no match → fall through to step 4. + 4. Default ALLOW. + +**Cedar-entity conventions** preserved from Phase 1: user-supplied values +(bash commands, file paths) use sentinel resource IDs (``Agent::File::file``, +``Agent::BashCommand::command``) with the real value in ``context.command`` / +``context.file_path``, because Cedar entity UIDs cannot contain arbitrary +characters. + +**Annotations** expected on every rule in hard_deny/soft_deny files +(§5.2): ``@rule_id`` (globally unique, kebab/snake_case), ``@tier`` +("hard"|"soft"), ``@approval_timeout_s`` (int seconds ≥ 30; soft-deny +only), ``@severity`` ("low"|"medium"|"high"; soft-deny only), ``@category`` +(free-form; UX grouping). Annotation recovery goes through cedarpy's +``policies_to_json_str()``; the round-trip contract is locked by +``tests/test_cedarpy_annotations_contract.py``. + +**Fail-closed posture** (§13): any cedarpy exception during evaluation +returns ``Outcome.DENY`` with reason ``"fail-closed: "``. +Invalid blueprint policies raise at ``PolicyEngine.__init__`` (task fails +to start rather than running with broken rules). Example — correct custom policy:: - forbid (principal, action == Agent::Action::"execute_bash", resource) - when { context.command like "*curl*" }; + @tier("soft") + @rule_id("webfetch_any") + @severity("medium") + forbid (principal, action == Agent::Action::"invoke_tool", + resource == Agent::Tool::"WebFetch"); -Example — WILL NOT WORK (resource is always ``"command"``):: +Example — WILL NOT WORK (resource is always the sentinel UID):: forbid (principal, action == Agent::Action::"execute_bash", resource == Agent::BashCommand::"curl http://evil.com"); """ +from __future__ import annotations + +import hashlib +import json import time +from collections import OrderedDict, deque from dataclasses import dataclass +from datetime import UTC, datetime +from enum import StrEnum +from fnmatch import fnmatch +from pathlib import Path +from typing import TYPE_CHECKING from shell import log -# Baseline: allow all. Specific forbid rules override (deny-list approach). -_DEFAULT_POLICIES = """\ -// Catch-all permit (deny-list model) -permit (principal, action, resource); - -// pr_review: forbid Write and Edit tools -forbid ( - principal == Agent::TaskAgent::"pr_review", - action == Agent::Action::"invoke_tool", - resource == Agent::Tool::"Write" -); -forbid ( - principal == Agent::TaskAgent::"pr_review", - action == Agent::Action::"invoke_tool", - resource == Agent::Tool::"Edit" -); - -// All agents: forbid writes to .git internals -forbid (principal, action == Agent::Action::"write_file", resource) -when { context.file_path like ".git/*" }; -forbid (principal, action == Agent::Action::"write_file", resource) -when { context.file_path like "*/.git/*" }; - -// All agents: forbid destructive bash commands -forbid (principal, action == Agent::Action::"execute_bash", resource) -when { context.command like "*rm -rf /*" }; -forbid (principal, action == Agent::Action::"execute_bash", resource) -when { context.command like "*git push --force*" }; -forbid (principal, action == Agent::Action::"execute_bash", resource) -when { context.command like "*git push -f *" }; -forbid (principal, action == Agent::Action::"execute_bash", resource) -when { context.command like "*git push -f" }; -""" +if TYPE_CHECKING: + from collections.abc import Callable, Iterable + +# --------------------------------------------------------------------------- +# Constants (§3, §5.2, §12.9) +# --------------------------------------------------------------------------- + +FLOOR_TIMEOUT_S: int = 30 # §6 decision #6: rejected below this at load +WARN_TIMEOUT_S: int = 120 # IMPL-25: sub-120s emits WARN on blueprint load +DEFAULT_TASK_TIMEOUT_S: int = 300 # §6 decision #6 default + + +def _load_shared_constants() -> dict: + """Read ``contracts/constants.json`` (S9 — see ``contracts/constants.md``). + + Two candidate paths cover both the deployed image + (``/app/contracts/constants.json`` — Dockerfile copies ``contracts/`` + to ``/app/contracts``) and the local repo layout + (``/contracts/constants.json`` — for tests + dev). Fail-fast on + missing: a missing contract should crash import, not silently fall + back to literals that would re-introduce the drift the contract is + designed to prevent. + """ + here = Path(__file__).resolve() + candidates = [ + here.parent.parent / "contracts" / "constants.json", # /app/contracts/ + here.parent.parent.parent / "contracts" / "constants.json", # /contracts/ + ] + for path in candidates: + if path.is_file(): + return json.loads(path.read_text()) + raise FileNotFoundError( + "contracts/constants.json not found; checked: " + ", ".join(str(p) for p in candidates), + ) + + +_SHARED_CONSTANTS = _load_shared_constants() +_AGC = _SHARED_CONSTANTS["approval_gate_cap"] +DEFAULT_APPROVAL_GATE_CAP: int = int(_AGC["default"]) # decision #13 default +APPROVAL_GATE_CAP_MIN: int = int(_AGC["min"]) +APPROVAL_GATE_CAP_MAX: int = int(_AGC["max"]) +CACHE_MAX_ENTRIES: int = 50 # §12.9: decoupled from approvalGateCap +CACHE_TTL_S: float = 60.0 # §12.8 sliding-window TTL on DENIED/TIMED_OUT +POLICIES_MAX_BYTES: int = 64 * 1024 # finding #12: reject blueprints > 64 KB +APPROVAL_RATE_LIMIT: int = 20 # §12.9 per-container per-minute approval writes +APPROVAL_RATE_WINDOW_S: float = 60.0 # sliding window paired with APPROVAL_RATE_LIMIT + +_SEVERITY_ORDER = {"low": 0, "medium": 1, "high": 2} +_DEFAULT_SEVERITY = "medium" +_VALID_SEVERITIES = frozenset(_SEVERITY_ORDER) +_VALID_TIERS = frozenset({"hard", "soft"}) + +# Tool group membership (§6.4 decision #21). Resolves tool_group:file_write +# scope to {Write, Edit} at runtime. +TOOL_GROUPS: dict[str, frozenset[str]] = { + "file_write": frozenset({"Write", "Edit"}), +} + +# Location of built-in policy files. Resolved relative to this module so the +# Docker image can ship them alongside src/. +_POLICIES_DIR = Path(__file__).resolve().parent.parent / "policies" + + +# --------------------------------------------------------------------------- +# Outcome + PolicyDecision (§6.1) +# --------------------------------------------------------------------------- + + +class Outcome(StrEnum): + """Three-outcome model returned by ``evaluate_tool_use``. + + REQUIRE_APPROVAL is the soft-deny surface: the hook pauses the tool call + and awaits a human decision. ALLOW / DENY are absolute from the engine's + perspective — the hook maps them to the SDK's binary permit/forbid API. + """ + + ALLOW = "allow" + DENY = "deny" + REQUIRE_APPROVAL = "require_approval" -@dataclass(frozen=True) class PolicyDecision: - """Result of a Cedar policy evaluation.""" + """Result of a Cedar policy evaluation. + + Fields beyond ``outcome`` + ``reason`` + ``duration_ms`` are populated + only on ``REQUIRE_APPROVAL``. ``.allowed`` is the backward-compat shim + for Phase 1a/1b/2 callers that predate the three-outcome engine and + treat this as a simple allow/deny boolean. + + Not a dataclass — a custom ``__init__`` supports BOTH the new + ``outcome=...``-keyed form and the legacy ``allowed=...``-keyed form + so Phase 1 tests keep working without caller changes. Instances are + immutable by convention (no mutator methods); callers should treat + them as read-only. + """ + + __slots__ = ( + "cache_hit_metadata", + "duration_ms", + "matching_rule_ids", + "outcome", + "reason", + "severity", + "timeout_s", + ) + + def __init__( + self, + *, + outcome: Outcome | None = None, + allowed: bool | None = None, # Legacy Phase 1 kwarg + reason: str = "", + timeout_s: int | None = None, + severity: str | None = None, + matching_rule_ids: tuple[str, ...] = (), + duration_ms: float = 0.0, + cache_hit_metadata: dict | None = None, + ) -> None: + if outcome is None and allowed is None: + raise TypeError("PolicyDecision requires either outcome= or allowed=") + if outcome is not None and allowed is not None: + raise TypeError("PolicyDecision: pass either outcome= or allowed=, not both") + if outcome is None: + outcome = Outcome.ALLOW if allowed else Outcome.DENY + self.outcome = outcome + self.reason = reason + self.timeout_s = timeout_s + self.severity = severity + self.matching_rule_ids = matching_rule_ids + self.duration_ms = duration_ms + # IMPL-23: populated only when Step 2.5 of evaluate_tool_use returns + # a cache-hit DENY. Contains the payload for the `policy_decision` + # milestone with `decision_source="recent_decision_cache"`; the hook + # forwards it to progress_writer.write_policy_decision_cached(). + # Observability-only — NOT part of __eq__/__hash__: two cache-hit + # decisions with different original_decision_ts values still + # represent the same deny outcome. + self.cache_hit_metadata = cache_hit_metadata + + @property + def allowed(self) -> bool: + """True only when outcome == ALLOW. DENY and REQUIRE_APPROVAL both + map to False so legacy ``if not decision.allowed: return deny`` callers + keep blocking soft-deny hits (preserving at-rest behavior until the + PreToolUse hook is extended to the three-outcome path in Chunk 3). + """ + return self.outcome == Outcome.ALLOW + + def __repr__(self) -> str: + base = ( + f"PolicyDecision(outcome={self.outcome.value!r}, " + f"reason={self.reason!r}, duration_ms={self.duration_ms}" + ) + if self.outcome == Outcome.REQUIRE_APPROVAL: + return ( + f"{base}, timeout_s={self.timeout_s}, " + f"severity={self.severity!r}, " + f"matching_rule_ids={self.matching_rule_ids!r})" + ) + return base + ")" + + def __eq__(self, other: object) -> bool: + if not isinstance(other, PolicyDecision): + return NotImplemented + return ( + self.outcome == other.outcome + and self.reason == other.reason + and self.timeout_s == other.timeout_s + and self.severity == other.severity + and self.matching_rule_ids == other.matching_rule_ids + and self.duration_ms == other.duration_ms + ) + + def __hash__(self) -> int: + return hash( + ( + self.outcome, + self.reason, + self.timeout_s, + self.severity, + self.matching_rule_ids, + self.duration_ms, + ) + ) + + @classmethod + def allow(cls, reason: str = "permitted", duration_ms: float = 0.0) -> PolicyDecision: + return cls(outcome=Outcome.ALLOW, reason=reason, duration_ms=duration_ms) - allowed: bool + @classmethod + def deny(cls, reason: str, duration_ms: float = 0.0) -> PolicyDecision: + return cls(outcome=Outcome.DENY, reason=reason, duration_ms=duration_ms) + + @classmethod + def require_approval( + cls, + reason: str, + timeout_s: int, + severity: str, + matching_rule_ids: tuple[str, ...], + duration_ms: float = 0.0, + ) -> PolicyDecision: + return cls( + outcome=Outcome.REQUIRE_APPROVAL, + reason=reason, + timeout_s=timeout_s, + severity=severity, + matching_rule_ids=matching_rule_ids, + duration_ms=duration_ms, + ) + + +# --------------------------------------------------------------------------- +# Allowlist (§6.4) +# --------------------------------------------------------------------------- + + +@dataclass +class _CachedDecision: + """In-process recent-decision cache entry. + + ``inserted_at`` is a monotonic timestamp used for TTL/LRU; it is NOT + safe to surface in events (monotonic clocks aren't wall-clock and + restart at container boot). ``original_decision_ts`` is the ISO-8601 + wall-clock string captured at record time so IMPL-23 cache-hit + events can report when the original decision landed. + """ + + decision: str # "DENIED" | "TIMED_OUT" reason: str - duration_ms: float = 0 + inserted_at: float + original_decision_ts: str + + +class ApprovalAllowlist: + """Runtime scope allowlist, seeded from ``initial_approvals`` at task start. + + See §6.4. ``matches`` checks tool-scope fast paths (all_session, + tool_type, tool_group, bash_pattern, write_path); rule-scope matches + are checked POST soft-deny-eval in ``evaluate_tool_use`` because + rule_ids are not known until Cedar reports matching policies. + """ + + def __init__(self, initial_scopes: list[str] | None = None) -> None: + self._all_session = False + self._tool_types: set[str] = set() + self._tool_groups: set[str] = set() + self._rule_ids: set[str] = set() + self._bash_patterns: list[str] = [] + self._write_path_patterns: list[str] = [] + + for scope in initial_scopes or []: + self.add(scope) + + def add(self, scope: str) -> None: + """Parse + install a scope. Raises ValueError on unknown prefixes. + + Whitespace around both the prefix and the value is stripped so + ``"tool_type: Read"`` and ``" tool_type:Read "`` normalize to the + same internal state; empty-after-strip values are rejected so + ``"tool_type:"`` fails loud (finding #6 from Chunk 2 review). + Case is preserved verbatim — ``"tool_type:read"`` will not match + the ``"Read"`` tool name at runtime. That's intentional (Cedar + `like` is case-sensitive) but the CLI surfaces a WARN on uppercase + ``write_path:`` globs to flag the dev-vs-prod fnmatch footgun + (§5.5 finding #15). + """ + normalized = scope.strip() + if normalized == "all_session": + self._all_session = True + return + prefix, sep, value = normalized.partition(":") + if not sep: + raise ValueError(f"unknown scope: {scope!r}") + value = value.strip() + if not value: + raise ValueError(f"scope {prefix!r} missing value (got {scope!r})") + if prefix == "tool_type": + self._tool_types.add(value) + elif prefix == "tool_group": + if value not in TOOL_GROUPS: + raise ValueError(f"unknown tool_group: {value!r}") + self._tool_groups.add(value) + elif prefix == "rule": + self._rule_ids.add(value) + elif prefix == "bash_pattern": + self._bash_patterns.append(value) + elif prefix == "write_path": + self._write_path_patterns.append(value) + else: + raise ValueError(f"unknown scope: {scope!r}") + + @property + def rule_ids(self) -> frozenset[str]: + """Snapshot of rule-ID scopes, checked post-soft-deny in the engine.""" + return frozenset(self._rule_ids) + + def matches(self, tool_name: str, tool_input: dict) -> bool: + """Return True if a non-rule scope pre-approves this tool call.""" + if self._all_session: + return True + if tool_name in self._tool_types: + return True + for group in self._tool_groups: + if tool_name in TOOL_GROUPS[group]: + return True + if tool_name == "Bash": + cmd = tool_input.get("command", "") + # fnmatch semantics documented as Cedar-`like` superset (§5.5). + if any(fnmatch(cmd, pat) for pat in self._bash_patterns): + return True + if tool_name in ("Write", "Edit"): + fp = tool_input.get("file_path", "") + if any(fnmatch(fp, pat) for pat in self._write_path_patterns): + return True + return False + + +# --------------------------------------------------------------------------- +# Recent-decision cache (§6.2, §12.8, §12.9) +# --------------------------------------------------------------------------- + + +class RecentDecisionCache: + """In-process LRU cache of recent DENIED/TIMED_OUT outcomes. + + Bounded at 50 entries (``CACHE_MAX_ENTRIES``) INDEPENDENT of the per-task + ``approvalGateCap`` (§12.9): a blueprint that raises the gate cap to 200 + does NOT get a larger cache. Two concerns, two bounds: cap = UX ceiling, + cache = engine memory bound. + + TTL is 60s on each entry; ``get`` skips expired entries without eviction + (eviction happens lazily on overflow). Cache is populated only on + DENIED/TIMED_OUT — NEVER on APPROVED (so a just-approved call does not + auto-deny on the next identical invocation). + + **Session-scoped**: cleared on container restart. Documented caveat in + §12.8 — not a bug. Persistent cache is §17.5 future work. + """ + + def __init__( + self, + *, + max_entries: int = CACHE_MAX_ENTRIES, + ttl_s: float = CACHE_TTL_S, + clock: Callable[[], float] = time.monotonic, + ) -> None: + self._entries: OrderedDict[tuple[str, str], _CachedDecision] = OrderedDict() + # Rule-level cache: keyed by ``(tool_name, rule_id)``, populated + # alongside the input-hash cache whenever a DENIED outcome + # carries ``matching_rule_ids``. The input-hash cache catches + # literal retries of the same command; the rule cache catches + # *semantic* retries — e.g. a user denied ``git push --force + # origin branch-a`` should also fast-deny ``git push --force + # origin branch-b`` because both resolve to the same + # ``force_push_any`` rule. Without this the agent can burn + # through its max_turns budget hammering on variations the + # user has already said no to (observed in E2E Phase 4). + self._rule_entries: OrderedDict[tuple[str, str], _CachedDecision] = OrderedDict() + self._max_entries = max_entries + self._ttl_s = ttl_s + self._clock = clock + + def record( + self, + tool_name: str, + input_sha256: str, + decision: str, + reason: str, + original_decision_ts: str | None = None, + ) -> None: + """Insert a DENIED/TIMED_OUT entry. Evicts LRU on overflow. + + ``original_decision_ts`` is the ISO-8601 wall-clock time of the + original approval decision that seeded this cache entry. Surfaced + on subsequent cache-hit events (IMPL-23) so operators can correlate + cache-driven denies back to the originating gate. Falsy values + (``None`` or empty string) fall back to "now" at record time, so + legacy test callers and corrupted outcome rows keep working. + """ + if decision not in ("DENIED", "TIMED_OUT"): + raise ValueError(f"RecentDecisionCache only accepts DENIED/TIMED_OUT, got {decision!r}") + key = (tool_name, input_sha256) + self._entries[key] = _CachedDecision( + decision=decision, + reason=reason, + inserted_at=self._clock(), + original_decision_ts=original_decision_ts + or datetime.now(UTC).isoformat().replace("+00:00", "Z"), + ) + self._entries.move_to_end(key) + while len(self._entries) > self._max_entries: + self._entries.popitem(last=False) + + def record_rule_decision( + self, + tool_name: str, + rule_id: str, + decision: str, + reason: str, + original_decision_ts: str | None = None, + ) -> None: + """Insert a rule-level DENIED/TIMED_OUT entry. + + Called once per ``matching_rule_ids`` entry on a DENIED outcome + so subsequent tool calls that hit the same rule are + auto-denied without a fresh approval round-trip. Only ``DENIED`` + is recorded here — ``TIMED_OUT`` is more ambiguous (user was + away, not actively refusing) so the existing input-hash cache + is the safer bound for it. + """ + if decision != "DENIED": + raise ValueError( + f"record_rule_decision only accepts DENIED, got {decision!r} " + "(TIMED_OUT is handled by the input-hash cache only)" + ) + key = (tool_name, rule_id) + self._rule_entries[key] = _CachedDecision( + decision=decision, + reason=reason, + inserted_at=self._clock(), + original_decision_ts=original_decision_ts + or datetime.now(UTC).isoformat().replace("+00:00", "Z"), + ) + self._rule_entries.move_to_end(key) + while len(self._rule_entries) > self._max_entries: + self._rule_entries.popitem(last=False) + + def get(self, tool_name: str, input_sha256: str) -> _CachedDecision | None: + """Return a non-expired cached entry or None.""" + key = (tool_name, input_sha256) + entry = self._entries.get(key) + if entry is None: + return None + if self._clock() - entry.inserted_at > self._ttl_s: + # Expired; drop it opportunistically. + del self._entries[key] + return None + # LRU-touch so an active retry pattern stays in the window. + self._entries.move_to_end(key) + return entry + + def get_rule_decision( + self, tool_name: str, rule_ids: Iterable[str] + ) -> tuple[str, _CachedDecision] | None: + """Return ``(rule_id, entry)`` for the first non-expired rule hit, or None. + + Returns the matched rule_id separately from the entry so the + caller can surface it on the cache-hit metadata for operators. + """ + for rule_id in rule_ids: + key = (tool_name, rule_id) + entry = self._rule_entries.get(key) + if entry is None: + continue + if self._clock() - entry.inserted_at > self._ttl_s: + del self._rule_entries[key] + continue + self._rule_entries.move_to_end(key) + return rule_id, entry + return None + + def __len__(self) -> int: + return len(self._entries) + + +# --------------------------------------------------------------------------- +# Annotation handling (§5.2, §6.3, §12.4) +# --------------------------------------------------------------------------- + + +@dataclass +class _ParsedRule: + """Internal shape: one Cedar policy's annotations, keyed by policy_id.""" + + policy_id: str # Internal cedarpy ID (e.g. "policy0") + rule_id: str | None + tier: str | None + effect: str | None # "permit" or "forbid" — used to gate base_permit exemption + approval_timeout_s: int | None + severity: str | None + category: str | None + + +def _parse_policy_annotations(cedarpy_module, policies_text: str) -> list[_ParsedRule]: + """Parse every static policy's annotations via cedarpy.policies_to_json_str. + + Returns a list in cedarpy's positional order. Raises on malformed input. + """ + try: + parsed = json.loads(cedarpy_module.policies_to_json_str(policies_text)) + except Exception as exc: + raise ValueError(f"Cedar policy parse failed: {type(exc).__name__}: {exc}") from exc + + rules: list[_ParsedRule] = [] + for pid, body in parsed.get("staticPolicies", {}).items(): + ann = body.get("annotations", {}) or {} + timeout_raw = ann.get("approval_timeout_s") + timeout: int | None = None + if timeout_raw is not None: + try: + timeout = int(timeout_raw) + except (TypeError, ValueError): + raise ValueError( + f"policy {pid!r}: @approval_timeout_s must be integer, got {timeout_raw!r}" + ) from None + rules.append( + _ParsedRule( + policy_id=pid, + rule_id=ann.get("rule_id"), + tier=ann.get("tier"), + effect=body.get("effect"), + approval_timeout_s=timeout, + severity=ann.get("severity"), + category=ann.get("category"), + ) + ) + return rules + + +def _validate_tier(rules: list[_ParsedRule], expected_tier: str, source: str) -> None: + """Every rule in a tier file MUST declare matching @tier + @rule_id. + + Exception: ``base_permit`` is allowed without @tier ONLY when it's a + ``permit`` effect — the neutral catch-all at the top of each tier. + A misnamed ``forbid`` annotated ``@rule_id("base_permit")`` would + otherwise bypass validation entirely (silent-failure finding #7 from + Chunk 2 review); restricting the exemption to ``effect == "permit"`` + forces genuine forbid rules through the regular validation path. + """ + seen_rule_ids: set[str] = set() + for rule in rules: + # Unannotated base permit with `permit` effect is the reserved + # catch-all; any other shape (forbid with rule_id "base_permit", + # permit with a non-neutral tier annotation) falls through to the + # regular validation below. + if rule.rule_id == "base_permit" and rule.tier is None and rule.effect == "permit": + seen_rule_ids.add("base_permit") + continue + if rule.tier is None: + raise ValueError(f"{source}: policy {rule.policy_id!r} missing @tier annotation") + if rule.tier != expected_tier: + raise ValueError( + f"{source}: policy {rule.rule_id or rule.policy_id!r} has " + f"@tier({rule.tier!r}) but lives in the {expected_tier!r} file" + ) + if not rule.rule_id: + raise ValueError(f"{source}: policy {rule.policy_id!r} missing @rule_id annotation") + if rule.rule_id in seen_rule_ids: + raise ValueError(f"{source}: duplicate @rule_id {rule.rule_id!r}") + seen_rule_ids.add(rule.rule_id) + if rule.tier == "soft": + if rule.approval_timeout_s is not None: + if rule.approval_timeout_s < FLOOR_TIMEOUT_S: + raise ValueError( + f"{source}: rule {rule.rule_id!r} has " + f"@approval_timeout_s({rule.approval_timeout_s}) below " + f"floor {FLOOR_TIMEOUT_S}s" + ) + if rule.approval_timeout_s < WARN_TIMEOUT_S: + # IMPL-25: advisory WARN, not strict reject. + log( + "WARN", + f"{source}: rule {rule.rule_id!r} has " + f"@approval_timeout_s({rule.approval_timeout_s}) below " + f"{WARN_TIMEOUT_S}s — humans rarely respond that fast; " + f"consider raising", + ) + if rule.severity and rule.severity not in _VALID_SEVERITIES: + raise ValueError( + f"{source}: rule {rule.rule_id!r} @severity must be one of " + f"{sorted(_VALID_SEVERITIES)}, got {rule.severity!r}" + ) + + +def _merge_annotations( + rules: list[_ParsedRule], + matching_policy_ids: list[str], + task_default_timeout_s: int, +) -> tuple[list[str], int, str]: + """Merge annotations across multiple matching soft-deny policies (§6.3). + + Timeout: min across rules (clamped by FLOOR_TIMEOUT_S). Severity: max. + rule_ids preserved in order of match. If a matching rule has no + annotation data (shouldn't happen post-validation), falls back to the + policy ID. + """ + by_id = {r.policy_id: r for r in rules} + rule_ids: list[str] = [] + timeouts: list[int] = [] + severities: list[str] = [] + for pid in matching_policy_ids: + rule = by_id.get(pid) + if rule is None: + continue + rule_ids.append(rule.rule_id or pid) + if rule.approval_timeout_s is not None: + timeouts.append(rule.approval_timeout_s) + severities.append(rule.severity or _DEFAULT_SEVERITY) + + timeouts.append(task_default_timeout_s) + # Defensive only: load-time validation already rejects below-floor values, + # but the clamp costs nothing and protects against a future caller that + # bypasses validation (e.g. programmatic rule injection). + effective_timeout = max(FLOOR_TIMEOUT_S, min(timeouts)) + + if severities: + effective_severity = max(severities, key=lambda s: _SEVERITY_ORDER.get(s, 0)) + else: + effective_severity = _DEFAULT_SEVERITY + + return rule_ids, effective_timeout, effective_severity + + +# --------------------------------------------------------------------------- +# Policy-text loaders +# --------------------------------------------------------------------------- + + +def _load_builtin_policies() -> tuple[str, str]: + """Read hard_deny.cedar + soft_deny.cedar from the agent/policies/ dir. + + Returns (hard_text, soft_text). Raises on missing files so the engine + fails loudly rather than running with stale or absent defaults. + """ + hard_path = _POLICIES_DIR / "hard_deny.cedar" + soft_path = _POLICIES_DIR / "soft_deny.cedar" + if not hard_path.is_file(): + raise FileNotFoundError(f"missing built-in hard-deny policies: {hard_path}") + if not soft_path.is_file(): + raise FileNotFoundError(f"missing built-in soft-deny policies: {soft_path}") + return hard_path.read_text(), soft_path.read_text() + + +def _sha256_tool_input(tool_input: dict) -> str: + """Stable SHA-256 over the tool_input dict (sorted keys, utf-8).""" + return hashlib.sha256(json.dumps(tool_input, sort_keys=True).encode("utf-8")).hexdigest() + + +# --------------------------------------------------------------------------- +# PolicyEngine — the main three-outcome engine (§6.2) +# --------------------------------------------------------------------------- class PolicyEngine: - """Evaluate tool-use requests against Cedar policies.""" + """Evaluate tool-use requests against Cedar policies with three outcomes. + + Construction loads the built-in hard_deny + soft_deny policy files, + concatenates any blueprint-provided rules (subject to 64 KB cap and + disable-list validation), probes a test authorization to catch syntax + errors early, and seeds the approval allowlist from ``initial_approvals``. + + Legacy callers that pass ``extra_policies=[...]`` (Phase 1 shape) are + supported in backward-compat mode: the extra text is appended to the + soft-deny tier WITHOUT strict annotation validation. New callers in + Chunks 3+ should use ``blueprint_hard_policies`` / ``blueprint_soft_policies`` + / ``blueprint_disable`` / ``initial_approvals`` / ``approval_gate_cap``. + """ def __init__( self, task_type: str, repo: str, + *, extra_policies: list[str] | None = None, + blueprint_hard_policies: str | None = None, + blueprint_soft_policies: str | None = None, + blueprint_disable: list[str] | None = None, + initial_approvals: list[str] | None = None, + approval_gate_cap: int = DEFAULT_APPROVAL_GATE_CAP, + initial_approval_gate_count: int = 0, + task_default_timeout_s: int = DEFAULT_TASK_TIMEOUT_S, ) -> None: self._task_type = task_type self._repo = repo self._disabled = False + self._task_default_timeout_s = task_default_timeout_s + + # Bounds check on approval_gate_cap (decision #13). + if not APPROVAL_GATE_CAP_MIN <= approval_gate_cap <= APPROVAL_GATE_CAP_MAX: + raise ValueError( + f"approval_gate_cap must be in " + f"[{APPROVAL_GATE_CAP_MIN}, {APPROVAL_GATE_CAP_MAX}], " + f"got {approval_gate_cap}" + ) + self._approval_gate_cap = approval_gate_cap + + # Negative seeds are a caller bug, not a container-restart state. + if initial_approval_gate_count < 0: + raise ValueError( + f"initial_approval_gate_count must be >= 0, got {initial_approval_gate_count}" + ) + + # §12.9 per-task gate counter + per-container sliding-window rate limit. + # The counter is session-scoped within a container but seeded from the + # persisted TaskTable value (§13.6) so container restarts resume the + # cumulative gate budget instead of resetting to 0. The rate-limit + # window stays per-container by design (§13.6 finding #10 scenario). + self._approval_gate_count: int = initial_approval_gate_count + self._approvals_last_minute: deque[float] = deque() + # §6.5 queue consumed by ``_denial_between_turns_hook``. Each entry + # is ``{"request_id", "reason", "decided_at"}``; reason is already + # sanitized by DenyTaskFn (§12.6). + self._denial_injection_queue: list[dict] = [] + # IMPL-26: ``approval_ceiling_shrinking`` is emit-once per task. + self._emitted_ceiling_shrinking: bool = False + + # Validate task_type (non-fatal WARN to match Phase 1 behavior). + from models import TaskType - # Import cedarpy at init time so failures are caught early + try: + TaskType(task_type) + except ValueError: + log("WARN", f"Unknown task_type '{task_type}' — using default deny-list policies") + + # Import cedarpy lazily so the module still loads in environments + # without the native extension (tests can monkey-patch). try: import cedarpy @@ -96,74 +793,274 @@ def __init__( log("ERROR", "cedarpy not available — policy engine disabled (fail-closed)") self._cedarpy = None self._disabled = True - self._policies = _DEFAULT_POLICIES + # Still construct empty state so legacy callers do not crash + # during attribute access. + self._hard_policies = "" + self._soft_policies = "" + self._hard_rules: list[_ParsedRule] = [] + self._soft_rules: list[_ParsedRule] = [] + self._allowlist = ApprovalAllowlist() + self._cache = RecentDecisionCache() return - # Validate task_type - from models import TaskType - + # Load built-in tiers. try: - TaskType(task_type) - except ValueError: - log("WARN", f"Unknown task_type '{task_type}' — using default deny-list policies") + builtin_hard, builtin_soft = _load_builtin_policies() + except FileNotFoundError as exc: + # Fatal: without built-ins the engine's hard-deny invariants + # (rm -rf /, .git writes, DROP TABLE) are missing. + raise RuntimeError(str(exc)) from exc - # Build combined policies - self._policies = _DEFAULT_POLICIES + # Blueprint customization: append blueprint rules into the tiers. + hard_text = builtin_hard + if blueprint_hard_policies: + hard_text = f"{hard_text}\n{blueprint_hard_policies}" + soft_text = builtin_soft + + # Legacy ``extra_policies`` goes into soft tier with a synthetic + # wrapper (@tier("soft") + @rule_id("legacy_extra_N")). Cedar's + # annotation semantics on duplicate keys within a single policy + # are implementation-defined (parse error in most versions), so + # we REJECT legacy text that already declares @tier or @rule_id + # (finding #2 from Chunk 2 review) instead of silently picking + # one interpretation. Callers should migrate to + # blueprint_soft_policies / blueprint_hard_policies with fully + # annotated rules. + legacy_extra: list[str] = [] if extra_policies: - combined = _DEFAULT_POLICIES + "\n" + "\n".join(extra_policies) - # Validate combined policies with a test authorization - try: - test_request = { - "principal": f'Agent::TaskAgent::"{task_type}"', + for idx, policy_text in enumerate(extra_policies): + if "@tier(" in policy_text or "@rule_id(" in policy_text: + raise ValueError( + f"extra_policies[{idx}] already declares @tier or " + f"@rule_id; the legacy extra_policies kwarg is for " + f"UNANNOTATED rules only. Migrate annotated rules " + f"to blueprint_soft_policies / blueprint_hard_policies." + ) + legacy_rule_id = f"legacy_extra_{idx}" + legacy_extra.append(f'@tier("soft")\n@rule_id("{legacy_rule_id}")\n{policy_text}') + if blueprint_soft_policies: + soft_text = f"{soft_text}\n{blueprint_soft_policies}" + if legacy_extra: + soft_text = soft_text + "\n" + "\n".join(legacy_extra) + + # 64 KB cap on combined blueprint text (finding #12). Built-ins do + # not count against the cap — they are trusted platform content. + blueprint_text = "".join(filter(None, [blueprint_hard_policies, blueprint_soft_policies])) + if len(blueprint_text.encode("utf-8")) > POLICIES_MAX_BYTES: + raise ValueError( + f"cedar_policies exceeds {POLICIES_MAX_BYTES // 1024} KB cap " + f"({len(blueprint_text.encode('utf-8'))} bytes)" + ) + + # Parse + validate annotations on each tier. + try: + self._hard_rules = _parse_policy_annotations(self._cedarpy, hard_text) + _validate_tier(self._hard_rules, "hard", "hard_deny") + except ValueError as exc: + # Blueprint or built-in problem — fail loud at task start. + raise ValueError(f"hard-deny policy validation failed: {exc}") from exc + + try: + self._soft_rules = _parse_policy_annotations(self._cedarpy, soft_text) + # Legacy extra_policies synthetic rules get generic "legacy_extra_N" + # @rule_id values; those are accepted by the validator. + _validate_tier(self._soft_rules, "soft", "soft_deny") + except ValueError as exc: + raise ValueError(f"soft-deny policy validation failed: {exc}") from exc + + # blueprint_disable: reject any entry that names a built-in hard-deny + # rule (finding #9, §5.1). Built-in hard rule IDs come from the + # original builtin_hard text, NOT the concatenated hard_text. + builtin_hard_rule_ids = { + r.rule_id + for r in _parse_policy_annotations(self._cedarpy, builtin_hard) + if r.rule_id and r.tier == "hard" + } + for disable_id in blueprint_disable or []: + if disable_id in builtin_hard_rule_ids: + raise ValueError( + f"blueprint disable[{disable_id!r}]: cannot disable built-in " + f"hard-deny rule; hard-deny is absolute (§5.1, §12.5)" + ) + # Disabled soft rules are filtered at evaluate_tool_use time: if a + # soft-deny eval's matching rule_ids are ALL in the disable set, + # the match is treated as no-match (fall through to ALLOW). If some + # are disabled but others match, the surviving rule_ids drive the + # REQUIRE_APPROVAL outcome. See evaluate_tool_use Step 3. + self._disabled_rule_ids: set[str] = set(blueprint_disable or []) + + # Rule-ID uniqueness ACROSS tiers. The reserved ``base_permit`` ID + # is expected to appear in both tiers (each tier needs its own + # catch-all permit so cedarpy doesn't default-deny non-matching + # inputs in isolation). + hard_ids = {r.rule_id for r in self._hard_rules if r.rule_id} + soft_ids = {r.rule_id for r in self._soft_rules if r.rule_id} + cross = (hard_ids & soft_ids) - {"base_permit"} + if cross: + raise ValueError(f"duplicate @rule_id across tiers: {sorted(cross)}") + + # Probe authorizations on each tier to catch runtime Cedar errors + # before the first evaluate_tool_use call. + self._probe_cedar(hard_text, "hard") + self._probe_cedar(soft_text, "soft") + + self._hard_policies = hard_text + self._soft_policies = soft_text + + # Approval allowlist + recent-decision cache. + try: + self._allowlist = ApprovalAllowlist(initial_approvals) + except ValueError as exc: + raise ValueError(f"initial_approvals: {exc}") from exc + self._cache = RecentDecisionCache() + + log( + "AGENT", + f"Cedar policy engine initialized: task_type={task_type}, " + f"hard_rules={len(self._hard_rules)}, soft_rules={len(self._soft_rules)}, " + f"pre_approvals={len(initial_approvals) if initial_approvals else 0}, " + f"approval_gate_cap={approval_gate_cap}, " + f"initial_approval_gate_count={initial_approval_gate_count}", + ) + + # ---- Public properties ------------------------------------------------- + + @property + def task_type(self) -> str: + return self._task_type + + @property + def repo(self) -> str: + return self._repo + + @property + def approval_gate_cap(self) -> int: + return self._approval_gate_cap + + @property + def task_default_timeout_s(self) -> int: + return self._task_default_timeout_s + + @property + def allowlist(self) -> ApprovalAllowlist: + return self._allowlist + + @property + def recent_decisions(self) -> RecentDecisionCache: + return self._cache + + # ---- Approval-gate counters + denial queue (§6.5, §12.9) -------------- + + @property + def approval_gate_count(self) -> int: + """Session-scoped count of REQUIRE_APPROVAL gates emitted this task.""" + return self._approval_gate_count + + def increment_approval_gate_count(self) -> None: + """Bump the per-task gate counter (called at row-write time).""" + self._approval_gate_count += 1 + + def record_approval_gate_timestamp(self, now: float | None = None) -> None: + """Record a new approval-gate timestamp for the sliding rate-limit window.""" + ts = time.monotonic() if now is None else now + self._approvals_last_minute.append(ts) + self._prune_rate_window(ts) + + def _prune_rate_window(self, now: float) -> None: + """Drop timestamps older than ``APPROVAL_RATE_WINDOW_S``.""" + cutoff = now - APPROVAL_RATE_WINDOW_S + while self._approvals_last_minute and self._approvals_last_minute[0] < cutoff: + self._approvals_last_minute.popleft() + + @property + def approvals_in_last_minute(self) -> int: + """Count of approval-gate writes in the last ``APPROVAL_RATE_WINDOW_S``. + + Prunes the window before returning so callers see the current count. + """ + self._prune_rate_window(time.monotonic()) + return len(self._approvals_last_minute) + + def queue_denial_injection( + self, *, request_id: str, reason: str, decided_at: str | None + ) -> None: + """Append a denial-injection payload for ``_denial_between_turns_hook``. + + Reason is expected to be pre-sanitized upstream (by ``DenyTaskFn``, + §12.6). The hook is responsible for XML-escaping at injection time. + """ + self._denial_injection_queue.append( + {"request_id": request_id, "reason": reason, "decided_at": decided_at} + ) + + def drain_denial_injections(self) -> list[dict]: + """Pop and return the queued denial-injection payloads.""" + out = list(self._denial_injection_queue) + self._denial_injection_queue.clear() + return out + + def mark_ceiling_shrinking_emitted(self) -> bool: + """Idempotency latch for ``approval_ceiling_shrinking`` (IMPL-26). + + Returns ``True`` the first time it is called (caller should emit the + milestone) and ``False`` on every subsequent call. + """ + if self._emitted_ceiling_shrinking: + return False + self._emitted_ceiling_shrinking = True + return True + + # ---- Probes + low-level evaluation ------------------------------------ + + def _probe_cedar(self, policies_text: str, tier_name: str) -> None: + """Run a synthetic is_authorized call so Cedar rejects bad syntax early.""" + # _probe_cedar is only called from __init__ AFTER the ImportError + # early-return, so ``_cedarpy`` is guaranteed non-None here. + if self._cedarpy is None: # pragma: no cover — invariant guard + raise RuntimeError("probe called on disabled engine") + try: + self._cedarpy.is_authorized( + { + "principal": f'Agent::TaskAgent::"{self._task_type}"', "action": 'Agent::Action::"invoke_tool"', "resource": 'Agent::Tool::"Read"', - "context": {"task_type": task_type, "repo": repo}, - } - test_entities = [ + "context": {"task_type": self._task_type, "repo": self._repo}, + }, + policies_text, + [ { - "uid": {"type": "Agent::TaskAgent", "id": task_type}, + "uid": {"type": "Agent::TaskAgent", "id": self._task_type}, "attrs": {}, "parents": [], }, - { - "uid": {"type": "Agent::Tool", "id": "Read"}, - "attrs": {}, - "parents": [], - }, - ] - cedarpy.is_authorized(test_request, combined, test_entities) - self._policies = combined - except Exception as e: - log( - "WARN", - f"Extra Cedar policies failed validation " - f"({type(e).__name__}: {e}) — using defaults only", - ) - - @property - def task_type(self) -> str: - return self._task_type + {"uid": {"type": "Agent::Tool", "id": "Read"}, "attrs": {}, "parents": []}, + ], + ) + except Exception as exc: + raise ValueError( + f"{tier_name}-deny Cedar probe failed: {type(exc).__name__}: {exc}" + ) from exc - def _evaluate( + def _eval_tier( self, + policies_text: str, action: str, resource_type: str, resource_id: str, context: dict, - ) -> tuple[bool, str]: - """Run a single Cedar authorization check. + ) -> tuple[str, list[str]]: + """Run a single Cedar authorization check against a single tier. - Returns (allowed, reason). Fails closed on NoDecision. + Returns (decision, matching_policy_ids). ``decision`` is one of + ``"allow"`` / ``"deny"`` / ``"no_decision"`` (lowercase string). + ``matching_policy_ids`` is cedarpy's ``diagnostics.reasons`` list + (internal positional IDs like ``["policy2", "policy3"]``). - ``resource_id`` must be a simple identifier safe for Cedar entity - UID parsing (no quotes, newlines, or special chars). Callers that - evaluate user-supplied values (bash commands, file paths) should - pass a fixed sentinel and put the real value in ``context`` where - the policies match against it. + Raises ``RuntimeError`` if cedarpy reports policy-parse errors — + these are fail-closed hazards (policy text is unusable). The + outer ``evaluate_tool_use`` catches and maps to Outcome.DENY with + reason ``"fail-closed: RuntimeError"``. """ - cedarpy = self._cedarpy - if cedarpy is None: - return False, "policy engine unavailable" request = { "principal": f'Agent::TaskAgent::"{self._task_type}"', "action": f'Agent::Action::"{action}"', @@ -176,91 +1073,299 @@ def _evaluate( "attrs": {}, "parents": [], }, - { - "uid": {"type": resource_type, "id": resource_id}, - "attrs": {}, - "parents": [], - }, + {"uid": {"type": resource_type, "id": resource_id}, "attrs": {}, "parents": []}, ] - result = cedarpy.is_authorized(request, self._policies, entities) - - if result.decision == cedarpy.Decision.NoDecision: - return False, "fail-closed: NoDecision (no valid policies loaded)" + # _eval_tier is only reached from evaluate_tool_use AFTER the + # ``_disabled``/``_cedarpy is None`` guard, so the attribute is + # narrowed to a live module here. + if self._cedarpy is None: # pragma: no cover — invariant guard + raise RuntimeError("eval called on disabled engine") + result = self._cedarpy.is_authorized(request, policies_text, entities) + if getattr(result.diagnostics, "errors", []): + # cedarpy reports parse errors via diagnostics.errors and returns + # Decision.NoDecision rather than raising — we re-raise so the + # outer fail-closed handler catches it. + errors = "; ".join(str(e) for e in result.diagnostics.errors) + raise RuntimeError(f"Cedar policy parse/eval errors: {errors}") + decision = result.decision.value.lower() + if decision not in ("allow", "deny"): + # NoDecision with no errors → treat as no match at the tier + # level; caller decides whether that's fail-closed (hard tier) + # or fall-through (soft). + return "no_decision", list(result.diagnostics.reasons) + return decision, list(result.diagnostics.reasons) - return result.allowed, "" + # ---- Three-outcome pipeline ------------------------------------------- def evaluate_tool_use(self, tool_name: str, tool_input: dict) -> PolicyDecision: - """Evaluate whether a tool call is permitted. + """Walk the three-outcome pipeline for a single tool call. - Returns PolicyDecision with allowed=True/False and reason. - Fails closed on errors and NoDecision. + Fail-closed: any cedarpy exception maps to Outcome.DENY with + reason ``"fail-closed: "``. """ start = time.monotonic() if self._disabled or self._cedarpy is None: - elapsed = (time.monotonic() - start) * 1000 - return PolicyDecision( - allowed=False, + return PolicyDecision.deny( reason="policy engine unavailable", - duration_ms=elapsed, + duration_ms=(time.monotonic() - start) * 1000, ) + base_context = {"task_type": self._task_type, "repo": self._repo} + + # Compute input_sha separately so a TypeError from json.dumps + # surfaces with a distinct fail-closed reason instead of being + # mis-attributed to Cedar evaluation (finding #5 from review). try: - base_context = {"task_type": self._task_type, "repo": self._repo} + input_sha = _sha256_tool_input(tool_input) + except (TypeError, ValueError) as exc: + log( + "WARN", + f"tool_input not hashable (fail-closed): {type(exc).__name__}: {exc}", + ) + return PolicyDecision.deny( + reason="fail-closed: unhashable_tool_input", + duration_ms=(time.monotonic() - start) * 1000, + ) - # Base evaluation: is this tool allowed? - allowed, deny_reason = self._evaluate( - "invoke_tool", - "Agent::Tool", + try: + # STEP 1 — Hard-deny evaluation (absolute). + hard_decision = self._eval_for_tool( + self._hard_policies, tool_name, + tool_input, base_context, + tier_name="hard", ) - if not allowed: - elapsed = (time.monotonic() - start) * 1000 + if hard_decision and hard_decision[0] == "deny": + rule_ids = _matching_rule_ids(self._hard_rules, hard_decision[1], tier_name="hard") reason = ( - deny_reason - or f"Cedar policy denied {tool_name} for task_type={self._task_type}" + f"Hard-deny: {', '.join(rule_ids)}" if rule_ids else "Hard-deny (unknown rule)" + ) + return PolicyDecision.deny( + reason=reason, duration_ms=(time.monotonic() - start) * 1000 + ) + + # STEP 2 — Allowlist fast-path (tool-scope). + if self._allowlist.matches(tool_name, tool_input): + return PolicyDecision.allow( + reason="Pre-approved by allowlist", + duration_ms=(time.monotonic() - start) * 1000, + ) + + # STEP 2.5 — Recent-decision cache. + cached = self._cache.get(tool_name, input_sha) + if cached is not None: + # IMPL-23: attach cache-hit metadata so the hook can emit a + # `policy_decision` milestone to TaskEventsTable. Keeps the + # engine pure — policy.py never calls the progress writer. + return PolicyDecision( + outcome=Outcome.DENY, + reason=f"Recent {cached.decision} within {int(CACHE_TTL_S)}s: {cached.reason}", + duration_ms=(time.monotonic() - start) * 1000, + cache_hit_metadata={ + "tool_name": tool_name, + "tool_input_sha256": input_sha, + "cached_decision": cached.decision, + "cached_reason": cached.reason, + "original_decision_ts": cached.original_decision_ts, + }, + ) + + # STEP 3 — Soft-deny evaluation. + soft_decision = self._eval_for_tool( + self._soft_policies, + tool_name, + tool_input, + base_context, + tier_name="soft", + ) + if soft_decision and soft_decision[0] == "deny": + all_matching_ids = _matching_rule_ids( + self._soft_rules, soft_decision[1], tier_name="soft" ) - return PolicyDecision(allowed=False, reason=reason, duration_ms=elapsed) - - # Write/Edit: check file path against write_file policies. - # Sentinel resource_id avoids Cedar UID parsing issues (see _evaluate docstring). - if tool_name in ("Write", "Edit"): - file_path = tool_input.get("file_path", "") - if file_path: - allowed, deny_reason = self._evaluate( - "write_file", - "Agent::File", - "file", - {**base_context, "file_path": file_path}, + # Filter out blueprint-disabled rules (§5.1 `disable:` list). + # If ALL matches are disabled, the soft-deny hit is neutralized + # and we fall through to default ALLOW. If some are disabled + # but others remain, the surviving rules drive REQUIRE_APPROVAL. + active_ids = [rid for rid in all_matching_ids if rid not in self._disabled_rule_ids] + if not active_ids: + # Every matching rule was disabled by the blueprint. + return PolicyDecision.allow( + reason="permitted (all matching soft-deny rules disabled by blueprint)", + duration_ms=(time.monotonic() - start) * 1000, + ) + # Rule-scope allowlist check AFTER the eval: rule_ids are + # only known once Cedar reports which policies matched. + if any(rid in self._allowlist.rule_ids for rid in active_ids): + return PolicyDecision.allow( + reason=f"Allowlist rule: {', '.join(active_ids)}", + duration_ms=(time.monotonic() - start) * 1000, ) - if not allowed: - elapsed = (time.monotonic() - start) * 1000 - reason = deny_reason or f"Cedar policy denied write to {file_path}" - return PolicyDecision(allowed=False, reason=reason, duration_ms=elapsed) - - # Bash: check command against execute_bash policies. - # Sentinel resource_id avoids Cedar UID parsing issues (see _evaluate docstring). - if tool_name == "Bash": - command = tool_input.get("command", "") - if command: - allowed, deny_reason = self._evaluate( - "execute_bash", - "Agent::BashCommand", - "command", - {**base_context, "command": command}, + + # Rule-level recent-deny cache (§12.8 extension). + # If the user recently denied any of these rule_ids on + # this tool, fast-deny without a new approval gate. + # Catches semantic retries the input-hash cache misses + # (e.g. force-push to a different branch name). + rule_hit = self._cache.get_rule_decision(tool_name, active_ids) + if rule_hit is not None: + matched_rule_id, cached = rule_hit + return PolicyDecision( + outcome=Outcome.DENY, + reason=( + f"Recent {cached.decision} on rule {matched_rule_id!r} " + f"within {int(CACHE_TTL_S)}s: {cached.reason}" + ), + duration_ms=(time.monotonic() - start) * 1000, + cache_hit_metadata={ + "tool_name": tool_name, + "tool_input_sha256": input_sha, + "matched_rule_id": matched_rule_id, + "cached_decision": cached.decision, + "cached_reason": cached.reason, + "original_decision_ts": cached.original_decision_ts, + }, ) - if not allowed: - elapsed = (time.monotonic() - start) * 1000 - reason = deny_reason or "Cedar policy denied bash command" - return PolicyDecision(allowed=False, reason=reason, duration_ms=elapsed) - - elapsed = (time.monotonic() - start) * 1000 - return PolicyDecision(allowed=True, reason="permitted", duration_ms=elapsed) - - except Exception as e: - elapsed = (time.monotonic() - start) * 1000 - log("WARN", f"Cedar evaluation error (fail-closed): {type(e).__name__}: {e}") - return PolicyDecision( - allowed=False, reason=f"fail-closed: {type(e).__name__}", duration_ms=elapsed + # Rebuild the policy-id list for annotation merging, keeping + # only the policy IDs whose rule_id survived the disable filter. + active_policy_ids = [ + pid + for pid in soft_decision[1] + if _rule_id_for_policy(self._soft_rules, pid) not in self._disabled_rule_ids + ] + merged_ids, timeout_s, severity = _merge_annotations( + self._soft_rules, + active_policy_ids, + self._task_default_timeout_s, + ) + return PolicyDecision.require_approval( + reason=f"Soft-deny: {', '.join(merged_ids)}", + timeout_s=timeout_s, + severity=severity, + matching_rule_ids=tuple(merged_ids), + duration_ms=(time.monotonic() - start) * 1000, + ) + + # STEP 4 — Default allow. + return PolicyDecision.allow( + reason="permitted", + duration_ms=(time.monotonic() - start) * 1000, + ) + + except Exception as exc: + log("WARN", f"Cedar evaluation error (fail-closed): {type(exc).__name__}: {exc}") + return PolicyDecision.deny( + reason=f"fail-closed: {type(exc).__name__}", + duration_ms=(time.monotonic() - start) * 1000, + ) + + # ---- Per-action routing ------------------------------------------------ + + def _eval_for_tool( + self, + policies_text: str, + tool_name: str, + tool_input: dict, + base_context: dict, + *, + tier_name: str = "", + ) -> tuple[str, list[str]] | None: + """Run the appropriate Cedar eval(s) for a given tool + input. + + Returns the first deny decision + matching policy IDs, or None if + no eval at this tier matched anything. Mirrors the Phase 1 routing + so existing tests (invoke_tool sentinel for tool-type, write_file + for Write/Edit, execute_bash for Bash) keep working. + + ``no_decision`` responses are logged at WARN — Cedar should always + reach a definite allow/deny given the base_permit catch-all, so + no_decision means the catch-all is missing or malformed (finding + #9 from Chunk 2 review). Fall-through to subsequent action evals + continues either way; the log gives operators signal without + changing behavior. + """ + + def _run(action: str, resource_type: str, resource_id: str, ctx: dict): + decision, reasons = self._eval_tier( + policies_text, action, resource_type, resource_id, ctx + ) + if decision == "no_decision": + log( + "WARN", + f"{tier_name or 'tier'}: Cedar no_decision for " + f"action={action!r} tool={tool_name!r} — base_permit " + f"catch-all missing or malformed", + ) + return decision, reasons + + # Check tool-type eval first (invoke_tool on the real tool sentinel). + invoke_decision, invoke_reasons = _run( + "invoke_tool", "Agent::Tool", tool_name, base_context + ) + if invoke_decision == "deny": + return ("deny", invoke_reasons) + + # Write/Edit: evaluate write_file with file_path in context. + if tool_name in ("Write", "Edit"): + file_path = tool_input.get("file_path", "") + if file_path: + write_decision, write_reasons = _run( + "write_file", + "Agent::File", + "file", + {**base_context, "file_path": file_path}, + ) + if write_decision == "deny": + return ("deny", write_reasons) + + # Bash: evaluate execute_bash with command in context. + if tool_name == "Bash": + command = tool_input.get("command", "") + if command: + bash_decision, bash_reasons = _run( + "execute_bash", + "Agent::BashCommand", + "command", + {**base_context, "command": command}, + ) + if bash_decision == "deny": + return ("deny", bash_reasons) + + return None + + +def _matching_rule_ids( + rules: list[_ParsedRule], + matching_policy_ids: list[str], + *, + tier_name: str = "", +) -> list[str]: + """Map positional Cedar policy IDs to their @rule_id annotations. + + Logs WARN on any policy ID that doesn't resolve to a parsed rule — + the condition indicates a state inconsistency (e.g. ``_hard_policies`` + mutated without re-parsing) and was silently ignored in earlier + revisions (finding #3 from Chunk 2 review). + """ + by_id = {r.policy_id: r for r in rules} + resolved: list[str] = [] + for pid in matching_policy_ids: + if pid not in by_id: + log( + "WARN", + f"{tier_name or 'tier'}: Cedar reported matching policy " + f"{pid!r} but no parsed rule carries that ID; " + f"policy/rule lists may be out of sync", ) + continue + resolved.append(by_id[pid].rule_id or pid) + return resolved + + +def _rule_id_for_policy(rules: list[_ParsedRule], policy_id: str) -> str | None: + """Return the @rule_id annotation for a given positional policy ID.""" + for rule in rules: + if rule.policy_id == policy_id: + return rule.rule_id + return None diff --git a/agent/src/progress_writer.py b/agent/src/progress_writer.py index 46b6a6cd..86fd459d 100644 --- a/agent/src/progress_writer.py +++ b/agent/src/progress_writer.py @@ -608,3 +608,302 @@ def write_agent_error(self, error_type: str, message: str) -> None: "message_preview": self._preview(message), }, ) + + # -- approval-gate milestones (§11.1, Chunk 3) ----------------------------- + # + # Each helper is a thin wrapper over ``_put_event("agent_milestone", ...)`` + # carrying the structured metadata shape documented in §11.1. Centralizing + # the milestone name + metadata keys here keeps the agent/Lambda/CLI + # contract consistent — every approval_* producer goes through a named + # method rather than stringly-typed ``write_agent_milestone`` calls. + # + # ``approval_decision_recorded`` is NOT emitted here: it is the + # Lambda-side audit event (written by ApproveTaskFn / DenyTaskFn directly + # to TaskEventsTable in Chunk 5). See IMPL-6. + # + # ``approval_timeout_capped_at_submit`` is emitted by CreateTaskFn and + # also lives on the Lambda side (Chunk 5). Agent-side helpers cover the + # 14 events the agent runtime emits. + + def _put_approval_milestone(self, milestone: str, metadata: dict) -> None: + """Emit an ``agent_milestone`` with ``milestone`` + extra metadata. + + Mirrors the shape of ``write_agent_milestone`` but preserves the + structured metadata keys so consumers do not need to parse a + free-form ``details`` string. + """ + payload: dict = {"milestone": milestone} + payload.update(metadata) + self._put_event("agent_milestone", payload) + + def write_approval_pre_approvals_loaded(self, count: int, scopes: list[str]) -> None: + """Emit ``pre_approvals_loaded`` (agent init, §11.1).""" + self._put_approval_milestone( + "pre_approvals_loaded", + {"count": count, "scopes": list(scopes)}, + ) + + def write_approval_requested( + self, + *, + request_id: str, + tool_name: str, + input_preview: str, + reason: str, + severity: str, + timeout_s: int, + matching_rule_ids: list[str], + ) -> None: + """Emit ``approval_requested`` (PENDING row written, §11.1).""" + self._put_approval_milestone( + "approval_requested", + { + "request_id": request_id, + "tool_name": tool_name, + "input_preview": self._preview(input_preview), + "reason": self._preview(reason), + "severity": severity, + "timeout_s": timeout_s, + "matching_rule_ids": list(matching_rule_ids), + }, + ) + + def write_approval_granted( + self, + *, + request_id: str, + scope: str, + decided_at: str | None, + created_at: str | None = None, + ) -> None: + """Emit ``approval_granted`` (user APPROVED, §11.1). + + Chunk 8a: ``created_at`` propagated from the approval row so the + ApprovalMetricsPublisher Lambda can compute ``decided_at - + created_at`` latency without a round-trip GetItem. Optional with + default ``None`` so any legacy caller path still constructs a + valid event — the publisher skips the latency emit + logs a + ``METRICS_SCHEMA_MISMATCH`` if the field is absent, rather than + emitting ``latency=0`` which would poison percentile widgets. + + Field is omitted (not present as ``None``) from the emitted + event metadata when ``created_at`` is ``None``. This keeps the + event stream free of ``None`` values that consumers would have + to re-check, and makes "absent" vs "present but null" a + single well-defined case downstream. + """ + payload: dict = { + "request_id": request_id, + "scope": scope, + "decided_at": decided_at, + } + if created_at is not None: + payload["created_at"] = created_at + self._put_approval_milestone("approval_granted", payload) + + def write_approval_denied( + self, + *, + request_id: str, + reason: str, + decided_at: str | None, + created_at: str | None = None, + ) -> None: + """Emit ``approval_denied`` (user DENIED, §11.1). + + Chunk 8a: see ``write_approval_granted`` for the ``created_at`` + rationale (field omitted from metadata when ``None``). + """ + payload: dict = { + "request_id": request_id, + "reason": self._preview(reason), + "decided_at": decided_at, + } + if created_at is not None: + payload["created_at"] = created_at + self._put_approval_milestone("approval_denied", payload) + + def write_approval_timed_out( + self, + *, + request_id: str, + timeout_s: int, + created_at: str | None = None, + effective_timeout_s: int | None = None, + matching_rule_ids: list[str] | None = None, + ) -> None: + """Emit ``approval_timed_out`` (timer expired, §11.1). + + Chunk 8a additions to support the ApprovalMetricsPublisher + dashboard widgets (IMPL-28): + + - ``created_at`` — propagated from the approval row so the + publisher Lambda can compute decision latency at + ``outcome=timed_out`` the same way it does for granted/denied. + - ``effective_timeout_s`` — the clipped timeout actually applied + (from the approval row's ``timeout_s`` field, which is the + post-clip value). Required by the ``ApprovalTimeoutBreakdown`` + histogram. When absent (legacy caller), the metric emit is + skipped + a ``METRICS_SCHEMA_MISMATCH`` counter fires, rather + than polluting percentile widgets with zero-valued samples. + - ``matching_rule_ids`` — rule ids that matched at gate-fire + time, propagated from the approval row's + ``matching_rule_ids``. Used by the publisher Lambda to emit + ``TimedOutEffectiveTimeout`` with a ``rule_id`` dimension + (normalized against an allowlist so unknown rules collapse + to ``other`` — cardinality cap, H4 adversarial finding). + + ``timeout_s`` is retained for backward-compat — pre-Chunk-8a + events and consumers that only want the raw timer value see + the original field. ``effective_timeout_s`` is a separate field + (not a rename) to keep the event schema a pure superset. + """ + payload: dict = {"request_id": request_id, "timeout_s": timeout_s} + if created_at is not None: + payload["created_at"] = created_at + if effective_timeout_s is not None: + payload["effective_timeout_s"] = effective_timeout_s + if matching_rule_ids is not None: + payload["matching_rule_ids"] = list(matching_rule_ids) + self._put_approval_milestone("approval_timed_out", payload) + + def write_approval_stranded( + self, + *, + request_id: str, + age_s: int, + reason: str, + ) -> None: + """Emit ``approval_stranded`` (reconciler surface, §11.1).""" + self._put_approval_milestone( + "approval_stranded", + {"request_id": request_id, "age_s": age_s, "reason": self._preview(reason)}, + ) + + def write_approval_write_failed(self, *, request_id: str | None, error: str) -> None: + """Emit ``approval_write_failed`` (TransactWriteItems failure, §11.1).""" + self._put_approval_milestone( + "approval_write_failed", + {"request_id": request_id, "error": self._preview(error)}, + ) + + def write_approval_resume_failed(self, *, request_id: str, error: str) -> None: + """Emit ``approval_resume_failed`` (resume transaction failure, §11.1).""" + self._put_approval_milestone( + "approval_resume_failed", + {"request_id": request_id, "error": self._preview(error)}, + ) + + def write_approval_poll_degraded(self, *, request_id: str, consecutive_failures: int) -> None: + """Emit ``approval_poll_degraded`` (3+ consecutive GetItem failures).""" + self._put_approval_milestone( + "approval_poll_degraded", + { + "request_id": request_id, + "consecutive_failures": consecutive_failures, + }, + ) + + def write_approval_timeout_capped( + self, + *, + request_id: str, + requested_timeout_s: int, + effective_timeout_s: int, + reason: str, + matching_rule_ids: list[str] | None = None, + ) -> None: + """Emit ``approval_timeout_capped`` when min-wins clips the timeout. + + ``reason`` ∈ {rule_annotation, maxLifetime_ceiling, runtime_jwt_ceiling}. + ``matching_rule_ids`` populated when ``reason == "rule_annotation"`` + (IMPL-26). + """ + payload: dict = { + "request_id": request_id, + "requested_timeout_s": requested_timeout_s, + "effective_timeout_s": effective_timeout_s, + "reason": reason, + } + if matching_rule_ids is not None: + payload["matching_rule_ids"] = list(matching_rule_ids) + self._put_approval_milestone("approval_timeout_capped", payload) + + def write_approval_ceiling_shrinking( + self, + *, + request_id: str, + max_lifetime_remaining_s: int, + cleanup_margin_s: int, + task_default_timeout_s: int, + ) -> None: + """Emit once-per-task ``approval_ceiling_shrinking`` (IMPL-26, §11.1).""" + self._put_approval_milestone( + "approval_ceiling_shrinking", + { + "request_id": request_id, + "maxLifetime_remaining_s": max_lifetime_remaining_s, + "cleanup_margin_s": cleanup_margin_s, + "task_default_timeout_s": task_default_timeout_s, + }, + ) + + def write_approval_cap_exceeded(self, *, request_id: str, count: int, cap: int) -> None: + """Emit ``approval_cap_exceeded`` (per-task gate-cap fired).""" + self._put_approval_milestone( + "approval_cap_exceeded", + {"request_id": request_id, "count": count, "cap": cap}, + ) + + def write_approval_rate_limit_exceeded(self, *, request_id: str, rate: int, limit: int) -> None: + """Emit ``approval_rate_limit_exceeded`` (per-minute rate limit hit).""" + self._put_approval_milestone( + "approval_rate_limit_exceeded", + {"request_id": request_id, "rate": rate, "limit": limit}, + ) + + def write_approval_late_win(self, *, request_id: str, outcome: str, reason: str) -> None: + """Emit ``approval_late_win`` (VM-throttle race mitigation, IMPL-24).""" + self._put_approval_milestone( + "approval_late_win", + { + "request_id": request_id, + "outcome": outcome, + "reason": self._preview(reason), + }, + ) + + def write_policy_decision_cached( + self, + *, + tool_name: str, + tool_input_sha256: str, + cached_decision: str, + cached_reason: str, + original_decision_ts: str, + ) -> None: + """Emit ``policy_decision`` for a recent-decision-cache hit (IMPL-23). + + Event name is ``policy_decision`` (not ``approval_*``) because a + cache hit intentionally avoids creating a new approval row — the + original decision already produced the audit trail. The + ``decision_source`` field distinguishes cache-driven denies from + fresh policy evaluations so operators can correlate retry patterns + with container restarts (§12.8, §13.6). + + Reuses ``_put_approval_milestone`` for the ``agent_milestone`` + event shape; the ``milestone`` key is ``policy_decision`` here + instead of an ``approval_*`` name. No new approval row, no + counter increment — purely observability (§12.8 caveat). + """ + self._put_approval_milestone( + "policy_decision", + { + "decision_source": "recent_decision_cache", + "tool_name": tool_name, + "tool_input_sha256": tool_input_sha256, + "cached_decision": cached_decision, + "cached_reason": self._preview(cached_reason), + "original_decision_ts": original_decision_ts, + }, + ) diff --git a/agent/src/runner.py b/agent/src/runner.py index a498d508..c7315084 100644 --- a/agent/src/runner.py +++ b/agent/src/runner.py @@ -33,7 +33,7 @@ from config import AGENT_WORKSPACE from models import AgentResult, TaskConfig, TokenUsage from progress_writer import _ProgressWriter -from shell import log, truncate +from shell import log, log_error_cw, truncate from telemetry import _TrajectoryWriter @@ -175,6 +175,104 @@ def _setup_agent_env(config: TaskConfig) -> tuple[str | None, str | None]: return otlp_endpoint, otlp_protocol +def _initialize_policy_engine_and_hooks( + *, + config: TaskConfig, + trajectory: _TrajectoryWriter | None, + progress: _ProgressWriter, +) -> tuple[Any, dict]: + """Construct the per-task ``PolicyEngine`` and wire its hook matchers. + + Extracted from ``run_agent`` so the policy-engine bootstrap path is + directly unit-testable without spinning up the full SDK / agent loop. + Handles: + + * Threading per-task approval params (``initial_approvals``, + ``approval_timeout_s``, and Chunk 7's ``initial_approval_gate_count``) + through to ``PolicyEngine.__init__``. + * Emitting the ``pre_approvals_loaded`` milestone (§4 step 7, §11.1) + unconditionally so "no pre-approvals seeded" is explicit rather than + inferred from silence. + * Building the SDK hook matchers that route PreToolUse / PostToolUse / + Stop invocations through the engine. + """ + from hooks import build_hook_matchers + from policy import PolicyEngine + + cedar_policies = config.cedar_policies + # Cedar HITL (§7.3, §10.2) — per-task approval defaults threaded + # from the orchestrator payload. Engine clamps invalid values + # at construction. + engine_kwargs: dict = {} + if config.initial_approvals: + engine_kwargs["initial_approvals"] = list(config.initial_approvals) + if config.approval_timeout_s is not None: + engine_kwargs["task_default_timeout_s"] = config.approval_timeout_s + # Chunk 7 (§13.6): seed the session counter from the TaskTable + # persisted value so a container restart mid-task resumes the + # cumulative gate budget and the ``approval_gate_cap`` remains + # the terminal bound across restarts. + if config.initial_approval_gate_count: + engine_kwargs["initial_approval_gate_count"] = config.initial_approval_gate_count + # Chunk 7b (§4 step 5, decision #13): adopt the per-task cap + # resolved at submit-time (blueprint override or platform default, + # frozen on the TaskRecord). When absent (legacy task predating + # Chunk 7b), ``PolicyEngine`` falls back to DEFAULT_APPROVAL_GATE_CAP + # so the behavior matches pre-Chunk-7b deploys. + if config.approval_gate_cap is not None: + engine_kwargs["approval_gate_cap"] = config.approval_gate_cap + policy_engine = PolicyEngine( + task_type=config.task_type, + repo=config.repo_url, + extra_policies=cedar_policies if cedar_policies else None, + **engine_kwargs, + ) + # Chunk 7c: surface the resolved cap + its source so operators can + # distinguish a blueprint-threaded value from the engine's compile-time + # default on a container restart. Mirrors the ``approval_gate_cap_source`` + # field on the handler's "Task created" log so both ends of the cascade + # carry the same key name — CloudWatch Insights queries can + # filter/group by ``approval_gate_cap_source`` across handler + + # agent events. Value domains differ intentionally: the handler + # distinguishes ``blueprint`` vs ``platform_default``, but the + # agent only sees the threaded number (blueprint-set or default-50 + # frozen on the TaskRecord both look the same from here), so it + # emits ``threaded`` vs ``engine_default`` (the latter only fires + # for legacy tasks that predate Chunk 7b and have no cap on the + # TaskRecord at all). Cross-reference handler log at + # ``create-task-core.ts::logger.info('Task created', ...)`` for the + # ground-truth blueprint-vs-default distinction. + if config.approval_gate_cap is not None: + cap_log = f" approval_gate_cap={config.approval_gate_cap} approval_gate_cap_source=threaded" + else: + cap_log = " approval_gate_cap=unset approval_gate_cap_source=engine_default" + log( + "AGENT", + f"Cedar policy engine initialized for task_type={config.task_type}" + + (f" with {len(cedar_policies)} extra policies" if cedar_policies else "") + + cap_log, + ) + + # §4 step 7, §11.1: surface the starting pre-approval posture to the + # live SSE stream + 90d DDB record so operators can see exactly which + # scopes were seeded at task start. Emit unconditionally (count=0, + # scopes=[]) so "no pre-approvals seeded" is explicit rather than + # inferred from silence. + progress.write_approval_pre_approvals_loaded( + count=len(config.initial_approvals), + scopes=list(config.initial_approvals), + ) + + hooks = build_hook_matchers( + engine=policy_engine, + trajectory=trajectory, + task_id=config.task_id or "", + progress=progress, + user_id=config.user_id or "", + ) + return policy_engine, hooks + + async def run_agent( prompt: str, system_prompt: str, @@ -243,27 +341,11 @@ def _on_stderr(line: str) -> None: # in UserMessages (ToolResultBlock carries only the id, not the name). tool_use_id_to_name: dict[str, str] = {} - from hooks import build_hook_matchers - from policy import PolicyEngine - - task_type = config.task_type - repo_url = config.repo_url - cedar_policies = config.cedar_policies - policy_engine = PolicyEngine( - task_type=task_type, - repo=repo_url, - extra_policies=cedar_policies if cedar_policies else None, - ) - log( - "AGENT", - f"Cedar policy engine initialized for task_type={task_type}" - + (f" with {len(cedar_policies)} extra policies" if cedar_policies else ""), - ) - - hooks = build_hook_matchers( - engine=policy_engine, + # Engine is consumed by ``build_hook_matchers`` inside the helper; the + # caller only needs the hook matchers for ``ClaudeAgentOptions``. + _policy_engine, hooks = _initialize_policy_engine_and_hooks( + config=config, trajectory=trajectory, - task_id=config.task_id or "", progress=progress, ) @@ -430,6 +512,12 @@ def _on_stderr(line: str) -> None: f"cost=${message.total_cost_usd or 0:.4f} " f"duration={message.duration_ms / 1000:.1f}s", ) + if message.is_error and message.result: + # Mirror SDK-level result errors to APPLICATION_LOGS + # so the dashboard + ``bgagent status`` see them + # (stdout-only logs route to runtime-DEFAULT, not + # the group TaskDashboard reads). + log_error_cw(str(message.result), task_id=config.task_id or None) # Write trajectory result summary (use effective status after is_error remap) trajectory.write_result( @@ -483,7 +571,13 @@ def _on_stderr(line: str) -> None: ) except Exception as e: - log("ERROR", f"Exception during receive_response(): {type(e).__name__}: {e}") + # Mirror the SDK-loop crash to APPLICATION_LOGS so operators + # see it on the dashboard widget + ``bgagent status`` and not + # just on the runtime-DEFAULT stream. + log_error_cw( + f"Exception during receive_response(): {type(e).__name__}: {e}", + task_id=config.task_id or None, + ) progress.write_agent_error(error_type=type(e).__name__, message=str(e)) if result.status == "unknown": result.status = "error" diff --git a/agent/src/server.py b/agent/src/server.py index aa547a2d..041df5a5 100644 --- a/agent/src/server.py +++ b/agent/src/server.py @@ -29,6 +29,19 @@ from observability import set_session_id from pipeline import run_task +# --- _debug_cw / _warn_cw failure counter ------------------------------- +# Shared counter for BOTH the debug and warn CloudWatch writers. AgentCore +# doesn't forward container stdout to APPLICATION_LOGS, so a broken writer +# is invisible except for this metric. Single counter = single alarm +# surface — the trade-off is that the alarm can't distinguish which writer +# is broken (see Chunk 7c review notes). Defined BEFORE any function that +# references it (including ``_debug_cw`` / ``_warn_cw``) so the ordering is +# import-time safe: a daemon thread spawned from a write-blocking function +# can never race with module-level globals still being assigned. +_debug_cw_failures = 0 +_debug_cw_failures_lock = threading.Lock() +_DEBUG_CW_FAILURE_EMIT_EVERY = 5 + def _redact_cached_credentials(text: str) -> str: """Remove cached env secrets from debug text before stdout / CloudWatch.""" @@ -91,13 +104,71 @@ def _debug_cw_exc( _debug_cw(f"{message} [{type(exc).__name__}: {exc}]\n{tb}", task_id=task_id) -# --- _debug_cw failure counter ------------------------------------------- -# Counts write failures from the daemon thread. AgentCore doesn't forward -# container stdout to APPLICATION_LOGS, so a broken _debug_cw is invisible -# except for this metric. -_debug_cw_failures = 0 -_debug_cw_failures_lock = threading.Lock() -_DEBUG_CW_FAILURE_EMIT_EVERY = 5 +def _warn_cw(msg: str, *, task_id: str | None = None) -> None: + """Emit a server-level warning to stdout AND CloudWatch. + + Chunk 7c — AgentCore doesn't forward container stdout to + APPLICATION_LOGS (see the ``_debug_cw`` comment block above), so + warning ``print`` calls about malformed invocation payloads are + effectively invisible in production. Route them through the same + daemon-thread CloudWatch writer used by ``_debug_cw`` (writing to + the ``server_warn/`` stream so operators can alarm on + warn traffic separately from debug noise). + + The stdout ``print`` is preserved so local ``docker-compose`` runs + and the existing ``capsys``-based unit tests still observe the + line. CloudWatch delivery is fire-and-forget — failures bump the + shared ``_debug_cw_failures`` counter via ``_warn_cw_write_blocking`` + so a silently broken writer still surfaces via that single metric. + """ + stamped = f"[server/warn] {msg}" + print(stamped, flush=True) + + log_group = os.environ.get("LOG_GROUP_NAME") + if not log_group: + return + + _t = threading.Thread( + target=_warn_cw_write_blocking, + args=(log_group, task_id, stamped), + name="warn-cw-write", + daemon=True, + ) + _t.start() + + +def _warn_cw_write_blocking(log_group: str, task_id: str | None, stamped: str) -> None: + """Blocking CloudWatch write for ``_warn_cw`` — only called from a background thread. + + Mirrors ``_debug_cw_write_blocking`` but writes to the + ``server_warn/`` stream so warn-level traffic is easy to + alarm on independently of debug breadcrumbs. Failures bump the + shared ``_debug_cw_failures`` counter — a single alarm surface + covers both writers. + """ + try: + import boto3 + + region = os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") + client = boto3.client("logs", region_name=region) + + stream = f"server_warn/{task_id or 'server'}" + with _ctx_for_debug.suppress(client.exceptions.ResourceAlreadyExistsException): + client.create_log_stream(logGroupName=log_group, logStreamName=stream) + + client.put_log_events( + logGroupName=log_group, + logStreamName=stream, + logEvents=[{"timestamp": int(_time_for_debug.time() * 1000), "message": stamped}], + ) + except Exception as _exc: + global _debug_cw_failures + with _debug_cw_failures_lock: + _debug_cw_failures += 1 + print( + f"[server/warn/self] CloudWatch write failed: {type(_exc).__name__}: {_exc}", + flush=True, + ) def _debug_cw_write_blocking(log_group: str, task_id: str | None, stamped: str) -> None: @@ -273,6 +344,10 @@ def _run_task_background( branch_name: str = "", pr_number: str = "", cedar_policies: list[str] | None = None, + approval_timeout_s: int | None = None, + initial_approvals: list[str] | None = None, + initial_approval_gate_count: int = 0, + approval_gate_cap: int | None = None, channel_source: str = "", channel_metadata: dict[str, str] | None = None, trace: bool = False, @@ -322,6 +397,10 @@ def _run_task_background( branch_name=branch_name, pr_number=pr_number, cedar_policies=cedar_policies, + approval_timeout_s=approval_timeout_s, + initial_approvals=initial_approvals, + initial_approval_gate_count=initial_approval_gate_count, + approval_gate_cap=approval_gate_cap, channel_source=channel_source, channel_metadata=channel_metadata, trace=trace, @@ -371,6 +450,46 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: branch_name = inp.get("branch_name", "") pr_number = str(inp.get("pr_number", "")) cedar_policies = inp.get("cedar_policies") or [] + # Cedar HITL (§7.3) — per-task approval defaults + seeded allowlist. + # Both are forwarded verbatim to the pipeline; the engine + # validates shape at construction time and raises on bad input. + approval_timeout_s = inp.get("approval_timeout_s") + initial_approvals = inp.get("initial_approvals") or [] + # Chunk 7: TaskTable-persisted ``approval_gate_count`` threaded by + # the orchestrator so a container restart (§13.6) resumes the + # cumulative gate budget instead of resetting to 0. Non-int payloads + # coerce to 0 to keep the invocation path fail-open on a malformed + # field; the downstream PolicyEngine rejects negatives loudly. + raw_gate_count = inp.get("initial_approval_gate_count", 0) + try: + initial_approval_gate_count = int(raw_gate_count) + except (TypeError, ValueError): + _warn_cw( + "initial_approval_gate_count payload field is not an int " + f"(type={type(raw_gate_count).__name__}, value={raw_gate_count!r}); " + f"coerced to 0. task_id={inp.get('task_id', '')!r}", + task_id=inp.get("task_id"), + ) + initial_approval_gate_count = 0 + # Chunk 7b (§4 step 5, decision #13): per-task cap resolved by the + # submit path and persisted on the TaskRecord. Threaded so a + # blueprint-configured cap (or the default-50 frozen at submit) wins + # over the PolicyEngine's compile-time fallback on restarts. A + # malformed payload coerces to ``None`` so the engine can still + # construct; its own bounds check would reject anything out-of-range. + raw_approval_gate_cap = inp.get("approval_gate_cap") + approval_gate_cap: int | None = None + if raw_approval_gate_cap is not None: + try: + approval_gate_cap = int(raw_approval_gate_cap) + except (TypeError, ValueError): + _warn_cw( + "approval_gate_cap payload field is not an int " + f"(type={type(raw_approval_gate_cap).__name__}, value={raw_approval_gate_cap!r}); " + f"falling back to engine default. task_id={inp.get('task_id', '')!r}", + task_id=inp.get("task_id"), + ) + approval_gate_cap = None channel_source = inp.get("channel_source", "") or "" channel_metadata = inp.get("channel_metadata") or {} # ``trace`` is strictly opt-in (design §10.1). Accept only real @@ -389,16 +508,27 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: if isinstance(raw_user_id, str): user_id = raw_user_id else: - print( - "[server/warn] user_id payload field is not a string " + _warn_cw( + "user_id payload field is not a string " f"(type={type(raw_user_id).__name__}); coerced to empty. " f"task_id={inp.get('task_id', '')!r}", - flush=True, + task_id=inp.get("task_id"), ) user_id = "" session_id = request.headers.get("x-amzn-bedrock-agentcore-runtime-session-id", "") + # Cedar HITL: stamp TASK_STARTED_AT so the PreToolUse hook's + # ``_remaining_maxlifetime_s`` (agent/src/hooks.py §6.5) has the + # real per-task clock to compute the maxLifetime ceiling. Without + # this the hook's ceiling computation silently falls back to + # "unknown, don't clip" (fail-open) and the user may be asked for + # approval on a gate whose window will expire before they can + # respond. + started_at = inp.get("task_started_at", "") + if started_at and isinstance(started_at, str): + os.environ["TASK_STARTED_AT"] = started_at + return { "repo_url": repo_url, "task_description": task_description, @@ -418,6 +548,10 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: "branch_name": branch_name, "pr_number": pr_number, "cedar_policies": cedar_policies, + "approval_timeout_s": approval_timeout_s, + "initial_approvals": initial_approvals, + "initial_approval_gate_count": initial_approval_gate_count, + "approval_gate_cap": approval_gate_cap, "channel_source": channel_source, "channel_metadata": channel_metadata, "trace": trace, diff --git a/agent/src/shell.py b/agent/src/shell.py index f5e99eea..0538ec97 100644 --- a/agent/src/shell.py +++ b/agent/src/shell.py @@ -1,8 +1,10 @@ """Shell utilities: logging, command execution, and text helpers.""" +import contextlib import os import re import subprocess +import threading import time @@ -12,6 +14,76 @@ def log(prefix: str, text: str): print(f"[{ts}] {prefix} {redact_secrets(text)}", flush=True) +def log_error_cw(message: str, *, task_id: str | None = None) -> None: + """Emit an ERROR line to stdout AND the APPLICATION_LOGS CloudWatch group. + + Chunk 10 observability gap: ``log("ERROR", ...)`` writes to container + stdout, which AgentCore routes to + ``/aws/bedrock-agentcore/runtimes/-DEFAULT`` rather than + the APPLICATION_LOGS group that ``TaskDashboard`` LogQueryWidgets + and ``bgagent status`` read. Agent-fatal errors were therefore + invisible in the two places operators normally look — discovered + during E2E 2026-05-11 T2.2 when a ``missing built-in hard-deny + policies`` crash surfaced only as a cryptic "unknown" on the CLI. + + This helper mirrors the ERROR line to APPLICATION_LOGS via a + fire-and-forget daemon thread (so it cannot block the failing + code path) using the same writer pattern as ``server.py::_warn_cw``. + Delivery failures are swallowed silently — the stdout ``log`` call + above still runs, and a caller that wants strict delivery should + use ``server._warn_cw`` directly from the server-only code paths. + """ + # Always log to stdout for local / docker-compose parity with the + # normal ``log()`` path. + log("ERROR", message) + + log_group = os.environ.get("LOG_GROUP_NAME") + if not log_group: + return + + stamped = f"[agent/error] {redact_secrets(message)}" + _t = threading.Thread( + target=_log_error_cw_blocking, + args=(log_group, task_id, stamped), + name="agent-error-cw-write", + daemon=True, + ) + _t.start() + + +def _log_error_cw_blocking(log_group: str, task_id: str | None, stamped: str) -> None: + """Blocking CloudWatch put for ``log_error_cw`` — daemon-thread only. + + Mirrors ``server.py::_warn_cw_write_blocking`` but targets a + separate ``agent_error/`` stream so operators can alarm + on agent-runtime fatal errors distinctly from server-layer + warnings. Failures are swallowed (any surfaceable alarm should + fire on the absence of the expected stream, not on this helper). + """ + try: + import boto3 + + region = os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") + client = boto3.client("logs", region_name=region) + stream = f"agent_error/{task_id or 'unknown'}" + with contextlib.suppress(client.exceptions.ResourceAlreadyExistsException): + client.create_log_stream(logGroupName=log_group, logStreamName=stream) + client.put_log_events( + logGroupName=log_group, + logStreamName=stream, + logEvents=[{"timestamp": int(time.time() * 1000), "message": stamped}], + ) + except Exception: # noqa: S110 - best-effort telemetry; stdout path already logged + # Intentionally silent. The caller (``log_error_cw``) has + # already written the same message to stdout via the regular + # ``log("ERROR", ...)`` path, so a CloudWatch delivery failure + # (IAM, network, quota) does not lose the signal — it only + # degrades it to the runtime-DEFAULT log group. Raising here + # would unwind the daemon thread mid-shutdown and emit a + # confusing secondary traceback during a primary failure. + pass + + def truncate(text: str, max_len: int = 200) -> str: """Truncate text for log display.""" if not text: diff --git a/agent/src/task_state.py b/agent/src/task_state.py index 65d4599b..98302187 100644 --- a/agent/src/task_state.py +++ b/agent/src/task_state.py @@ -7,6 +7,38 @@ import os import time +from typing import TypedDict + +from shell import log, log_error_cw + + +class ApprovalRow(TypedDict): + """Schema for the approval row written by ``transact_write_approval_request``. + + Mirrors the DDB column layout described in design §10.1 and the + TypeScript ``ApprovalRecord`` discriminated union in + ``cdk/src/handlers/shared/types.ts``. Used as the typed contract + between the PreToolUse hook (which builds the row) and the + transactional writer (which serializes it to DDB attributes). + Pre-S7 the function accepted a bare ``dict`` so missing or + misspelled fields would fail at runtime, not at the call site. + """ + + task_id: str + request_id: str + tool_name: str + tool_input_preview: str + tool_input_sha256: str + reason: str + severity: str # 'low' | 'medium' | 'high' — matches TS Severity literal. + matching_rule_ids: list[str] + status: str # always 'PENDING' on initial write. + created_at: str + timeout_s: int + ttl: int + user_id: str + repo: str + _table = None @@ -69,7 +101,7 @@ def write_submitted( item["task_description"] = task_description table.put_item(Item=item) except Exception as e: - print(f"[task_state] write_submitted failed (best-effort): {e}") + log("WARN", f"[task_state] write_submitted failed (best-effort): {e}") def write_heartbeat(task_id: str) -> None: @@ -93,7 +125,7 @@ def write_heartbeat(task_id: str) -> None: and e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException" ): return - print(f"[task_state] write_heartbeat failed (best-effort): {type(e).__name__}: {e}") + log("WARN", f"[task_state] write_heartbeat failed (best-effort): {type(e).__name__}: {e}") def write_session_info(task_id: str, session_id: str, agent_runtime_arn: str) -> None: @@ -147,7 +179,10 @@ def write_session_info(task_id: str, session_id: str, agent_runtime_arn: str) -> ): # Task already advanced — concurrent legitimate transition wins. return - print(f"[task_state] write_session_info failed (best-effort): {type(e).__name__}: {e}") + log( + "WARN", + f"[task_state] write_session_info failed (best-effort): {type(e).__name__}: {e}", + ) def write_running(task_id: str) -> None: @@ -194,9 +229,9 @@ def write_running(task_id: str) -> None: isinstance(e, ClientError) and e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException" ): - print("[task_state] write_running skipped: status precondition not met") + log("INFO", "[task_state] write_running skipped: status precondition not met") return - print(f"[task_state] write_running failed (best-effort): {type(e).__name__}") + log("WARN", f"[task_state] write_running failed (best-effort): {type(e).__name__}") def write_terminal(task_id: str, status: str, result: dict | None = None) -> None: @@ -218,6 +253,14 @@ def write_terminal(task_id: str, status: str, result: dict | None = None) -> Non ":running": "RUNNING", ":hydrating": "HYDRATING", ":finalizing": "FINALIZING", + # AWAITING_APPROVAL is included so a container shutdown + # mid-gate can still record the terminal transition. Without + # it, a crash while the user is deciding leaves the task + # stuck until the stranded-task reconciler catches it (~2h). + # Cedar HITL state machine (design §9): RUNNING ↔ + # AWAITING_APPROVAL, both can transition straight to a + # terminal state. + ":awaiting_approval": "AWAITING_APPROVAL", } update_parts = ["#s = :s", "completed_at = :t", "status_created_at = :sca"] @@ -262,7 +305,7 @@ def write_terminal(task_id: str, status: str, result: dict | None = None) -> Non table.update_item( Key={"task_id": task_id}, UpdateExpression="SET " + ", ".join(update_parts), - ConditionExpression="#s IN (:running, :hydrating, :finalizing)", + ConditionExpression="#s IN (:running, :hydrating, :finalizing, :awaiting_approval)", ExpressionAttributeNames=expr_names, ExpressionAttributeValues=expr_values, ) @@ -273,9 +316,10 @@ def write_terminal(task_id: str, status: str, result: dict | None = None) -> Non isinstance(e, ClientError) and e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException" ): - print( + log( + "INFO", "[task_state] write_terminal skipped: " - "status precondition not met (task may have been cancelled)" + "status precondition not met (task may have been cancelled)", ) # K2 final review SIG-1: ConditionalCheckFailed on the # happy path after a successful S3 trace upload orphans @@ -293,7 +337,10 @@ def write_terminal(task_id: str, status: str, result: dict | None = None) -> Non # back on the record and the orphan log below documents # the original race for operators. if result and result.get("trace_s3_uri"): - print( + # ERROR severity: a real orphan is operator-actionable. + # Routed via log_error_cw so it shows up in the + # APPLICATION_LOGS group that TaskDashboard reads. + log_error_cw( f"[task_state] trace_s3_uri orphaned by " f"ConditionalCheckFailed: task_id={task_id!r} " f"trace_s3_uri={result['trace_s3_uri']!r}. " @@ -301,18 +348,18 @@ def write_terminal(task_id: str, status: str, result: dict | None = None) -> Non f"updated; presigned-URL endpoint will 404 for " f"this task. Object will be reaped by the 7-day " f"lifecycle.", - flush=True, + task_id=task_id, ) healed = write_trace_uri_conditional(task_id, result["trace_s3_uri"]) if healed: - print( + log( + "INFO", f"[task_state] trace_s3_uri self-healed for " f"task_id={task_id!r} after ConditionalCheckFailed " f"(terminal-state race).", - flush=True, ) return - print(f"[task_state] write_terminal failed (best-effort): {type(e).__name__}") + log("WARN", f"[task_state] write_terminal failed (best-effort): {type(e).__name__}") def write_trace_uri_conditional(task_id: str, uri: str) -> bool: @@ -357,17 +404,17 @@ def write_trace_uri_conditional(task_id: str, uri: str) -> bool: and e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException" ): # Benign: URI was already persisted, or status isn't terminal yet. - print( + log( + "INFO", f"[task_state] write_trace_uri_conditional skipped for " f"task_id={task_id!r}: precondition not met " f"(trace_s3_uri already set or status not terminal).", - flush=True, ) return False - print( + log( + "WARN", f"[task_state] write_trace_uri_conditional failed for " f"task_id={task_id!r}: {type(e).__name__}: {e}", - flush=True, ) return False @@ -405,6 +452,460 @@ def get_task(task_id: str) -> dict | None: try: resp = table.get_item(Key={"task_id": task_id}) except Exception as e: - print(f"[task_state] get_task failed: {type(e).__name__}: {e}") + log("WARN", f"[task_state] get_task failed: {type(e).__name__}: {e}") raise TaskFetchError(f"{type(e).__name__}: {e}") from e return resp.get("Item") + + +# --------------------------------------------------------------------------- +# Cedar HITL approval primitives (§6.5, §9.1, IMPL-24) +# --------------------------------------------------------------------------- +# +# ``TaskApprovalsTable`` and the AWAITING_APPROVAL status transitions land +# physically in Chunk 4 (CDK). The agent-side helpers below are written to +# that contract and exposed so Chunk 3's ``pre_tool_use_hook`` can be +# implemented + unit-tested now (via mocked boto3 clients); Chunk 4 sets +# ``TASK_APPROVALS_TABLE_NAME`` + grants IAM and the same helpers start +# making real DDB calls with no further code change on the agent side. +# +# Primitives exposed: +# - ``transact_write_approval_request`` — atomic Put(TaskApprovals) + +# Update(TaskTable: RUNNING → AWAITING_APPROVAL). Raises +# ``ApprovalWriteError`` on ``TransactionCanceledException`` so the +# hook can return DENY + ``approval_write_failed`` (§13.1). +# - ``transact_resume_from_approval`` — atomic Update(TaskTable: +# AWAITING_APPROVAL → RUNNING) gated on +# ``awaiting_approval_request_id = request_id``. Raises +# ``ApprovalResumeError`` on cancellation (§13.9). +# - ``best_effort_update_approval_status`` — conditional Update on the +# approval row (``status = :pending`` guard). Returns ``False`` on +# ``ConditionCheckFailed`` so IMPL-24's re-read re-read path fires. +# - ``get_approval_row`` — strongly-consistent GetItem; default +# ``consistent_read=True`` because IMPL-24's race fix relies on it. +# +# Errors beyond the structural conditions (unreachable DDB, IAM drift, +# missing env var) raise ``ApprovalTablesUnavailable`` so the hook can +# fail CLOSED without guessing. The hook maps that to DENY so a +# pre-Chunk-4 deploy cannot silently bypass gates. + +TASK_APPROVALS_TABLE_ENV = "TASK_APPROVALS_TABLE_NAME" +TASK_TABLE_ENV = "TASK_TABLE_NAME" + +# TaskTable status values referenced by the approval primitives. Kept as +# constants so a rename in CDK cannot silently diverge the Python path. +_STATUS_RUNNING = "RUNNING" +_STATUS_AWAITING_APPROVAL = "AWAITING_APPROVAL" + + +class ApprovalTablesUnavailable(RuntimeError): + """Either ``TASK_APPROVALS_TABLE_NAME`` or ``TASK_TABLE_NAME`` is unset. + + Hook maps to DENY (fail-closed); see §13.15. Distinct from + ``TaskFetchError`` so callers do not collapse a config problem with a + transient read failure. + """ + + +class ApprovalWriteError(RuntimeError): + """``transact_write_approval_request`` TransactionCanceledException. + + Fired when the cross-table atomic write is cancelled — either the + TaskTable precondition fails (task already cancelled / advanced past + RUNNING) or the approval row already exists. Hook maps to DENY + + ``approval_write_failed`` (§13.1). The underlying cancellation reasons + are stashed on ``.cancellation_reasons`` for triage. + """ + + def __init__(self, message: str, cancellation_reasons: list | None = None) -> None: + super().__init__(message) + self.cancellation_reasons = cancellation_reasons or [] + + +class ApprovalResumeError(RuntimeError): + """``transact_resume_from_approval`` TransactionCanceledException. + + Fired when the resume transition fails — typically because the user + cancelled the task mid-approval (§13.9). Hook maps to DENY + + ``approval_resume_failed``. + """ + + def __init__(self, message: str, cancellation_reasons: list | None = None) -> None: + super().__init__(message) + self.cancellation_reasons = cancellation_reasons or [] + + +def _get_ddb_client(*, client=None): + """Return a boto3 DDB low-level client, or the injected ``client`` for tests. + + Tests inject a mock client rather than relying on moto because the + primitives here touch ``transact_write_items`` with cross-table + conditions, which moto's older versions do not fully emulate. + """ + if client is not None: + return client + import boto3 + + region = os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") + return boto3.client("dynamodb", region_name=region) + + +def _require_tables() -> tuple[str, str]: + """Return ``(task_table, approvals_table)`` or raise. + + Kept as a single guard so every caller surfaces the same error class. + """ + task_table = os.environ.get(TASK_TABLE_ENV) + approvals_table = os.environ.get(TASK_APPROVALS_TABLE_ENV) + if not task_table or not approvals_table: + raise ApprovalTablesUnavailable( + f"{TASK_TABLE_ENV}/{TASK_APPROVALS_TABLE_ENV} unset; approval gates cannot be recorded" + ) + return task_table, approvals_table + + +def _py_to_ddb_attr(value): + """Translate a Python value into the DDB low-level attribute shape. + + Handles the subset we actually write: ``str``, ``int``, ``bool``, + ``None``, lists-of-str. More exotic types would need marshalling + support; ``approval_row`` values are constrained to the §10.1 schema + which falls entirely inside this subset. + """ + if value is None: + return {"NULL": True} + if isinstance(value, bool): + return {"BOOL": value} + if isinstance(value, int): + return {"N": str(value)} + if isinstance(value, str): + return {"S": value} + if isinstance(value, list): + # Lists of strings (matching_rule_ids); other shapes are rejected + # loudly so a future schema drift surfaces in tests rather than + # silently writing an empty list. + if all(isinstance(v, str) for v in value): + return {"L": [{"S": v} for v in value]} + raise TypeError(f"unsupported list element types in approval row: {value!r}") + raise TypeError(f"unsupported approval-row attribute type: {type(value).__name__}") + + +def _ddb_attr_to_py(attr): + """Inverse of ``_py_to_ddb_attr`` — enough to rehydrate an approval row.""" + if attr is None: + return None + if "NULL" in attr: + return None + if "BOOL" in attr: + return attr["BOOL"] + if "N" in attr: + raw = attr["N"] + # Keep integers integer-shaped for downstream arithmetic (ttl, + # timeout_s). ``decided_at`` is a string so no floats to worry + # about. + try: + return int(raw) + except ValueError: + return raw + if "S" in attr: + return attr["S"] + if "L" in attr: + return [_ddb_attr_to_py(item) for item in attr["L"]] + # Unsupported shape; return raw to aid debugging rather than losing data. + return attr + + +def transact_write_approval_request( + task_id: str, + request_id: str, + approval_row: ApprovalRow, + *, + client=None, +) -> None: + """Atomically record a pending approval + transition the task to AWAITING_APPROVAL. + + Two items: + 1. Put on ``TaskApprovalsTable`` with ``ConditionExpression: + attribute_not_exists(request_id)`` — guards against ULID collisions + and duplicate writes on retry. + 2. Update on ``TaskTable`` with ``ConditionExpression: status = + :running`` — fails if the task has already been cancelled, failed, + or is still pre-RUNNING. On success sets + ``status = AWAITING_APPROVAL`` and + ``awaiting_approval_request_id = `` so the resume + transition can verify it's resuming the right approval. + + Raises ``ApprovalTablesUnavailable`` if either env var is unset; + ``ApprovalWriteError`` on ``TransactionCanceledException``; other + DDB-layer exceptions propagate so the hook's outer try/except can + fail-closed with a specific reason. + """ + task_table, approvals_table = _require_tables() + ddb = _get_ddb_client(client=client) + + approval_item = {k: _py_to_ddb_attr(v) for k, v in approval_row.items()} + # Belt-and-braces: ensure the keys we rely on downstream are present. + approval_item.setdefault("task_id", {"S": task_id}) + approval_item.setdefault("request_id", {"S": request_id}) + approval_item.setdefault("status", {"S": "PENDING"}) + + try: + ddb.transact_write_items( + TransactItems=[ + { + "Put": { + "TableName": approvals_table, + "Item": approval_item, + "ConditionExpression": "attribute_not_exists(request_id)", + } + }, + { + "Update": { + "TableName": task_table, + "Key": {"task_id": {"S": task_id}}, + "UpdateExpression": ( + "SET #s = :awaiting, awaiting_approval_request_id = :rid" + ), + "ConditionExpression": "#s = :running", + "ExpressionAttributeNames": {"#s": "status"}, + "ExpressionAttributeValues": { + ":awaiting": {"S": _STATUS_AWAITING_APPROVAL}, + ":running": {"S": _STATUS_RUNNING}, + ":rid": {"S": request_id}, + }, + } + }, + ] + ) + except Exception as exc: + # TransactionCanceledException carries per-item reasons. Keep the + # detection structural (duck-typed on ``response``) so we do not + # need botocore at import time. + reasons = _extract_cancellation_reasons(exc) + code = _extract_error_code(exc) + if code == "TransactionCanceledException": + raise ApprovalWriteError( + f"approval write cancelled: reasons={reasons}", + cancellation_reasons=reasons, + ) from exc + # Otherwise propagate so outer handler classifies it fail-closed. + raise + + +def transact_resume_from_approval( + task_id: str, + request_id: str, + *, + client=None, +) -> None: + """Atomically resume RUNNING from AWAITING_APPROVAL for ``request_id``. + + The condition ``status = AWAITING_APPROVAL AND + awaiting_approval_request_id = :rid`` prevents: + - resuming a task that's been cancelled mid-approval (§13.9); + - resuming with a stale request_id after a race with the + reconciler / a concurrent approval. + + Raises ``ApprovalResumeError`` on ``TransactionCanceledException`` so + the hook can emit ``approval_resume_failed`` + DENY. + """ + task_table, _ = _require_tables() + ddb = _get_ddb_client(client=client) + + try: + ddb.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": task_table, + "Key": {"task_id": {"S": task_id}}, + "UpdateExpression": ( + "SET #s = :running REMOVE awaiting_approval_request_id" + ), + "ConditionExpression": ( + "#s = :awaiting AND awaiting_approval_request_id = :rid" + ), + "ExpressionAttributeNames": {"#s": "status"}, + "ExpressionAttributeValues": { + ":running": {"S": _STATUS_RUNNING}, + ":awaiting": {"S": _STATUS_AWAITING_APPROVAL}, + ":rid": {"S": request_id}, + }, + } + } + ] + ) + except Exception as exc: + reasons = _extract_cancellation_reasons(exc) + code = _extract_error_code(exc) + if code == "TransactionCanceledException": + raise ApprovalResumeError( + f"approval resume cancelled: reasons={reasons}", + cancellation_reasons=reasons, + ) from exc + raise + + +def best_effort_update_approval_status( + task_id: str, + request_id: str, + new_status: str, + *, + reason: str | None = None, + client=None, +) -> bool: + """Conditionally flip ``status`` on an approval row. + + The condition ``status = :pending`` is the design-doc guard from §6.5. + Used on the TIMED_OUT write path: if the row has already transitioned + to APPROVED or DENIED, the update fails and the caller (the hook) must + re-read the row with ConsistentRead (IMPL-24). + + Returns ``True`` on successful write, ``False`` on + ``ConditionalCheckFailedException``. All other errors propagate. + """ + _, approvals_table = _require_tables() + ddb = _get_ddb_client(client=client) + + update_expr_parts = ["#s = :new"] + expr_values = { + ":new": {"S": new_status}, + ":pending": {"S": "PENDING"}, + } + if reason is not None: + update_expr_parts.append("deny_reason = :reason") + expr_values[":reason"] = {"S": reason} + + try: + ddb.update_item( + TableName=approvals_table, + Key={"task_id": {"S": task_id}, "request_id": {"S": request_id}}, + UpdateExpression="SET " + ", ".join(update_expr_parts), + ConditionExpression="#s = :pending", + ExpressionAttributeNames={"#s": "status"}, + ExpressionAttributeValues=expr_values, + ) + return True + except Exception as exc: + code = _extract_error_code(exc) + if code == "ConditionalCheckFailedException": + return False + raise + + +def get_approval_row( + task_id: str, + request_id: str, + *, + consistent_read: bool = True, + client=None, +) -> dict | None: + """Fetch an approval row. Defaults to strongly-consistent read (IMPL-24). + + Returns a Python dict with unmarshalled attribute values, or ``None`` if + the row does not exist (TTL reaped, wrong IDs, etc.). Callers use the + ``None`` return to detect the row-gone branch in §13.12. + """ + _, approvals_table = _require_tables() + ddb = _get_ddb_client(client=client) + resp = ddb.get_item( + TableName=approvals_table, + Key={"task_id": {"S": task_id}, "request_id": {"S": request_id}}, + ConsistentRead=consistent_read, + ) + item = resp.get("Item") + if item is None: + return None + return {k: _ddb_attr_to_py(v) for k, v in item.items()} + + +def increment_approval_gate_count_in_ddb( + task_id: str, + *, + client=None, +) -> bool: + """Best-effort atomic increment of ``approval_gate_count`` on TaskTable. + + Chunk 7 persistence layer for decision #13's per-task gate counter. The + session counter (``PolicyEngine._approval_gate_count``) stays + authoritative WITHIN a container — this write exists so that a container + restart (§13.6) can seed the new container's counter from the persisted + value instead of resetting to 0 and re-exposing the user to another + ``approval_gate_cap`` worth of gates. + + **Best-effort semantics (§13.6):** the counter is a safety bound, not a + correctness bound. A DDB write failure here MUST NOT block the gate — + the session counter still enforces the cap within this container, and + the §13.6 analysis accepts at most one lost increment per restart as + acceptable damage. Returns ``True`` on success, ``False`` on any + failure (config missing, IAM drift, throttling). Never raises. + + Uses a pure ADD UpdateExpression without a ConditionExpression — the + counter is monotonic and concurrent writes from different hooks on the + same task are not expected (gates are serialized by the PreToolUse + lifecycle). If the attribute is missing, DDB initializes it to 0 before + applying the ADD, matching the CreateTaskFn seed of + ``approval_gate_count: 0`` (see cdk/src/handlers/shared/create-task-core.ts). + + Deliberately kept separate from the resume TransactWriteItems (§6.5): the + joint-update invariant on ``status`` + ``awaiting_approval_request_id`` + (§10.2) must not be burdened with a non-safety-critical counter bump. + """ + try: + task_table, _ = _require_tables() + except ApprovalTablesUnavailable as exc: + log( + "WARN", + f"[task_state] increment_approval_gate_count_in_ddb: tables unavailable, " + f"skipping counter persistence: {exc}", + ) + return False + + try: + ddb = _get_ddb_client(client=client) + ddb.update_item( + TableName=task_table, + Key={"task_id": {"S": task_id}}, + UpdateExpression="ADD approval_gate_count :one", + ExpressionAttributeValues={":one": {"N": "1"}}, + ) + except Exception as exc: + # Best-effort: do not raise. Log at WARN with the error code so + # operators can spot IAM drift / throttle / misconfigured tables. + code = _extract_error_code(exc) or type(exc).__name__ + log( + "WARN", + f"[task_state] increment_approval_gate_count_in_ddb failed for task_id={task_id}: " + f"{code}: {exc}", + ) + return False + return True + + +# ---- Exception-introspection helpers ------------------------------------ + + +def _extract_error_code(exc: BaseException) -> str | None: + """Pull the AWS error code off a ``ClientError``-shaped exception. + + Duck-typed so tests (and environments without botocore at import time) + stay decoupled from the concrete exception type. + """ + response = getattr(exc, "response", None) + if not isinstance(response, dict): + return None + error_block = response.get("Error") or {} + if not isinstance(error_block, dict): + return None + code = error_block.get("Code") + return code if isinstance(code, str) else None + + +def _extract_cancellation_reasons(exc: BaseException) -> list: + """Pull CancellationReasons (best-effort) off a TransactionCanceledException.""" + response = getattr(exc, "response", None) + if not isinstance(response, dict): + return [] + reasons = response.get("CancellationReasons") + if isinstance(reasons, list): + return reasons + return [] diff --git a/agent/src/telemetry.py b/agent/src/telemetry.py index 8234e9bf..8f962cec 100644 --- a/agent/src/telemetry.py +++ b/agent/src/telemetry.py @@ -484,16 +484,45 @@ def upload_trace_to_s3( return None -# Values under these keys may contain tool stderr, paths, or incidental secrets. -_METRICS_REDACT_KEYS = frozenset({"error"}) +# Values under these keys are scanned for secret patterns before emission. +# Previously these fields were blanket-redacted to ``"[redacted]"``, which +# swallowed legitimate structural errors (e.g. ``missing built-in hard-deny +# policies: /app/policies/hard_deny.cedar``) whose diagnostic value is +# high and secret-risk is zero. See E2E 2026-05-11 T2.2 for the incident. +# The new policy runs ``scan_tool_output`` over the value and only +# substitutes ``[REDACTED-