From 3dd27afdde9e7ccd28d3d4dd94e5cd395acc1555 Mon Sep 17 00:00:00 2001 From: abrichr Date: Tue, 18 Aug 2026 12:27:52 -0400 Subject: [PATCH 1/6] fix: enforce governed Desktop egress boundaries --- DESIGN.md | 60 ++-- README.md | 20 +- engine/backends/__init__.py | 2 +- engine/backends/hosted_ingest.py | 77 +---- engine/cli.py | 135 +++++---- engine/db.py | 25 +- engine/dispatch.py | 13 +- engine/flow_bridge.py | 52 +++- engine/hosted.py | 379 ++++++++++++++----------- engine/qualification_lifecycle.py | 13 +- engine/review.py | 214 +++++++++++++- engine/scrubber.py | 135 +++++---- engine/upload_manager.py | 287 ++++++++++++++++--- src/screens/WorkflowLibrary.test.tsx | 35 +++ src/screens/WorkflowLibrary.tsx | 23 +- tests/test_e2e/test_pipeline.py | 28 +- tests/test_engine/test_backends.py | 55 +--- tests/test_engine/test_cli.py | 49 ++++ tests/test_engine/test_dispatch.py | 23 ++ tests/test_engine/test_flow_bridge.py | 54 ++++ tests/test_engine/test_hosted.py | 365 ++++++++++++++++++++---- tests/test_engine/test_review_state.py | 241 +++++++++++++++- tests/test_engine/test_scrubber.py | 43 +++ tests/test_engine/test_upload.py | 331 ++++++++++++++++++--- 24 files changed, 2044 insertions(+), 615 deletions(-) create mode 100644 src/screens/WorkflowLibrary.test.tsx diff --git a/DESIGN.md b/DESIGN.md index bd1e884..ab5e90c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -366,12 +366,10 @@ Every recording has a **review status** that persists in the index database. Thi │ scrubbed │ │ │ copy) │ │ └─────┬─────┘ │ - │ │ - ▼ ▼ + │ + ▼ ┌─────────────────────────┐ - │ CLEARED FOR EGRESS │ ← Data can now be sent to: - │ │ storage backends, VLM APIs, - │ │ annotation pipelines, FL, etc. + │ CLEARED FOR EGRESS │ ← Reviewed scrubbed copy only └─────────────────────────┘ ``` @@ -382,19 +380,17 @@ Every recording has a **review status** that persists in the index database. Thi | `captured` | Raw recording just created. Pending review. | **No** — blocked from all egress | | `scrubbed` | Scrub pass completed, awaiting user review | **No** — still pending human approval | | `reviewed` | User reviewed scrubbed copy and approved | **Yes** — scrubbed copy only | -| `dismissed` | User skipped scrubbing, accepted PII risks | **Yes** — raw data, user's choice | +| `dismissed` | User skipped scrubbing and kept the raw capture local | **No** — dismissal never grants egress | | `deleted` | User deleted the recording | N/A | #### Where data can leave the machine (ALL gated by review state) | Egress Path | What is sent | Gated? | |-------------|-------------|--------| -| S3 / R2 / HF Hub / MinIO upload | Full recording archive | Yes — must be `reviewed` or `dismissed` | -| OpenAI Vision API (annotation) | Individual screenshots | Yes — must be `reviewed` or `dismissed` | -| Anthropic Claude API (annotation) | Individual screenshots | Yes — must be `reviewed` or `dismissed` | -| Google Gemini API (annotation) | Individual screenshots | Yes — must be `reviewed` or `dismissed` | -| Federated learning (gradient upload) | Model gradients (derived from data) | Yes — must be `reviewed` or `dismissed` | -| Magic Wormhole (P2P sharing) | Full recording | Yes — must be `reviewed` or `dismissed` | +| OpenAdapt hosted ingest | Flow-approved immutable sanitized archive plus its exact manifest | Yes — Flow review, approval, and hash verification | +| S3 / R2 / MinIO upload | Reviewed sanitized derivative archive | Yes — must be `reviewed` | +| Model or annotation service | Reviewed sanitized derivative content | Yes — must be `reviewed` | +| Peer-to-peer sharing | Reviewed sanitized derivative archive | Yes — must be `reviewed` | | Error reporting / telemetry | Could contain screenshot fragments | Yes — stripped of all capture data | **Implementation**: A single `check_egress_allowed(capture_id) -> bool` function that every outbound code path calls. If the recording is in `captured` or `scrubbed` state, the function raises `EgressBlockedError` with a user-facing message: "This recording hasn't been reviewed yet. Open the review panel to approve it for sharing." @@ -423,9 +419,9 @@ The tray icon can show a badge count of pending reviews. Periodic reminders (con 2. **User opens review panel** (from tray menu, capture browser, or reminder notification) 3. **User chooses action**: - **"Run Scrubbing"** → scrub worker creates parallel scrubbed copy → state becomes `scrubbed` → review UI shows before/after diff → user approves → state becomes `reviewed` - - **"Dismiss (skip scrubbing)"** → warning: "Your raw recordings may contain passwords, personal information, or sensitive data. They will be uploadable as-is." → user confirms → state becomes `dismissed` + - **"Dismiss (skip scrubbing)"** → warning: "Your raw recording remains local and cannot be uploaded." → user confirms → state becomes `dismissed` - **"Delete"** → recording deleted from disk → state becomes `deleted` -4. **Once `reviewed` or `dismissed`** → recording is cleared for any egress path (upload, VLM annotation, sharing, etc.) +4. **Once `reviewed`** → only the reviewed scrubbed copy is eligible for an approved egress path. This means: - The raw capture is never modified @@ -496,9 +492,11 @@ The `scrub_manifest.json` enables the review UI to show exactly what changed: } ``` -#### 5.5 Scrubbing is Optional +#### 5.5 Scrubbing Is Required for Egress -Scrubbing is **recommended but not mandatory**. Users who are uploading their own personal recordings and don't care about PII can skip the scrub step entirely — they just confirm they've reviewed the raw data and consent to upload it as-is. The consent dialog makes this explicit (see Section 8). +A user can dismiss a local review without scrubbing. That choice keeps the raw +recording local. Every egress path requires a separate scrubbed derivative and +an explicit review of that derivative. --- @@ -613,7 +611,7 @@ class StorageBackend(Protocol): def estimate_cost(self, size_bytes: int) -> float | None: ... ``` -All backends share the same upload pipeline: +Customer-owned storage adapters share the legacy upload pipeline: ``` [User approves in review UI] → Compress (tar.zst) → Queue → Upload Worker @@ -625,6 +623,12 @@ All backends share the same upload pipeline: - Wormhole: P2P direct ``` +Hosted ingest is not one of these generic adapters. `push` delegates to Flow, +which inventories and sanitizes every file, pauses for review, freezes the +approved exact bytes, and sends the archive with its sanitization manifest. +The old `HostedIngestBackend` remains importable for compatibility but refuses +direct uploads. `upload --backend hosted_ingest` routes to governed `push`. + ### 7.4 Recommended Backend Combinations | Profile | Backends | Target User | @@ -832,11 +836,8 @@ You are clearing [N] recording sessions ([X.X] GB) for sharing with external services. This data includes screenshots of your desktop and records -of your mouse and keyboard actions. [If scrubbing was applied: -"PII scrubbing was applied — review the highlighted regions -above to verify nothing sensitive remains." / If dismissed: -"PII scrubbing was not applied. The raw recordings will be -shared as-is."] +of your mouse and keyboard actions. PII scrubbing was applied. +Review the highlighted regions above to verify nothing sensitive remains. Once cleared, this data may be sent to: @@ -866,7 +867,7 @@ Once cleared, this data may be sent to: this data will also be open-source By clicking "Clear for Sharing", you confirm: - 1. You have reviewed the [scrubbed/raw] recordings above + 1. You have reviewed the scrubbed derivative above 2. You consent to this data being sent to the services listed above 3. [If any public destination: "You understand this data @@ -890,20 +891,21 @@ Key design choices: ### 8.4 Dismiss Flow (Skip Scrubbing) -Users CAN skip scrubbing entirely via the "Dismiss" action in the review panel. This still shows the full consent dialog: +Users can skip scrubbing through the "Dismiss" action. This action does not +grant consent and does not enable egress: 1. User clicks "Dismiss (skip scrubbing)" on a pending recording -2. Warning dialog: "Your recordings will be shared WITHOUT PII removal. Screenshots may contain passwords, personal information, or sensitive data visible on your screen." -3. The consent dialog (8.3) is shown with the full list of configured destinations, with the note "PII scrubbing was not applied. The raw recordings will be shared as-is." -4. User must explicitly confirm → state becomes `dismissed` → recording is cleared for egress +2. Warning dialog: "Your raw recording remains local and cannot be uploaded." +3. User confirms the local dismissal. +4. The state becomes `dismissed`, and all egress checks continue to refuse it. ### 8.5 Batch Operations For users with many accumulated pending recordings: - **"Review All"**: Opens a batch review panel. User can scrub all, review summary of redactions across all recordings, and clear them in one action. -- **"Dismiss All"**: Shows the warning + consent dialog once, covering all pending recordings. Good for users who don't care about PII (e.g., recording on a dedicated test machine). -- **Per-app policies** (future): "Auto-dismiss recordings from [app name]" — for users who know certain apps never show PII. Requires explicit opt-in per app. +- **"Dismiss All"**: Keeps all selected raw recordings local and blocked from egress. +- **Per-app policies**: A policy can automate a sanitization step. It cannot mark raw recordings as uploadable. --- diff --git a/README.md b/README.md index a622dcc..6539866 100644 --- a/README.md +++ b/README.md @@ -221,9 +221,9 @@ pinned sources, hashes, and modification status are recorded in each surface may then truthfully claim) is in [docs/CODE_SIGNING.md](docs/CODE_SIGNING.md). -Do not treat the legacy `upload` command or optional upload extras as the -supported hosted path; they predate the current workflow-bundle and -break-report contract. +The legacy `upload --backend hosted_ingest` command is a compatibility alias +for the supported governed `push` path. It does not call the old direct ingest +adapter. Optional customer-owned storage adapters remain separate legacy paths. ## Architecture @@ -281,11 +281,11 @@ The Python engine exposes these Beta commands: | `openadapt-desktop record` | Capture a local session | | `openadapt-desktop list` / `info` | Inspect capture metadata | | `openadapt-desktop scrub` | Run configured PII scrubbing | -| `openadapt-desktop review` / `approve` / `dismiss` | Operate the local review state machine | +| `openadapt-desktop review` / `approve` / `dismiss` | Operate the local review state machine; dismissal keeps raw data local | | `openadapt-desktop compile` / `replay` / `run` | Invoke the bundled, pinned `openadapt-flow` runtime on a capture or bundle | | `openadapt-desktop login` / `push` / `report-break` | Authenticate to the hosted control plane, push a bundle, report a halted run | | `openadapt-desktop storage` / `health` / `cleanup` | Inspect and maintain local storage | -| `openadapt-desktop backends` / `upload` | Inspect or invoke legacy upload adapters | +| `openadapt-desktop backends` / `upload` | Inspect legacy customer-owned storage adapters; the hosted alias uses governed `push` | | `openadapt-desktop config` / `doctor` | Inspect local configuration and dependencies | Raw recordings are local by default. Any egress path still requires careful @@ -293,6 +293,14 @@ review of the selected adapter, configuration, logs, and data-classification policy. This repository does not by itself establish a HIPAA-compliant or production-safe deployment. +The supported `push` command delegates to Flow's exact-hash sanitized +derivative contract. It distinguishes a local review pause from an upload. It +requires a returned hosted workflow identity before it reports success. It +never falls back to a direct Desktop upload when Flow is missing or returns an +error. The former direct hosted-ingest backend now refuses every upload. The +legacy customer-owned adapter queue selects the reviewed scrubbed path again +immediately before egress; a dismissed raw capture is not uploadable. + ## Development Prerequisites are Python 3.11+ and [`uv`](https://docs.astral.sh/uv/). The main @@ -316,7 +324,7 @@ current public product boundary. | [`openadapt-flow`](https://github.com/OpenAdaptAI/openadapt-flow) | Canonical workflow compiler, runtime, certification, and governed repair engine | | [`OpenAdapt`](https://github.com/OpenAdaptAI/OpenAdapt) | Flagship launcher and meta-repository | | [`openadapt-tray`](https://github.com/OpenAdaptAI/openadapt-tray) | Experimental system-tray status and launcher companion for this cockpit | -| [`openadapt-capture`](https://github.com/OpenAdaptAI/openadapt-capture) | Experimental capture component used by this Python engine | +| [`openadapt-capture`](https://github.com/OpenAdaptAI/openadapt-capture) | Beta canonical native screen, mouse, keyboard, timing, window-scoping, and media-capture component | | [`openadapt-privacy`](https://github.com/OpenAdaptAI/openadapt-privacy) | Experimental PII detection and redaction component | Documentation for the wider stack lives at diff --git a/engine/backends/__init__.py b/engine/backends/__init__.py index 916f732..6d61711 100644 --- a/engine/backends/__init__.py +++ b/engine/backends/__init__.py @@ -5,6 +5,6 @@ runtime configuration. Available backends: - hosted_ingest Hosted control plane (POST /api/ingest, bearer token) + hosted_ingest Deprecated compatibility adapter; direct upload is refused s3 S3-compatible (AWS S3, Cloudflare R2, MinIO) -- optional BYOC storage """ diff --git a/engine/backends/hosted_ingest.py b/engine/backends/hosted_ingest.py index 33f5894..30b62a3 100644 --- a/engine/backends/hosted_ingest.py +++ b/engine/backends/hosted_ingest.py @@ -1,30 +1,16 @@ -"""HostedIngestBackend -- pushes a zipped recording/bundle to POST /api/ingest. +"""Deprecated direct hosted-ingest adapter. -This is the cloud-lane egress sink: a :class:`~engine.backends.protocol.StorageBackend` -that uploads a ``.zip`` (a recording directory OR a compiled bundle) to the -hosted control plane as ``multipart/form-data`` with a bearer ingest token -resolved through :func:`engine.auth.store.auth_header`. - -Contract (spec section 3b, cloud PR #19 ``docs/INGEST.md``): - - POST {host}/api/ingest - Content-Type: multipart/form-data - Authorization: Bearer - fields: file (required .zip), kind ("recording"|"bundle"), name (optional) - -> 201 { "ingest": { workflow_id, workflow_name, kind, compile{...}, auth } } - errors: 401 auth · 400 bad request · 502 store/compile failure - -The archive is expected to already be a ``.zip`` -- :mod:`engine.hosted` zips -the recording/bundle directory before enqueuing. +The former adapter accepted any ZIP file and sent it to ``POST /api/ingest``. +It could not prove that Flow had inventoried, sanitized, reviewed, and frozen +the exact bytes. The adapter remains importable for protocol compatibility, +but it fails closed. Use :func:`engine.hosted.push`, which delegates to Flow's +approved sanitized-derivative contract. """ from __future__ import annotations from pathlib import Path -import httpx -from loguru import logger - from engine.auth.store import DEFAULT_HOST, auth_header from engine.backends.protocol import UploadRecord, UploadResult @@ -48,58 +34,21 @@ def __init__(self, host: str = DEFAULT_HOST, timeout: float = 120.0) -> None: self._timeout = timeout def upload(self, archive_path: Path, metadata: dict) -> UploadResult: - """POST a ``.zip`` archive to ``/api/ingest``. + """Refuse the obsolete direct upload path without making a request. Args: archive_path: Path to the ``.zip`` (recording dir or bundle dir). metadata: May carry ``kind`` ("recording"|"bundle") and ``name``. Returns: - UploadResult with the resulting workflow_id/dashboard URL on success. + A failed result that identifies the governed replacement path. """ - headers = auth_header() - if "Authorization" not in headers: - return UploadResult(success=False, error="Not logged in (no ingest token).") - - if not archive_path.exists(): - return UploadResult(success=False, error=f"Archive not found: {archive_path}") - - kind = metadata.get("kind", "recording") - data = {"kind": kind} - name = metadata.get("name") - if name: - data["name"] = name - - url = f"{self.host}{INGEST_PATH}" - try: - with open(archive_path, "rb") as fh: - files = {"file": (archive_path.name, fh, "application/zip")} - resp = httpx.post( - url, headers=headers, data=data, files=files, timeout=self._timeout - ) - except httpx.HTTPError as exc: - return UploadResult(success=False, error=f"Ingest request failed: {exc}") - - if resp.status_code == 401: - return UploadResult(success=False, error="Ingest token was rejected (401).") - if resp.status_code >= 400: - return UploadResult( - success=False, error=f"Ingest failed ({resp.status_code}): {resp.text[:200]}" - ) - - try: - body = resp.json() - except ValueError: - body = {} - ingest = body.get("ingest", {}) - workflow_id = ingest.get("workflow_id", "") - remote_url = f"{self.host}/dashboard/workflows/{workflow_id}" if workflow_id else "" - logger.info("Pushed {kind} to hosted ingest: workflow {wid}", kind=kind, wid=workflow_id) return UploadResult( - success=True, - remote_url=remote_url, - bytes_sent=archive_path.stat().st_size, - metadata=ingest, + success=False, + error=( + "Direct hosted ingest is disabled. Use `openadapt-desktop push` so Flow " + "can inventory, sanitize, review, approve, and freeze the exact artifact." + ), ) def delete(self, recording_id: str) -> bool: diff --git a/engine/cli.py b/engine/cli.py index 8028f6e..23e9fe9 100644 --- a/engine/cli.py +++ b/engine/cli.py @@ -66,24 +66,24 @@ def _init_engine(config: EngineConfig) -> types.SimpleNamespace: def _create_backends(config: EngineConfig) -> list: """Create backend instances based on config. - The hosted ingest backend (POST /api/ingest, bearer token) is always - registered -- it is the default cloud-lane sink. S3 is optional BYOC - customer-owned storage. + S3 is an optional customer-owned storage adapter. Hosted ingest is not a + generic storage backend: the ``push`` command routes it through Flow's + reviewed, exact-hash artifact contract. """ - from engine.backends.hosted_ingest import HostedIngestBackend - - backends = [HostedIngestBackend(host=config.hosted_host)] + backends = [] if config.s3_bucket: from engine.backends.s3 import S3Backend - backends.append(S3Backend( - bucket=config.s3_bucket, - region=config.s3_region, - access_key_id=config.s3_access_key_id, - secret_access_key=config.s3_secret_access_key, - endpoint=config.s3_endpoint, - )) + backends.append( + S3Backend( + bucket=config.s3_bucket, + region=config.s3_region, + access_key_id=config.s3_access_key_id, + secret_access_key=config.s3_secret_access_key, + endpoint=config.s3_endpoint, + ) + ) return backends @@ -138,10 +138,7 @@ def cmd_list(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: dur = f"{c.get('duration_secs', 0):.0f}s" if c.get("duration_secs") else "..." size = _format_bytes(c.get("size_bytes", 0)) started = c.get("started_at", "")[:19] - print( - f"{c['capture_id']:<12} {started:<22} {dur:<10} " - f"{c['review_status']:<12} {size:<10}" - ) + print(f"{c['capture_id']:<12} {started:<22} {dur:<10} {c['review_status']:<12} {size:<10}") def cmd_info(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: @@ -216,7 +213,7 @@ def cmd_approve(args: argparse.Namespace, engine: types.SimpleNamespace) -> None def cmd_dismiss(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: - """Dismiss scrubbing, accept PII risks.""" + """Dismiss scrubbing while keeping the raw capture local.""" from engine.review import ReviewStatus, transition_status transition_status( @@ -226,11 +223,26 @@ def cmd_dismiss(args: argparse.Namespace, engine: types.SimpleNamespace) -> None db=engine.db, audit=engine.audit, ) - print(f"Dismissed (raw data cleared for egress): {args.capture_id}") + print(f"Dismissed (raw data remains local and blocked from egress): {args.capture_id}") def cmd_upload(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: """Upload a capture to a backend.""" + if args.backend == "hosted_ingest": + capture = engine.db.get_capture(args.capture_id) + if capture is None: + print(f"Capture not found: {args.capture_id}") + sys.exit(1) + governed_args = types.SimpleNamespace( + path=capture["capture_path"], + kind="recording", + name=None, + host=None, + token=None, + ) + cmd_push(governed_args, engine) + return + from engine.upload_manager import UploadManager backends = _create_backends(engine.config) @@ -300,7 +312,7 @@ def cmd_rotate(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: def cmd_push(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: - """Zip a recording/bundle directory and push it to /api/ingest.""" + """Use Flow to review and push an exact sanitized artifact.""" from engine import hosted host = getattr(args, "host", None) or engine.config.hosted_host @@ -319,7 +331,16 @@ def cmd_push(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: print(f"Nothing to push: {exc}") sys.exit(1) - if result["success"]: + if result.get("pending_review"): + print(f"Sanitized derivative created: {result['sanitized_path']}") + print("Upload paused. Review and approve the derivative, then push that path.") + print(result["review_command"]) + engine.audit.log( + "hosted_push_paused_for_review", + kind=args.kind, + sanitized_path=result["sanitized_path"], + ) + elif result["success"]: print(f"Pushed. Workflow: {result['workflow_id']}") if result["dashboard_url"]: print(f" {result['dashboard_url']}") @@ -401,10 +422,8 @@ def cmd_report_break(args: argparse.Namespace, engine: types.SimpleNamespace) -> def cmd_backends(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: """List available backends.""" + print(" hosted_ingest: governed Flow push (local review and exact-hash approval required)") backends = _create_backends(engine.config) - if not backends: - print("No backends configured.") - return for b in backends: print(f" {b.name}: credentials={'valid' if b.verify_credentials() else 'invalid'}") @@ -461,8 +480,7 @@ def cmd_capabilities(args: argparse.Namespace, engine: types.SimpleNamespace) -> host = report["host"] print("Execution surface availability") - print(f" Host: {host['os']} {host['os_version']} ({host['arch']}), " - f"app v{host['app_version']}") + print(f" Host: {host['os']} {host['os_version']} ({host['arch']}), app v{host['app_version']}") print("=" * 72) for surface, cap in report["surfaces"].items(): driver = cap.get("driver") or {} @@ -648,20 +666,24 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: activate_provisioned_vision_runtime() try: import openadapt_capture + ver = getattr(openadapt_capture, "__version__", "installed") ok, detail = capture_contract_status(ver) checks.append(("openadapt-capture", ok, detail)) except ImportError as exc: - checks.append(( - "openadapt-capture", - False, - f"unusable: {exc} (run a record or replay once to provision the " - "local vision runtime, or pip install openadapt-capture)", - )) + checks.append( + ( + "openadapt-capture", + False, + f"unusable: {exc} (run a record or replay once to provision the " + "local vision runtime, or pip install openadapt-capture)", + ) + ) # openadapt-privacy try: import openadapt_privacy + ver = getattr(openadapt_privacy, "__version__", "installed") checks.append(("openadapt-privacy", True, ver)) except ImportError: @@ -670,6 +692,7 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: # psutil try: import psutil + checks.append(("psutil", True, psutil.__version__)) except ImportError: checks.append(("psutil", False, "not installed (health monitoring disabled)")) @@ -677,6 +700,7 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: # httpx (hosted ingest / auth) try: import httpx + checks.append(("httpx (hosted ingest)", True, httpx.__version__)) except ImportError: checks.append(("httpx (hosted ingest)", False, "not installed")) @@ -684,27 +708,38 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: # keyring (credential store) try: import keyring - checks.append(("keyring (credential store)", True, - getattr(keyring, "__version__", "installed"))) + + checks.append( + ("keyring (credential store)", True, getattr(keyring, "__version__", "installed")) + ) except ImportError: checks.append(("keyring (credential store)", False, "not installed")) # openadapt-flow (the loop engine) from engine.flow_bridge import flow_available, flow_runtime_source + flow_ready = flow_available() - checks.append(( - "openadapt-flow (loop engine)", - flow_ready, - flow_runtime_source() if flow_ready else "not found (pip install openadapt-flow)", - )) + checks.append( + ( + "openadapt-flow (loop engine)", + flow_ready, + flow_runtime_source() if flow_ready else "not found (pip install openadapt-flow)", + ) + ) # boto3 (optional BYOC storage) try: import boto3 + checks.append(("boto3 (S3 backend)", True, boto3.__version__)) except ImportError: - checks.append(("boto3 (S3 backend)", False, - "not installed (pip install openadapt-desktop[enterprise])")) + checks.append( + ( + "boto3 (S3 backend)", + False, + "not installed (pip install openadapt-desktop[enterprise])", + ) + ) # Hosted control plane checks.append(("Hosted host", True, engine.config.hosted_host)) @@ -712,9 +747,15 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: # Hosted credential from engine.auth.store import auth_header + logged_in = "Authorization" in auth_header() - checks.append(("Hosted credential", logged_in, - "present" if logged_in else "not logged in (run 'openadapt login')")) + checks.append( + ( + "Hosted credential", + logged_in, + "present" if logged_in else "not logged in (run 'openadapt login')", + ) + ) # S3 credentials (if configured) if engine.config.s3_bucket: @@ -811,8 +852,9 @@ def main(argv: list[str] | None = None) -> None: # login p = subparsers.add_parser("login", help="Authenticate to the hosted control plane") p.add_argument("--host", default=None, help="Hosted base URL") - p.add_argument("--provider", default=None, choices=["paste", "browser_pkce"], - help="Force an auth provider") + p.add_argument( + "--provider", default=None, choices=["paste", "browser_pkce"], help="Force an auth provider" + ) # credential lifetime / rotation p = subparsers.add_parser("credential", help="Show Cloud credential lifetime") @@ -824,9 +866,7 @@ def main(argv: list[str] | None = None) -> None: p = subparsers.add_parser("push", help="Push a recording/bundle to /api/ingest") p.add_argument("path", nargs="?", default=None, help="Recording/bundle dir (default: latest)") p.add_argument("--kind", default="recording", choices=["recording", "bundle"]) - p.add_argument("--name", default=None, help="Workflow name") p.add_argument("--host", default=None, help="Hosted base URL") - p.add_argument("--token", default=None, help="Ingest token (else keychain/env)") # compile p = subparsers.add_parser("compile", help="Compile a recording into a flow bundle") @@ -850,7 +890,6 @@ def main(argv: list[str] | None = None) -> None: p.add_argument("run_dir", help="Run directory containing report.json") p.add_argument("--workflow-id", dest="workflow_id", default=None, help="Hosted workflow id") p.add_argument("--host", default=None, help="Hosted base URL") - p.add_argument("--token", default=None, help="Ingest token (else keychain/env)") # backends subparsers.add_parser("backends", help="List available backends") diff --git a/engine/db.py b/engine/db.py index 0f11426..6506f0d 100644 --- a/engine/db.py +++ b/engine/db.py @@ -244,16 +244,33 @@ def delete_capture(self, capture_id: str) -> None: # --- Upload job operations --- def insert_upload_job( - self, job_id: str, capture_id: str, backend_name: str + self, + job_id: str, + capture_id: str, + backend_name: str, + *, + archive_path: str | None = None, ) -> None: """Create a new upload job in 'pending' status.""" now = _now() self.conn.execute( - "INSERT INTO upload_jobs (job_id, capture_id, backend_name, created_at, completed_at)" - " VALUES (?, ?, ?, ?, ?)", - (job_id, capture_id, backend_name, now, now), + "INSERT INTO upload_jobs " + "(job_id, capture_id, backend_name, archive_path, created_at, completed_at)" + " VALUES (?, ?, ?, ?, ?, ?)", + (job_id, capture_id, backend_name, archive_path, now, None), + ) + self.conn.commit() + + def recover_interrupted_upload_jobs(self) -> int: + """Return crash-interrupted jobs to the pending queue.""" + + cursor = self.conn.execute( + "UPDATE upload_jobs SET status = 'pending', " + "error = 'Desktop stopped during the prior upload attempt' " + "WHERE status = 'in_progress'" ) self.conn.commit() + return int(cursor.rowcount) def get_pending_jobs(self) -> list[dict]: """Get all jobs in 'pending' status, ordered by created_at.""" diff --git a/engine/dispatch.py b/engine/dispatch.py index 2a72c21..7b67350 100644 --- a/engine/dispatch.py +++ b/engine/dispatch.py @@ -2587,7 +2587,7 @@ def deploy_qualification_workflow(self, **params: Any) -> dict: # ------------------------------------------------------- sync / push def push_workflow(self, **params: Any) -> dict: - """Push a compiled bundle to ``/api/ingest`` and mirror sync state.""" + """Start a governed push and preserve a required local-review pause.""" from engine import hosted workflow_id = params.get("workflow_id") @@ -2606,9 +2606,18 @@ def push_workflow(self, **params: Any) -> dict: except Exception as exc: self._emit_sync("offline") return {"ok": False, "error": str(exc), "workflow_id": ""} - self._emit_sync("synced" if result.get("success") else "offline") + if result.get("success"): + self._emit_sync("synced") + elif result.get("pending_review"): + self._emit_sync("paused") + else: + self._emit_sync("offline") return { "ok": bool(result.get("success")), + "pending_review": bool(result.get("pending_review")), + "delivery_uncertain": bool(result.get("delivery_uncertain")), + "sanitized_path": result.get("sanitized_path", ""), + "review_command": result.get("review_command", ""), "workflow_id": result.get("workflow_id", ""), "dashboard_url": result.get("dashboard_url", ""), "error": result.get("error", ""), diff --git a/engine/flow_bridge.py b/engine/flow_bridge.py index 8f958f1..0f010df 100644 --- a/engine/flow_bridge.py +++ b/engine/flow_bridge.py @@ -120,6 +120,7 @@ } ) _SHA256_RE = re.compile(r"^[a-f0-9]{64}$") +_INGEST_TOKEN_ENV = "OPENADAPT_INGEST_TOKEN" def _contract_counts(value: object) -> dict[str, int] | None: @@ -426,11 +427,22 @@ def _safe_command_for_log(cmd: list[str]) -> str: } safe: list[str] = [] redact_next = False - for value in cmd: + egress_verb_index = next( + (index for index, value in enumerate(cmd) if value in {"push", "report-break"}), + None, + ) + for index, value in enumerate(cmd): if redact_next: safe.append("[REDACTED]") redact_next = False continue + if ( + egress_verb_index is not None + and index == egress_verb_index + 1 + and not value.startswith("-") + ): + safe.append("[LOCAL_PATH]") + continue safe.append(value) redact_next = value in redacted_after return " ".join(safe) @@ -782,9 +794,43 @@ def push( args = ["push", str(path), "--kind", kind, "--host", host] if name: - args += ["--name", name] + # A Desktop task description can contain a record identity. Do not + # put it in argv or logs. Cloud can suggest a safe display name. + logger.warning("Desktop omitted a local workflow name from the Flow command") + child_env = dict(env_overrides or {}) if token: - args += ["--token", token] + child_env[_INGEST_TOKEN_ENV] = token + return self._run(args, timeout=timeout, env_overrides=child_env or None) + + def report_break( + self, + run_dir: Path, + *, + workflow_id: str, + host: str, + deployment_kind: str = "cloud", + org_id: str | None = None, + timeout: float | None = None, + env_overrides: dict[str, str] | None = None, + ) -> FlowResult: + """Send Flow's closed-schema break summary. + + The bearer token belongs in ``env_overrides``. It must not appear in + the child process argument list. + """ + + args = [ + "report-break", + str(run_dir), + "--workflow-id", + workflow_id, + "--host", + host, + "--deployment-kind", + deployment_kind, + ] + if org_id: + args += ["--org-id", org_id] return self._run(args, timeout=timeout, env_overrides=env_overrides) def teach( diff --git a/engine/hosted.py b/engine/hosted.py index 59685dd..555b2c9 100644 --- a/engine/hosted.py +++ b/engine/hosted.py @@ -1,37 +1,31 @@ """hosted.py -- the cloud-lane egress verbs: ``push`` and ``report_break``. -``push`` zips a flow recording (or compiled bundle) directory and uploads it to -``POST /api/ingest`` (spec section 3b). ``report_break`` reads a local run's -``report.json`` and posts a PHI-free break descriptor to -``POST /api/runs/ingest-report`` (spec section 3c) so a BYOC halt is triageable -centrally without any PHI leaving the machine. +``push`` delegates to Flow's sanitized-derivative upload contract. It never +constructs or uploads an archive from raw Desktop data. ``report_break`` also +delegates to Flow. Flow validates ``report.json`` and sends only its +closed-schema, PHI-minimal summary. Credentials come exclusively from :mod:`engine.auth` (``auth_header()``); this -module never implements login. If the ``openadapt-flow`` CLI grows ``push`` -(workstream W4), :func:`push` prefers delegating to it; otherwise it runs -in-tree against the identical contract. +module never implements login. :func:`push` delegates to the pinned Flow +runtime and fails closed when that command is unavailable. """ from __future__ import annotations import os +import re import tempfile import zipfile from pathlib import Path from typing import Any +from uuid import UUID -import httpx from loguru import logger -from engine.auth.store import DEFAULT_HOST, active_credential, auth_header -from engine.backends.hosted_ingest import HostedIngestBackend +from engine.auth.store import DEFAULT_HOST, INGEST_TOKEN_ENV, active_credential from engine.flow_bridge import FlowBridge -INGEST_REPORT_PATH = "/api/runs/ingest-report" - -# Keys that MUST NOT appear in a break descriptor on any lane (fail-closed; -# server returns 422 if they leak). We strip them client-side too. -_PHI_FORBIDDEN_KEYS = frozenset({"field_values", "report_body", "dom"}) +_MAX_FLOW_ERROR_CHARS = 500 class PhiBoundaryError(RuntimeError): @@ -49,6 +43,13 @@ def zip_dir(src_dir: Path, dest: Path | None = None) -> Path: Path to the created ``.zip``. """ src_dir = Path(src_dir) + if src_dir.is_symlink() or not src_dir.is_dir(): + raise ValueError("Archive source must be a real directory, not a symlink.") + members = sorted(src_dir.rglob("*")) + symlink = next((path for path in members if path.is_symlink()), None) + if symlink is not None: + raise ValueError(f"Archive source contains a symlink: {symlink.relative_to(src_dir)}") + temporary = dest is None if dest is None: fd, tmp = tempfile.mkstemp(suffix=".zip", prefix=f"{src_dir.name}_") # Close the handle mkstemp opened before touching the path -- on Windows @@ -56,10 +57,19 @@ def zip_dir(src_dir: Path, dest: Path | None = None) -> Path: os.close(fd) Path(tmp).unlink(missing_ok=True) dest = Path(tmp) - with zipfile.ZipFile(dest, "w", zipfile.ZIP_DEFLATED) as zf: - for path in sorted(src_dir.rglob("*")): - if path.is_file(): - zf.write(path, path.relative_to(src_dir)) + try: + with zipfile.ZipFile(dest, "w", zipfile.ZIP_DEFLATED) as zf: + for path in members: + if path.is_symlink(): + raise ValueError( + f"Archive source contains a symlink: {path.relative_to(src_dir)}" + ) + if path.is_file(): + zf.write(path, path.relative_to(src_dir)) + except Exception: + if temporary: + dest.unlink(missing_ok=True) + raise return dest @@ -81,12 +91,12 @@ def push( host: str = DEFAULT_HOST, token: str | None = None, recordings_dir: Path | None = None, - backend: HostedIngestBackend | None = None, + backend: Any = None, prefer_flow: bool = True, db: Any = None, bundle_id: str | None = None, ) -> dict[str, Any]: - """Zip a recording/bundle directory and push it to ``/api/ingest``. + """Push through Flow's approved sanitized-derivative contract. Signature mirrors ``openadapt_flow.hosted.push(path, kind, name, host, token)`` (flow PR #119) so the two are swappable. On success the returned hosted @@ -102,8 +112,8 @@ def push( host: Hosted base URL. token: Explicit ingest token (else resolved from the auth store/env). recordings_dir: Where to look for the default recording. - backend: Injected backend (tests); defaults to a real HostedIngestBackend. - prefer_flow: Delegate to ``openadapt-flow push`` when that CLI supports it. + backend: Deprecated direct backend injection. Supplying it fails closed. + prefer_flow: Deprecated bypass. Setting it false fails closed. db: Optional :class:`~engine.db.IndexDB` to persist the workflow_id into. bundle_id: Local bundle id to map to the returned hosted workflow_id. @@ -123,23 +133,32 @@ def push( if not path.exists(): raise FileNotFoundError(f"Nothing to push at {path}.") - if prefer_flow and _flow_supports_push(): + if backend is not None or not prefer_flow: + return { + "success": False, + "workflow_id": "", + "dashboard_url": "", + "error": ( + "Direct Desktop ingest is disabled. Use the pinned Flow push command so " + "only an approved, exact-hash sanitized derivative can leave the machine." + ), + } + try: result_dict = _push_via_flow(path, kind=kind, name=name, host=host, token=token) - else: - backend = backend or HostedIngestBackend(host=host) - zip_path = zip_dir(path) - try: - metadata: dict[str, Any] = {"kind": kind, "capture_id": path.name} - if name: - metadata["name"] = name - result = backend.upload(zip_path, metadata) - finally: - zip_path.unlink(missing_ok=True) - result_dict = { - "success": result.success, - "workflow_id": result.metadata.get("workflow_id", "") if result.success else "", - "dashboard_url": result.remote_url, - "error": result.error, + except Exception as exc: + # A launch or transport failure must never select a raw upload + # fallback. The exception can occur after Flow dispatched a request, + # so Desktop must not claim that no bytes crossed the boundary. + logger.warning("Flow push did not return a confirmed outcome: {e}", e=exc) + return { + "success": False, + "delivery_uncertain": True, + "workflow_id": "", + "dashboard_url": "", + "error": ( + "Flow did not return a confirmed upload outcome. Do not retry blindly; " + "reconcile the exact artifact in Cloud first." + ), } # Persist the hosted workflow_id so report_break can reference it later. @@ -153,86 +172,105 @@ def push( try: db.update_bundle(bundle_id, workflow_id=result_dict["workflow_id"]) except Exception as exc: # non-fatal -- push already succeeded - logger.warning("Could not persist workflow_id to bundle {bid}: {e}", - bid=bundle_id, e=exc) + logger.warning( + "Could not persist workflow_id to bundle {bid}: {e}", bid=bundle_id, e=exc + ) return result_dict -def _flow_supports_push(flow_bin: str = "openadapt-flow") -> bool: - """Best-effort check whether the flow CLI exposes a ``push`` subcommand.""" - return FlowBridge(flow_bin=flow_bin).supports_command("push") - - def _push_via_flow( path: Path, *, kind: str, name: str | None, host: str, token: str | None = None ) -> dict[str, Any]: - """Delegate to ``openadapt-flow push`` (flow PR #119); parse its workflow id.""" + """Delegate to Flow and preserve upload versus local-review outcomes.""" logger.info("Delegating push to openadapt-flow") - result = FlowBridge().push(path, kind=kind, name=name, host=host, token=token) - workflow_id = "" - for token in (result.stdout or "").split(): - if token.startswith("wf_") or token.startswith("workflow_"): - workflow_id = token - break + resolved_token = _token_for_host(host, explicit=token) + env = {INGEST_TOKEN_ENV: resolved_token} if resolved_token else None + result = FlowBridge().push( + path, + kind=kind, + name=name, + host=host, + token=None, + env_overrides=env, + ) + stdout = result.stdout or "" + if result.ok and "Upload paused for local review" in stdout: + derivative_match = re.search( + r"^Sanitized derivative created at (.+)\.$", stdout, re.MULTILINE + ) + review_command = next( + ( + line + for line in stdout.splitlines() + if line.startswith("openadapt-flow review-sanitized ") + ), + "", + ) + if derivative_match is None or not review_command: + return { + "success": False, + "pending_review": False, + "workflow_id": "", + "dashboard_url": "", + "error": "Flow paused, but Desktop could not verify the review handoff.", + } + return { + "success": False, + "pending_review": True, + "sanitized_path": derivative_match.group(1), + "review_command": review_command, + "workflow_id": "", + "dashboard_url": "", + "error": "", + } + + workflow_match = re.search(r"\bworkflow_id=([^\s,\)]+)", stdout) + workflow_id = workflow_match.group(1) if workflow_match else "" + try: + workflow_id = str(UUID(workflow_id)) + except (ValueError, AttributeError): + workflow_id = "" + dashboard_match = re.search(r"^Dashboard:\s+(\S+)\s*$", stdout, re.MULTILINE) + dashboard_url = dashboard_match.group(1) if dashboard_match else "" + success = bool(result.ok and workflow_id) + error = ( + _bounded_flow_error(result.stderr or result.stdout, secret=resolved_token) + if not result.ok + else "" + ) + if result.ok and not workflow_id: + error = "Flow returned success without an authenticated hosted workflow identity." return { - "success": result.ok, + "success": success, + "pending_review": False, + "delivery_uncertain": not result.ok, "workflow_id": workflow_id, - "dashboard_url": f"{host.rstrip('/')}/dashboard/workflows/{workflow_id}" - if workflow_id - else "", - "error": result.stderr if not result.ok else "", + "dashboard_url": dashboard_url, + "error": error, } -def build_break_descriptor( - report: dict, - *, - workflow_id: str | None = None, - deployment_kind: str = "cloud", - org_id: str | None = None, - report_path: str | None = None, -) -> dict[str, Any]: - """Build a PHI-free break descriptor from a run's ``report.json``. +def _token_for_host(host: str, *, explicit: str | None = None) -> str: + """Resolve a Desktop credential without sending it to another origin.""" - Only whitelisted, PHI-free fields are included. Screenshots, field values, - DOM, and report bodies are never sent from here (spec section 3c). ``report`` - is expected to be the ``halt`` block or a halt-shaped report. + if explicit and explicit.strip(): + return explicit.strip() + env_token = os.environ.get(INGEST_TOKEN_ENV, "").strip() + if env_token: + return env_token + credential = active_credential() + if credential and str(credential.get("host", "")).rstrip("/") == host.rstrip("/"): + return str(credential.get("token") or "").strip() + return "" - Args: - report: The halt/report dict (from :meth:`FlowBridge.read_halt`). - workflow_id: The HOSTED workflow id (persisted at push time). A run's - ``report.json`` only carries ``workflow_name``, so this must be - supplied by the caller; it falls back to any id embedded in the report. - deployment_kind: ``"cloud"`` or ``"byoc"``. - org_id: The org the token resolves to (from the active credential). - report_path: A pointer to the local report (path string only, no body). - Returns: - The JSON-serializable descriptor. - """ - metrics = report.get("metrics", {}) or {} - descriptor: dict[str, Any] = { - "org_id": org_id, - "workflow_id": workflow_id or report.get("workflow_id"), - "deployment_kind": "byoc" if deployment_kind == "byoc" else "cloud", - "status": report.get("status", "halt"), - "step_intent": report.get("step_intent", ""), - "reason": report.get("reason", ""), - "resolver_rung": report.get("resolver_rung"), - "drift_signature": report.get("drift_signature"), - "metrics": { - "steps": metrics.get("steps", report.get("steps", 0)), - "duration_s": metrics.get("duration_s", report.get("duration_s", 0)), - }, - } - if report.get("error"): - descriptor["error"] = report["error"] - if report_path: - descriptor["report_path"] = report_path - # Defensive: never forward forbidden keys even if a report carries them. - for key in _PHI_FORBIDDEN_KEYS: - descriptor.pop(key, None) - return descriptor +def _bounded_flow_error(message: str, *, secret: str = "") -> str: + """Return one bounded CLI diagnostic without reflecting a bearer token.""" + + detail = (message or "").strip() + if secret: + detail = detail.replace(secret, "[REDACTED]") + return detail[:_MAX_FLOW_ERROR_CHARS] def report_break( @@ -246,86 +284,85 @@ def report_break( allow_local_fallback: bool = True, timeout: float = 30.0, ) -> dict[str, Any]: - """Post a PHI-free break descriptor for a halted run to ``/api/runs/ingest-report``. - - Signature mirrors ``openadapt_flow.hosted.report_break(run_dir, workflow_id, - host, token, deployment_kind, org_id, allow_local_fallback)`` (flow PR #119) - so the two are swappable. - - Args: - run_dir: The local run directory containing ``report.json``. - workflow_id: The HOSTED workflow id (persisted at push time). Required to - attribute the halt to the right hosted workflow -- ``report.json`` - only has ``workflow_name``. - host: Hosted base URL. - token: Explicit ingest token (else resolved from the auth store/env). - deployment_kind: ``"cloud"`` or ``"byoc"``. - org_id: Org override (else read from the active credential). - allow_local_fallback: On a 422 PHI-boundary rejection, return a - ``local_teach`` result instead of raising. - timeout: HTTP timeout in seconds. - - Returns: - Result dict with ``{"ok", "run_id", "halt_id", "status", "teach_url", "error"}``. - On a 422 with ``allow_local_fallback`` set, ``{"ok": False, "local_teach": True}``. - - Raises: - PhiBoundaryError: On a 422 fail-closed response when ``allow_local_fallback`` - is False -- the caller must fall back to LOCAL teach. - """ + """Delegate break reporting to Flow's closed-schema egress boundary.""" halt = FlowBridge.read_halt(run_dir) if halt is None: return {"ok": False, "error": "No halt found in report.json.", "run_id": None} + if not workflow_id: + return {"ok": False, "error": "A hosted workflow id is required.", "run_id": None} - headers = {"Authorization": f"Bearer {token}"} if token else auth_header() - if "Authorization" not in headers: + resolved_token = _token_for_host(host, explicit=token) + if not resolved_token: return {"ok": False, "error": "Not logged in (no ingest token).", "run_id": None} if org_id is None: cred = active_credential() - org_id = cred.get("org_id") if cred else None - report_path = str(Path(run_dir) / "report.json") - descriptor = build_break_descriptor( - halt, workflow_id=workflow_id, deployment_kind=deployment_kind, - org_id=org_id, report_path=report_path, - ) - - url = f"{host.rstrip('/')}{INGEST_REPORT_PATH}" + if cred and str(cred.get("host", "")).rstrip("/") == host.rstrip("/"): + org_id = cred.get("org_id") try: - resp = httpx.post(url, headers=headers, json=descriptor, timeout=timeout) - except httpx.HTTPError as exc: - return {"ok": False, "error": f"ingest-report request failed: {exc}", "run_id": None} - - if resp.status_code == 422: - if allow_local_fallback: - logger.warning("Break report rejected (422); falling back to local teach") - return { - "ok": False, - "local_teach": True, - "error": "PHI boundary violation (422); use local teach.", - "run_id": None, - } - raise PhiBoundaryError( - "Break report rejected as a PHI boundary violation (422); " - "fall back to local teach." + result = FlowBridge().report_break( + Path(run_dir), + workflow_id=workflow_id, + host=host, + deployment_kind=deployment_kind, + org_id=org_id, + timeout=timeout, + env_overrides={INGEST_TOKEN_ENV: resolved_token}, ) - if resp.status_code >= 400: + except Exception as exc: + logger.warning("Flow report-break did not return a confirmed outcome: {e}", e=exc) return { "ok": False, - "error": f"ingest-report failed ({resp.status_code}): {resp.text[:200]}", + "delivery_uncertain": True, + "error": ( + "Flow did not return a confirmed report outcome. Reconcile the run in " + "Cloud before another report attempt." + ), "run_id": None, } - - try: - body = resp.json() - except ValueError: - body = {} - logger.info("Reported break: run {run_id}", run_id=body.get("run_id")) + stdout = result.stdout or "" + if not result.ok: + return { + "ok": False, + "delivery_uncertain": True, + "error": _bounded_flow_error(result.stderr or stdout, secret=resolved_token) + or "Flow report-break failed.", + "run_id": None, + } + if stdout.startswith("Break kept LOCAL-ONLY:"): + if not allow_local_fallback: + raise PhiBoundaryError( + "Break report was kept local by Flow's PHI boundary; use local teach." + ) + return { + "ok": False, + "local_teach": True, + "error": stdout.partition(":")[2].strip(), + "run_id": None, + } + if stdout.startswith("Nothing emitted:"): + return { + "ok": False, + "error": stdout.partition(":")[2].strip() or "Flow emitted no break summary.", + "run_id": None, + } + match = re.search( + r"Break reported \(run_id=([^,]+), halt_id=([^,]+), status=([^\)]+)\)\.", + stdout, + ) + if match is None: + return { + "ok": False, + "error": "Flow reported success without a verified break identity.", + "run_id": None, + } + teach_match = re.search(r"^Teach:\s+(\S+)\s*$", stdout, re.MULTILINE) + logger.info("Reported break: run {run_id}", run_id=match.group(1)) return { - "ok": body.get("ok", True), - "run_id": body.get("run_id"), - "halt_id": body.get("halt_id"), - "status": body.get("status"), - "teach_url": body.get("teach_url"), + "ok": True, + "run_id": match.group(1), + "halt_id": match.group(2), + "status": match.group(3), + "teach_url": teach_match.group(1) if teach_match else None, "error": "", } diff --git a/engine/qualification_lifecycle.py b/engine/qualification_lifecycle.py index 954a23d..87e9da8 100644 --- a/engine/qualification_lifecycle.py +++ b/engine/qualification_lifecycle.py @@ -10,6 +10,7 @@ import zipfile from pathlib import Path from typing import Any +from uuid import UUID class QualificationLifecycleError(RuntimeError): @@ -267,7 +268,13 @@ def parse_flow_push(stdout: str, stderr: str, *, ok: bool) -> dict[str, Any]: """Project Flow's bounded push states without treating review as deployment.""" if not ok: - return {"ok": False, "deployed": False, "error": stderr or "Cloud deploy failed"} + detail = (stderr or stdout or "Cloud deploy failed").strip()[:500] + return { + "ok": False, + "deployed": False, + "delivery_uncertain": True, + "error": detail, + } if "Upload paused for local review" in stdout: sanitized_path = "" marker = "Sanitized derivative created at " @@ -288,6 +295,10 @@ def parse_flow_push(stdout: str, stderr: str, *, ok: bool) -> dict[str, Any]: workflow_id = line.split("workflow_id=", 1)[1].split()[0].rstrip(",).") if line.startswith("Dashboard: "): dashboard_url = line.removeprefix("Dashboard: ").strip() + try: + workflow_id = str(UUID(workflow_id)) + except (ValueError, AttributeError): + workflow_id = "" return { "ok": bool(workflow_id), "deployed": bool(workflow_id), diff --git a/engine/review.py b/engine/review.py index 501d097..a568e5c 100644 --- a/engine/review.py +++ b/engine/review.py @@ -29,13 +29,13 @@ | copy) | | +-----+-----+ | | | - v v + v +-------------------------+ - | CLEARED FOR EGRESS | <- Data can now be sent to: - | | storage backends, VLM APIs, - | | annotation pipelines, FL, etc. + | CLEARED FOR EGRESS | <- Only the reviewed scrubbed copy +-------------------------+ +``DISMISSED`` records a local choice only. It never permits raw-data egress. + All outbound data paths MUST call check_egress_allowed() before sending any data off-machine. This is the single enforcement point. """ @@ -43,6 +43,11 @@ from __future__ import annotations import enum +import hashlib +import json +import os +import re +from pathlib import Path from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -57,7 +62,7 @@ class ReviewStatus(enum.Enum): CAPTURED: Raw recording just created. Pending review. Blocked from all egress. SCRUBBED: Scrub pass completed, awaiting user review. Still blocked. REVIEWED: User reviewed scrubbed copy and approved. Scrubbed copy cleared for egress. - DISMISSED: User skipped scrubbing, accepted PII risks. Raw data cleared for egress. + DISMISSED: User skipped scrubbing. Raw data remains local and blocked from egress. DELETED: Recording deleted from disk. """ @@ -68,8 +73,9 @@ class ReviewStatus(enum.Enum): DELETED = "deleted" -# States that allow data to leave the machine. -EGRESS_ALLOWED_STATES = frozenset({ReviewStatus.REVIEWED, ReviewStatus.DISMISSED}) +# Only a reviewed sanitized derivative can leave the machine. `DISMISSED` +# remains a persisted legacy/local-review state, but it never grants egress. +EGRESS_ALLOWED_STATES = frozenset({ReviewStatus.REVIEWED}) # Valid state transitions. VALID_TRANSITIONS: dict[ReviewStatus, frozenset[ReviewStatus]] = { @@ -98,6 +104,151 @@ def __init__(self, capture_id: str, current_status: ReviewStatus) -> None: ) +class EgressArtifactError(Exception): + """Raised when the approved sanitized derivative is absent or unsafe.""" + + +_SHA256_RE = re.compile(r"^[a-f0-9]{64}$") + + +def load_derivative_approval(path: Path) -> str: + """Return the exact approved tree digest from a closed local schema.""" + + try: + review = json.loads((path / "review_status.json").read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise EgressArtifactError("The sanitized derivative has no valid approval.") from exc + if not isinstance(review, dict) or set(review) != {"status", "approved_tree_sha256"}: + raise EgressArtifactError("The sanitized derivative approval schema is invalid.") + approved_digest = review.get("approved_tree_sha256") + if review.get("status") != "reviewed" or not isinstance( + approved_digest, str + ) or not _SHA256_RE.fullmatch(approved_digest): + raise EgressArtifactError("The sanitized derivative has no exact approval.") + return approved_digest + + +def _stream_sha256(path: Path) -> str: + digest = hashlib.sha256() + with path.open("rb") as source: + while chunk := source.read(1024 * 1024): + digest.update(chunk) + return digest.hexdigest() + + +def derivative_tree_sha256(path: Path) -> str: + """Hash a derivative tree without loading recording media into memory.""" + + digest = hashlib.sha256() + members = [path, *sorted(path.rglob("*"))] if path.is_dir() else [path] + for member in members: + if member.is_symlink(): + raise EgressArtifactError("The sanitized derivative contains a symlink.") + relative = "." if member == path else member.relative_to(path).as_posix() + if relative == "review_status.json": + continue + digest.update(relative.encode("utf-8")) + if member.is_file(): + stat = os.stat(member, follow_symlinks=False) + if stat.st_nlink != 1: + raise EgressArtifactError( + "The sanitized derivative contains a hard-linked file." + ) + digest.update(_stream_sha256(member).encode("ascii")) + return digest.hexdigest() + + +def approved_egress_path(capture_id: str, db: IndexDB) -> Path: + """Return the exact reviewed derivative that an upload worker may send. + + The database status is not sufficient. This function also proves that the + selected path is the distinct scrubbed copy and contains no symlinks. The + upload worker calls it again immediately before network egress. + """ + + capture = db.get_capture(capture_id) + if capture is None: + raise ValueError(f"Unknown capture: {capture_id}") + status = ReviewStatus(capture["review_status"]) + if status not in EGRESS_ALLOWED_STATES: + raise EgressBlockedError(capture_id, status) + + raw_value = str(capture.get("capture_path") or "").strip() + scrubbed_value = str(capture.get("scrubbed_path") or "").strip() + if not scrubbed_value: + raise EgressArtifactError(f"Recording '{capture_id}' has no approved sanitized derivative.") + scrubbed_candidate = Path(scrubbed_value) + if scrubbed_candidate.is_symlink(): + raise EgressArtifactError( + f"Recording '{capture_id}' has a symlink as its sanitized derivative." + ) + try: + scrubbed = scrubbed_candidate.resolve(strict=True) + raw = Path(raw_value).resolve(strict=True) + except OSError as exc: + raise EgressArtifactError( + f"Recording '{capture_id}' has an unavailable sanitized derivative." + ) from exc + if scrubbed == raw: + raise EgressArtifactError( + f"Recording '{capture_id}' points its sanitized derivative at raw data." + ) + expected = raw.parent / f"{raw.name}.scrubbed" + if scrubbed != expected: + raise EgressArtifactError( + f"Recording '{capture_id}' does not use its canonical sanitized derivative." + ) + paths = [scrubbed, *scrubbed.rglob("*")] if scrubbed.is_dir() else [scrubbed] + if any(path.is_symlink() for path in paths): + raise EgressArtifactError( + f"Recording '{capture_id}' has a symlink in its sanitized derivative." + ) + linked_file = next( + ( + path + for path in paths + if path.is_file() and os.stat(path, follow_symlinks=False).st_nlink != 1 + ), + None, + ) + if linked_file is not None: + raise EgressArtifactError( + f"Recording '{capture_id}' has a hard-linked file in its sanitized derivative." + ) + if not scrubbed.is_dir(): + raise EgressArtifactError( + f"Recording '{capture_id}' has no approved sanitized derivative directory." + ) + documents: dict[str, dict] = {} + for name in ("scrub_manifest.json", "review_status.json"): + manifest_path = scrubbed / name + try: + value = json.loads(manifest_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise EgressArtifactError( + f"Recording '{capture_id}' has no valid {name}." + ) from exc + if not isinstance(value, dict): + raise EgressArtifactError( + f"Recording '{capture_id}' has no valid {name}." + ) + documents[name] = value + manifest = documents["scrub_manifest.json"] + if manifest.get("scrub_level") == "basic" and any( + path.is_file() for path in (scrubbed / "screenshots").rglob("*") + ): + raise EgressArtifactError( + f"Recording '{capture_id}' used basic scrubbing for screenshots. " + "Image-capable scrubbing is required before egress." + ) + approved_digest = load_derivative_approval(scrubbed) + if derivative_tree_sha256(scrubbed) != approved_digest: + raise EgressArtifactError( + f"Recording '{capture_id}' changed after its local review." + ) + return scrubbed + + def check_egress_allowed(capture_id: str, db: IndexDB) -> bool: """Check whether a capture is cleared for egress. @@ -114,18 +265,14 @@ def check_egress_allowed(capture_id: str, db: IndexDB) -> bool: db: The index database instance. Returns: - True if the capture is cleared for egress. + True if the capture has a reviewed sanitized derivative. Raises: - EgressBlockedError: If the capture is in captured or scrubbed state. + EgressBlockedError: If the capture is not in reviewed state. + EgressArtifactError: If the reviewed derivative is absent or unsafe. ValueError: If the capture does not exist. """ - capture = db.get_capture(capture_id) - if capture is None: - raise ValueError(f"Unknown capture: {capture_id}") - status = ReviewStatus(capture["review_status"]) - if status not in EGRESS_ALLOWED_STATES: - raise EgressBlockedError(capture_id, status) + approved_egress_path(capture_id, db) return True @@ -174,6 +321,43 @@ def transition_status( f"Status mismatch for '{capture_id}': " f"expected {from_status.value}, got {current.value}" ) + if from_status == ReviewStatus.SCRUBBED and to_status == ReviewStatus.REVIEWED: + scrubbed_value = str(capture.get("scrubbed_path") or "").strip() + if not scrubbed_value: + raise EgressArtifactError( + f"Recording '{capture_id}' has no sanitized derivative to approve." + ) + scrubbed = Path(scrubbed_value) + expected = Path(capture["capture_path"]).parent / ( + Path(capture["capture_path"]).name + ".scrubbed" + ) + if scrubbed.is_symlink() or scrubbed.resolve(strict=True) != expected.resolve(): + raise EgressArtifactError( + f"Recording '{capture_id}' does not use its canonical sanitized derivative." + ) + manifest_path = scrubbed / "scrub_manifest.json" + try: + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise EgressArtifactError( + f"Recording '{capture_id}' has no valid scrub_manifest.json." + ) from exc + if not isinstance(manifest, dict): + raise EgressArtifactError( + f"Recording '{capture_id}' has no valid scrub_manifest.json." + ) + approval_path = scrubbed / "review_status.json" + temporary = scrubbed / ".review_status.json.tmp" + approval = { + "status": "reviewed", + "approved_tree_sha256": derivative_tree_sha256(scrubbed), + } + temporary.write_text( + json.dumps(approval, sort_keys=True, separators=(",", ":")), + encoding="utf-8", + ) + os.chmod(temporary, 0o600) + temporary.replace(approval_path) db.update_capture(capture_id, review_status=to_status.value) if audit is not None: diff --git a/engine/scrubber.py b/engine/scrubber.py index eab878f..8192fd1 100644 --- a/engine/scrubber.py +++ b/engine/scrubber.py @@ -32,6 +32,7 @@ import json import re import shutil +import tempfile from datetime import datetime, timezone from pathlib import Path @@ -124,53 +125,88 @@ def scrub_capture(self, capture_path: Path) -> Path: self._require_provider() scrubbed_path = capture_path.parent / (capture_path.name + ".scrubbed") - scrubbed_path.mkdir(parents=True, exist_ok=True) - - all_redactions: list[dict] = [] - - # Scrub meta.json text fields - meta_path = capture_path / "meta.json" - if meta_path.exists(): - meta = json.loads(meta_path.read_text()) - for key in ("task_description",): - if key in meta and isinstance(meta[key], str): - scrubbed_text, redactions = self.scrub_text(meta[key]) - meta[key] = scrubbed_text - for r in redactions: - r["source"] = f"meta.json:{key}" - all_redactions.extend(redactions) - (scrubbed_path / "meta.json").write_text(json.dumps(meta, indent=2)) - - # Copy and scrub screenshots - screenshots_src = capture_path / "screenshots" - if screenshots_src.exists(): - screenshots_dst = scrubbed_path / "screenshots" - screenshots_dst.mkdir(exist_ok=True) - for img_path in sorted(screenshots_src.glob("*.png")): - output_path = screenshots_dst / img_path.name - img_redactions = self.scrub_image(img_path, output_path) - for r in img_redactions: - r["source"] = f"screenshots/{img_path.name}" - all_redactions.extend(img_redactions) - - # Write scrub manifest - manifest = { - "scrub_level": self.level.value, - "timestamp": datetime.now(timezone.utc).isoformat(), - "total_redactions": len(all_redactions), - "redactions": all_redactions, - } - (scrubbed_path / "scrub_manifest.json").write_text(json.dumps(manifest, indent=2)) - - # Write review status - review_status = { - "status": "pending_review", - "scrubbed_at": datetime.now(timezone.utc).isoformat(), - "scrub_level": self.level.value, - } - (scrubbed_path / "review_status.json").write_text(json.dumps(review_status, indent=2)) - - return scrubbed_path + if scrubbed_path.is_symlink(): + raise RuntimeError("The scrubbed derivative path cannot be a symlink") + staging_path = Path( + tempfile.mkdtemp( + prefix=f".{capture_path.name}.scrubbing-", + dir=capture_path.parent, + ) + ) + staging_path.chmod(0o700) + + try: + all_redactions: list[dict] = [] + + # Scrub meta.json text fields + meta_path = capture_path / "meta.json" + if meta_path.exists(): + meta = json.loads(meta_path.read_text()) + for key in ("task_description",): + if key in meta and isinstance(meta[key], str): + scrubbed_text, redactions = self.scrub_text(meta[key]) + meta[key] = scrubbed_text + for r in redactions: + r["source"] = f"meta.json:{key}" + all_redactions.extend(redactions) + (staging_path / "meta.json").write_text(json.dumps(meta, indent=2)) + + # Copy and scrub screenshots + screenshots_src = capture_path / "screenshots" + if screenshots_src.exists(): + screenshots_dst = staging_path / "screenshots" + screenshots_dst.mkdir(exist_ok=True) + for img_path in sorted(screenshots_src.glob("*.png")): + output_path = screenshots_dst / img_path.name + img_redactions = self.scrub_image(img_path, output_path) + for r in img_redactions: + r["source"] = f"screenshots/{img_path.name}" + all_redactions.extend(img_redactions) + + # Write scrub manifest + manifest = { + "scrub_level": self.level.value, + "timestamp": datetime.now(timezone.utc).isoformat(), + "total_redactions": len(all_redactions), + "redactions": all_redactions, + } + (staging_path / "scrub_manifest.json").write_text( + json.dumps(manifest, indent=2) + ) + + # Write review status + review_status = { + "status": "pending_review", + "scrubbed_at": datetime.now(timezone.utc).isoformat(), + "scrub_level": self.level.value, + } + (staging_path / "review_status.json").write_text( + json.dumps(review_status, indent=2) + ) + + backup_path: Path | None = None + if scrubbed_path.exists(): + backup_path = Path( + tempfile.mkdtemp( + prefix=f".{capture_path.name}.previous-", + dir=capture_path.parent, + ) + ) + backup_path.rmdir() + scrubbed_path.replace(backup_path) + try: + staging_path.replace(scrubbed_path) + except Exception: + if backup_path is not None and backup_path.exists(): + backup_path.replace(scrubbed_path) + raise + if backup_path is not None: + shutil.rmtree(backup_path) + return scrubbed_path + except Exception: + if staging_path.exists(): + shutil.rmtree(staging_path) + raise def _require_provider(self): # noqa: ANN202 - provider type is optional dep """Return a Presidio provider PROVEN able to run, or refuse. @@ -246,7 +282,10 @@ def scrub_image(self, image_path: Path, output_path: Path) -> list[dict]: scrubbed_img.save(output_path) except Exception as exc: raise ScrubbingUnavailableError(self.level, exc) from exc - return [{"type": "image_scrub", "path": str(output_path)}] + # The caller adds a normalized derivative-relative ``source``. Never + # retain an absolute local path because capture directory names can + # contain a record identity. + return [{"type": "image_scrub"}] def _scrub_text_regex(self, text: str) -> tuple[str, list[dict]]: """Scrub PII using regex patterns only (basic level). diff --git a/engine/upload_manager.py b/engine/upload_manager.py index f37a6b4..8a082b6 100644 --- a/engine/upload_manager.py +++ b/engine/upload_manager.py @@ -1,4 +1,4 @@ -"""Upload manager -- multi-backend upload with persistent queue and bandwidth limiting. +"""Legacy customer-owned storage queue with bandwidth limiting. All uploads go through a persistent queue (stored in index.db) that survives app restarts. The upload pipeline: @@ -12,6 +12,8 @@ - Wormhole: P2P direct Before any upload, the recording must pass check_egress_allowed() from review.py. +OpenAdapt hosted ingest does not use this queue. It uses Flow's stronger +inventory, sanitization, exact-hash approval, and immutable archive contract. Bandwidth limiting uses a token bucket algorithm (configurable via OPENADAPT_UPLOAD_BANDWIDTH_LIMIT). @@ -21,7 +23,12 @@ from __future__ import annotations +import hashlib +import os +import re import uuid +import zipfile +from dataclasses import dataclass from datetime import datetime, timedelta, timezone from pathlib import Path @@ -31,7 +38,13 @@ from engine.backends.protocol import StorageBackend, UploadResult from engine.config import EngineConfig from engine.db import IndexDB -from engine.review import check_egress_allowed +from engine.review import ( + EgressArtifactError, + EgressBlockedError, + approved_egress_path, + derivative_tree_sha256, + load_derivative_approval, +) # Durable/offline retry policy (spec section 5): jobs survive restarts (they # live in the DB), retry with exponential backoff, and flush when connectivity @@ -39,6 +52,22 @@ DEFAULT_MAX_ATTEMPTS = 6 _BACKOFF_BASE_S = 30 _BACKOFF_CAP_S = 3600 +_FLOW_ONLY_BACKENDS = frozenset({"hosted_ingest"}) +_FLOW_ONLY_ERROR = ( + "Direct hosted ingest is disabled. Use `openadapt-desktop push` so Flow can " + "bind the upload to a reviewed, exact-hash sanitized artifact." +) +_APPROVED_ARCHIVE_RE = re.compile( + r"^(?P[a-f0-9]{32})-(?P[a-f0-9]{64})\.approved\.zip$" +) + + +@dataclass(frozen=True) +class _FrozenArtifact: + """One queue-owned archive whose exact bytes passed local approval.""" + + path: Path + sha256: str def _backoff_seconds(attempts: int) -> int: @@ -80,7 +109,7 @@ def __init__( def enqueue(self, capture_id: str, backend_name: str) -> str: """Add a capture to the upload queue. - The capture must be cleared for egress (reviewed or dismissed). + The capture must have a reviewed sanitized derivative. Args: capture_id: ID of the capture to upload. @@ -93,34 +122,48 @@ def enqueue(self, capture_id: str, backend_name: str) -> str: EgressBlockedError: If the capture hasn't been reviewed. ValueError: If the backend is not available. """ - check_egress_allowed(capture_id, self._db) + if backend_name in _FLOW_ONLY_BACKENDS: + raise ValueError(_FLOW_ONLY_ERROR) if backend_name not in self.backends: raise ValueError(f"Backend not available: {backend_name}") job_id = uuid.uuid4().hex - self._db.insert_upload_job(job_id, capture_id, backend_name) + artifact_path = approved_egress_path(capture_id, self._db) + frozen = self._freeze_artifact( + job_id, + artifact_path, + approved_tree_sha256=load_derivative_approval(artifact_path), + ) + try: + self._db.insert_upload_job( + job_id, + capture_id, + backend_name, + archive_path=str(frozen.path), + ) + except Exception: + frozen.path.unlink(missing_ok=True) + raise return job_id - def upload(self, archive_path: Path, backend_name: str, metadata: dict) -> UploadResult: - """Upload an archive to a specific backend. - - Args: - archive_path: Path to the archive file. - backend_name: Name of the target storage backend. - metadata: Capture metadata to include with the upload. - - Returns: - UploadResult from the backend. - """ + def _upload_frozen( + self, + artifact: _FrozenArtifact, + backend_name: str, + metadata: dict, + ) -> UploadResult: + """Send only an artifact returned by the queue's hash verifier.""" + if backend_name in _FLOW_ONLY_BACKENDS: + return UploadResult(success=False, error=_FLOW_ONLY_ERROR) backend = self.backends[backend_name] - size_bytes = archive_path.stat().st_size if archive_path.exists() else 0 + size_bytes = artifact.path.stat().st_size dest = f"{backend_name}://{metadata.get('capture_id', 'unknown')}" self._audit.log_upload_start(backend_name, dest, size_bytes) try: - result = backend.upload(archive_path, metadata) + result = backend.upload(artifact.path, metadata) except Exception as e: self._audit.log_upload_failed(backend_name, dest, str(e)) return UploadResult(success=False, error=str(e)) @@ -132,6 +175,107 @@ def upload(self, archive_path: Path, backend_name: str, metadata: dict) -> Uploa return result + def _approved_archive_root(self) -> Path: + """Return the private queue archive directory.""" + + root = self.config.data_dir / "approved_uploads" + root.mkdir(parents=True, exist_ok=True, mode=0o700) + if root.is_symlink(): + raise EgressArtifactError("The approved upload directory cannot be a symlink.") + os.chmod(root, 0o700) + if hasattr(os, "getuid") and root.stat().st_uid != os.getuid(): + raise EgressArtifactError("The approved upload directory has the wrong owner.") + return root.resolve(strict=True) + + @staticmethod + def _stream_sha256(path: Path) -> str: + digest = hashlib.sha256() + with path.open("rb") as source: + while chunk := source.read(1024 * 1024): + digest.update(chunk) + return digest.hexdigest() + + def _freeze_artifact( + self, + job_id: str, + source: Path, + *, + approved_tree_sha256: str, + ) -> _FrozenArtifact: + """Freeze only bytes that reproduce the review-time tree digest.""" + + root = self._approved_archive_root() + temporary = root / f".{job_id}.tmp" + try: + frozen_tree = hashlib.sha256() + members = [source, *sorted(source.rglob("*"))] + with zipfile.ZipFile( + temporary, "w", zipfile.ZIP_DEFLATED, compresslevel=9 + ) as archive: + for member in members: + if member.is_symlink(): + raise EgressArtifactError( + "The sanitized derivative contains a symlink." + ) + relative = ( + "." if member == source else member.relative_to(source).as_posix() + ) + if relative == "review_status.json": + continue + frozen_tree.update(relative.encode("utf-8")) + if not member.is_file(): + continue + stat = os.stat(member, follow_symlinks=False) + if stat.st_nlink != 1: + raise EgressArtifactError( + "The sanitized derivative contains a hard-linked file." + ) + info = zipfile.ZipInfo(relative) + info.date_time = (1980, 1, 1, 0, 0, 0) + info.compress_type = zipfile.ZIP_DEFLATED + info.external_attr = 0o600 << 16 + file_digest = hashlib.sha256() + with member.open("rb") as input_file, archive.open(info, "w") as output_file: + while chunk := input_file.read(1024 * 1024): + file_digest.update(chunk) + output_file.write(chunk) + frozen_tree.update(file_digest.hexdigest().encode("ascii")) + if ( + frozen_tree.hexdigest() != approved_tree_sha256 + or derivative_tree_sha256(source) != approved_tree_sha256 + ): + raise EgressArtifactError( + "The sanitized derivative does not match the exact reviewed bytes." + ) + archive_digest = self._stream_sha256(temporary) + destination = root / f"{job_id}-{archive_digest}.approved.zip" + temporary.replace(destination) + os.chmod(destination, 0o600) + return _FrozenArtifact(path=destination, sha256=archive_digest) + except Exception: + temporary.unlink(missing_ok=True) + raise + + def _load_frozen_artifact(self, job: dict) -> _FrozenArtifact: + """Verify a persisted queue archive before any network call.""" + + value = str(job.get("archive_path") or "").strip() + candidate = Path(value) + if not value or candidate.is_symlink() or not candidate.is_file(): + raise EgressArtifactError("The approved queue archive is unavailable or unsafe.") + root = self._approved_archive_root() + path = candidate.resolve(strict=True) + if path.parent != root: + raise EgressArtifactError("The queue archive is outside the approved upload directory.") + match = _APPROVED_ARCHIVE_RE.fullmatch(path.name) + if match is None or match.group("job") != job["job_id"]: + raise EgressArtifactError("The queue archive identity is invalid.") + expected = match.group("digest") + actual = self._stream_sha256(path) + if actual != expected: + raise EgressArtifactError("The approved queue archive changed after enqueue.") + return _FrozenArtifact(path=path, sha256=actual) + def get_queue_status(self) -> list[dict]: """Get the current state of the upload queue. @@ -159,6 +303,7 @@ def process_queue(self) -> list[dict]: Returns: List of result dicts for each job attempted this cycle. """ + self._db.recover_interrupted_upload_jobs() due = self._db.get_due_jobs() results = [] self.offline = False @@ -170,22 +315,38 @@ def process_queue(self) -> list[dict]: self._db.update_upload_job(job_id, status="in_progress") + if backend_name in _FLOW_ONLY_BACKENDS: + self._db.update_upload_job(job_id, status="failed", error=_FLOW_ONLY_ERROR) + self._cleanup_job_archive(job) + results.append( + self._result(job_id, capture_id, backend_name, False, "", _FLOW_ONLY_ERROR) + ) + continue + capture = self._db.get_capture(capture_id) if not capture: self._db.update_upload_job( job_id, status="failed", error=f"Capture {capture_id} not found" ) - results.append(self._result(job_id, capture_id, backend_name, False, - "", f"Capture {capture_id} not found")) + self._cleanup_job_archive(job) + results.append( + self._result( + job_id, + capture_id, + backend_name, + False, + "", + f"Capture {capture_id} not found", + ) + ) continue - capture_path = Path(capture["capture_path"]) - if not capture_path.exists(): - self._db.update_upload_job( - job_id, status="failed", error=f"Path not found: {capture_path}" - ) - results.append(self._result(job_id, capture_id, backend_name, False, - "", f"Path not found: {capture_path}")) + try: + approved_egress_path(capture_id, self._db) + except (ValueError, EgressBlockedError, EgressArtifactError) as exc: + self._db.update_upload_job(job_id, status="failed", error=str(exc)) + self._cleanup_job_archive(job) + results.append(self._result(job_id, capture_id, backend_name, False, "", str(exc))) continue metadata = { @@ -195,7 +356,14 @@ def process_queue(self) -> list[dict]: "event_count": capture.get("event_count", 0), } - result = self.upload(capture_path, backend_name, metadata) + try: + frozen = self._load_frozen_artifact(job) + result = self._upload_frozen(frozen, backend_name, metadata) + except (OSError, ValueError, EgressArtifactError) as exc: + self._db.update_upload_job(job_id, status="failed", error=str(exc)) + self._cleanup_job_archive(job) + results.append(self._result(job_id, capture_id, backend_name, False, "", str(exc))) + continue if result.success: self._db.update_upload_job( @@ -203,19 +371,46 @@ def process_queue(self) -> list[dict]: status="completed", remote_url=result.remote_url, bytes_sent=result.bytes_sent, + completed_at=datetime.now(timezone.utc).isoformat(), ) + self._cleanup_job_archive(job) else: - self._schedule_retry(job, result.error) + terminal = self._schedule_retry(job, result.error) + if terminal: + self._cleanup_job_archive(job) - results.append(self._result( - job_id, capture_id, backend_name, result.success, - result.remote_url if result.success else "", - result.error if not result.success else "", - )) + results.append( + self._result( + job_id, + capture_id, + backend_name, + result.success, + result.remote_url if result.success else "", + result.error if not result.success else "", + ) + ) return results - def _schedule_retry(self, job: dict, error: str) -> None: + def _cleanup_job_archive(self, job: dict) -> None: + """Remove only the queue-owned archive for a terminal job.""" + + value = str(job.get("archive_path") or "").strip() + if not value: + return + candidate = Path(value) + try: + root = self._approved_archive_root() + if ( + not candidate.is_symlink() + and candidate.parent.resolve(strict=True) == root + and candidate.name.startswith(f"{job['job_id']}-") + ): + candidate.unlink(missing_ok=True) + except OSError: + logger.warning("Could not remove terminal queue archive for {jid}", jid=job["job_id"]) + + def _schedule_retry(self, job: dict, error: str) -> bool: """Requeue a transiently-failed job with backoff, or fail it permanently.""" attempts = (job.get("attempts") or 0) + 1 self.offline = True @@ -225,12 +420,11 @@ def _schedule_retry(self, job: dict, error: str) -> None: ) logger.warning( "Upload job {jid} permanently failed after {n} attempts", - jid=job["job_id"], n=attempts, + jid=job["job_id"], + n=attempts, ) - return - next_retry = datetime.now(timezone.utc) + timedelta( - seconds=_backoff_seconds(attempts) - ) + return True + next_retry = datetime.now(timezone.utc) + timedelta(seconds=_backoff_seconds(attempts)) self._db.update_upload_job( job["job_id"], status="pending", @@ -240,13 +434,20 @@ def _schedule_retry(self, job: dict, error: str) -> None: ) logger.info( "Upload job {jid} deferred (attempt {n}); retry at {t}", - jid=job["job_id"], n=attempts, t=next_retry.isoformat(), + jid=job["job_id"], + n=attempts, + t=next_retry.isoformat(), ) + return False @staticmethod def _result( - job_id: str, capture_id: str, backend: str, success: bool, - remote_url: str, error: str, + job_id: str, + capture_id: str, + backend: str, + success: bool, + remote_url: str, + error: str, ) -> dict: return { "job_id": job_id, diff --git a/src/screens/WorkflowLibrary.test.tsx b/src/screens/WorkflowLibrary.test.tsx new file mode 100644 index 0000000..32cb89a --- /dev/null +++ b/src/screens/WorkflowLibrary.test.tsx @@ -0,0 +1,35 @@ +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, expect, it, vi } from "vitest"; +import { engineTry } from "../lib/engine"; +import { WorkflowLibrary } from "./WorkflowLibrary"; + +vi.mock("../lib/engine", async (importOriginal) => { + const original = await importOriginal(); + return { ...original, engineTry: vi.fn() }; +}); + +afterEach(cleanup); + +it("routes deployment preparation through qualification", async () => { + vi.mocked(engineTry).mockResolvedValue([ + { id: "workflow-1", name: "Example", steps: 2, synced: false }, + ]); + const onQualify = vi.fn(); + render( + {}} + onTeach={() => {}} + onRecord={() => {}} + />, + ); + + await waitFor(() => + expect( + screen.getByRole("button", { name: "Prepare to deploy" }), + ).toBeTruthy(), + ); + fireEvent.click(screen.getByRole("button", { name: "Prepare to deploy" })); + + expect(onQualify).toHaveBeenCalledWith("workflow-1"); +}); diff --git a/src/screens/WorkflowLibrary.tsx b/src/screens/WorkflowLibrary.tsx index 8a50e19..8fb7498 100644 --- a/src/screens/WorkflowLibrary.tsx +++ b/src/screens/WorkflowLibrary.tsx @@ -1,7 +1,7 @@ // Workflow library — local compiled workflows, their last-run state, halts, and // sync status. Push to cloud (cloud lane) or open the local teach view (byoc). import { useEffect, useState } from "react"; -import { CMD, engineInvoke, engineTry } from "../lib/engine"; +import { CMD, engineTry } from "../lib/engine"; import type { Workflow } from "../lib/types"; import { Button, @@ -25,7 +25,6 @@ export function WorkflowLibrary({ }) { const [workflows, setWorkflows] = useState([]); const [loading, setLoading] = useState(true); - const [pushing, setPushing] = useState(null); async function refresh() { const list = await engineTry(CMD.GET_WORKFLOWS, {}, []); @@ -37,18 +36,6 @@ export function WorkflowLibrary({ void refresh(); }, []); - async function push(id: string) { - setPushing(id); - try { - await engineInvoke(CMD.PUSH_WORKFLOW, { workflow_id: id }); - await refresh(); - } catch { - /* surfaced via sync state elsewhere */ - } finally { - setPushing(null); - } - } - return (
@@ -127,12 +114,8 @@ export function WorkflowLibrary({ Teach fix ) : ( - )}
diff --git a/tests/test_e2e/test_pipeline.py b/tests/test_e2e/test_pipeline.py index 4b0e068..6a60dab 100644 --- a/tests/test_e2e/test_pipeline.py +++ b/tests/test_e2e/test_pipeline.py @@ -108,18 +108,25 @@ def test_record_scrub_approve_upload(self, pipeline) -> None: scrubbed_path = scrubber.scrub_capture(capture_path) assert scrubbed_path.exists() assert (scrubbed_path / "scrub_manifest.json").exists() + pipeline.db.update_capture(capture_id, scrubbed_path=str(scrubbed_path)) transition_status( - capture_id, ReviewStatus.CAPTURED, ReviewStatus.SCRUBBED, - db=pipeline.db, audit=pipeline.audit, + capture_id, + ReviewStatus.CAPTURED, + ReviewStatus.SCRUBBED, + db=pipeline.db, + audit=pipeline.audit, ) cap = pipeline.db.get_capture(capture_id) assert cap["review_status"] == "scrubbed" # Step 4: Approve the scrubbed capture transition_status( - capture_id, ReviewStatus.SCRUBBED, ReviewStatus.REVIEWED, - db=pipeline.db, audit=pipeline.audit, + capture_id, + ReviewStatus.SCRUBBED, + ReviewStatus.REVIEWED, + db=pipeline.db, + audit=pipeline.audit, ) # Step 5: Verify egress is now allowed @@ -140,8 +147,8 @@ def test_record_scrub_approve_upload(self, pipeline) -> None: # Verify audit log has entries assert pipeline.audit.log_path.exists() - def test_dismiss_allows_egress(self, pipeline) -> None: - """Dismissed captures should be cleared for egress.""" + def test_dismiss_keeps_raw_capture_local(self, pipeline) -> None: + """A dismissal never makes the raw capture uploadable.""" controller = RecordingController( captures_dir=pipeline.config.data_dir / "captures", storage_manager=pipeline.storage, @@ -151,11 +158,16 @@ def test_dismiss_allows_egress(self, pipeline) -> None: # Dismiss (skip scrubbing) transition_status( - capture_id, ReviewStatus.CAPTURED, ReviewStatus.DISMISSED, + capture_id, + ReviewStatus.CAPTURED, + ReviewStatus.DISMISSED, db=pipeline.db, ) - assert check_egress_allowed(capture_id, pipeline.db) is True + from engine.review import EgressBlockedError + + with pytest.raises(EgressBlockedError): + check_egress_allowed(capture_id, pipeline.db) def test_upload_blocked_before_review(self, pipeline) -> None: """Upload should be blocked for unreviewed captures.""" diff --git a/tests/test_engine/test_backends.py b/tests/test_engine/test_backends.py index 01fd4e6..7e05f8f 100644 --- a/tests/test_engine/test_backends.py +++ b/tests/test_engine/test_backends.py @@ -57,65 +57,24 @@ def test_estimate_cost_none(self) -> None: """Hosted ingest surfaces no per-upload storage cost to the client.""" assert HostedIngestBackend().estimate_cost(1024**3) is None - def test_upload_without_auth_fails(self, monkeypatch) -> None: - """Upload without a resolvable bearer token fails closed.""" - monkeypatch.delenv("OPENADAPT_INGEST_TOKEN", raising=False) - monkeypatch.setattr( - "engine.backends.hosted_ingest.auth_header", lambda: {} - ) + def test_direct_upload_fails_closed(self) -> None: + """The obsolete adapter never sends unverified archive bytes.""" from pathlib import Path result = HostedIngestBackend().upload(Path("/nonexistent.zip"), {}) assert result.success is False - assert "Not logged in" in result.error + assert "Direct hosted ingest is disabled" in result.error def test_delete_not_supported(self) -> None: """Hosted ingest does not support client-side delete.""" with pytest.raises(NotImplementedError): HostedIngestBackend().delete("any") - def test_upload_success(self, tmp_path, monkeypatch) -> None: - """A 201 response yields success + dashboard URL from workflow_id.""" - from .conftest import FakeResponse - + def test_direct_upload_makes_no_network_request(self, tmp_path, monkeypatch) -> None: + """Even valid-looking bytes and credentials cannot bypass Flow.""" archive = tmp_path / "rec.zip" archive.write_bytes(b"zipdata") - monkeypatch.setattr( - "engine.backends.hosted_ingest.auth_header", - lambda: {"Authorization": "Bearer oai_ingest_x"}, - ) - captured = {} - - def _post(url, headers=None, data=None, files=None, timeout=None): - captured["url"] = url - captured["data"] = data - captured["has_file"] = files is not None and "file" in files - return FakeResponse(201, {"ingest": {"workflow_id": "wf_7"}}) - - monkeypatch.setattr("engine.backends.hosted_ingest.httpx.post", _post) - result = HostedIngestBackend(host="https://app").upload( - archive, {"kind": "recording", "name": "My Flow"} - ) - assert result.success is True - assert result.remote_url == "https://app/dashboard/workflows/wf_7" - assert captured["url"] == "https://app/api/ingest" - assert captured["data"]["kind"] == "recording" - assert captured["has_file"] is True - - def test_upload_401(self, tmp_path, monkeypatch) -> None: - """A 401 is surfaced as a failed upload.""" - from .conftest import FakeResponse - - archive = tmp_path / "rec.zip" - archive.write_bytes(b"z") - monkeypatch.setattr( - "engine.backends.hosted_ingest.auth_header", - lambda: {"Authorization": "Bearer bad"}, - ) - monkeypatch.setattr( - "engine.backends.hosted_ingest.httpx.post", - lambda *a, **k: FakeResponse(401, {}), - ) result = HostedIngestBackend().upload(archive, {}) + assert result.success is False - assert "401" in result.error + assert result.bytes_sent == 0 diff --git a/tests/test_engine/test_cli.py b/tests/test_engine/test_cli.py index 5d86dae..5a76978 100644 --- a/tests/test_engine/test_cli.py +++ b/tests/test_engine/test_cli.py @@ -20,6 +20,7 @@ def cli_config(tmp_data_dir: Path) -> EngineConfig: storage_mode="air-gapped", max_storage_gb=1.0, log_level="WARNING", + audit_log_path=tmp_data_dir / "audit.jsonl", ) @@ -187,3 +188,51 @@ def test_push_failure_exits(self, cli_config: EngineConfig) -> None: patch("engine.hosted.push", return_value=result): with pytest.raises(SystemExit): main(["push", "/tmp/rec"]) + + def test_push_review_pause_is_not_failure_or_upload_success( + self, cli_config: EngineConfig, capsys + ) -> None: + """A local review pause gives the operator the exact next command.""" + result = { + "success": False, + "pending_review": True, + "sanitized_path": "/tmp/sanitized/artifact-abc", + "review_command": ( + "openadapt-flow review-sanitized /tmp/sanitized/artifact-abc " + "--original /tmp/rec" + ), + "workflow_id": "", + "dashboard_url": "", + "error": "", + } + with patch("engine.cli.EngineConfig", return_value=cli_config), patch( + "engine.hosted.push", return_value=result + ): + main(["push", "/tmp/rec"]) + + output = capsys.readouterr().out + assert "Upload paused" in output + assert result["review_command"] in output + assert "Pushed. Workflow" not in output + + def test_legacy_hosted_upload_alias_routes_to_governed_push( + self, cli_config: EngineConfig, tmp_path: Path + ) -> None: + """The old command name keeps working without using the direct adapter.""" + from engine.db import IndexDB + + raw = tmp_path / "capture" + raw.mkdir() + db = IndexDB(cli_config.data_dir / "index.db") + db.initialize() + db.insert_capture("cap1", str(raw), "2026-08-18T00:00:00Z") + db.close() + + with patch("engine.cli.EngineConfig", return_value=cli_config), patch( + "engine.cli.cmd_push" + ) as governed_push: + main(["upload", "cap1", "--backend", "hosted_ingest"]) + + args, _kwargs = governed_push.call_args + assert args[0].path == str(raw) + assert args[0].kind == "recording" diff --git a/tests/test_engine/test_dispatch.py b/tests/test_engine/test_dispatch.py index 897d77e..faa61f5 100644 --- a/tests/test_engine/test_dispatch.py +++ b/tests/test_engine/test_dispatch.py @@ -1384,6 +1384,29 @@ def test_push_workflow(self, deps, monkeypatch) -> None: assert r["workflow_id"] == "wf_1" assert any(e == "sync_state" for e, _ in events) + def test_push_workflow_preserves_local_review_handoff(self, deps, monkeypatch) -> None: + disp, db, events = deps + db.insert_bundle("bnd1", str(disp.config.data_dir), capture_id="cap1") + monkeypatch.setattr( + "engine.hosted.push", + lambda *a, **k: { + "success": False, + "pending_review": True, + "sanitized_path": "/private/sanitized/artifact", + "review_command": "openadapt-flow review-sanitized ...", + "workflow_id": "", + "error": "", + }, + ) + + result = disp.dispatch("push_workflow", {"workflow_id": "bnd1"}) + + assert result["ok"] is False + assert result["pending_review"] is True + assert result["sanitized_path"] == "/private/sanitized/artifact" + assert result["review_command"].startswith("openadapt-flow review-sanitized") + assert ("sync_state", {"state": "paused", "queued": 0}) in events + class TestAuthCommands: def test_login_paste(self, deps, monkeypatch, fake_keyring) -> None: diff --git a/tests/test_engine/test_flow_bridge.py b/tests/test_engine/test_flow_bridge.py index add070c..77433e0 100644 --- a/tests/test_engine/test_flow_bridge.py +++ b/tests/test_engine/test_flow_bridge.py @@ -66,6 +66,47 @@ def test_compile_builds_args(self, tmp_path: Path, monkeypatch) -> None: assert "--name" in command assert command[command.index("--name") + 1] == "bundle" + def test_report_break_keeps_token_out_of_argv( + self, tmp_path: Path, monkeypatch + ) -> None: + monkeypatch.setattr("engine.flow_bridge.shutil.which", lambda _: "/usr/bin/openadapt-flow") + calls: list = [] + bridge = FlowBridge(runner=_runner(calls, stdout="Nothing emitted: no halt")) + + bridge.report_break( + tmp_path / "run", + workflow_id="workflow-1", + host="https://app.openadapt.ai", + env_overrides={"OPENADAPT_INGEST_TOKEN": "secret-value"}, + ) + + command, env = calls[0] + assert "secret-value" not in command + assert "--token" not in command + assert env["OPENADAPT_INGEST_TOKEN"] == "secret-value" + + def test_push_keeps_token_and_local_name_out_of_argv( + self, tmp_path: Path, monkeypatch + ) -> None: + monkeypatch.setattr("engine.flow_bridge.shutil.which", lambda _: "/usr/bin/openadapt-flow") + calls: list = [] + bridge = FlowBridge(runner=_runner(calls, stdout="ok")) + + bridge.push( + tmp_path / "bundle", + kind="bundle", + host="https://app.openadapt.ai", + name="Jane Doe patient transfer", + token="secret-value", + ) + + command, env = calls[0] + assert "secret-value" not in command + assert "Jane Doe patient transfer" not in command + assert "--token" not in command + assert "--name" not in command + assert env["OPENADAPT_INGEST_TOKEN"] == "secret-value" + def test_demo_record_uses_canonical_bundled_flow_command( self, tmp_path: Path, monkeypatch ) -> None: @@ -418,6 +459,19 @@ def test_secret_flag_values_are_redacted_from_debug_command(self) -> None: assert "oar_secret" not in rendered assert rendered == "openadapt-flow push --token [REDACTED] --kind bundle" + def test_egress_local_paths_are_redacted_from_debug_command(self) -> None: + rendered = _safe_command_for_log( + [ + "openadapt-flow", + "push", + "/captures/Jane-Doe-12345.scrubbed", + "--kind", + "recording", + ] + ) + assert "Jane-Doe" not in rendered + assert rendered == "openadapt-flow push [LOCAL_PATH] --kind recording" + def test_phi_capable_selector_values_are_redacted_from_debug_command(self) -> None: rendered = _safe_command_for_log( [ diff --git a/tests/test_engine/test_hosted.py b/tests/test_engine/test_hosted.py index 805e8ab..e21c392 100644 --- a/tests/test_engine/test_hosted.py +++ b/tests/test_engine/test_hosted.py @@ -10,14 +10,8 @@ from engine import hosted from engine.backends.protocol import UploadResult -from engine.hosted import ( - PhiBoundaryError, - build_break_descriptor, - report_break, - zip_dir, -) - -from .conftest import FakeResponse +from engine.flow_bridge import FlowResult +from engine.hosted import PhiBoundaryError, report_break, zip_dir class _StubBackend: @@ -47,9 +41,23 @@ def test_zips_recursively(self, tmp_path: Path) -> None: assert "meta.json" in names assert "frames/0001.png" in names + def test_refuses_symlink_members(self, tmp_path: Path) -> None: + src = tmp_path / "rec" + outside = tmp_path / "outside" + src.mkdir() + outside.write_bytes(b"raw-secret") + (src / "escape").symlink_to(outside) + + with pytest.raises(ValueError, match="symlink"): + zip_dir(src) + class TestPush: - def test_push_success_persists_workflow_id(self, tmp_path: Path) -> None: + def test_push_success_persists_workflow_id( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: from engine.db import IndexDB rec = tmp_path / "rec1" @@ -60,20 +68,33 @@ def test_push_success_persists_workflow_id(self, tmp_path: Path) -> None: db.initialize() db.insert_bundle("bnd1", str(rec), capture_id="rec1") - backend = _StubBackend( - UploadResult(success=True, remote_url="https://app/dashboard/workflows/wf_1", - metadata={"workflow_id": "wf_1"}) + monkeypatch.setattr( + hosted, + "_push_via_flow", + lambda *args, **kwargs: { + "success": True, + "workflow_id": "wf_1", + "dashboard_url": "https://app/dashboard/workflows/wf_1", + "error": "", + }, ) result = hosted.push( - rec, kind="recording", host="https://app", backend=backend, - prefer_flow=False, db=db, bundle_id="bnd1", + rec, + kind="recording", + host="https://app", + db=db, + bundle_id="bnd1", ) assert result["success"] is True assert result["workflow_id"] == "wf_1" assert db.get_bundle("bnd1")["workflow_id"] == "wf_1" db.close() - def test_push_default_latest_recording(self, tmp_path: Path) -> None: + def test_push_default_latest_recording( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: recordings = tmp_path / "recordings" (recordings / "old").mkdir(parents=True) (recordings / "new").mkdir() @@ -84,42 +105,229 @@ def test_push_default_latest_recording(self, tmp_path: Path) -> None: os.utime(recordings / "new", (time.time() + 10, time.time() + 10)) - backend = _StubBackend(UploadResult(success=True, metadata={"workflow_id": "wf_x"})) - result = hosted.push( - None, recordings_dir=recordings, backend=backend, prefer_flow=False, host="https://app" - ) + selected: dict[str, Path] = {} + + def push_via_flow(path: Path, **_kwargs): + selected["path"] = path + return { + "success": True, + "workflow_id": "wf_x", + "dashboard_url": "", + "error": "", + } + + monkeypatch.setattr(hosted, "_push_via_flow", push_via_flow) + result = hosted.push(None, recordings_dir=recordings, host="https://app") assert result["success"] is True - assert backend.uploaded is not None - assert backend.metadata["capture_id"] == "new" + assert selected["path"] == recordings / "new" def test_push_missing_path_raises(self, tmp_path: Path) -> None: with pytest.raises(FileNotFoundError): - hosted.push(tmp_path / "nope", prefer_flow=False) - - -class TestBreakDescriptor: - def test_phi_free_fields_only(self) -> None: - report = { - "workflow_id": "ignored", - "step_intent": "click Submit", - "reason": "element not found", - "resolver_rung": "template", - "drift_signature": "sig123", - "metrics": {"steps": 5, "duration_s": 12.3}, - # PHI that must never be forwarded: - "field_values": {"ssn": "123-45-6789"}, - "report_body": "raw", - "dom": "", - "screenshots": ["a.png"], + hosted.push(tmp_path / "nope") + + def test_direct_backend_bypass_fails_without_upload(self, tmp_path: Path) -> None: + rec = tmp_path / "rec" + rec.mkdir() + backend = _StubBackend(UploadResult(success=True)) + + result = hosted.push(rec, backend=backend, prefer_flow=False) + + assert result["success"] is False + assert "Direct Desktop ingest is disabled" in result["error"] + assert backend.uploaded is None + + def test_missing_flow_push_never_falls_back_to_direct_ingest( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + monkeypatch.setattr( + hosted, + "_push_via_flow", + lambda *args, **kwargs: (_ for _ in ()).throw(FileNotFoundError("flow")), + ) + + result = hosted.push(rec) + + assert result["success"] is False + assert result["workflow_id"] == "" + assert result["delivery_uncertain"] is True + assert "Do not retry blindly" in result["error"] + + def test_flow_review_pause_is_not_reported_as_upload_success( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + derivative = tmp_path / "sanitized" / "artifact-abc" + output = ( + f"Sanitized derivative created at {derivative}.\n" + "Upload paused for local review; the original was not modified or uploaded.\n" + f"openadapt-flow review-sanitized {derivative} --original {rec}\n" + ) + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout=output), + ) + + result = hosted.push(rec) + + assert result["success"] is False + assert result["pending_review"] is True + assert result["sanitized_path"] == str(derivative) + assert result["workflow_id"] == "" + + def test_desktop_credential_reaches_flow_only_through_environment( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + calls: list[dict] = [] + monkeypatch.setattr( + hosted, + "active_credential", + lambda: { + "host": "https://app.openadapt.ai", + "token": "stored-secret", + "org_id": "org-1", + }, + ) + + def fake_push(*_args, **kwargs): + calls.append(kwargs) + return FlowResult( + ok=True, + returncode=0, + stdout=( + "Pushed. workflow_id=123e4567-e89b-12d3-a456-426614174000 " + "(name='Example', kind=recording, compile=ok).\n" + ), + ) + + monkeypatch.setattr("engine.hosted.FlowBridge.push", fake_push) + + result = hosted.push(rec) + + assert result["success"] is True + assert calls[0]["token"] is None + assert calls[0]["env_overrides"] == { + "OPENADAPT_INGEST_TOKEN": "stored-secret" } - d = build_break_descriptor( - report, workflow_id="wf_1", deployment_kind="byoc", org_id="org_1" + + def test_credential_for_another_host_is_not_forwarded( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + calls: list[dict] = [] + monkeypatch.setattr( + hosted, + "active_credential", + lambda: {"host": "https://other.example", "token": "wrong-host-secret"}, ) - assert d["workflow_id"] == "wf_1" - assert d["deployment_kind"] == "byoc" - assert d["metrics"] == {"steps": 5, "duration_s": 12.3} - for forbidden in ("field_values", "report_body", "dom", "screenshots"): - assert forbidden not in d + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *_args, **kwargs: ( + calls.append(kwargs) + or FlowResult(ok=False, returncode=1, stdout="Not logged in") + ), + ) + + hosted.push(rec, host="https://app.openadapt.ai") + + assert calls[0]["env_overrides"] is None + + def test_flow_success_requires_and_parses_hosted_workflow_identity( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + workflow_id = "123e4567-e89b-12d3-a456-426614174000" + output = ( + f"Pushed. workflow_id={workflow_id} (name='Example', kind=recording, compile=ok).\n" + f"Dashboard: https://app.openadapt.ai/dashboard/workflows/{workflow_id}\n" + ) + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout=output), + ) + + result = hosted.push(rec) + + assert result["success"] is True + assert result["workflow_id"] == workflow_id + assert result["dashboard_url"].endswith(workflow_id) + + def test_flow_exit_zero_without_identity_fails_closed( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout="Pushed."), + ) + + result = hosted.push(rec) + + assert result["success"] is False + assert "without an authenticated hosted workflow identity" in result["error"] + + def test_flow_none_workflow_identity_is_not_success( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *args, **kwargs: FlowResult( + ok=True, + returncode=0, + stdout="Pushed. workflow_id=None (name='Example', kind=recording, compile=?).", + ), + ) + + result = hosted.push(rec) + + assert result["success"] is False + assert result["workflow_id"] == "" + + def test_flow_failure_uses_bounded_stdout_and_marks_delivery_uncertain( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + monkeypatch.setattr( + "engine.hosted.FlowBridge.push", + lambda *args, **kwargs: FlowResult( + ok=False, + returncode=1, + stdout="request outcome unknown", + stderr="", + ), + ) + + result = hosted.push(rec, token="secret-value") + + assert result["success"] is False + assert result["delivery_uncertain"] is True + assert result["error"] == "request outcome unknown" class TestReportBreak: @@ -138,26 +346,43 @@ def test_no_halt_returns_error(self, tmp_path: Path) -> None: def test_success(self, tmp_path: Path, monkeypatch) -> None: run_dir = tmp_path / "run" self._write_report(run_dir, {"reason": "drift", "step_intent": "click"}) + calls: list[dict] = [] monkeypatch.setattr( - "engine.hosted.httpx.post", - lambda *a, **k: FakeResponse(202, { - "ok": True, "run_id": "r1", "halt_id": "h1", - "status": "halt", "teach_url": "/dashboard/runs/r1/teach", - }), + "engine.hosted.FlowBridge.report_break", + lambda *a, **k: ( + calls.append(k) + or FlowResult( + ok=True, + returncode=0, + stdout=( + "Break reported (run_id=r1, halt_id=h1, status=halt).\n" + "Teach: https://app.openadapt.ai/dashboard/runs/r1/teach\n" + ), + ) + ), ) result = report_break(run_dir, workflow_id="wf_1", token="oai_ingest_x") assert result["ok"] is True assert result["halt_id"] == "h1" assert result["teach_url"].endswith("/teach") + assert calls[0]["env_overrides"] == { + "OPENADAPT_INGEST_TOKEN": "oai_ingest_x" + } def test_422_local_fallback(self, tmp_path: Path, monkeypatch) -> None: run_dir = tmp_path / "run" self._write_report(run_dir, {"reason": "drift"}) monkeypatch.setattr( - "engine.hosted.httpx.post", lambda *a, **k: FakeResponse(422, {}) + "engine.hosted.FlowBridge.report_break", + lambda *a, **k: FlowResult( + ok=True, + returncode=0, + stdout="Break kept LOCAL-ONLY: server rejected PHI boundary\n", + ), + ) + result = report_break( + run_dir, workflow_id="wf_1", token="oai_ingest_x", allow_local_fallback=True ) - result = report_break(run_dir, workflow_id="wf_1", token="oai_ingest_x", - allow_local_fallback=True) assert result["ok"] is False assert result["local_teach"] is True @@ -165,11 +390,17 @@ def test_422_raises_without_fallback(self, tmp_path: Path, monkeypatch) -> None: run_dir = tmp_path / "run" self._write_report(run_dir, {"reason": "drift"}) monkeypatch.setattr( - "engine.hosted.httpx.post", lambda *a, **k: FakeResponse(422, {}) + "engine.hosted.FlowBridge.report_break", + lambda *a, **k: FlowResult( + ok=True, + returncode=0, + stdout="Break kept LOCAL-ONLY: server rejected PHI boundary\n", + ), ) with pytest.raises(PhiBoundaryError): - report_break(run_dir, workflow_id="wf_1", token="oai_ingest_x", - allow_local_fallback=False) + report_break( + run_dir, workflow_id="wf_1", token="oai_ingest_x", allow_local_fallback=False + ) def test_not_logged_in(self, tmp_path: Path, fake_keyring) -> None: run_dir = tmp_path / "run" @@ -177,3 +408,25 @@ def test_not_logged_in(self, tmp_path: Path, fake_keyring) -> None: result = report_break(run_dir, workflow_id="wf_1") assert result["ok"] is False assert "Not logged in" in result["error"] + + def test_free_text_is_not_sent_by_desktop(self, tmp_path: Path, monkeypatch) -> None: + run_dir = tmp_path / "run" + secret = "patient Jane Doe has record 12345" + self._write_report(run_dir, {"reason": secret, "step_intent": secret}) + calls: list[tuple[tuple, dict]] = [] + monkeypatch.setattr( + "engine.hosted.FlowBridge.report_break", + lambda *args, **kwargs: ( + calls.append((args, kwargs)) + or FlowResult( + ok=True, + returncode=0, + stdout="Break reported (run_id=r1, halt_id=h1, status=halt).\n", + ) + ), + ) + + result = report_break(run_dir, workflow_id="wf_1", token="token") + + assert result["ok"] is True + assert secret not in repr(calls) diff --git a/tests/test_engine/test_review_state.py b/tests/test_engine/test_review_state.py index 9a30f69..897c426 100644 --- a/tests/test_engine/test_review_state.py +++ b/tests/test_engine/test_review_state.py @@ -9,8 +9,10 @@ from engine.db import IndexDB from engine.review import ( EGRESS_ALLOWED_STATES, + EgressArtifactError, EgressBlockedError, ReviewStatus, + approved_egress_path, check_egress_allowed, get_pending_reviews, transition_status, @@ -38,9 +40,9 @@ def test_all_states_defined(self) -> None: assert ReviewStatus.DELETED.value == "deleted" def test_egress_allowed_states(self) -> None: - """Only REVIEWED and DISMISSED should allow egress.""" + """Only REVIEWED should be eligible for egress.""" assert ReviewStatus.REVIEWED in EGRESS_ALLOWED_STATES - assert ReviewStatus.DISMISSED in EGRESS_ALLOWED_STATES + assert ReviewStatus.DISMISSED not in EGRESS_ALLOWED_STATES assert ReviewStatus.CAPTURED not in EGRESS_ALLOWED_STATES assert ReviewStatus.SCRUBBED not in EGRESS_ALLOWED_STATES assert ReviewStatus.DELETED not in EGRESS_ALLOWED_STATES @@ -81,11 +83,25 @@ class TestTransitionStatus: ], ) def test_valid_transitions( - self, db: IndexDB, from_status: ReviewStatus, to_status: ReviewStatus, + self, + db: IndexDB, + tmp_path: Path, + from_status: ReviewStatus, + to_status: ReviewStatus, ) -> None: """All valid transitions should succeed.""" - db.insert_capture("test-id", "/tmp/cap", "2026-03-02T10:00:00Z") + raw = tmp_path / "cap" + raw.mkdir() + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") db.update_capture("test-id", review_status=from_status.value) + if from_status == ReviewStatus.SCRUBBED and to_status == ReviewStatus.REVIEWED: + scrubbed = tmp_path / "cap.scrubbed" + scrubbed.mkdir() + (scrubbed / "scrub_manifest.json").write_text("{}") + (scrubbed / "review_status.json").write_text( + '{"status":"pending_review"}' + ) + db.update_capture("test-id", scrubbed_path=str(scrubbed)) transition_status("test-id", from_status, to_status, db=db) cap = db.get_capture("test-id") assert cap["review_status"] == to_status.value @@ -101,7 +117,9 @@ def test_valid_transitions( ], ) def test_invalid_transitions_raise( - self, from_status: ReviewStatus, to_status: ReviewStatus, + self, + from_status: ReviewStatus, + to_status: ReviewStatus, ) -> None: """Invalid transitions should raise ValueError.""" with pytest.raises(ValueError): @@ -111,18 +129,221 @@ def test_invalid_transitions_raise( class TestCheckEgress: """Tests for the egress check function.""" - def test_egress_allowed_reviewed(self, db: IndexDB) -> None: - """Reviewed captures should be allowed for egress.""" - db.insert_capture("test-id", "/tmp/cap", "2026-03-02T10:00:00Z") + def test_egress_allowed_reviewed(self, db: IndexDB, tmp_path: Path) -> None: + """Reviewed captures expose only the distinct sanitized derivative.""" + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + (scrubbed / "scrub_manifest.json").write_text( + '{"scrub_level":"standard"}' + ) + from engine.review import derivative_tree_sha256 + + (scrubbed / "review_status.json").write_text( + '{"status":"reviewed","approved_tree_sha256":"' + + derivative_tree_sha256(scrubbed) + + '"}' + ) + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") db.update_capture("test-id", review_status="reviewed") + db.update_capture("test-id", scrubbed_path=str(scrubbed)) assert check_egress_allowed("test-id", db) is True + assert approved_egress_path("test-id", db) == scrubbed.resolve() - def test_egress_allowed_dismissed(self, db: IndexDB) -> None: - """Dismissed captures should be allowed for egress.""" + def test_egress_blocked_dismissed(self, db: IndexDB) -> None: + """A local dismissal never grants raw-data egress.""" db.insert_capture("test-id", "/tmp/cap", "2026-03-02T10:00:00Z") db.update_capture("test-id", review_status="dismissed") + with pytest.raises(EgressBlockedError): + check_egress_allowed("test-id", db) + + def test_egress_blocked_when_reviewed_row_has_no_derivative( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + raw.mkdir() + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture("test-id", review_status="reviewed") + with pytest.raises(EgressArtifactError, match="no approved sanitized"): + check_egress_allowed("test-id", db) + + def test_egress_blocked_when_derivative_points_to_raw( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + raw.mkdir() + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture("test-id", review_status="reviewed", scrubbed_path=str(raw)) + with pytest.raises(EgressArtifactError, match="raw data"): + check_egress_allowed("test-id", db) + + def test_egress_blocked_when_derivative_contains_symlink( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + (scrubbed / "escape").symlink_to(raw, target_is_directory=True) + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture("test-id", review_status="reviewed", scrubbed_path=str(scrubbed)) + with pytest.raises(EgressArtifactError, match="symlink"): + check_egress_allowed("test-id", db) + + def test_review_binds_exact_derivative_bytes( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + (scrubbed / "scrub_manifest.json").write_text("{}") + (scrubbed / "review_status.json").write_text( + '{"status":"pending_review"}' + ) + artifact = scrubbed / "data.bin" + artifact.write_bytes(b"reviewed") + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", + review_status="scrubbed", + scrubbed_path=str(scrubbed), + ) + + transition_status( + "test-id", + ReviewStatus.SCRUBBED, + ReviewStatus.REVIEWED, + db=db, + ) assert check_egress_allowed("test-id", db) is True + artifact.write_bytes(b"changed after review") + with pytest.raises(EgressArtifactError, match="changed after"): + check_egress_allowed("test-id", db) + + def test_egress_blocks_hard_linked_derivative_file( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + source = tmp_path / "outside-secret" + source.write_bytes(b"secret") + (scrubbed / "linked").hardlink_to(source) + (scrubbed / "scrub_manifest.json").write_text("{}") + (scrubbed / "review_status.json").write_text("{}") + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", review_status="reviewed", scrubbed_path=str(scrubbed) + ) + + with pytest.raises(EgressArtifactError, match="hard-linked"): + check_egress_allowed("test-id", db) + + def test_basic_scrubbed_screenshot_is_blocked_even_without_raw_screenshot( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + from engine.review import derivative_tree_sha256 + + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + (scrubbed / "screenshots").mkdir(parents=True) + (scrubbed / "screenshots" / "frame.png").write_bytes(b"raw screenshot") + (scrubbed / "scrub_manifest.json").write_text('{"scrub_level":"basic"}') + (scrubbed / "review_status.json").write_text( + '{"status":"reviewed","approved_tree_sha256":"' + + derivative_tree_sha256(scrubbed) + + '"}' + ) + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", review_status="reviewed", scrubbed_path=str(scrubbed) + ) + + with pytest.raises(EgressArtifactError, match="Image-capable"): + check_egress_allowed("test-id", db) + + def test_regular_file_cannot_replace_derivative_directory( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.write_bytes(b"raw replacement") + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", review_status="reviewed", scrubbed_path=str(scrubbed) + ) + + with pytest.raises(EgressArtifactError, match="directory"): + check_egress_allowed("test-id", db) + + def test_approval_file_rejects_extra_free_text( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + from engine.review import derivative_tree_sha256 + + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + (scrubbed / "scrub_manifest.json").write_text( + '{"scrub_level":"standard"}' + ) + digest = derivative_tree_sha256(scrubbed) + (scrubbed / "review_status.json").write_text( + '{"status":"reviewed","approved_tree_sha256":"' + + digest + + '","secret":"Jane Doe record 12345"}' + ) + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", review_status="reviewed", scrubbed_path=str(scrubbed) + ) + + with pytest.raises(EgressArtifactError, match="schema"): + check_egress_allowed("test-id", db) + + def test_egress_blocked_when_derivative_is_symlink( + self, + db: IndexDB, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + derivative_target = tmp_path / "cap.scrubbed.target" + derivative_link = tmp_path / "cap.scrubbed" + raw.mkdir() + derivative_target.mkdir() + derivative_link.symlink_to(derivative_target, target_is_directory=True) + db.insert_capture("test-id", str(raw), "2026-03-02T10:00:00Z") + db.update_capture( + "test-id", + review_status="reviewed", + scrubbed_path=str(derivative_link), + ) + with pytest.raises(EgressArtifactError, match="symlink"): + check_egress_allowed("test-id", db) + def test_egress_blocked_captured(self, db: IndexDB) -> None: """Captured captures should be blocked from egress.""" db.insert_capture("test-id", "/tmp/cap", "2026-03-02T10:00:00Z") diff --git a/tests/test_engine/test_scrubber.py b/tests/test_engine/test_scrubber.py index 6041e5d..3501f64 100644 --- a/tests/test_engine/test_scrubber.py +++ b/tests/test_engine/test_scrubber.py @@ -223,6 +223,49 @@ def test_scrub_capture_writes_review_status( status_path = scrubbed_path / "review_status.json" assert status_path.exists() + def test_rescrub_atomically_replaces_stale_derivative_files( + self, sample_capture_dir: Path, + ) -> None: + scrubber = Scrubber(level=ScrubLevel.BASIC) + scrubbed_path = scrubber.scrub_capture(sample_capture_dir) + stale = scrubbed_path / "stale-secret.txt" + stale.write_text("Jane Doe account 12345") + + replacement = scrubber.scrub_capture(sample_capture_dir) + + assert replacement == scrubbed_path + assert not stale.exists() + assert (replacement / "scrub_manifest.json").exists() + + def test_manifest_never_contains_identity_bearing_local_path( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + capture = _capture_with_screenshot(tmp_path) + identity_name = "Jane-Doe-record-12345" + named_capture = capture.with_name(identity_name) + capture.replace(named_capture) + scrubber = Scrubber(level=ScrubLevel.STANDARD) + monkeypatch.setattr(scrubber, "_require_provider", lambda: object()) + monkeypatch.setattr( + scrubber, + "scrub_text", + lambda text: ("redacted", []), + ) + + def scrub_image(_source: Path, output: Path) -> list[dict]: + output.write_bytes(b"redacted") + return [{"type": "image_scrub"}] + + monkeypatch.setattr( + scrubber, + "scrub_image", + scrub_image, + ) + + derivative = scrubber.scrub_capture(named_capture) + + assert identity_name not in (derivative / "scrub_manifest.json").read_text() + def test_scrub_capture_nonexistent_raises(self) -> None: """Scrubbing a nonexistent path should raise FileNotFoundError.""" scrubber = Scrubber(level=ScrubLevel.BASIC) diff --git a/tests/test_engine/test_upload.py b/tests/test_engine/test_upload.py index 4721871..003610a 100644 --- a/tests/test_engine/test_upload.py +++ b/tests/test_engine/test_upload.py @@ -38,11 +38,29 @@ def mock_backend() -> MagicMock: return backend +def _write_review_files(path: Path) -> None: + from engine.review import derivative_tree_sha256 + + (path / "scrub_manifest.json").write_text('{"scrub_level":"standard"}') + (path / "review_status.json").write_text( + '{"status":"reviewed","approved_tree_sha256":"' + + derivative_tree_sha256(path) + + '"}' + ) + + +def _config(tmp_path: Path) -> EngineConfig: + return EngineConfig(data_dir=tmp_path / "openadapt-data") + + class TestUploadManager: """Tests for UploadManager operations.""" def test_enqueue_checks_egress( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, ) -> None: """Enqueue should block unreviewed captures.""" db.insert_capture("cap1", "/tmp/cap1", "2026-03-01T10:00:00Z") @@ -52,74 +70,159 @@ def test_enqueue_checks_egress( manager.enqueue("cap1", "test_backend") def test_enqueue_valid_creates_job( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: """Enqueue should create a job for reviewed captures.""" - db.insert_capture("cap1", "/tmp/cap1", "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) job_id = manager.enqueue("cap1", "test_backend") assert job_id is not None jobs = db.get_pending_jobs() assert len(jobs) == 1 + assert jobs[0]["completed_at"] is None def test_enqueue_invalid_backend_raises( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: """Enqueue with unknown backend should raise ValueError.""" - db.insert_capture("cap1", "/tmp/cap1", "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) with pytest.raises(ValueError, match="Backend not available"): manager.enqueue("cap1", "nonexistent") def test_get_queue_status( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: """Queue status should return pending jobs.""" - db.insert_capture("cap1", "/tmp/cap1", "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) manager.enqueue("cap1", "test_backend") status = manager.get_queue_status() assert len(status) == 1 def test_process_queue_calls_backend( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, tmp_path: Path, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: """Processing queue should call the backend upload.""" cap_dir = tmp_path / "captures" / "test_cap" cap_dir.mkdir(parents=True) - (cap_dir / "data.bin").write_bytes(b"test") + (cap_dir / "data.bin").write_bytes(b"raw-secret") + scrubbed_dir = tmp_path / "captures" / "test_cap.scrubbed" + scrubbed_dir.mkdir() + (scrubbed_dir / "data.bin").write_bytes(b"sanitized") + _write_review_files(scrubbed_dir) db.insert_capture("cap1", str(cap_dir), "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed_dir)) + uploaded: dict[str, bytes] = {} + + def inspect_archive(path: Path, _metadata: dict) -> UploadResult: + import zipfile + + with zipfile.ZipFile(path) as archive: + uploaded["data"] = archive.read("data.bin") + return UploadResult(success=True, remote_url="test://uploaded", bytes_sent=100) + + mock_backend.upload.side_effect = inspect_archive + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) manager.enqueue("cap1", "test_backend") results = manager.process_queue() assert len(results) == 1 assert results[0]["success"] is True + assert uploaded["data"] == b"sanitized" mock_backend.upload.assert_called_once() + job = db.get_jobs_for_capture("cap1")[0] + assert not Path(job["archive_path"]).exists() - def test_upload_logs_audit( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, tmp_path: Path, + def test_queue_upload_logs_audit( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: """Upload should log to audit trail.""" - archive = tmp_path / "test.tar.gz" - archive.write_bytes(b"fake archive") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) - result = manager.upload(archive, "test_backend", {"capture_id": "cap1"}) - assert result.success + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + (scrubbed / "data.bin").write_bytes(b"sanitized") + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) + manager.enqueue("cap1", "test_backend") + result = manager.process_queue()[0] + assert result["success"] is True # Verify audit log was written assert audit.log_path.exists() def test_get_active_backends( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, ) -> None: """Active backends should return configured backend names.""" manager = UploadManager(EngineConfig(), [mock_backend], db, audit) assert "test_backend" in manager.get_active_backends() + def test_direct_hosted_backend_is_refused_even_when_injected( + self, + db: IndexDB, + audit: AuditLogger, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + backend = MagicMock() + backend.name = "hosted_ingest" + manager = UploadManager(_config(tmp_path), [backend], db, audit) + + with pytest.raises(ValueError, match="Direct hosted ingest is disabled"): + manager.enqueue("cap1", "hosted_ingest") + + backend.upload.assert_not_called() + class TestDurableRetry: """Durable/offline queue behavior (spec section 5).""" @@ -128,19 +231,26 @@ def _prepare(self, db: IndexDB, tmp_path: Path): cap_dir = tmp_path / "captures" / "cap" cap_dir.mkdir(parents=True) (cap_dir / "data.bin").write_bytes(b"x") + scrubbed_dir = tmp_path / "captures" / "cap.scrubbed" + scrubbed_dir.mkdir() + (scrubbed_dir / "data.bin").write_bytes(b"sanitized") + _write_review_files(scrubbed_dir) db.insert_capture("cap1", str(cap_dir), "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed_dir)) def test_transient_failure_requeues_with_backoff( - self, db: IndexDB, audit: AuditLogger, tmp_path: Path, + self, + db: IndexDB, + audit: AuditLogger, + tmp_path: Path, ) -> None: self._prepare(db, tmp_path) backend = MagicMock() - backend.name = "hosted_ingest" + backend.name = "test_backend" backend.upload.return_value = UploadResult(success=False, error="network down") - manager = UploadManager(EngineConfig(), [backend], db, audit) - manager.enqueue("cap1", "hosted_ingest") + manager = UploadManager(_config(tmp_path), [backend], db, audit) + manager.enqueue("cap1", "test_backend") manager.process_queue() # Job is offline-deferred, not permanently failed. @@ -151,27 +261,172 @@ def test_transient_failure_requeues_with_backoff( assert job["next_retry_at"] is not None def test_permanent_failure_after_max_attempts( - self, db: IndexDB, audit: AuditLogger, tmp_path: Path, + self, + db: IndexDB, + audit: AuditLogger, + tmp_path: Path, ) -> None: self._prepare(db, tmp_path) backend = MagicMock() - backend.name = "hosted_ingest" + backend.name = "test_backend" backend.upload.return_value = UploadResult(success=False, error="network down") - manager = UploadManager(EngineConfig(), [backend], db, audit, max_attempts=1) - manager.enqueue("cap1", "hosted_ingest") + manager = UploadManager(_config(tmp_path), [backend], db, audit, max_attempts=1) + manager.enqueue("cap1", "test_backend") manager.process_queue() job = db.get_jobs_for_capture("cap1")[0] assert job["status"] == "failed" + def test_existing_legacy_hosted_job_is_failed_without_network( + self, + db: IndexDB, + audit: AuditLogger, + tmp_path: Path, + ) -> None: + self._prepare(db, tmp_path) + backend = MagicMock() + backend.name = "hosted_ingest" + db.insert_upload_job("legacy-job", "cap1", "hosted_ingest") + manager = UploadManager(_config(tmp_path), [backend], db, audit) + + result = manager.process_queue()[0] + + assert result["success"] is False + assert "Direct hosted ingest is disabled" in result["error"] + assert db.get_jobs_for_capture("cap1")[0]["status"] == "failed" + backend.upload.assert_not_called() + def test_missing_path_is_permanent( - self, db: IndexDB, audit: AuditLogger, mock_backend: MagicMock, + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, ) -> None: - db.insert_capture("cap1", "/no/such/path", "2026-03-01T10:00:00Z") - db.update_capture("cap1", review_status="reviewed") - manager = UploadManager(EngineConfig(), [mock_backend], db, audit) + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) manager.enqueue("cap1", "test_backend") + for path in scrubbed.iterdir(): + path.unlink() + scrubbed.rmdir() manager.process_queue() job = db.get_jobs_for_capture("cap1")[0] assert job["status"] == "failed" + mock_backend.upload.assert_not_called() + + def test_queue_revalidates_derivative_path_before_network_egress( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) + manager.enqueue("cap1", "test_backend") + + db.update_capture("cap1", scrubbed_path=str(raw)) + result = manager.process_queue()[0] + + assert result["success"] is False + assert "raw data" in result["error"] + mock_backend.upload.assert_not_called() + + def test_post_review_mutation_is_refused_and_does_not_strand_job( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap" + scrubbed = tmp_path / "cap.scrubbed" + raw.mkdir() + scrubbed.mkdir() + artifact = scrubbed / "data.bin" + artifact.write_bytes(b"reviewed") + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) + manager.enqueue("cap1", "test_backend") + + artifact.write_bytes(b"changed") + result = manager.process_queue()[0] + + assert result["success"] is False + assert "changed after" in result["error"] + assert db.get_jobs_for_capture("cap1")[0]["status"] == "failed" + mock_backend.upload.assert_not_called() + + def test_mutated_frozen_archive_is_refused( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, + ) -> None: + self._prepare(db, tmp_path) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) + manager.enqueue("cap1", "test_backend") + job = db.get_jobs_for_capture("cap1")[0] + Path(job["archive_path"]).write_bytes(b"tampered") + + result = manager.process_queue()[0] + + assert result["success"] is False + assert "changed after enqueue" in result["error"] + assert db.get_jobs_for_capture("cap1")[0]["status"] == "failed" + mock_backend.upload.assert_not_called() + + def test_interrupted_job_returns_to_queue_after_restart( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, + ) -> None: + self._prepare(db, tmp_path) + first = UploadManager(_config(tmp_path), [mock_backend], db, audit) + job_id = first.enqueue("cap1", "test_backend") + db.update_upload_job(job_id, status="in_progress") + + restarted = UploadManager(_config(tmp_path), [mock_backend], db, audit) + result = restarted.process_queue()[0] + + assert result["success"] is True + assert db.get_jobs_for_capture("cap1")[0]["status"] == "completed" + + def test_freeze_failure_does_not_create_pending_job( + self, + db: IndexDB, + audit: AuditLogger, + mock_backend: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + self._prepare(db, tmp_path) + manager = UploadManager(_config(tmp_path), [mock_backend], db, audit) + monkeypatch.setattr( + manager, + "_freeze_artifact", + lambda *_args, **_kwargs: (_ for _ in ()).throw(OSError("disk full")), + ) + + with pytest.raises(OSError, match="disk full"): + manager.enqueue("cap1", "test_backend") + + assert db.get_jobs_for_capture("cap1") == [] From 5c746eb365e2d137a9e66ea9302b3fb230380707 Mon Sep 17 00:00:00 2001 From: abrichr Date: Tue, 18 Aug 2026 12:29:57 -0400 Subject: [PATCH 2/6] fix: close residual local artifact leaks --- README.md | 9 ++++++--- engine/cli.py | 6 +++++- engine/upload_manager.py | 27 +++++++++++++++++++++++++++ tests/test_engine/test_upload.py | 22 ++++++++++++++++++++++ 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 6539866..7980b83 100644 --- a/README.md +++ b/README.md @@ -223,7 +223,8 @@ pinned sources, hashes, and modification status are recorded in The legacy `upload --backend hosted_ingest` command is a compatibility alias for the supported governed `push` path. It does not call the old direct ingest -adapter. Optional customer-owned storage adapters remain separate legacy paths. +adapter. Customer-owned storage upload is paused until it uses Flow's complete +inventory, image-capable sanitization, and exact in-app review contract. ## Architecture @@ -285,7 +286,7 @@ The Python engine exposes these Beta commands: | `openadapt-desktop compile` / `replay` / `run` | Invoke the bundled, pinned `openadapt-flow` runtime on a capture or bundle | | `openadapt-desktop login` / `push` / `report-break` | Authenticate to the hosted control plane, push a bundle, report a halted run | | `openadapt-desktop storage` / `health` / `cleanup` | Inspect and maintain local storage | -| `openadapt-desktop backends` / `upload` | Inspect legacy customer-owned storage adapters; the hosted alias uses governed `push` | +| `openadapt-desktop backends` / `upload` | Inspect legacy customer-owned storage adapters; hosted uses governed `push`, and customer-owned upload remains paused behind a fail-closed release gate | | `openadapt-desktop config` / `doctor` | Inspect local configuration and dependencies | Raw recordings are local by default. Any egress path still requires careful @@ -298,7 +299,9 @@ derivative contract. It distinguishes a local review pause from an upload. It requires a returned hosted workflow identity before it reports success. It never falls back to a direct Desktop upload when Flow is missing or returns an error. The former direct hosted-ingest backend now refuses every upload. The -legacy customer-owned adapter queue selects the reviewed scrubbed path again +legacy customer-owned adapter queue remains paused for this release; its exit +condition is a Flow-owned complete inventory, image-capable scrub, and exact +in-app review. The dormant queue also selects the reviewed scrubbed path again immediately before egress; a dismissed raw capture is not uploadable. ## Development diff --git a/engine/cli.py b/engine/cli.py index 23e9fe9..3108cb1 100644 --- a/engine/cli.py +++ b/engine/cli.py @@ -248,7 +248,11 @@ def cmd_upload(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: backends = _create_backends(engine.config) manager = UploadManager(engine.config, backends, engine.db, engine.audit) - job_id = manager.enqueue(args.capture_id, args.backend) + try: + job_id = manager.enqueue(args.capture_id, args.backend) + except ValueError as exc: + print(f"Upload refused: {exc}") + sys.exit(1) print(f"Upload queued: job {job_id[:8]}") results = manager.process_queue() diff --git a/engine/upload_manager.py b/engine/upload_manager.py index 8a082b6..c8350c3 100644 --- a/engine/upload_manager.py +++ b/engine/upload_manager.py @@ -57,6 +57,13 @@ "Direct hosted ingest is disabled. Use `openadapt-desktop push` so Flow can " "bind the upload to a reviewed, exact-hash sanitized artifact." ) +_PAUSED_STORAGE_BACKENDS = frozenset({"s3"}) +_PAUSED_STORAGE_ERROR = ( + "Customer-storage upload is paused for this release. Its exit condition is a " + "Flow-owned complete artifact inventory, image-capable sanitization, and an " + "in-app exact-artifact review contract. Use governed hosted push or keep the " + "recording local." +) _APPROVED_ARCHIVE_RE = re.compile( r"^(?P[a-f0-9]{32})-(?P[a-f0-9]{64})\.approved\.zip$" ) @@ -124,6 +131,8 @@ def enqueue(self, capture_id: str, backend_name: str) -> str: """ if backend_name in _FLOW_ONLY_BACKENDS: raise ValueError(_FLOW_ONLY_ERROR) + if backend_name in _PAUSED_STORAGE_BACKENDS: + raise ValueError(_PAUSED_STORAGE_ERROR) if backend_name not in self.backends: raise ValueError(f"Backend not available: {backend_name}") @@ -156,6 +165,8 @@ def _upload_frozen( """Send only an artifact returned by the queue's hash verifier.""" if backend_name in _FLOW_ONLY_BACKENDS: return UploadResult(success=False, error=_FLOW_ONLY_ERROR) + if backend_name in _PAUSED_STORAGE_BACKENDS: + return UploadResult(success=False, error=_PAUSED_STORAGE_ERROR) backend = self.backends[backend_name] size_bytes = artifact.path.stat().st_size dest = f"{backend_name}://{metadata.get('capture_id', 'unknown')}" @@ -322,6 +333,22 @@ def process_queue(self) -> list[dict]: self._result(job_id, capture_id, backend_name, False, "", _FLOW_ONLY_ERROR) ) continue + if backend_name in _PAUSED_STORAGE_BACKENDS: + self._db.update_upload_job( + job_id, status="failed", error=_PAUSED_STORAGE_ERROR + ) + self._cleanup_job_archive(job) + results.append( + self._result( + job_id, + capture_id, + backend_name, + False, + "", + _PAUSED_STORAGE_ERROR, + ) + ) + continue capture = self._db.get_capture(capture_id) if not capture: diff --git a/tests/test_engine/test_upload.py b/tests/test_engine/test_upload.py index 003610a..b4d33e3 100644 --- a/tests/test_engine/test_upload.py +++ b/tests/test_engine/test_upload.py @@ -223,6 +223,28 @@ def test_direct_hosted_backend_is_refused_even_when_injected( backend.upload.assert_not_called() + def test_customer_storage_is_paused_without_network( + self, + db: IndexDB, + audit: AuditLogger, + tmp_path: Path, + ) -> None: + raw = tmp_path / "cap1" + scrubbed = tmp_path / "cap1.scrubbed" + raw.mkdir() + scrubbed.mkdir() + _write_review_files(scrubbed) + db.insert_capture("cap1", str(raw), "2026-03-01T10:00:00Z") + db.update_capture("cap1", review_status="reviewed", scrubbed_path=str(scrubbed)) + backend = MagicMock() + backend.name = "s3" + manager = UploadManager(_config(tmp_path), [backend], db, audit) + + with pytest.raises(ValueError, match="paused for this release"): + manager.enqueue("cap1", "s3") + + backend.upload.assert_not_called() + class TestDurableRetry: """Durable/offline queue behavior (spec section 5).""" From fb9e281a3ee31d220ac2fb3d650341b74799a6b7 Mon Sep 17 00:00:00 2001 From: abrichr Date: Tue, 18 Aug 2026 13:49:35 -0400 Subject: [PATCH 3/6] fix: close hosted handoff and runner safety gaps --- README.md | 21 +- docs/POLICY_SYNC.md | 72 +-- engine/auth/store.py | 64 +- engine/backends/hosted_ingest.py | 2 +- engine/cli.py | 2 +- engine/dispatch.py | 162 +++++- engine/flow_bridge.py | 4 + engine/hosted.py | 104 ++-- engine/policy.py | 249 +++++++- engine/qualification_lifecycle.py | 508 ++++++++++++++-- engine/review.py | 42 +- engine/runner_loop.py | 547 ++++++++++++++---- engine/upload_manager.py | 20 +- src-tauri/src/commands.rs | 73 ++- src/lib/engine.ts | 8 +- src/screens/Settings.tsx | 30 +- tests/test_engine/test_auth_store.py | 41 ++ tests/test_engine/test_dispatch.py | 113 ++++ tests/test_engine/test_flow_bridge.py | 13 + tests/test_engine/test_hosted.py | 220 +++++-- tests/test_engine/test_policy.py | 217 ++++++- .../test_engine/test_push_result_contract.py | 249 ++++++++ tests/test_engine/test_review_state.py | 21 + tests/test_engine/test_runner_loop.py | 220 ++++++- 24 files changed, 2602 insertions(+), 400 deletions(-) create mode 100644 tests/test_engine/test_push_result_contract.py diff --git a/README.md b/README.md index 7980b83..ab9b83f 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ teaching, escalation, and terminal receipts. | Python sidecar IPC | JSON-lines handler backed by a shared `EngineDispatcher` (recording, compile/replay/run/teach, auth, sync/push, review, config) | Beta; unit and e2e tests with mocked boundaries | | Tray IPC socket server | Token-authenticated loopback TCP server plus a `~/.openadapt/desktop_ipc.json` discovery file for `openadapt-tray` | Beta; not yet validated end to end against the shipped tray | | Desktop-to-flow handoff | `FlowBridge` launches the pinned Flow runtime embedded in the frozen sidecar as an isolated subprocess | Self-contained; no separate Python or Flow installation | -| Hosted auth and push | Browser-PKCE and paste-token sign-in, keychain-stored credential, bundle push, and halted-run break reports to the hosted control plane | Beta | +| Hosted auth and governed handoff | Browser-PKCE and paste-token sign-in; host-bound keychain credentials; exact `openadapt.push-result/v1` review, accepted-ingest, and uncertain-delivery state; local handoff retention; and halted-run break reports | Beta implementation candidate; distribution requires a release-qualified Flow build and live Cloud acceptance before Desktop updates its exact runtime pin | | Attended phone decisions | One-use QR pairing, protected local evidence, typed allowed actions, runner revalidation, receipts, device revocation, and an optional outbound hosted lane | Beta; device pairing does not replace the deployment's authenticated operator principal | | Build artifacts | Wheel/sdist, a self-contained PyInstaller engine+Flow runtime, and DMG/MSI/NSIS/DEB/AppImage native jobs | Native jobs prove the frozen browser lifecycle, structurally install/uninstall, and label every platform, architecture, and signing state | | Native installers | Distinct `desktop-v*` draft-prerelease workflow with final-byte checksums and GitHub provenance, auto-triggered at each engine release | Beta distribution lane; signing state is encoded in every filename and workflow qualification remains specific | @@ -294,12 +294,19 @@ review of the selected adapter, configuration, logs, and data-classification policy. This repository does not by itself establish a HIPAA-compliant or production-safe deployment. -The supported `push` command delegates to Flow's exact-hash sanitized -derivative contract. It distinguishes a local review pause from an upload. It -requires a returned hosted workflow identity before it reports success. It -never falls back to a direct Desktop upload when Flow is missing or returns an -error. The former direct hosted-ingest backend now refuses every upload. The -legacy customer-owned adapter queue remains paused for this release; its exit +The governed `push` implementation delegates to Flow's exact-hash sanitized +derivative contract. It consumes the closed `openadapt.push-result/v1` schema +and retains the exact review or ingest handoff locally. A recording acceptance +requires the server-owned `artifact_ingest_id` and a governed next action. A +bundle acceptance additionally requires the server-owned workflow identity, +the runtime-attestation binding, and the exact trusted dashboard path. An +unknown child or delivery outcome requires reconciliation and never becomes an +automatic retry. The command never falls back to a direct Desktop upload when +Flow is missing or returns an error. The former direct hosted-ingest backend +now refuses every upload. The +This path does not enter a native release until the exact pinned Flow artifact +and the managed Cloud runtime pass the same live acceptance contract. The legacy +customer-owned adapter queue remains paused for this release; its exit condition is a Flow-owned complete inventory, image-capable scrub, and exact in-app review. The dormant queue also selects the reviewed scrubbed path again immediately before egress; a dismissed raw capture is not uploadable. diff --git a/docs/POLICY_SYNC.md b/docs/POLICY_SYNC.md index fb35794..5028f54 100644 --- a/docs/POLICY_SYNC.md +++ b/docs/POLICY_SYNC.md @@ -10,16 +10,17 @@ the cloud control plane. Policy is layered in three tiers: | 3 | `safety` | Safety guardrails that gate whether a run may proceed | Org admins, **via the cloud API only** | The engine owns the **fetch + cache + fail-closed** half of this contract -(`engine/policy.py`, this PR). The frontend owns the **read-only rendering + -Tier-1 editing** half (`src/screens/Settings.tsx` on branch -`feat/tauri-frontend` — see the TODO section below; NOT built here). +(`engine/policy.py`). The frontend renders the effective policy read-only in +`src/screens/Settings.tsx`. Hosted organization and safety policy changes stay +in the authenticated Cloud administration surface. --- ## The contract: `GET /api/policy/effective` Bearer-authed (the same `Authorization: Bearer ` the rest of the hosted -loop uses, resolved by `engine.auth.store.auth_header()`). The cloud returns the +loop uses, resolved for the exact destination by +`engine.auth.store.auth_header(host)`). The cloud returns the org's fully-resolved policy: ```jsonc @@ -72,19 +73,25 @@ of every safety key. A missing or unreachable value **always resolves to these** ## Cache location -The **raw** last-known-good response body is cached at: +The last-known-good response body is cached in a closed envelope at: ``` ~/.openadapt/policy.json ``` -(the same `~/.openadapt/` directory as `config.toml`). Writes are **atomic** +(the same `~/.openadapt/` directory as `config.toml`). The envelope binds the +policy to the canonical hosted origin, a domain-separated credential digest, +the organization id, the policy version, the exact policy-body digest, and the +fetch time. A different host, credential, organization, version, or body cannot +reuse it. A network response cannot move the version backwards or change a body +without a new version. The cache expires after 24 hours. The old unbound cache +format is rejected. Writes are **atomic** (temp file in the same dir + `os.replace`), so a reader can never observe a half-written file. The path is overridable in tests via `OPENADAPT_POLICY_CACHE`. -The cache stores the raw server body **without** the synthetic `source` field — -`source` is added only on the in-memory resolved result. +The nested policy body does not contain the synthetic `source` field. `source` +is added only to the in-memory resolved result. --- @@ -92,8 +99,7 @@ The cache stores the raw server body **without** the synthetic `source` field Policy is refreshed: -- **on app start** (warm the cache before the first render); -- **on a 300s (5-minute) interval** while the app runs; and +- **when Settings loads** (the first policy render performs a network-first resolve); - **immediately before a run** (a run must never start on a stale safety view). `refresh_policy` (dispatcher command) forces a network fetch and rewrites the @@ -190,7 +196,7 @@ config contents, paths, or exception text. |--------|---------| | `engine/policy.py :: SAFE_SAFETY_DEFAULTS` | Safest value for every safety key | | `engine/policy.py :: fetch_effective_policy(host, timeout=10.0)` | Bearer GET, atomic-cache-write, raises `PolicyFetchError` on failure | -| `engine/policy.py :: load_cached_policy()` | Read `~/.openadapt/policy.json`; `None` on any error (degrade-not-raise) | +| `engine/policy.py :: load_cached_policy(host)` | Read only a fresh cache bound to this exact host and credential; `None` on any mismatch or error | | `engine/policy.py :: harden_safety(policy)` | Fill missing/`None` safety keys with safe defaults (fail-closed) | | `engine/policy.py :: resolve_effective_policy(host)` | network → cache → fail-closed, always hardened, adds `source` | | `engine/policy.py :: SAFETY_VALUE_DOMAINS` | Exact allowed values per safety key (mirrors the cloud registry) | @@ -206,41 +212,11 @@ and the tray loopback (`engine/socket_server.py`) because they are registered in --- -## TODO — FRONTEND (NOT done here; lives on `feat/tauri-frontend`) +## Frontend behavior -The read-only rendering + Tier-1 editing half of this contract is **not** part of -this PR because the real frontend lives on a different branch -(`feat/tauri-frontend`, worktree `.worktrees/app`). To complete the loop there: - -1. **Add the command name** to `src/lib/engine.ts` `CMD` (e.g. - `GET_EFFECTIVE_POLICY: "get_effective_policy"`, `REFRESH_POLICY: "refresh_policy"`) - and a `Policy` TypeScript type mirroring the contract above (`safety` keys, - `role`, `is_admin`, `source`). - -2. **Extend `src/screens/Settings.tsx`** to call `get_effective_policy` on mount - (and `refresh_policy` on demand), then render three sections reusing the - existing primitives from `src/ui/primitives.tsx` — `Field`, `SegControl`, and - `Callout` (all already imported in `Settings.tsx` today): - - - **Tier-1 `user`** → render as **editable** controls; persist each change via - the existing `set_config` command (the same path the lane/PHI settings - already use). These are per-user preferences. - - - **Tier-2 `org`** → render as **read-only** cards **unless** `policy.is_admin`. - Admin edits go to the **cloud API**, never `set_config` — the desktop must - not write org policy locally. - - - **Tier-3 `safety`** → render as **read-only** cards **unless** - `policy.is_admin`; likewise admin edits go to the cloud API only. Use a - `Callout` to explain when a card is locked (non-admin) and when the view is - running on `source === "fail-closed-default"` (control plane unreachable — - safest values are in force, and a consequential run should be blocked until - policy can be confirmed). - -3. **Surface `source`** in the UI (`network` / `cache` / `fail-closed-default`) - so the user can tell whether they are looking at a live, cached, or - safest-default policy. - -The engine side (fetch, cache, fail-closed hardening, and the two dispatcher -commands) is fully implemented and tested in this PR; the frontend work above is -the only remaining piece of the unified policy-sync system. +`src/lib/engine.ts` carries the effective-policy commands and +`src/screens/Settings.tsx` loads the policy on mount. The Desktop renders the +resolved grounding and governance state. It does not grant local authority to +edit organization or safety policy. Host changes pass through the engine's +canonical HTTPS-origin validator. A refused host value returns a visible error +and the UI restores the last saved engine configuration. diff --git a/engine/auth/store.py b/engine/auth/store.py index cb4e116..f791a7f 100644 --- a/engine/auth/store.py +++ b/engine/auth/store.py @@ -40,6 +40,7 @@ import json import os +from urllib.parse import urlsplit from loguru import logger @@ -707,7 +708,64 @@ def clear_runner_credential(host: str) -> None: _kr_delete(_keyring(), host + _RUNNER_SUFFIX) -def auth_header() -> dict[str, str]: +def canonical_host_origin(host: str) -> str: + """Return a safe web origin for credential binding, or ``""``. + + Keychain credentials must never follow URL user-info or cross a clear-text + remote transport. Local HTTP remains available for development. + """ + + try: + parsed = urlsplit(host.strip()) + scheme = parsed.scheme.lower() + if ( + scheme not in {"http", "https"} + or not parsed.hostname + or parsed.username is not None + or parsed.password is not None + ): + return "" + port = parsed.port + except (ValueError, AttributeError): + return "" + hostname = parsed.hostname.lower() + if scheme == "http" and hostname not in {"localhost", "127.0.0.1", "::1"}: + return "" + default_port = (scheme == "https" and port == 443) or ( + scheme == "http" and port == 80 + ) + authority = f"[{hostname}]" if ":" in hostname else hostname + if port is not None and not default_port: + authority = f"{authority}:{port}" + return f"{scheme}://{authority}" + + +def token_for_host(host: str, *, explicit: str | None = None) -> str: + """Resolve a bearer only for the requested destination origin. + + An explicit argument or environment token is an operator-scoped override. + A keychain credential remains bound to the origin recorded at login. + """ + + requested_origin = canonical_host_origin(host) + if not requested_origin: + return "" + if explicit and explicit.strip(): + return explicit.strip() + env_token = os.environ.get(INGEST_TOKEN_ENV, "").strip() + if env_token: + return env_token + credential = active_credential() + if ( + requested_origin + and credential + and canonical_host_origin(str(credential.get("host") or "")) == requested_origin + ): + return str(credential.get("token") or "").strip() + return "" + + +def auth_header(host: str | None = None) -> dict[str, str]: """Resolve the active bearer token to an HTTP ``Authorization`` header. Resolution order (spec section 3e): ``OPENADAPT_INGEST_TOKEN`` env, then @@ -717,6 +775,10 @@ def auth_header() -> dict[str, str]: Returns: ``{"Authorization": "Bearer "}`` or ``{}``. """ + if host is not None: + token = token_for_host(host) + return {"Authorization": f"Bearer {token}"} if token else {} + env_token = os.environ.get(INGEST_TOKEN_ENV, "").strip() if env_token: return {"Authorization": f"Bearer {env_token}"} diff --git a/engine/backends/hosted_ingest.py b/engine/backends/hosted_ingest.py index 30b62a3..91d2186 100644 --- a/engine/backends/hosted_ingest.py +++ b/engine/backends/hosted_ingest.py @@ -61,7 +61,7 @@ def list_uploads(self) -> list[UploadRecord]: def verify_credentials(self) -> bool: """True when a bearer token is resolvable from the auth store/env.""" - return "Authorization" in auth_header() + return "Authorization" in auth_header(self.host) def estimate_cost(self, size_bytes: int) -> float | None: """Hosted ingest has no per-upload storage cost surfaced to the client.""" diff --git a/engine/cli.py b/engine/cli.py index 3108cb1..3a0a973 100644 --- a/engine/cli.py +++ b/engine/cli.py @@ -752,7 +752,7 @@ def cmd_doctor(args: argparse.Namespace, engine: types.SimpleNamespace) -> None: # Hosted credential from engine.auth.store import auth_header - logged_in = "Authorization" in auth_header() + logged_in = "Authorization" in auth_header(engine.config.hosted_host) checks.append( ( "Hosted credential", diff --git a/engine/dispatch.py b/engine/dispatch.py index 7b67350..a144806 100644 --- a/engine/dispatch.py +++ b/engine/dispatch.py @@ -38,6 +38,7 @@ from datetime import datetime, timezone from pathlib import Path from typing import Any, Callable +from urllib.parse import urlsplit from loguru import logger @@ -67,6 +68,25 @@ def _noop_emit(event: str, data: dict) -> None: """Default event sink -- drops events when no emitter is wired.""" +def _canonical_hosted_origin(value: Any) -> str: + """Return a safe hosted origin or raise before any credential is sent.""" + + from engine.auth.store import canonical_host_origin + + raw_value = str(value or "").strip() + parsed_value = urlsplit(raw_value) + if parsed_value.scheme.lower() == "http" and parsed_value.hostname not in { + "localhost", + "127.0.0.1", + "::1", + }: + raise ValueError("A remote hosted origin must use HTTPS") + normalized = canonical_host_origin(raw_value) + if not normalized: + raise ValueError("Host must be an HTTP(S) origin") + return normalized + + @dataclass class _ActiveFlowRecording: capture_id: str @@ -2541,7 +2561,10 @@ def deploy_qualification_workflow(self, **params: Any) -> dict: from engine.auth.store import auth_header from engine.bundle_keys import bundle_key_environment from engine.qualification import inspect_bundle - from engine.qualification_lifecycle import parse_flow_push + from engine.qualification_lifecycle import ( + parse_flow_push, + persist_deployment_handoff, + ) workflow_id = str(params.get("workflow_id") or "") try: @@ -2563,23 +2586,49 @@ def deploy_qualification_workflow(self, **params: Any) -> dict: if not self.services.flow_bridge.supports_command("push"): raise ValueError("The bundled Flow runtime does not support governed deployment") env = bundle_key_environment(workflow_id) - authorization = auth_header().get("Authorization", "") + authorization = auth_header(self.config.hosted_host).get("Authorization", "") if authorization.startswith("Bearer "): env["OPENADAPT_INGEST_TOKEN"] = authorization.removeprefix("Bearer ") - pushed = self.services.flow_bridge.push( - bundle, - kind="bundle", - host=self.config.hosted_host, - env_overrides=env, - ) - result = parse_flow_push(pushed.stdout, pushed.stderr, ok=pushed.ok) - result["workflow_id"] = result.get("workflow_id") or workflow_id + try: + pushed = self.services.flow_bridge.push( + bundle, + kind="bundle", + host=self.config.hosted_host, + env_overrides=env, + json_output=True, + ) + result = parse_flow_push( + pushed.stdout, + pushed.stderr, + ok=pushed.ok, + expected_host=self.config.hosted_host, + ) + except Exception: + # The child can fail after it dispatched a request. No raw + # exception or blind retry can replace reconciliation. + result = parse_flow_push("", "", ok=False) + try: + handoff = persist_deployment_handoff( + self.config.data_dir, + local_workflow_id=workflow_id, + result=result, + ) + result["handoff_path"] = str(handoff) + except Exception: + result["ok"] = False + result["local_persistence_error"] = True + result["error"] = ( + "Desktop could not retain the deployment handoff. Do not retry an " + "attempted upload. Reconcile the exact artifact in Cloud." + ) if result.get("deployed"): + hosted_workflow_id = result["workflow_id"] self.services.db.update_bundle( workflow_id, - workflow_id=result["workflow_id"], + workflow_id=hosted_workflow_id, status="deployed", ) + result["local_workflow_id"] = workflow_id return result except Exception as exc: return {"ok": False, "workflow_id": workflow_id, "error": str(exc)} @@ -2589,6 +2638,7 @@ def deploy_qualification_workflow(self, **params: Any) -> dict: def push_workflow(self, **params: Any) -> dict: """Start a governed push and preserve a required local-review pause.""" from engine import hosted + from engine.qualification_lifecycle import persist_deployment_handoff workflow_id = params.get("workflow_id") bundle = self._bundle_dir(workflow_id) @@ -2603,6 +2653,21 @@ def push_workflow(self, **params: Any) -> dict: db=self.services.db, bundle_id=workflow_id, ) + try: + handoff = persist_deployment_handoff( + self.config.data_dir, + local_workflow_id=str(workflow_id), + result=result, + ) + result["handoff_path"] = str(handoff) + except Exception: + result["local_persistence_error"] = True + if result.get("success"): + result["success"] = False + result["error"] = ( + "Desktop could not retain the deployment handoff. Do not retry an " + "attempted upload. Reconcile the exact artifact in Cloud." + ) except Exception as exc: self._emit_sync("offline") return {"ok": False, "error": str(exc), "workflow_id": ""} @@ -2619,6 +2684,11 @@ def push_workflow(self, **params: Any) -> dict: "sanitized_path": result.get("sanitized_path", ""), "review_command": result.get("review_command", ""), "workflow_id": result.get("workflow_id", ""), + "artifact_ingest_id": result.get("artifact_ingest_id"), + "next_action": result.get("next_action"), + "delivery": result.get("delivery"), + "handoff_path": result.get("handoff_path", ""), + "local_persistence_error": bool(result.get("local_persistence_error")), "dashboard_url": result.get("dashboard_url", ""), "error": result.get("error", ""), } @@ -2656,23 +2726,35 @@ def login_browser(self, **params: Any) -> dict: """Log in via the browser-PKCE provider; return an ``AuthStatus``.""" from engine import auth - host = params.get("host") or self.config.hosted_host try: + host = _canonical_hosted_origin( + params.get("host") or self.config.hosted_host + ) cred = auth.login(host=host, prefer="browser_pkce") + if _canonical_hosted_origin(cred.get("host")) != host: + raise ValueError("Authenticated host did not match the requested host") except Exception as exc: return {"authenticated": False, "error": str(exc)} + self.config.hosted_host = host + self._persist_config_key("hosted_host", host) return self._auth_status(cred) def login_paste(self, **params: Any) -> dict: """Log in with a pasted ingest token; return an ``AuthStatus``.""" from engine.auth.paste import PasteTokenProvider - host = params.get("host") or self.config.hosted_host token = params.get("token") try: + host = _canonical_hosted_origin( + params.get("host") or self.config.hosted_host + ) cred = PasteTokenProvider(host=host).login(token=token) + if _canonical_hosted_origin(cred.get("host")) != host: + raise ValueError("Authenticated host did not match the requested host") except Exception as exc: return {"authenticated": False, "error": str(exc)} + self.config.hosted_host = host + self._persist_config_key("hosted_host", host) return self._auth_status(cred) def connect_uri(self, **params: Any) -> dict: @@ -2683,30 +2765,53 @@ def connect_uri(self, **params: Any) -> dict: if not isinstance(uri, str): raise ValueError("uri is required") result = connect_uri(uri) - self.config.hosted_host = result["host"] - self._persist_config_key("hosted_host", result["host"]) + host = _canonical_hosted_origin(result["host"]) + result = {**result, "host": host} + self.config.hosted_host = host + self._persist_config_key("hosted_host", host) self.emit( "pairing_state", - {"status": "connected", "host": result["host"]}, + {"status": "connected", "host": host}, ) return result def logout(self, **params: Any) -> dict: - """Clear the active credential.""" - from engine.auth.store import active_host, clear_credential + """Clear only the credential for the selected safe hosted origin.""" + from engine.auth.store import ( + active_host, + canonical_host_origin, + clear_credential, + ) - host = params.get("host") or active_host() - if host: - clear_credential(host) + stored_host = active_host() + requested_host = params.get("host") + if requested_host: + try: + requested_origin = _canonical_hosted_origin(requested_host) + except ValueError: + return {"authenticated": False, "error": "Hosted origin is invalid"} + if stored_host and canonical_host_origin(stored_host) == requested_origin: + clear_credential(stored_host) + else: + clear_credential(requested_origin) + elif stored_host: + if not canonical_host_origin(stored_host): + return {"authenticated": False, "error": "Hosted origin is invalid"} + clear_credential(stored_host) return {"authenticated": False} def get_auth_status(self, **params: Any) -> dict: - """Return the current :class:`AuthStatus` from the active credential.""" - from engine.auth.store import active_credential + """Return auth only when the credential belongs to the configured host.""" + from engine.auth.store import active_credential, canonical_host_origin cred = active_credential() - if not cred: - return {"authenticated": False} + configured_host = canonical_host_origin(self.config.hosted_host) + if ( + not cred + or not configured_host + or canonical_host_origin(str(cred.get("host") or "")) != configured_host + ): + return {"authenticated": False, "host": configured_host or None} return self._auth_status(cred) def _auth_status(self, cred: Any) -> dict: @@ -2738,9 +2843,16 @@ def set_config(self, **params: Any) -> dict: """ key = params.get("key") value = params.get("value") + if key == "host": + key = "hosted_host" allowed = {"hosted_host", "deployment_lane", "phi_mode", "poll_interval_s"} if key not in allowed: return {"ok": False, "error": f"Unknown or non-settable key: {key}"} + if key == "hosted_host": + try: + value = _canonical_hosted_origin(value) + except ValueError as exc: + return {"ok": False, "error": str(exc)} # Update the live config object so subsequent commands see the change. try: setattr(self.config, key, value) diff --git a/engine/flow_bridge.py b/engine/flow_bridge.py index 0f010df..aed5659 100644 --- a/engine/flow_bridge.py +++ b/engine/flow_bridge.py @@ -411,6 +411,7 @@ def _safe_command_for_log(cmd: list[str]) -> str: redacted_after = { "--token", + "--host", "--password", "--rdp-password", "--agent-token", @@ -789,10 +790,13 @@ def push( token: str | None = None, timeout: float | None = None, env_overrides: dict[str, str] | None = None, + json_output: bool = True, ) -> FlowResult: """Upload through the same pinned Flow runtime as every other verb.""" args = ["push", str(path), "--kind", kind, "--host", host] + if json_output: + args.append("--json") if name: # A Desktop task description can contain a record identity. Do not # put it in argv or logs. Cloud can suggest a safe display name. diff --git a/engine/hosted.py b/engine/hosted.py index 555b2c9..c241c2d 100644 --- a/engine/hosted.py +++ b/engine/hosted.py @@ -18,12 +18,18 @@ import zipfile from pathlib import Path from typing import Any -from uuid import UUID from loguru import logger -from engine.auth.store import DEFAULT_HOST, INGEST_TOKEN_ENV, active_credential +from engine.auth.store import ( + DEFAULT_HOST, + INGEST_TOKEN_ENV, + active_credential, + canonical_host_origin, + token_for_host, +) from engine.flow_bridge import FlowBridge +from engine.qualification_lifecycle import parse_flow_push _MAX_FLOW_ERROR_CHARS = 500 @@ -136,8 +142,8 @@ def push( if backend is not None or not prefer_flow: return { "success": False, - "workflow_id": "", - "dashboard_url": "", + "workflow_id": None, + "dashboard_url": None, "error": ( "Direct Desktop ingest is disabled. Use the pinned Flow push command so " "only an approved, exact-hash sanitized derivative can leave the machine." @@ -149,12 +155,18 @@ def push( # A launch or transport failure must never select a raw upload # fallback. The exception can occur after Flow dispatched a request, # so Desktop must not claim that no bytes crossed the boundary. - logger.warning("Flow push did not return a confirmed outcome: {e}", e=exc) + logger.warning( + "Flow push did not return a confirmed outcome ({kind})", + kind=type(exc).__name__, + ) return { "success": False, "delivery_uncertain": True, - "workflow_id": "", - "dashboard_url": "", + "workflow_id": None, + "artifact_ingest_id": None, + "next_action": "reconcile", + "error_code": "delivery_uncertain", + "dashboard_url": None, "error": ( "Flow did not return a confirmed upload outcome. Do not retry blindly; " "reconcile the exact artifact in Cloud first." @@ -192,76 +204,25 @@ def _push_via_flow( host=host, token=None, env_overrides=env, + json_output=True, ) - stdout = result.stdout or "" - if result.ok and "Upload paused for local review" in stdout: - derivative_match = re.search( - r"^Sanitized derivative created at (.+)\.$", stdout, re.MULTILINE - ) - review_command = next( - ( - line - for line in stdout.splitlines() - if line.startswith("openadapt-flow review-sanitized ") - ), - "", - ) - if derivative_match is None or not review_command: - return { - "success": False, - "pending_review": False, - "workflow_id": "", - "dashboard_url": "", - "error": "Flow paused, but Desktop could not verify the review handoff.", - } - return { - "success": False, - "pending_review": True, - "sanitized_path": derivative_match.group(1), - "review_command": review_command, - "workflow_id": "", - "dashboard_url": "", - "error": "", - } - - workflow_match = re.search(r"\bworkflow_id=([^\s,\)]+)", stdout) - workflow_id = workflow_match.group(1) if workflow_match else "" - try: - workflow_id = str(UUID(workflow_id)) - except (ValueError, AttributeError): - workflow_id = "" - dashboard_match = re.search(r"^Dashboard:\s+(\S+)\s*$", stdout, re.MULTILINE) - dashboard_url = dashboard_match.group(1) if dashboard_match else "" - success = bool(result.ok and workflow_id) - error = ( - _bounded_flow_error(result.stderr or result.stdout, secret=resolved_token) - if not result.ok - else "" + parsed = parse_flow_push( + result.stdout or "", + result.stderr or "", + ok=result.ok, + expected_host=host, ) - if result.ok and not workflow_id: - error = "Flow returned success without an authenticated hosted workflow identity." + success = bool(parsed.get("accepted_for_ingest")) return { + **parsed, "success": success, - "pending_review": False, - "delivery_uncertain": not result.ok, - "workflow_id": workflow_id, - "dashboard_url": dashboard_url, - "error": error, } def _token_for_host(host: str, *, explicit: str | None = None) -> str: """Resolve a Desktop credential without sending it to another origin.""" - if explicit and explicit.strip(): - return explicit.strip() - env_token = os.environ.get(INGEST_TOKEN_ENV, "").strip() - if env_token: - return env_token - credential = active_credential() - if credential and str(credential.get("host", "")).rstrip("/") == host.rstrip("/"): - return str(credential.get("token") or "").strip() - return "" + return token_for_host(host, explicit=explicit) def _bounded_flow_error(message: str, *, secret: str = "") -> str: @@ -297,7 +258,9 @@ def report_break( if org_id is None: cred = active_credential() - if cred and str(cred.get("host", "")).rstrip("/") == host.rstrip("/"): + if cred and canonical_host_origin( + str(cred.get("host", "")) + ) == canonical_host_origin(host): org_id = cred.get("org_id") try: result = FlowBridge().report_break( @@ -310,7 +273,10 @@ def report_break( env_overrides={INGEST_TOKEN_ENV: resolved_token}, ) except Exception as exc: - logger.warning("Flow report-break did not return a confirmed outcome: {e}", e=exc) + logger.warning( + "Flow report-break did not return a confirmed outcome ({kind})", + kind=type(exc).__name__, + ) return { "ok": False, "delivery_uncertain": True, diff --git a/engine/policy.py b/engine/policy.py index 1c50b75..1b30854 100644 --- a/engine/policy.py +++ b/engine/policy.py @@ -30,9 +30,10 @@ missing-backend error). Only :func:`fetch_effective_policy` raises, so its caller can decide whether to fall back to cache. -The raw server body is cached to ``~/.openadapt/policy.json`` (same dir as -``config.toml``) with an atomic temp-file + :func:`os.replace` write so a -half-written file can never be read back. +The server body is cached in a closed envelope at ``~/.openadapt/policy.json`` +(same dir as ``config.toml``). The envelope binds it to the canonical host, +credential identity, organization, policy version, and fetch time. An atomic +temp-file + :func:`os.replace` write prevents partial reads. BINDING THE POLICY TO A RUN --------------------------- @@ -57,17 +58,25 @@ from __future__ import annotations +import hashlib import json import os import tempfile from collections.abc import Mapping +from datetime import UTC, datetime from pathlib import Path from typing import Any +from urllib.parse import urlsplit import httpx from loguru import logger -from engine.auth.store import auth_header +from engine.auth.store import ( + active_credential, + auth_header, + canonical_host_origin, + token_for_host, +) from engine.config import DEFAULT_CONFIG_TOML # Endpoint the cloud serves the resolved (merged) org policy from. @@ -81,6 +90,11 @@ # control plane degrades quickly to cache rather than stalling a run. DEFAULT_TIMEOUT = 10.0 +# A cached org policy is an offline continuity aid, not permanent authority. +# After one day the Desktop must reconnect before it can govern another run. +DEFAULT_CACHE_MAX_AGE_S = 24 * 60 * 60 +CACHE_SCHEMA = "openadapt.policy-cache/v2" + # The SAFEST value for every safety key the contract defines. A missing or # unreachable value MUST resolve to the entry here (fail-closed): more checking, # stricter gates, no unverified writes, no model calls, managed-strict egress. @@ -157,6 +171,103 @@ def _policy_cache_path() -> Path: return Path(override) if override else DEFAULT_POLICY_CACHE +def _credential_sha256(host: str) -> str | None: + """Return a non-secret, destination-bound identity for the active bearer.""" + + token = token_for_host(host) + if not token: + return None + digest = hashlib.sha256() + digest.update(b"openadapt.policy-cache-credential/v1\0") + digest.update(token.encode("utf-8")) + return digest.hexdigest() + + +def _credential_org_id(host: str) -> str | None: + """Return the keychain org only when it belongs to this host and bearer.""" + + credential = active_credential() + if not credential: + return None + if canonical_host_origin(str(credential.get("host") or "")) != canonical_host_origin(host): + return None + if str(credential.get("token") or "").strip() != token_for_host(host): + return None + org_id = credential.get("org_id") + return org_id if isinstance(org_id, str) and org_id else None + + +def _safe_policy_origin(host: str) -> str: + """Return the exact permitted origin for policy fetch and cache binding.""" + + parsed_host = urlsplit(str(host or "").strip()) + if parsed_host.scheme.lower() == "http" and parsed_host.hostname not in { + "localhost", + "127.0.0.1", + "::1", + }: + raise PolicyFetchError("A remote policy host must use HTTPS.") + origin = canonical_host_origin(host) + if not origin: + raise PolicyFetchError("Policy host is not a valid HTTP(S) origin.") + return origin + + +def _policy_version(policy: Mapping[str, Any]) -> int | None: + """Return a valid monotonic policy version, else ``None``.""" + + value = policy.get("policy_version") + if isinstance(value, int) and not isinstance(value, bool) and value >= 0: + return value + return None + + +def _policy_sha256(policy: Mapping[str, Any]) -> str: + """Bind one exact policy body without retaining another sensitive copy.""" + + canonical = json.dumps( + policy, + sort_keys=True, + separators=(",", ":"), + ensure_ascii=False, + ).encode("utf-8") + digest = hashlib.sha256() + digest.update(b"openadapt.policy-cache-body/v1\0") + digest.update(canonical) + return digest.hexdigest() + + +def _policy_authority_sha256(policy: Mapping[str, Any]) -> str: + """Bind only version-controlled organization policy authority. + + The effective-policy response also carries per-request and per-user fields. + Those fields can change without an organization policy version change. They + must not disable a valid refresh. The fields below define the organization + authority that must move only with ``policy_version``. + """ + + authority = { + key: policy.get(key) + for key in ( + "org_id", + "baseline_version", + "org", + "safety", + "grounding_model", + ) + } + canonical = json.dumps( + authority, + sort_keys=True, + separators=(",", ":"), + ensure_ascii=False, + ).encode("utf-8") + digest = hashlib.sha256() + digest.update(b"openadapt.policy-authority/v1\0") + digest.update(canonical) + return digest.hexdigest() + + def fetch_effective_policy(host: str, timeout: float = DEFAULT_TIMEOUT) -> dict[str, Any]: """Fetch the org's effective policy over the network and refresh the cache. @@ -164,7 +275,7 @@ def fetch_effective_policy(host: str, timeout: float = DEFAULT_TIMEOUT) -> dict[ credential resolved by :func:`~engine.auth.store.auth_header`, modeled on :meth:`engine.auth.paste.PasteTokenProvider._validate`. On success, the RAW response body is written atomically to the cache file so a later offline - :func:`load_cached_policy` returns the last-known-good policy. + :func:`load_cached_policy` can return it only to the same principal. Args: host: Hosted control-plane base URL (e.g. ``https://app.openadapt.ai``). @@ -176,8 +287,9 @@ def fetch_effective_policy(host: str, timeout: float = DEFAULT_TIMEOUT) -> dict[ Raises: PolicyFetchError: On any non-2xx response, network error, or invalid JSON. """ - url = f"{host.rstrip('/')}{POLICY_PATH}" - headers = {**auth_header(), "Accept": "application/json"} + origin = _safe_policy_origin(host) + url = f"{origin}{POLICY_PATH}" + headers = {**auth_header(origin), "Accept": "application/json"} try: resp = httpx.get(url, headers=headers, timeout=timeout) except httpx.HTTPError as exc: @@ -194,13 +306,36 @@ def fetch_effective_policy(host: str, timeout: float = DEFAULT_TIMEOUT) -> dict[ raise PolicyFetchError(f"Policy response was not valid JSON: {exc}") from exc if not isinstance(policy, dict): raise PolicyFetchError("Policy response was not a JSON object.") - - _write_cache(policy) + if _policy_version(policy) is None: + raise PolicyFetchError("Policy response did not include a valid policy version.") + + org_id = policy.get("org_id") + if not isinstance(org_id, str) or not org_id: + raise PolicyFetchError("Policy response did not identify its organization.") + credential_org_id = _credential_org_id(origin) + if credential_org_id is not None and credential_org_id != org_id: + raise PolicyFetchError("Policy organization did not match the active credential.") + + cached = load_cached_policy(origin, max_age_s=float("inf")) + if cached is not None: + cached_version = _policy_version(cached) + policy_version = _policy_version(policy) + if cached_version is not None and policy_version is not None: + if policy_version < cached_version: + raise PolicyFetchError("Policy response version moved backwards.") + if ( + policy_version == cached_version + and _policy_authority_sha256(policy) + != _policy_authority_sha256(cached) + ): + raise PolicyFetchError("Policy response changed without a new version.") + + _write_cache(policy, origin) return policy -def _write_cache(policy: dict[str, Any]) -> None: - """Atomically persist the raw policy body to the cache file. +def _write_cache(policy: dict[str, Any], host: str) -> None: + """Atomically persist a host, credential, org, and time-bound cache envelope. Writes to a temp file in the cache directory, then :func:`os.replace`s it into place so a reader can never observe a half-written file. Degrades @@ -208,12 +343,37 @@ def _write_cache(policy: dict[str, Any]) -> None: still usable in-memory even when the disk is read-only. """ path = _policy_cache_path() + host_origin = canonical_host_origin(host) + credential_sha256 = _credential_sha256(host) + org_id = policy.get("org_id") + policy_version = _policy_version(policy) + if ( + not host_origin + or not credential_sha256 + or not isinstance(org_id, str) + or not org_id + or policy_version is None + ): + logger.warning("Could not bind policy cache to the current hosted credential") + return + envelope = { + "schema": CACHE_SCHEMA, + "binding": { + "host_origin": host_origin, + "credential_sha256": credential_sha256, + "org_id": org_id, + "policy_version": policy_version, + "policy_sha256": _policy_sha256(policy), + }, + "fetched_at": datetime.now(UTC).isoformat(), + "policy": policy, + } try: path.parent.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=str(path.parent), prefix=".policy.", suffix=".tmp") try: with os.fdopen(fd, "w", encoding="utf-8") as fh: - json.dump(policy, fh) + json.dump(envelope, fh) os.replace(tmp, path) except Exception: # Clean up the temp file on any failure so we don't leak turds. @@ -226,8 +386,13 @@ def _write_cache(policy: dict[str, Any]) -> None: logger.warning("Could not persist policy cache: {e}", e=exc) -def load_cached_policy() -> dict[str, Any] | None: - """Read the last-cached policy body, or ``None`` if absent/unreadable. +def load_cached_policy( + host: str, + *, + now: datetime | None = None, + max_age_s: float = DEFAULT_CACHE_MAX_AGE_S, +) -> dict[str, Any] | None: + """Read only a fresh cache bound to this exact host and credential. Degrade-not-raise (mirrors :func:`engine.auth.store._kr_get`): a missing file, unreadable file, or corrupt JSON all resolve to ``None`` rather than @@ -247,9 +412,59 @@ def load_cached_policy() -> dict[str, Any] | None: except (json.JSONDecodeError, ValueError): logger.warning("Cached policy at {p} is corrupt; ignoring", p=path) return None - if not isinstance(data, dict): + if not isinstance(data, dict) or set(data) != { + "schema", + "binding", + "fetched_at", + "policy", + }: + return None + binding = data.get("binding") + policy = data.get("policy") + if data.get("schema") != CACHE_SCHEMA or not isinstance(binding, dict): + return None + if set(binding) != { + "host_origin", + "credential_sha256", + "org_id", + "policy_version", + "policy_sha256", + }: + return None + if not isinstance(policy, dict): + return None + expected_origin = canonical_host_origin(host) + expected_credential = _credential_sha256(host) + if not expected_origin or not expected_credential: + return None + if binding.get("host_origin") != expected_origin: return None - return data + if binding.get("credential_sha256") != expected_credential: + return None + policy_org_id = policy.get("org_id") + if not isinstance(policy_org_id, str) or not policy_org_id: + return None + if binding.get("org_id") != policy_org_id: + return None + credential_org_id = _credential_org_id(host) + if credential_org_id is not None and credential_org_id != policy_org_id: + return None + if binding.get("policy_version") != policy.get("policy_version"): + return None + if _policy_version(policy) is None: + return None + if binding.get("policy_sha256") != _policy_sha256(policy): + return None + try: + fetched_at = datetime.fromisoformat(str(data.get("fetched_at"))) + if fetched_at.tzinfo is None: + return None + age_s = ((now or datetime.now(UTC)) - fetched_at.astimezone(UTC)).total_seconds() + except (TypeError, ValueError, OverflowError): + return None + if age_s < 0 or age_s > max_age_s: + return None + return policy def harden_safety(policy: dict[str, Any]) -> dict[str, Any]: @@ -330,7 +545,7 @@ def resolve_effective_policy( policy = fetch_effective_policy(host, timeout=timeout) except PolicyFetchError as exc: logger.warning("Policy fetch failed ({e}); falling back to cache", e=exc) - policy = load_cached_policy() + policy = load_cached_policy(host) source = "cache" if policy is None: diff --git a/engine/qualification_lifecycle.py b/engine/qualification_lifecycle.py index 87e9da8..a172e3e 100644 --- a/engine/qualification_lifecycle.py +++ b/engine/qualification_lifecycle.py @@ -5,12 +5,15 @@ import hashlib import json import os +import re import shutil import stat +import tempfile import zipfile +from datetime import UTC, datetime from pathlib import Path from typing import Any -from uuid import UUID +from urllib.parse import urlsplit class QualificationLifecycleError(RuntimeError): @@ -264,45 +267,476 @@ def export_certified_bundle(bundle_dir: Path, destination: Path) -> str: return hashlib.sha256(destination.read_bytes()).hexdigest() -def parse_flow_push(stdout: str, stderr: str, *, ok: bool) -> dict[str, Any]: - """Project Flow's bounded push states without treating review as deployment.""" +_PUSH_SCHEMA = "openadapt.push-result/v1" +_PUSH_TOP_LEVEL = { + "schema", + "status", + "workflow_id", + "artifact_ingest_id", + "review", + "attestation", + "binding", + "next_action", + "dashboard_url", + "delivery", + "error", +} +_PUSH_BINDING_KEYS = { + "kind", + "source_tree_sha256", + "derivative_tree_sha256", + "approved_archive_sha256", + "artifact_sha256", + "bundle_sha256", + "source_recording_sha256", + "sanitization_policy", + "certification_policy", + "certification_evidence_sha256", + "governed_authorization_template_sha256", + "parameter_schema_sha256", + "attested_run_report_sha256", + "resolves_run_id", + "organization_id", + "bundle_version_id", + "bundle_version", + "runtime_validation_id", +} +_PUSH_HASH_KEYS = _PUSH_BINDING_KEYS - { + "kind", + "sanitization_policy", + "certification_policy", + "resolves_run_id", + "organization_id", + "bundle_version_id", + "bundle_version", + "runtime_validation_id", +} +_UUID_RE = re.compile( + r"[a-f0-9]{8}-[a-f0-9]{4}-[1-5][a-f0-9]{3}-[89ab][a-f0-9]{3}-[a-f0-9]{12}" +) + + +def _is_sha256(value: Any) -> bool: + return ( + isinstance(value, str) + and len(value) == 64 + and all(char in "0123456789abcdef" for char in value) + ) + + +def _is_uuid(value: Any) -> bool: + return isinstance(value, str) and _UUID_RE.fullmatch(value) is not None + + +def _invalid_push_result() -> dict[str, Any]: + return { + "ok": False, + "deployed": False, + "pending_review": False, + "accepted_for_ingest": False, + "delivery_uncertain": True, + "workflow_id": None, + "artifact_ingest_id": None, + "dashboard_url": None, + "next_action": "reconcile", + "error_code": "invalid_ingest_response", + "error": ( + "Flow did not return a valid push result. Reconcile the exact artifact " + "in Cloud before any retry." + ), + } + - if not ok: - detail = (stderr or stdout or "Cloud deploy failed").strip()[:500] - return { - "ok": False, - "deployed": False, - "delivery_uncertain": True, - "error": detail, +def _require_exact_object(value: Any, keys: set[str], label: str) -> dict[str, Any]: + if not isinstance(value, dict) or set(value) != keys: + raise QualificationLifecycleError(f"{label} did not match its closed schema") + return value + + +def _origin(value: str) -> tuple[str, str, int | None]: + """Return a safe web origin tuple for a controller trust comparison.""" + + parsed = urlsplit(value) + if ( + parsed.scheme not in {"http", "https"} + or not parsed.hostname + or parsed.username + or parsed.password + ): + raise QualificationLifecycleError("Flow dashboard URL is unsafe") + hostname = parsed.hostname.lower() + if parsed.scheme == "http" and hostname not in {"localhost", "127.0.0.1", "::1"}: + raise QualificationLifecycleError("Flow dashboard URL is unsafe") + try: + port = parsed.port + except ValueError as exc: + raise QualificationLifecycleError("Flow dashboard URL is unsafe") from exc + if (parsed.scheme == "https" and port == 443) or ( + parsed.scheme == "http" and port == 80 + ): + port = None + return parsed.scheme, hostname, port + + +def _validate_push_document( + document: Any, *, process_ok: bool, expected_host: str | None = None +) -> dict[str, Any]: + """Validate Flow's complete V1 result and its phase-specific invariants.""" + + doc = _require_exact_object(document, _PUSH_TOP_LEVEL, "Flow push result") + if doc["schema"] != _PUSH_SCHEMA: + raise QualificationLifecycleError("Flow push result schema is unsupported") + status = doc["status"] + if status not in { + "paused_for_review", + "accepted_for_ingest", + "failed", + "delivery_uncertain", + }: + raise QualificationLifecycleError("Flow push result status is unsupported") + if process_ok != (status in {"paused_for_review", "accepted_for_ingest"}): + raise QualificationLifecycleError("Flow push process outcome conflicts with its status") + + for field in ("workflow_id", "artifact_ingest_id"): + if doc[field] is not None and not _is_uuid(doc[field]): + raise QualificationLifecycleError(f"Flow push {field} is invalid") + + review = doc["review"] + if review is not None: + review = _require_exact_object( + review, {"id", "scope", "sanitized_path", "command"}, "Flow review" + ) + if not _is_sha256(review["id"]) or review["scope"] != "local_non_authoritative": + raise QualificationLifecycleError("Flow review binding is invalid") + for field in ("sanitized_path", "command"): + if review[field] is not None and ( + not isinstance(review[field], str) or not review[field] + ): + raise QualificationLifecycleError("Flow review handoff is invalid") + + attestation = doc["attestation"] + if attestation is not None: + attestation = _require_exact_object( + attestation, {"id", "schema"}, "Flow attestation" + ) + schema = attestation["schema"] + if ( + not isinstance(attestation["id"], str) + or not 1 <= len(attestation["id"]) <= 200 + or not isinstance(schema, str) + or not schema.startswith("openadapt.runtime-validation/v") + or not schema.removeprefix("openadapt.runtime-validation/v").isdigit() + ): + raise QualificationLifecycleError("Flow runtime attestation is invalid") + + binding = _require_exact_object( + doc["binding"], _PUSH_BINDING_KEYS, "Flow artifact binding" + ) + if binding["kind"] not in {"recording", "bundle", None}: + raise QualificationLifecycleError("Flow artifact kind is invalid") + for field in _PUSH_HASH_KEYS: + if binding[field] is not None and not _is_sha256(binding[field]): + raise QualificationLifecycleError(f"Flow artifact binding {field} is invalid") + for field in ("sanitization_policy", "certification_policy"): + if binding[field] is not None and ( + not isinstance(binding[field], str) or not binding[field] + ): + raise QualificationLifecycleError(f"Flow artifact binding {field} is invalid") + if binding["resolves_run_id"] is not None and not _is_uuid( + binding["resolves_run_id"] + ): + raise QualificationLifecycleError("Flow resolved run binding is invalid") + for field in ("organization_id", "bundle_version_id", "runtime_validation_id"): + if binding[field] is not None and not _is_uuid(binding[field]): + raise QualificationLifecycleError( + f"Flow artifact binding {field} is invalid" + ) + bundle_version = binding["bundle_version"] + if bundle_version is not None and ( + not isinstance(bundle_version, int) + or isinstance(bundle_version, bool) + or bundle_version < 1 + ): + raise QualificationLifecycleError("Flow bundle version is invalid") + + if doc["next_action"] not in { + "review_local", + "parameterize", + "validate_runtime", + "open_dashboard", + "reconcile", + None, + }: + raise QualificationLifecycleError("Flow next action is invalid") + dashboard = doc["dashboard_url"] + if dashboard is not None: + if not isinstance(dashboard, str): + raise QualificationLifecycleError("Flow dashboard URL is invalid") + dashboard_origin = _origin(dashboard) + if expected_host is not None and dashboard_origin != _origin(expected_host): + raise QualificationLifecycleError("Flow dashboard origin is not trusted") + + delivery = _require_exact_object( + doc["delivery"], {"attempted", "certainty"}, "Flow delivery" + ) + if ( + delivery["attempted"] is not True + and delivery["attempted"] is not False + and delivery["attempted"] is not None + ) or delivery["certainty"] not in { + "not_attempted", + "not_accepted", + "accepted", + "unknown", + }: + raise QualificationLifecycleError("Flow delivery binding is invalid") + error = doc["error"] + if error is not None: + error = _require_exact_object(error, {"code", "message"}, "Flow push error") + if error["code"] not in { + "push_failed", + "delivery_uncertain", + "invalid_ingest_response", + } or not isinstance(error["message"], str) or not 1 <= len(error["message"]) <= 500: + raise QualificationLifecycleError("Flow push error is invalid") + + if status == "paused_for_review": + pause_only_nulls = { + "approved_archive_sha256", + "artifact_sha256", + "bundle_sha256", + "source_recording_sha256", + "certification_policy", + "certification_evidence_sha256", + "governed_authorization_template_sha256", + "parameter_schema_sha256", + "attested_run_report_sha256", + "resolves_run_id", + "organization_id", + "bundle_version_id", + "bundle_version", + "runtime_validation_id", } - if "Upload paused for local review" in stdout: - sanitized_path = "" - marker = "Sanitized derivative created at " - for line in stdout.splitlines(): - if line.startswith(marker): - sanitized_path = line[len(marker) :].rstrip(".") - break - return { - "ok": True, - "deployed": False, - "pending_review": True, - "sanitized_path": sanitized_path, + if not ( + doc["workflow_id"] is None + and doc["artifact_ingest_id"] is None + and isinstance(review, dict) + and isinstance(review["sanitized_path"], str) + and isinstance(review["command"], str) + and attestation is None + and doc["next_action"] == "review_local" + and dashboard is None + and error is None + and delivery == {"attempted": False, "certainty": "not_attempted"} + and binding["kind"] in {"recording", "bundle"} + and _is_sha256(binding["source_tree_sha256"]) + and _is_sha256(binding["derivative_tree_sha256"]) + and isinstance(binding["sanitization_policy"], str) + and all(binding[field] is None for field in pause_only_nulls) + ): + raise QualificationLifecycleError("Flow review pause is incomplete") + elif status == "accepted_for_ingest": + if not ( + _is_uuid(doc["artifact_ingest_id"]) + and isinstance(review, dict) + and review["sanitized_path"] is None + and review["command"] is None + and error is None + and delivery == {"attempted": True, "certainty": "accepted"} + and _is_sha256(binding["source_tree_sha256"]) + and _is_sha256(binding["derivative_tree_sha256"]) + and _is_sha256(binding["approved_archive_sha256"]) + and binding["approved_archive_sha256"] == binding["artifact_sha256"] + and isinstance(binding["sanitization_policy"], str) + ): + raise QualificationLifecycleError("Flow accepted ingest binding is incomplete") + if binding["kind"] == "recording": + recording_only_nulls = { + "bundle_sha256", + "source_recording_sha256", + "certification_policy", + "certification_evidence_sha256", + "governed_authorization_template_sha256", + "parameter_schema_sha256", + "attested_run_report_sha256", + "resolves_run_id", + "organization_id", + "bundle_version_id", + "bundle_version", + "runtime_validation_id", + } + if not ( + doc["workflow_id"] is None + and attestation is None + and doc["next_action"] in {"parameterize", "validate_runtime"} + and dashboard is None + and all(binding[field] is None for field in recording_only_nulls) + ): + raise QualificationLifecycleError("Flow recording ingest state is invalid") + elif binding["kind"] == "bundle": + required_bundle_hashes = ( + "bundle_sha256", + "source_recording_sha256", + "certification_evidence_sha256", + "parameter_schema_sha256", + "attested_run_report_sha256", + ) + if not ( + _is_uuid(doc["workflow_id"]) + and isinstance(attestation, dict) + and doc["next_action"] == "open_dashboard" + and all(_is_sha256(binding[field]) for field in required_bundle_hashes) + and binding["bundle_sha256"] == binding["artifact_sha256"] + and isinstance(binding["certification_policy"], str) + and _is_uuid(binding["organization_id"]) + and _is_uuid(binding["bundle_version_id"]) + and isinstance(binding["bundle_version"], int) + and not isinstance(binding["bundle_version"], bool) + and binding["bundle_version"] >= 1 + and _is_uuid(binding["runtime_validation_id"]) + and isinstance(dashboard, str) + and urlsplit(dashboard).path == f"/dashboard/workflows/{doc['workflow_id']}" + and not urlsplit(dashboard).query + and not urlsplit(dashboard).fragment + ): + raise QualificationLifecycleError("Flow bundle ingest state is invalid") + else: + raise QualificationLifecycleError("Flow accepted ingest has no artifact kind") + elif status == "delivery_uncertain": + uncertain_server_nulls = { + "organization_id", + "bundle_version_id", + "bundle_version", + "runtime_validation_id", } - workflow_id = "" - dashboard_url = "" - for line in stdout.splitlines(): - if "workflow_id=" in line: - workflow_id = line.split("workflow_id=", 1)[1].split()[0].rstrip(",).") - if line.startswith("Dashboard: "): - dashboard_url = line.removeprefix("Dashboard: ").strip() + if not ( + doc["workflow_id"] is None + and doc["artifact_ingest_id"] is None + and doc["next_action"] == "reconcile" + and dashboard is None + and delivery == {"attempted": True, "certainty": "unknown"} + and isinstance(error, dict) + and error["code"] == "delivery_uncertain" + and all(binding[field] is None for field in uncertain_server_nulls) + ): + raise QualificationLifecycleError("Flow uncertain delivery state is invalid") + else: + if not ( + doc["workflow_id"] is None + and doc["artifact_ingest_id"] is None + and review is None + and attestation is None + and dashboard is None + and isinstance(error, dict) + and all(value is None for value in binding.values()) + ): + raise QualificationLifecycleError("Flow failed state is invalid") + if error["code"] == "push_failed": + if not ( + doc["next_action"] is None + and delivery == {"attempted": None, "certainty": "not_accepted"} + ): + raise QualificationLifecycleError("Flow rejected delivery state is invalid") + elif error["code"] == "invalid_ingest_response": + if not ( + doc["next_action"] == "reconcile" + and delivery == {"attempted": True, "certainty": "unknown"} + ): + raise QualificationLifecycleError("Flow invalid response state is invalid") + else: + raise QualificationLifecycleError("Flow failed state has the wrong error") + return doc + + +def parse_flow_push( + stdout: str, stderr: str, *, ok: bool, expected_host: str | None = None +) -> dict[str, Any]: + """Project only Flow's exact JSON V1 result into Desktop state.""" + + del stderr # Raw child diagnostics must not cross the local UI boundary. try: - workflow_id = str(UUID(workflow_id)) - except (ValueError, AttributeError): - workflow_id = "" + document = _validate_push_document( + json.loads(stdout), process_ok=ok, expected_host=expected_host + ) + except (json.JSONDecodeError, QualificationLifecycleError, TypeError, ValueError): + return _invalid_push_result() + status = document["status"] + accepted = status == "accepted_for_ingest" + bundle_accepted = accepted and document["binding"]["kind"] == "bundle" + error = document["error"] or {} return { - "ok": bool(workflow_id), - "deployed": bool(workflow_id), - "workflow_id": workflow_id, - "dashboard_url": dashboard_url, - "error": "" if workflow_id else "Cloud did not return a deployed workflow id", + "ok": status in {"paused_for_review", "accepted_for_ingest"}, + "deployed": bundle_accepted, + "pending_review": status == "paused_for_review", + "accepted_for_ingest": accepted, + "delivery_uncertain": status == "delivery_uncertain" + or document["delivery"]["certainty"] == "unknown", + "status": status, + "workflow_id": document["workflow_id"], + "artifact_ingest_id": document["artifact_ingest_id"], + "review": document["review"], + "attestation": document["attestation"], + "binding": document["binding"], + "next_action": document["next_action"], + "dashboard_url": document["dashboard_url"], + "delivery": document["delivery"], + "error_code": error.get("code"), + "error": error.get("message", ""), + "push_result": document, + "sanitized_path": (document["review"] or {}).get("sanitized_path") or "", + "review_command": (document["review"] or {}).get("command") or "", } + + +def persist_deployment_handoff( + data_dir: Path, *, local_workflow_id: str, result: dict[str, Any] +) -> Path: + """Persist the exact typed Flow state so Desktop never loses a handoff.""" + + local_workflow_id = validate_path_token(local_workflow_id, label="Workflow id") + push_result = result.get("push_result") + if isinstance(push_result, dict): + state = { + "paused_for_review": "needs_review", + "accepted_for_ingest": ( + "deployed" + if push_result["binding"]["kind"] == "bundle" + else "accepted_recording" + ), + "failed": "failed", + "delivery_uncertain": "delivery_uncertain", + }[push_result["status"]] + elif result.get("delivery_uncertain"): + state = "delivery_uncertain" + push_result = None + else: + raise QualificationLifecycleError("No valid Flow push result is available") + root = Path(data_dir) / "deployment-handoffs" + root.mkdir(parents=True, exist_ok=True) + try: + root.chmod(0o700) + except OSError: + pass + destination = root / f"{local_workflow_id}.json" + document = { + "schema": "openadapt.desktop-deployment-handoff/v1", + "local_workflow_id": local_workflow_id, + "state": state, + "updated_at": datetime.now(UTC).isoformat(), + "push_result": push_result, + "error_code": result.get("error_code"), + } + fd, temporary_name = tempfile.mkstemp( + dir=str(root), prefix=f".{local_workflow_id}.", suffix=".tmp" + ) + temporary = Path(temporary_name) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(document, handle, sort_keys=True, separators=(",", ":")) + handle.flush() + os.fsync(handle.fileno()) + os.chmod(temporary, stat.S_IRUSR | stat.S_IWUSR) + os.replace(temporary, destination) + finally: + temporary.unlink(missing_ok=True) + return destination diff --git a/engine/review.py b/engine/review.py index a568e5c..fe790eb 100644 --- a/engine/review.py +++ b/engine/review.py @@ -48,7 +48,7 @@ import os import re from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any if TYPE_CHECKING: from engine.audit import AuditLogger @@ -136,6 +136,28 @@ def _stream_sha256(path: Path) -> str: return digest.hexdigest() +def update_derivative_tree_digest( + digest: Any, + *, + relative_path: str, + member_type: str, + file_sha256: str | None = None, +) -> None: + """Add one unambiguous, type-aware record to a derivative tree hash.""" + + path_bytes = relative_path.encode("utf-8") + if member_type not in {"directory", "file"}: + raise EgressArtifactError("The sanitized derivative has an unsupported member type.") + digest.update(b"openadapt.desktop-derivative-member/v1\0") + digest.update(b"D" if member_type == "directory" else b"F") + digest.update(len(path_bytes).to_bytes(8, "big")) + digest.update(path_bytes) + if member_type == "file": + if not isinstance(file_sha256, str) or not _SHA256_RE.fullmatch(file_sha256): + raise EgressArtifactError("The sanitized derivative file digest is invalid.") + digest.update(bytes.fromhex(file_sha256)) + + def derivative_tree_sha256(path: Path) -> str: """Hash a derivative tree without loading recording media into memory.""" @@ -147,14 +169,28 @@ def derivative_tree_sha256(path: Path) -> str: relative = "." if member == path else member.relative_to(path).as_posix() if relative == "review_status.json": continue - digest.update(relative.encode("utf-8")) if member.is_file(): stat = os.stat(member, follow_symlinks=False) if stat.st_nlink != 1: raise EgressArtifactError( "The sanitized derivative contains a hard-linked file." ) - digest.update(_stream_sha256(member).encode("ascii")) + update_derivative_tree_digest( + digest, + relative_path=relative, + member_type="file", + file_sha256=_stream_sha256(member), + ) + elif member.is_dir(): + update_derivative_tree_digest( + digest, + relative_path=relative, + member_type="directory", + ) + else: + raise EgressArtifactError( + "The sanitized derivative has an unsupported member type." + ) return digest.hexdigest() diff --git a/engine/runner_loop.py b/engine/runner_loop.py index ced9008..efa4361 100644 --- a/engine/runner_loop.py +++ b/engine/runner_loop.py @@ -45,15 +45,20 @@ import asyncio import hashlib import json +import os import platform as _platform import random +import re +import shutil +import stat import sys +import tempfile import threading -import uuid import zipfile -from datetime import datetime, timezone -from pathlib import Path +from datetime import datetime, timedelta, timezone +from pathlib import Path, PurePosixPath from typing import Any, Callable +from urllib.parse import urlsplit import httpx import yaml @@ -90,6 +95,14 @@ def evidence_path(run_id: str) -> str: LEASE_EXTEND_INTERVAL_S = 300 BACKOFF_BASE_S = 1.0 BACKOFF_CAP_S = 60.0 +MAX_BUNDLE_ARCHIVE_BYTES = 1024 * 1024 * 1024 +MAX_BUNDLE_UNPACKED_BYTES = 4 * MAX_BUNDLE_ARCHIVE_BYTES +MAX_BUNDLE_MEMBERS = 100_000 + +_SHA256_RE = re.compile(r"[a-f0-9]{64}") +_CONTRACT_HASH_RE = re.compile(r"sha256:[a-f0-9]{64}") +_SAFE_ID_RE = re.compile(r"[A-Za-z0-9_.:-]{1,64}") +_RUNGS = frozenset({"structural", "template", "ocr", "geometry"}) # --- PHI boundary (spec section 3) ---------------------------------------------------- @@ -107,13 +120,6 @@ def evidence_path(run_id: str) -> str: "step_id", "rung", "effect_contract_hashes", "effect_verified", "effect_approved_unverified", "identity_verified", "elapsed_ms", ) -_HALT_FIELDS = ( - "task_id", "kind", "substrate", "effect_kind", "contract_hash", "verdict", - "reason", "evidence_digest", "suggested_action", "step_id", "rung", - "drift_signature", -) - - class PhiBoundaryError(RuntimeError): """Raised when a payload would violate the PHI-free evidence boundary.""" @@ -126,6 +132,10 @@ class ReauthRequired(RuntimeError): """The cloud rejected our token (401); the user must re-login. Never retry-loop.""" +class RunnerJournalError(RuntimeError): + """The durable runner journal cannot prove a safe prior run state.""" + + def assert_phi_free(obj: Any, path: str = "$") -> None: """Fail-closed recursive scan: refuse any payload carrying a forbidden key. @@ -216,14 +226,74 @@ def bundle_content_digest(bundle_dir: Path) -> str: def safe_extract_zip(archive: Path, dest: Path) -> None: - """Extract a bundle archive, refusing path-traversal member names.""" + """Extract only bounded regular ZIP members beneath ``dest``.""" + + root = dest.resolve() dest.mkdir(parents=True, exist_ok=True) + if any(dest.iterdir()): + raise Refusal("bundle staging directory is not empty") + seen: set[str] = set() with zipfile.ZipFile(archive) as zf: - for member in zf.namelist(): - member_path = (dest / member).resolve() - if not str(member_path).startswith(str(dest.resolve())): + members = zf.infolist() + if len(members) > MAX_BUNDLE_MEMBERS: + raise Refusal("bundle archive has too many members") + if sum(member.file_size for member in members) > MAX_BUNDLE_UNPACKED_BYTES: + raise Refusal("bundle archive expands beyond the runner limit") + for member in members: + name = member.filename + path = PurePosixPath(name) + if ( + not name + or "\x00" in name + or "\\" in name + or path.is_absolute() + or any(part in {"", ".", ".."} for part in path.parts) + or name in seen + ): raise Refusal("bundle archive contains an unsafe member path") - zf.extractall(dest) + seen.add(name) + target = (root / Path(*path.parts)).resolve() + try: + target.relative_to(root) + except ValueError: + raise Refusal("bundle archive contains an unsafe member path") from None + file_type = stat.S_IFMT(member.external_attr >> 16) + if file_type not in {0, stat.S_IFREG, stat.S_IFDIR}: + raise Refusal("bundle archive contains an unsupported member type") + if member.is_dir(): + target.mkdir(parents=True, exist_ok=True) + continue + target.parent.mkdir(parents=True, exist_ok=True) + try: + with zf.open(member) as source, target.open("xb") as output: + shutil.copyfileobj(source, output, length=1024 * 1024) + target.chmod(0o600) + except FileExistsError: + raise Refusal("bundle archive contains a duplicate member path") from None + + +def _safe_bundle_url(value: Any) -> str: + """Return a safe HTTPS URL, or loopback HTTP URL, for bundle download.""" + + if not isinstance(value, str): + raise Refusal("bundle staging URL is invalid") + try: + parsed = urlsplit(value) + port = parsed.port + except (TypeError, ValueError): + raise Refusal("bundle staging URL is invalid") from None + hostname = (parsed.hostname or "").lower() + if ( + parsed.scheme not in {"http", "https"} + or not hostname + or parsed.username is not None + or parsed.password is not None + or parsed.fragment + or (parsed.scheme == "http" and hostname not in {"localhost", "127.0.0.1", "::1"}) + or (port is not None and not 1 <= port <= 65535) + ): + raise Refusal("bundle staging URL is invalid") + return value def validate_dispatch(job: dict, bundle_dir: Path, *, now: datetime | None = None) -> None: @@ -254,7 +324,9 @@ def validate_dispatch(job: dict, bundle_dir: Path, *, now: datetime | None = Non if expires_at: try: deadline = datetime.fromisoformat(str(expires_at).replace("Z", "+00:00")) - except ValueError: + if deadline.tzinfo is None: + raise ValueError + except (TypeError, ValueError): raise Refusal("dispatch expires_at is unparseable") from None if (now or datetime.now(timezone.utc)) >= deadline: raise Refusal("dispatch expired before start") @@ -279,6 +351,37 @@ def validate_dispatch(job: dict, bundle_dir: Path, *, now: datetime | None = Non _flow_validate(authorization, bundle_dir) +def _lease_deadline(job: dict, *, received_at: datetime) -> tuple[str, datetime]: + """Return the exact lease id and the earliest locally enforceable deadline.""" + + lease = job.get("lease") + if not isinstance(lease, dict): + raise Refusal("dispatch missing lease") + job_id = lease.get("job_id") + if not isinstance(job_id, str) or not re.fullmatch(r"[A-Za-z0-9_-]{1,200}", job_id): + raise Refusal("dispatch lease id is invalid") + visibility_timeout = lease.get("visibility_timeout_s") + if ( + not isinstance(visibility_timeout, int) + or isinstance(visibility_timeout, bool) + or not 1 <= visibility_timeout <= DEFAULT_LEASE_S + ): + raise Refusal("dispatch lease timeout is invalid") + local_deadline = received_at + timedelta(seconds=visibility_timeout) + raw_deadline = lease.get("expires_at") or job.get("lease_expires_at") + if raw_deadline is None: + return job_id, local_deadline + try: + server_deadline = datetime.fromisoformat( + str(raw_deadline).replace("Z", "+00:00") + ) + if server_deadline.tzinfo is None: + raise ValueError + except (TypeError, ValueError): + raise Refusal("dispatch lease expiry is unparseable") from None + return job_id, min(local_deadline, server_deadline.astimezone(timezone.utc)) + + def _flow_validate(authorization: dict, bundle_dir: Path) -> None: """Run openadapt-flow's ``validate_execution_snapshot`` when importable. @@ -299,48 +402,100 @@ def _flow_validate(authorization: dict, bundle_dir: Path) -> None: auth = GovernedRunAuthorization.model_validate(authorization) validate_execution_snapshot(auth, Path(bundle_dir)) except Exception as exc: - raise Refusal(f"authorization revalidation refused: {exc}") from None + raise Refusal( + f"authorization revalidation refused ({type(exc).__name__})" + ) from None # --- evidence builders ---------------------------------------------------------------- +def _safe_identifier(value: Any, *, fallback: str) -> str: + """Return a bounded structural identifier without forwarding free text.""" + + return value if isinstance(value, str) and _SAFE_ID_RE.fullmatch(value) else fallback + + def _step_event(step: dict, index: int) -> dict: """Whitelist one report step into a spec ``step`` evidence payload.""" hashes = step.get("effect_contract_hashes") if not isinstance(hashes, list): single = step.get("contract_hash") hashes = [single] if single else [] + rung = step.get("rung") or step.get("resolver_rung") payload: dict[str, Any] = { - "step_id": step.get("step_id") or f"s{index}", - "rung": step.get("rung") or step.get("resolver_rung"), - "effect_contract_hashes": [str(h) for h in hashes], + "step_id": _safe_identifier(step.get("step_id"), fallback=f"s{index}"), + "rung": rung if rung in _RUNGS else None, + "effect_contract_hashes": [ + value + for value in hashes + if isinstance(value, str) and _CONTRACT_HASH_RE.fullmatch(value) + ], "effect_verified": bool( step.get("effect_verified", step.get("effect") == "verified") ), "effect_approved_unverified": bool(step.get("effect_approved_unverified", False)), - "elapsed_ms": step.get("elapsed_ms", step.get("latency_ms")), + "elapsed_ms": max( + 0, + int(step.get("elapsed_ms", step.get("latency_ms")) or 0), + ), } if "identity_verified" in step: payload["identity_verified"] = bool(step["identity_verified"]) return payload -def _halt_event(halt: dict) -> dict: - """Whitelist a halt block into the spec ``halt`` payload (digests/counts only).""" - payload: dict[str, Any] = {} - for key in _HALT_FIELDS: - if key in halt: - payload[key] = halt[key] - payload["task_id"] = payload.get("task_id") or f"recon-{uuid.uuid4().hex[:8]}" - payload["kind"] = payload.get("kind") or "resolver_halt" +def _halt_event( + halt: dict, + *, + run_id: str, + workflow_id: str, + step_count: int, +) -> dict: + """Build a structural halt event without forwarding any free text.""" + + rung = halt.get("rung") or halt.get("resolver_rung") + rung = rung if rung in _RUNGS else None + step_id = _safe_identifier( + halt.get("step_id") or ( + f"s{halt['step_index']}" if isinstance(halt.get("step_index"), int) else None + ), + fallback="", + ) + kind = halt.get("kind") + if kind not in { + "authorization_refused", + "identity_halt", + "effect_refuted", + "effect_indeterminate", + "compensation_failed", + "resolver_halt", + }: + kind = "resolver_halt" + payload: dict[str, Any] = { + "task_id": f"halt-{run_id}"[:64], + "kind": kind, + "reason": f"halt at step {step_id}" if step_id else "halt at unidentified step", + "drift_signature": hashlib.sha256( + f"{workflow_id}|{rung}|{step_count}".encode("utf-8") + ).hexdigest()[:16], + } + for key, allowed in { + "substrate": {"api", "fhir", "sql", "onscreen", "web", "desktop"}, + "effect_kind": {"create", "update", "delete", "send", "submit", "write"}, + "verdict": {"confirmed", "refuted", "indeterminate"}, + }.items(): + value = halt.get(key) + if value in allowed: + payload[key] = value payload["evidence_digest"] = _counts_only(halt.get("evidence_digest")) - if "step_id" not in payload and halt.get("step_index") is not None: - payload["step_id"] = f"s{halt['step_index']}" - if "rung" not in payload and halt.get("resolver_rung"): - payload["rung"] = halt["resolver_rung"] - if "reason" not in payload: - payload["reason"] = str(halt.get("reason", ""))[:500] + contract_hash = halt.get("contract_hash") + if isinstance(contract_hash, str) and _CONTRACT_HASH_RE.fullmatch(contract_hash): + payload["contract_hash"] = contract_hash + if step_id: + payload["step_id"] = step_id + if rung is not None: + payload["rung"] = rung return payload @@ -390,29 +545,64 @@ class RunnerJournal: def __init__(self, journal_dir: Path) -> None: self._dir = journal_dir + self._lock = threading.RLock() def _path(self, run_id: str) -> Path: - safe = "".join(c for c in run_id if c.isalnum() or c in "-_") or "run" - return self._dir / f"{safe}.json" + if not re.fullmatch(r"[A-Za-z0-9_-]{1,200}", run_id): + raise RunnerJournalError("runner run id is not a safe journal key") + return self._dir / f"{run_id}.json" + + def _read_path(self, path: Path, *, expected_run_id: str) -> dict: + try: + entry = json.loads(path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError, UnicodeError) as exc: + raise RunnerJournalError("runner journal is corrupt") from exc + if ( + not isinstance(entry, dict) + or entry.get("run_id") != expected_run_id + or entry.get("phase") not in {"leased", "starting", "started", "finished"} + ): + raise RunnerJournalError("runner journal has an invalid state") + return entry def record(self, run_id: str, phase: str, **extra: Any) -> None: """Persist a phase transition for ``run_id`` (merges over prior fields).""" - self._dir.mkdir(parents=True, exist_ok=True) - entry = self.get(run_id) or {"run_id": run_id} - entry.update(extra) - entry["phase"] = phase - entry["updated_at"] = datetime.now(timezone.utc).isoformat() - self._path(run_id).write_text(json.dumps(entry, indent=2)) + if phase not in {"leased", "starting", "started", "finished"}: + raise RunnerJournalError("runner journal phase is invalid") + with self._lock: + self._dir.mkdir(parents=True, exist_ok=True) + try: + self._dir.chmod(0o700) + except OSError: + pass + entry = self.get(run_id) or {"run_id": run_id} + entry.update(extra) + entry["phase"] = phase + entry["updated_at"] = datetime.now(timezone.utc).isoformat() + destination = self._path(run_id) + fd, temporary = tempfile.mkstemp( + dir=str(self._dir), prefix=f".{run_id}.", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(entry, handle, indent=2) + handle.flush() + os.fsync(handle.fileno()) + os.chmod(temporary, 0o600) + os.replace(temporary, destination) + finally: + try: + os.unlink(temporary) + except FileNotFoundError: + pass def get(self, run_id: str) -> dict | None: """Return the journal entry for ``run_id``, or None.""" - path = self._path(run_id) - if not path.is_file(): - return None - try: - return json.loads(path.read_text()) - except (json.JSONDecodeError, OSError): - return None + with self._lock: + path = self._path(run_id) + if not path.is_file(): + return None + return self._read_path(path, expected_run_id=run_id) def entries(self) -> list[dict]: """All journal entries, newest first.""" @@ -421,15 +611,14 @@ def entries(self) -> list[dict]: out: list[dict] = [] for path in sorted(self._dir.glob("*.json"), key=lambda p: p.stat().st_mtime, reverse=True): - try: - out.append(json.loads(path.read_text())) - except (json.JSONDecodeError, OSError): - continue + out.append(self._read_path(path, expected_run_id=path.stem)) return out def unfinished_started(self) -> list[dict]: """Runs that began executing but never reached a terminal phase.""" - return [e for e in self.entries() if e.get("phase") == "started"] + return [ + e for e in self.entries() if e.get("phase") in {"starting", "started"} + ] def last_runs(self, limit: int = 10) -> list[dict]: """Recent runs for the UI (run_id / phase / outcome / timestamps only).""" @@ -563,6 +752,9 @@ def __init__( self._attempt = 0 self._thread: threading.Thread | None = None self._stop = threading.Event() + self._tick_lock = asyncio.Lock() + self._handle_lock = asyncio.Lock() + self._lifecycle_lock = threading.Lock() # ---- status / lifecycle ---- @@ -599,14 +791,15 @@ def deregister(self) -> None: def start(self) -> None: """Start the background loop thread (no-op if already running).""" - if self._thread and self._thread.is_alive(): - return - self._stop.clear() - self._set_state("offline") - self._thread = threading.Thread( - target=self._thread_main, daemon=True, name="runner-loop" - ) - self._thread.start() + with self._lifecycle_lock: + if self._thread and self._thread.is_alive(): + return + self._stop.clear() + self._set_state("offline") + self._thread = threading.Thread( + target=self._thread_main, daemon=True, name="runner-loop" + ) + self._thread.start() def stop(self) -> None: """Signal the loop to stop and wait briefly for the thread to exit.""" @@ -618,8 +811,8 @@ def _thread_main(self) -> None: try: asyncio.run(self._main()) except Exception as exc: # pragma: no cover - crash guard - logger.exception("runner loop crashed") - self._last_error = str(exc) + logger.error("runner loop stopped ({kind})", kind=type(exc).__name__) + self._last_error = f"runner loop stopped ({type(exc).__name__})" self._set_state("error") def _set_state(self, state: str) -> None: @@ -724,7 +917,7 @@ async def ensure_registered(self, client: RunnerClient) -> bool: if cred and cred.get("runner_token"): client.token = cred["runner_token"] return True - session = auth_header().get("Authorization", "") + session = auth_header(self.config.hosted_host).get("Authorization", "") if not session.startswith("Bearer "): self._last_error = "not signed in; log in before enabling the runner" self._set_state("reauth_required") @@ -762,6 +955,11 @@ async def _main(self) -> None: async def _tick(self, client: RunnerClient) -> float | None: """One poll iteration; returns the next sleep delay, None to stop.""" + async with self._tick_lock: + return await self._tick_once(client) + + async def _tick_once(self, client: RunnerClient) -> float | None: + """Run one serialized poll and its complete leased job, if present.""" try: self._set_state("polling") job = await client.poll( @@ -773,7 +971,7 @@ async def _tick(self, client: RunnerClient) -> float | None: self._set_state("reauth_required") return None except (httpx.HTTPError, OSError) as exc: - self._last_error = str(exc) + self._last_error = f"runner transport failed ({type(exc).__name__})" self._set_state("offline") delay = backoff_delay(self._attempt, self._rng) self._attempt += 1 @@ -787,8 +985,11 @@ async def _tick(self, client: RunnerClient) -> float | None: except (httpx.HTTPError, OSError) as exc: # A dropped callback/ack never crashes the loop; the cloud's lease # expiry semantics land the run `uncertain` server-side. - self._last_error = str(exc) - logger.warning("job handling hit a network error: {e}", e=exc) + self._last_error = f"runner transport failed ({type(exc).__name__})" + logger.warning( + "job handling hit a network error ({kind})", + kind=type(exc).__name__, + ) delay = backoff_delay(self._attempt, self._rng) self._attempt += 1 return delay @@ -808,7 +1009,11 @@ async def reconcile_restart(self, client: RunnerClient) -> None: try: await client.ack(job_id, "uncertain", run_id=run_id, reason=reason) except (httpx.HTTPError, OSError) as exc: - logger.warning("uncertain ack for {r} deferred: {e}", r=run_id, e=exc) + logger.warning( + "uncertain ack for {r} deferred ({kind})", + r=run_id, + kind=type(exc).__name__, + ) continue self.journal.record(run_id, "finished", outcome="uncertain", reason=reason) @@ -816,14 +1021,39 @@ async def reconcile_restart(self, client: RunnerClient) -> None: async def handle_job(self, client: RunnerClient, job: dict) -> None: """Validate -> execute -> stream evidence -> ack for one leased job.""" + received_at = datetime.now(timezone.utc) + async with self._handle_lock: + await self._handle_job(client, job, received_at=received_at) + + async def _handle_job( + self, client: RunnerClient, job: dict, *, received_at: datetime + ) -> None: + """Handle one job under the process-local single-flight lock.""" + run_id = str(job.get("run_id") or "") - job_id = str((job.get("lease") or {}).get("job_id") or "") - if not run_id or not job_id: - logger.warning("dispatch missing run_id/lease.job_id; ignoring") + if not re.fullmatch(r"[A-Za-z0-9_-]{1,200}", run_id): + logger.warning("dispatch has an invalid run id; refusing") + return + try: + job_id, lease_deadline = _lease_deadline(job, received_at=received_at) + except Refusal as refusal: + raw_job_id = str((job.get("lease") or {}).get("job_id") or "") + if re.fullmatch(r"[A-Za-z0-9_-]{1,200}", raw_job_id): + await client.ack( + raw_job_id, + "refused", + run_id=run_id, + reason=str(refusal), + ) return - existing = self.journal.get(run_id) - if existing and existing.get("phase") == "started": + try: + existing = self.journal.get(run_id) + except RunnerJournalError: + reason = "local run journal is corrupt; outcome requires reconciliation" + await client.ack(job_id, "uncertain", run_id=run_id, reason=reason) + return + if existing and existing.get("phase") in {"starting", "started"}: # Idempotency: this run already began executing (e.g. re-leased # after a crash). NEVER silently re-execute. reason = "run was already started on this runner; outcome uncertain" @@ -848,6 +1078,8 @@ async def handle_job(self, client: RunnerClient, job: dict) -> None: # The org's safety policy binds THIS run, resolved fresh and before # any GUI action. An unenforceable policy refuses here. policy, deployment = await asyncio.to_thread(self.bind_effective_policy) + if datetime.now(timezone.utc) >= lease_deadline: + raise Refusal("dispatch lease expired before start") except Refusal as refusal: reason = str(refusal) logger.warning("dispatch {r} refused: {why}", r=run_id, why=reason) @@ -857,17 +1089,28 @@ async def handle_job(self, client: RunnerClient, job: dict) -> None: self.journal.record( run_id, - "started", + "starting", policy_source=policy.get("source"), policy_version=policy.get("policy_version"), ) - self._set_state("running") - await self._evidence( + start_confirmed = await self._evidence( client, run_id, authorization_id, seq, "state", {"state": "started", "at": datetime.now(timezone.utc).isoformat()}, ) + if not start_confirmed: + reason = "run start could not be confirmed; no action was dispatched" + await client.ack(job_id, "uncertain", run_id=run_id, reason=reason) + self.journal.record( + run_id, "finished", outcome="uncertain", reason=reason + ) + self._set_state("polling") + return + self.journal.record(run_id, "started") + self._set_state("running") run_dir = self.config.data_dir / "runner" / "runs" / run_id - extend_task = asyncio.ensure_future(self._extend_loop(client, job_id)) + extend_task = asyncio.ensure_future( + self._extend_loop(client, job_id, lease_deadline=lease_deadline) + ) try: result = await asyncio.to_thread( self._execute, @@ -882,7 +1125,15 @@ async def handle_job(self, client: RunnerClient, job: dict) -> None: exec_error = str(exc) exec_ok = False finally: - extend_task.cancel() + if extend_task.done(): + lease_error = extend_task.result() + else: + extend_task.cancel() + try: + await extend_task + except asyncio.CancelledError: + pass + lease_error = None report = FlowBridge.read_report(run_dir) halt = FlowBridge.read_halt(run_dir) @@ -892,35 +1143,69 @@ async def handle_job(self, client: RunnerClient, job: dict) -> None: await self._evidence( client, run_id, authorization_id, seq, "step", _step_event(step, index) ) - if halt: + if lease_error: + status = "uncertain" + elif halt: status = "halted-needs-attention" await self._evidence( - client, run_id, authorization_id, seq, "halt", _halt_event(halt) + client, + run_id, + authorization_id, + seq, + "halt", + _halt_event( + halt, + run_id=run_id, + workflow_id=str(job.get("workflow_id") or ""), + step_count=len(steps), + ), ) elif exec_ok: status = "confirmed" else: status = "failed" - await self._evidence( - client, run_id, authorization_id, seq, "run_summary", - _run_summary(job, report, status), - ) + if status != "uncertain": + await self._evidence( + client, run_id, authorization_id, seq, "run_summary", + _run_summary(job, report, status), + ) self._record_local_run(run_id, run_dir, job, halt, status) self.journal.record( run_id, "finished", outcome=status, - reason=(exec_error or "")[:200] or None, + reason=(lease_error or exec_error or "")[:200] or None, + ) + await client.ack( + job_id, + status, + run_id=run_id, + reason=lease_error if status == "uncertain" else None, ) - await client.ack(job_id, status, run_id=run_id) self._set_state("polling") - async def _extend_loop(self, client: RunnerClient, job_id: str) -> None: - """Renew the lease periodically while a run executes (spec Q6).""" + async def _extend_loop( + self, client: RunnerClient, job_id: str, *, lease_deadline: datetime + ) -> str | None: + """Renew a live lease and return an uncertainty reason if it expires.""" + while True: - await asyncio.sleep(LEASE_EXTEND_INTERVAL_S) + remaining_s = ( + lease_deadline - datetime.now(timezone.utc) + ).total_seconds() + if remaining_s <= 0: + return "lease expired while the run was in progress" + await asyncio.sleep(min(LEASE_EXTEND_INTERVAL_S, remaining_s)) + if datetime.now(timezone.utc) >= lease_deadline: + return "lease expired while the run was in progress" try: await client.extend(job_id) except (httpx.HTTPError, OSError) as exc: - logger.warning("lease extend failed: {e}", e=exc) + logger.warning( + "lease extend failed ({kind})", kind=type(exc).__name__ + ) + continue + lease_deadline = datetime.now(timezone.utc) + timedelta( + seconds=DEFAULT_LEASE_S + ) async def _stage_bundle(self, job: dict) -> Path: """Locate or download the sealed bundle for a dispatch. @@ -930,27 +1215,51 @@ async def _stage_bundle(self, job: dict) -> Path: """ bundle_info = job.get("bundle") or {} digest = str(bundle_info.get("content_digest") or "") - if not digest: - raise Refusal("dispatch missing bundle content digest") + if not _SHA256_RE.fullmatch(digest): + raise Refusal("dispatch bundle content digest is invalid") store_dir = self.config.data_dir / "runner" / "bundles" / digest if (store_dir / "manifest.json").is_file(): return store_dir - url = bundle_info.get("url") - if not url: + raw_url = bundle_info.get("url") + if not raw_url: raise Refusal( f"bundle {_digest_prefix(digest)} not in local store and no staging URL" ) - archive = store_dir.with_suffix(".zip") + url = _safe_bundle_url(raw_url) store_dir.parent.mkdir(parents=True, exist_ok=True) - async with self._http_factory() as http: - resp = await http.get(url) - resp.raise_for_status() - archive.write_bytes(resp.content) + archive_fd, archive_name = tempfile.mkstemp( + dir=str(store_dir.parent), prefix=f".{digest}.", suffix=".zip" + ) + os.close(archive_fd) + archive = Path(archive_name) + staging_dir = Path( + tempfile.mkdtemp(dir=str(store_dir.parent), prefix=f".{digest}.", suffix=".tmp") + ) try: - safe_extract_zip(archive, store_dir) + total = 0 + async with self._http_factory() as http: + async with http.stream("GET", url) as resp: + resp.raise_for_status() + with archive.open("wb") as output: + async for chunk in resp.aiter_bytes(): + total += len(chunk) + if total > MAX_BUNDLE_ARCHIVE_BYTES: + raise Refusal("bundle archive exceeds the runner limit") + output.write(chunk) + safe_extract_zip(archive, staging_dir) + try: + staging_dir.replace(store_dir) + except FileExistsError: + if not (store_dir / "manifest.json").is_file(): + raise Refusal("bundle staging destination is inconsistent") from None + return store_dir + except Refusal: + raise + except (OSError, zipfile.BadZipFile, zipfile.LargeZipFile): + raise Refusal("bundle staging failed safety validation") from None finally: archive.unlink(missing_ok=True) - return store_dir + shutil.rmtree(staging_dir, ignore_errors=True) def _execute( self, @@ -972,8 +1281,26 @@ def _execute( policy has been applied to. """ run_dir.mkdir(parents=True, exist_ok=True) + try: + run_dir.chmod(0o700) + except OSError: + pass auth_path = run_dir / "authorization.json" - auth_path.write_text(json.dumps(authorization, indent=2)) + fd, temporary = tempfile.mkstemp( + dir=str(run_dir), prefix=".authorization.", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(authorization, handle, indent=2) + handle.flush() + os.fsync(handle.fileno()) + os.chmod(temporary, 0o600) + os.replace(temporary, auth_path) + finally: + try: + os.unlink(temporary) + except FileNotFoundError: + pass bridge = self.services.flow_bridge kwargs: dict[str, Any] = {} probe = getattr(bridge, "run_supports_authorization", None) @@ -1004,8 +1331,8 @@ def _record_local_run(self, run_id: str, run_dir: Path, job: dict, async def _evidence(self, client: RunnerClient, run_id: str, authorization_id: str, seq: _Seq, kind: str, - payload: dict) -> None: - """Send one evidence event; PHI violations fail closed and abort nothing else.""" + payload: dict) -> bool: + """Send one evidence event and report whether Cloud confirmed receipt.""" event: dict[str, Any] = { "schema": EVIDENCE_SCHEMA, "run_id": run_id, @@ -1021,8 +1348,14 @@ async def _evidence(self, client: RunnerClient, run_id: str, # evidence stays in the local run dir (the operator's audit copy). logger.error("evidence event for {r} violated the PHI boundary; dropped", r=run_id) + return False except (httpx.HTTPError, OSError) as exc: - logger.warning("evidence POST failed (run continues): {e}", e=exc) + logger.warning( + "evidence POST failed; local execution state is retained ({kind})", + kind=type(exc).__name__, + ) + return False + return True class _Seq: diff --git a/engine/upload_manager.py b/engine/upload_manager.py index c8350c3..0547c95 100644 --- a/engine/upload_manager.py +++ b/engine/upload_manager.py @@ -44,6 +44,7 @@ approved_egress_path, derivative_tree_sha256, load_derivative_approval, + update_derivative_tree_digest, ) # Durable/offline retry policy (spec section 5): jobs survive restarts (they @@ -233,9 +234,17 @@ def _freeze_artifact( ) if relative == "review_status.json": continue - frozen_tree.update(relative.encode("utf-8")) if not member.is_file(): - continue + if member.is_dir(): + update_derivative_tree_digest( + frozen_tree, + relative_path=relative, + member_type="directory", + ) + continue + raise EgressArtifactError( + "The sanitized derivative has an unsupported member type." + ) stat = os.stat(member, follow_symlinks=False) if stat.st_nlink != 1: raise EgressArtifactError( @@ -250,7 +259,12 @@ def _freeze_artifact( while chunk := input_file.read(1024 * 1024): file_digest.update(chunk) output_file.write(chunk) - frozen_tree.update(file_digest.hexdigest().encode("ascii")) + update_derivative_tree_digest( + frozen_tree, + relative_path=relative, + member_type="file", + file_sha256=file_digest.hexdigest(), + ) if ( frozen_tree.hexdigest() != approved_tree_sha256 or derivative_tree_sha256(source) != approved_tree_sha256 diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index e253707..c1cbe53 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -14,6 +14,8 @@ use serde_json::{json, Value}; use tauri::{AppHandle, Manager, State}; +use tauri_plugin_shell::ShellExt; +use url::Url; use crate::sidecar::SidecarHandle; @@ -104,30 +106,59 @@ pub fn ensure_control_overlay_capture_excluded(app: AppHandle) -> Result<(), Str /// Used for the login deep-link ("open Settings -> Ingest tokens"), "Open cloud /// dashboard", and the OS System Settings permission panes. Implemented with the /// platform opener so no extra plugin capability is required. -#[tauri::command] -pub fn open_external(url: String) -> Result<(), String> { - // Only allow http(s), the app's custom scheme, and macOS System Settings deep - // links — never arbitrary shell strings. - let allowed = url.starts_with("https://") - || url.starts_with("http://") - || url.starts_with("openadapt://") - || url.starts_with("x-apple.systempreferences:"); - if !allowed { - return Err(format!("refusing to open non-web URL: {url}")); +fn validated_external_url(value: &str) -> Result { + let parsed = Url::parse(value).map_err(|_| "refusing to open an invalid URL".to_string())?; + let allowed_scheme = matches!(parsed.scheme(), "https" | "http" | "openadapt") + || cfg!(target_os = "macos") && parsed.scheme() == "x-apple.systempreferences"; + if !allowed_scheme { + return Err("refusing to open a URL with an unsupported scheme".to_string()); + } + if matches!(parsed.scheme(), "https" | "http") + && (parsed.host_str().is_none() + || !parsed.username().is_empty() + || parsed.password().is_some()) + { + return Err("refusing to open a web URL with invalid authority".to_string()); + } + if value.chars().any(char::is_control) { + return Err("refusing to open a URL with control characters".to_string()); } + Ok(parsed.to_string()) +} + +#[tauri::command] +pub fn open_external(app: AppHandle, url: String) -> Result<(), String> { + let safe_url = validated_external_url(&url)?; + app.shell() + .open(safe_url, None) + .map_err(|error| format!("failed to open URL: {error}")) +} - #[cfg(target_os = "macos")] - let result = std::process::Command::new("open").arg(&url).spawn(); - #[cfg(target_os = "windows")] - let result = std::process::Command::new("cmd") - .args(["/C", "start", "", &url]) - .spawn(); - #[cfg(all(unix, not(target_os = "macos")))] - let result = std::process::Command::new("xdg-open").arg(&url).spawn(); +#[cfg(test)] +mod external_url_tests { + use super::validated_external_url; - result - .map(|_| ()) - .map_err(|e| format!("failed to open URL: {e}")) + #[test] + fn accepts_clean_web_url() { + assert_eq!( + validated_external_url("https://app.openadapt.ai/dashboard").unwrap(), + "https://app.openadapt.ai/dashboard" + ); + } + + #[test] + fn rejects_shell_and_authority_tricks() { + for value in [ + "javascript:alert(1)", + "https://user:secret@example.com/", + "https://example.com/\nnext", + "https://example.com\" & calc.exe", + "https://example.com|calc.exe", + "https://example.com^calc.exe", + ] { + assert!(validated_external_url(value).is_err(), "accepted {value}"); + } + } } // -------------------------------------------------------------------------- diff --git a/src/lib/engine.ts b/src/lib/engine.ts index c74e48f..067dd6a 100644 --- a/src/lib/engine.ts +++ b/src/lib/engine.ts @@ -221,13 +221,11 @@ export function onFfmpegRuntimeStatus( /** Open a URL in the system browser (login deep-link, cloud dashboard, panes). */ export async function openExternal(url: string): Promise { - try { - await invoke("open_external", { url }); - } catch (e) { - // Fall back to a plain anchor navigation when running outside Tauri (dev). + if (!inTauri()) { window.open(url, "_blank"); - void e; + return; } + await invoke("open_external", { url }); } /** Subscribe to an engine event. Returns an unlisten fn. */ diff --git a/src/screens/Settings.tsx b/src/screens/Settings.tsx index 89b8d7b..81f4512 100644 --- a/src/screens/Settings.tsx +++ b/src/screens/Settings.tsx @@ -49,6 +49,7 @@ export function Settings({ overlayPresentationEnabled, ); const [overlayError, setOverlayError] = useState(null); + const [configError, setConfigError] = useState(null); useEffect(() => { engineTry(CMD.GET_CONFIG, {}, cfg).then(setCfg); @@ -60,11 +61,24 @@ export function Settings({ }, []); async function save(key: K, value: Cfg[K]) { - setCfg((c) => ({ ...c, [key]: value })); + setConfigError(null); try { - await engineInvoke(CMD.SET_CONFIG, { key, value }); - } catch { - /* offline: kept in local UI state */ + const result = await engineInvoke<{ ok: boolean; error?: string; host?: string }>( + CMD.SET_CONFIG, + { key, value }, + ); + if (!result.ok) { + setConfigError(result.error || "The setting was refused."); + setCfg(await engineTry(CMD.GET_CONFIG, {}, cfg)); + return; + } + setCfg((current) => ({ + ...current, + [key]: key === "host" && result.host ? result.host : value, + })); + } catch (error) { + setConfigError(`The setting was not saved: ${String(error)}`); + setCfg(await engineTry(CMD.GET_CONFIG, {}, cfg)); } } @@ -187,10 +201,16 @@ export function Settings({ save("host", e.target.value)} + onChange={(e) => setCfg((current) => ({ ...current, host: e.target.value }))} + onBlur={(e) => void save("host", e.target.value)} spellCheck={false} /> + {configError && ( + + {configError} + + )}
{auth.authenticated ? "signed in" : "not signed in"} diff --git a/tests/test_engine/test_auth_store.py b/tests/test_engine/test_auth_store.py index 5a9c938..f315983 100644 --- a/tests/test_engine/test_auth_store.py +++ b/tests/test_engine/test_auth_store.py @@ -114,6 +114,47 @@ def test_falls_back_to_active_credential(self, fake_keyring) -> None: def test_empty_when_no_credential(self, fake_keyring) -> None: assert store.auth_header() == {} + def test_host_bound_header_refuses_active_credential_for_other_origin( + self, fake_keyring + ) -> None: + store.store_credential(_cred(host="https://app.openadapt.ai")) + + assert store.auth_header("https://customer.example") == {} + assert store.auth_header("https://app.openadapt.ai/dashboard") == { + "Authorization": "Bearer oai_ingest_abc" + } + + def test_explicit_environment_token_is_operator_override( + self, fake_keyring, monkeypatch + ) -> None: + store.store_credential(_cred(host="https://app.openadapt.ai")) + monkeypatch.setenv("OPENADAPT_INGEST_TOKEN", "explicit-env-token") + + assert store.auth_header("https://customer.example") == { + "Authorization": "Bearer explicit-env-token" + } + + def test_host_bound_header_refuses_unsafe_destination_even_with_environment_token( + self, fake_keyring, monkeypatch + ) -> None: + monkeypatch.setenv("OPENADAPT_INGEST_TOKEN", "explicit-env-token") + + assert store.auth_header("http://customer.example") == {} + assert store.auth_header("https://user:secret@customer.example") == {} + assert store.auth_header("not-a-url") == {} + + def test_canonical_host_origin_allows_only_https_or_local_http(self) -> None: + assert ( + store.canonical_host_origin("HTTPS://Customer.Example:443/dashboard") + == "https://customer.example" + ) + assert ( + store.canonical_host_origin("http://localhost:3000/path") + == "http://localhost:3000" + ) + assert store.canonical_host_origin("http://customer.example") == "" + assert store.canonical_host_origin("https://user@example.com") == "" + class _RaisingKeyring: """A keyring backend with no usable store -- every call raises. diff --git a/tests/test_engine/test_dispatch.py b/tests/test_engine/test_dispatch.py index faa61f5..ff39033 100644 --- a/tests/test_engine/test_dispatch.py +++ b/tests/test_engine/test_dispatch.py @@ -1379,6 +1379,10 @@ def test_push_workflow(self, deps, monkeypatch) -> None: "error": "", }, ) + monkeypatch.setattr( + "engine.qualification_lifecycle.persist_deployment_handoff", + lambda *a, **k: disp.config.data_dir / "handoff.json", + ) r = disp.dispatch("push_workflow", {"workflow_id": "bnd1"}) assert r["ok"] is True assert r["workflow_id"] == "wf_1" @@ -1398,6 +1402,10 @@ def test_push_workflow_preserves_local_review_handoff(self, deps, monkeypatch) - "error": "", }, ) + monkeypatch.setattr( + "engine.qualification_lifecycle.persist_deployment_handoff", + lambda *a, **k: disp.config.data_dir / "handoff.json", + ) result = disp.dispatch("push_workflow", {"workflow_id": "bnd1"}) @@ -1426,10 +1434,77 @@ def test_login_paste(self, deps, monkeypatch, fake_keyring) -> None: assert r["authenticated"] is True assert r["org_id"] == "org_1" + def test_custom_origin_login_updates_live_and_persisted_host( + self, deps, monkeypatch, tmp_path + ) -> None: + disp, _db, _e = deps + host = "https://customer.example" + toml_path = tmp_path / "config.toml" + monkeypatch.setenv("OPENADAPT_CONFIG_TOML", str(toml_path)) + cred = { + "kind": "ingest_token", + "token": "oai_ingest_customer", + "refresh_token": None, + "org_id": "org_customer", + "host": host, + "expires_at": None, + } + monkeypatch.setattr( + "engine.auth.paste.PasteTokenProvider.login", lambda self, token=None: cred + ) + + result = disp.dispatch("login_paste", {"host": f"{host}/path", "token": "t"}) + + assert result["authenticated"] is True + assert disp.config.hosted_host == host + assert f'host = "{host}"' in toml_path.read_text() + + def test_login_refuses_remote_http_before_provider(self, deps, monkeypatch) -> None: + disp, _db, _e = deps + called = False + + def _login(self, token=None): + nonlocal called + called = True + raise AssertionError("provider must not receive credentials") + + monkeypatch.setattr("engine.auth.paste.PasteTokenProvider.login", _login) + result = disp.dispatch( + "login_paste", {"host": "http://customer.example", "token": "secret"} + ) + assert result["authenticated"] is False + assert "HTTPS" in result["error"] + assert called is False + def test_get_auth_status_unauthed(self, deps, fake_keyring) -> None: disp, _db, _e = deps assert disp.dispatch("get_auth_status", {})["authenticated"] is False + def test_get_auth_status_refuses_credential_for_other_configured_host( + self, deps, fake_keyring + ) -> None: + from engine.auth.provider import Credential + from engine.auth.store import store_credential + + disp, _db, _e = deps + credential: Credential = { + "kind": "ingest_token", + "token": "oai_ingest_host_bound", + "refresh_token": None, + "org_id": "org_42", + "host": "https://app.openadapt.ai", + "expires_at": None, + } + store_credential(credential) + disp.config.hosted_host = "https://customer.example" + + result = disp.dispatch("get_auth_status", {}) + + assert result == { + "authenticated": False, + "host": "https://customer.example", + } + def test_connect_uri_forwards_one_exact_string_and_emits_safe_state( self, deps, monkeypatch, tmp_path ) -> None: @@ -1458,6 +1533,25 @@ def _connect(exact_uri: str) -> dict: }, ) in events + def test_connect_uri_returns_the_canonical_authenticated_host( + self, deps, monkeypatch, tmp_path + ) -> None: + disp, _db, _events = deps + monkeypatch.setenv("OPENADAPT_CONFIG_TOML", str(tmp_path / "config.toml")) + monkeypatch.setattr( + "engine.auth.pairing.connect_uri", + lambda _uri: { + "authenticated": True, + "host": "HTTPS://Customer.Example:443/pairing", + "paired": True, + }, + ) + + result = disp.dispatch("connect_uri", {"uri": "openadapt://connect"}) + + assert result["host"] == "https://customer.example" + assert disp.config.hosted_host == "https://customer.example" + def test_connect_uri_requires_a_single_string_parameter(self, deps) -> None: disp, _db, _events = deps for params in ({}, {"uri": ["openadapt://connect"]}, {"argv": ["--uri", "x"]}): @@ -1494,6 +1588,25 @@ def test_set_config_rejects_unknown_key(self, deps) -> None: r = disp.dispatch("set_config", {"key": "s3_secret_access_key", "value": "x"}) assert r["ok"] is False + def test_set_config_canonicalizes_host_alias(self, deps, monkeypatch, tmp_path) -> None: + disp, _db, _e = deps + monkeypatch.setenv("OPENADAPT_CONFIG_TOML", str(tmp_path / "config.toml")) + result = disp.dispatch( + "set_config", + {"key": "host", "value": "HTTPS://Customer.Example:443/dashboard"}, + ) + assert result["ok"] is True + assert result["host"] == "https://customer.example" + + def test_set_config_refuses_remote_http(self, deps) -> None: + disp, _db, _e = deps + original = disp.config.hosted_host + result = disp.dispatch( + "set_config", {"key": "host", "value": "http://customer.example"} + ) + assert result["ok"] is False + assert disp.config.hosted_host == original + class TestRunReportMapping: """_run_report must map openadapt-flow's real report.json onto RunReport.""" diff --git a/tests/test_engine/test_flow_bridge.py b/tests/test_engine/test_flow_bridge.py index 77433e0..737b5d6 100644 --- a/tests/test_engine/test_flow_bridge.py +++ b/tests/test_engine/test_flow_bridge.py @@ -459,6 +459,19 @@ def test_secret_flag_values_are_redacted_from_debug_command(self) -> None: assert "oar_secret" not in rendered assert rendered == "openadapt-flow push --token [REDACTED] --kind bundle" + def test_host_is_redacted_from_debug_command(self) -> None: + rendered = _safe_command_for_log( + [ + "openadapt-flow", + "push", + "artifact", + "--host", + "https://customer-private.example", + ] + ) + assert "customer-private" not in rendered + assert "--host [REDACTED]" in rendered + def test_egress_local_paths_are_redacted_from_debug_command(self) -> None: rendered = _safe_command_for_log( [ diff --git a/tests/test_engine/test_hosted.py b/tests/test_engine/test_hosted.py index e21c392..20f3722 100644 --- a/tests/test_engine/test_hosted.py +++ b/tests/test_engine/test_hosted.py @@ -13,6 +13,109 @@ from engine.flow_bridge import FlowResult from engine.hosted import PhiBoundaryError, report_break, zip_dir +_SHA_A = "a" * 64 +_SHA_B = "b" * 64 +_SHA_C = "c" * 64 +_SHA_D = "d" * 64 +_WORKFLOW_ID = "123e4567-e89b-12d3-a456-426614174000" +_INGEST_ID = "223e4567-e89b-42d3-a456-426614174000" +_ORG_ID = "323e4567-e89b-42d3-a456-426614174000" +_BUNDLE_VERSION_ID = "423e4567-e89b-42d3-a456-426614174000" +_RUNTIME_VALIDATION_ID = "523e4567-e89b-42d3-a456-426614174000" + + +def _push_document( + *, status: str = "accepted_for_ingest", kind: str = "recording" +) -> dict: + document = { + "schema": "openadapt.push-result/v1", + "status": status, + "workflow_id": None, + "artifact_ingest_id": None, + "review": None, + "attestation": None, + "binding": { + "kind": kind, + "source_tree_sha256": _SHA_A, + "derivative_tree_sha256": _SHA_B, + "approved_archive_sha256": None, + "artifact_sha256": None, + "bundle_sha256": None, + "source_recording_sha256": None, + "sanitization_policy": "outbound-phi-v1", + "certification_policy": None, + "certification_evidence_sha256": None, + "governed_authorization_template_sha256": None, + "parameter_schema_sha256": None, + "attested_run_report_sha256": None, + "resolves_run_id": None, + "organization_id": None, + "bundle_version_id": None, + "bundle_version": None, + "runtime_validation_id": None, + }, + "next_action": None, + "dashboard_url": None, + "delivery": {"attempted": False, "certainty": "not_attempted"}, + "error": None, + } + if status == "paused_for_review": + document["review"] = { + "id": _SHA_C, + "scope": "local_non_authoritative", + "sanitized_path": "/safe/derivative", + "command": "openadapt-flow review-sanitized /safe/derivative", + } + document["next_action"] = "review_local" + elif status == "accepted_for_ingest": + document["artifact_ingest_id"] = _INGEST_ID + document["review"] = { + "id": _SHA_C, + "scope": "local_non_authoritative", + "sanitized_path": None, + "command": None, + } + document["binding"]["approved_archive_sha256"] = _SHA_D + document["binding"]["artifact_sha256"] = _SHA_D + document["delivery"] = {"attempted": True, "certainty": "accepted"} + if kind == "recording": + document["next_action"] = "parameterize" + else: + document["workflow_id"] = _WORKFLOW_ID + document["attestation"] = { + "id": "challenge-1", + "schema": "openadapt.runtime-validation/v3", + } + document["binding"].update( + { + "bundle_sha256": _SHA_D, + "source_recording_sha256": _SHA_A, + "certification_policy": "regulated", + "certification_evidence_sha256": _SHA_B, + "parameter_schema_sha256": _SHA_C, + "attested_run_report_sha256": _SHA_D, + "organization_id": _ORG_ID, + "bundle_version_id": _BUNDLE_VERSION_ID, + "bundle_version": 3, + "runtime_validation_id": _RUNTIME_VALIDATION_ID, + } + ) + document["next_action"] = "open_dashboard" + document["dashboard_url"] = ( + f"https://app.openadapt.ai/dashboard/workflows/{_WORKFLOW_ID}" + ) + elif status == "failed": + document["binding"]["kind"] = None + document["binding"]["source_tree_sha256"] = None + document["binding"]["derivative_tree_sha256"] = None + document["binding"]["sanitization_policy"] = None + document["delivery"] = {"attempted": None, "certainty": "not_accepted"} + document["error"] = { + "code": "push_failed", + "message": "The artifact was not accepted for ingest.", + } + return document + class _StubBackend: name = "hosted_ingest" @@ -152,10 +255,32 @@ def test_missing_flow_push_never_falls_back_to_direct_ingest( result = hosted.push(rec) assert result["success"] is False - assert result["workflow_id"] == "" + assert result["workflow_id"] is None assert result["delivery_uncertain"] is True assert "Do not retry blindly" in result["error"] + def test_push_exception_detail_is_not_logged( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + rec = tmp_path / "rec" + rec.mkdir() + secret = "/captures/Jane-Doe-12345/raw.sqlite" + warnings: list[tuple[tuple, dict]] = [] + monkeypatch.setattr( + hosted, + "_push_via_flow", + lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError(secret)), + ) + monkeypatch.setattr( + "engine.hosted.logger.warning", + lambda *args, **kwargs: warnings.append((args, kwargs)), + ) + + result = hosted.push(rec) + + assert result["delivery_uncertain"] is True + assert secret not in repr(warnings) + def test_flow_review_pause_is_not_reported_as_upload_success( self, tmp_path: Path, @@ -163,23 +288,20 @@ def test_flow_review_pause_is_not_reported_as_upload_success( ) -> None: rec = tmp_path / "rec" rec.mkdir() - derivative = tmp_path / "sanitized" / "artifact-abc" - output = ( - f"Sanitized derivative created at {derivative}.\n" - "Upload paused for local review; the original was not modified or uploaded.\n" - f"openadapt-flow review-sanitized {derivative} --original {rec}\n" - ) + document = _push_document(status="paused_for_review") monkeypatch.setattr( "engine.hosted.FlowBridge.push", - lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout=output), + lambda *args, **kwargs: FlowResult( + ok=True, returncode=0, stdout=json.dumps(document) + ), ) result = hosted.push(rec) assert result["success"] is False assert result["pending_review"] is True - assert result["sanitized_path"] == str(derivative) - assert result["workflow_id"] == "" + assert result["sanitized_path"] == "/safe/derivative" + assert result["workflow_id"] is None def test_desktop_credential_reaches_flow_only_through_environment( self, @@ -191,12 +313,8 @@ def test_desktop_credential_reaches_flow_only_through_environment( calls: list[dict] = [] monkeypatch.setattr( hosted, - "active_credential", - lambda: { - "host": "https://app.openadapt.ai", - "token": "stored-secret", - "org_id": "org-1", - }, + "token_for_host", + lambda host, explicit=None: explicit or "stored-secret", ) def fake_push(*_args, **kwargs): @@ -204,10 +322,7 @@ def fake_push(*_args, **kwargs): return FlowResult( ok=True, returncode=0, - stdout=( - "Pushed. workflow_id=123e4567-e89b-12d3-a456-426614174000 " - "(name='Example', kind=recording, compile=ok).\n" - ), + stdout=json.dumps(_push_document()), ) monkeypatch.setattr("engine.hosted.FlowBridge.push", fake_push) @@ -230,14 +345,18 @@ def test_credential_for_another_host_is_not_forwarded( calls: list[dict] = [] monkeypatch.setattr( hosted, - "active_credential", - lambda: {"host": "https://other.example", "token": "wrong-host-secret"}, + "token_for_host", + lambda host, explicit=None: explicit or "", ) monkeypatch.setattr( "engine.hosted.FlowBridge.push", lambda *_args, **kwargs: ( calls.append(kwargs) - or FlowResult(ok=False, returncode=1, stdout="Not logged in") + or FlowResult( + ok=False, + returncode=1, + stdout=json.dumps(_push_document(status="failed")), + ) ), ) @@ -252,17 +371,17 @@ def test_flow_success_requires_and_parses_hosted_workflow_identity( ) -> None: rec = tmp_path / "rec" rec.mkdir() - workflow_id = "123e4567-e89b-12d3-a456-426614174000" - output = ( - f"Pushed. workflow_id={workflow_id} (name='Example', kind=recording, compile=ok).\n" - f"Dashboard: https://app.openadapt.ai/dashboard/workflows/{workflow_id}\n" - ) + workflow_id = _WORKFLOW_ID monkeypatch.setattr( "engine.hosted.FlowBridge.push", - lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout=output), + lambda *args, **kwargs: FlowResult( + ok=True, + returncode=0, + stdout=json.dumps(_push_document(kind="bundle")), + ), ) - result = hosted.push(rec) + result = hosted.push(rec, kind="bundle") assert result["success"] is True assert result["workflow_id"] == workflow_id @@ -277,13 +396,16 @@ def test_flow_exit_zero_without_identity_fails_closed( rec.mkdir() monkeypatch.setattr( "engine.hosted.FlowBridge.push", - lambda *args, **kwargs: FlowResult(ok=True, returncode=0, stdout="Pushed."), + lambda *args, **kwargs: FlowResult( + ok=True, returncode=0, stdout=json.dumps(_push_document(status="failed")) + ), ) result = hosted.push(rec) assert result["success"] is False - assert "without an authenticated hosted workflow identity" in result["error"] + assert result["delivery_uncertain"] is True + assert result["error_code"] == "invalid_ingest_response" def test_flow_none_workflow_identity_is_not_success( self, @@ -292,19 +414,21 @@ def test_flow_none_workflow_identity_is_not_success( ) -> None: rec = tmp_path / "rec" rec.mkdir() + malformed = _push_document(kind="bundle") + malformed["workflow_id"] = None monkeypatch.setattr( "engine.hosted.FlowBridge.push", lambda *args, **kwargs: FlowResult( ok=True, returncode=0, - stdout="Pushed. workflow_id=None (name='Example', kind=recording, compile=?).", + stdout=json.dumps(malformed), ), ) - result = hosted.push(rec) + result = hosted.push(rec, kind="bundle") assert result["success"] is False - assert result["workflow_id"] == "" + assert result["workflow_id"] is None def test_flow_failure_uses_bounded_stdout_and_marks_delivery_uncertain( self, @@ -313,12 +437,13 @@ def test_flow_failure_uses_bounded_stdout_and_marks_delivery_uncertain( ) -> None: rec = tmp_path / "rec" rec.mkdir() + document = _push_document(status="failed") monkeypatch.setattr( "engine.hosted.FlowBridge.push", lambda *args, **kwargs: FlowResult( ok=False, returncode=1, - stdout="request outcome unknown", + stdout=json.dumps(document), stderr="", ), ) @@ -326,8 +451,8 @@ def test_flow_failure_uses_bounded_stdout_and_marks_delivery_uncertain( result = hosted.push(rec, token="secret-value") assert result["success"] is False - assert result["delivery_uncertain"] is True - assert result["error"] == "request outcome unknown" + assert result["delivery_uncertain"] is False + assert result["error"] == "The artifact was not accepted for ingest." class TestReportBreak: @@ -409,6 +534,25 @@ def test_not_logged_in(self, tmp_path: Path, fake_keyring) -> None: assert result["ok"] is False assert "Not logged in" in result["error"] + def test_report_exception_detail_is_not_logged(self, tmp_path: Path, monkeypatch) -> None: + run_dir = tmp_path / "run" + self._write_report(run_dir, {"reason": "drift"}) + secret = "/runs/Jane-Doe-12345/report.json" + warnings: list[tuple[tuple, dict]] = [] + monkeypatch.setattr( + "engine.hosted.FlowBridge.report_break", + lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError(secret)), + ) + monkeypatch.setattr( + "engine.hosted.logger.warning", + lambda *args, **kwargs: warnings.append((args, kwargs)), + ) + + result = report_break(run_dir, workflow_id="wf_1", token="oai_ingest_x") + + assert result["delivery_uncertain"] is True + assert secret not in repr(warnings) + def test_free_text_is_not_sent_by_desktop(self, tmp_path: Path, monkeypatch) -> None: run_dir = tmp_path / "run" secret = "patient Jane Doe has record 12345" diff --git a/tests/test_engine/test_policy.py b/tests/test_engine/test_policy.py index f7ea679..2e41212 100644 --- a/tests/test_engine/test_policy.py +++ b/tests/test_engine/test_policy.py @@ -14,6 +14,7 @@ from __future__ import annotations import json +from datetime import UTC, datetime, timedelta from pathlib import Path import httpx @@ -48,9 +49,14 @@ def cache_path(tmp_path: Path, monkeypatch) -> Path: """Redirect the policy cache to a tmp file for the duration of a test.""" path = tmp_path / "policy.json" monkeypatch.setenv("OPENADAPT_POLICY_CACHE", str(path)) + monkeypatch.setenv("OPENADAPT_INGEST_TOKEN", "oai_ingest_policy_test_principal") return path +def _write_bound_cache(policy: dict, host: str = "https://app.openadapt.ai") -> None: + policy_mod._write_cache(policy, host) + + class TestFetchAndCache: def test_network_success_writes_cache_and_returns_network( self, cache_path: Path, monkeypatch @@ -64,16 +70,23 @@ def test_network_success_writes_cache_and_returns_network( assert result["policy_version"] == 7 assert result["is_admin"] is True assert result["safety"] == policy_mod.SAFE_SAFETY_DEFAULTS - # Cache was written with the raw body (no source field). + # The cache uses a closed, principal-bound envelope. assert cache_path.exists() cached = json.loads(cache_path.read_text()) - assert cached["policy_version"] == 7 - assert "source" not in cached + assert cached["schema"] == policy_mod.CACHE_SCHEMA + assert cached["binding"]["host_origin"] == "https://app.openadapt.ai" + assert cached["binding"]["org_id"] == "org_42" + assert cached["binding"]["policy_version"] == 7 + assert cached["binding"]["policy_sha256"] == policy_mod._policy_sha256( + cached["policy"] + ) + assert cached["policy"]["policy_version"] == 7 + assert "source" not in cached["policy"] def test_network_failure_falls_back_to_cache( self, cache_path: Path, monkeypatch ) -> None: - cache_path.write_text(json.dumps(_full_policy(policy_version=3))) + _write_bound_cache(_full_policy(policy_version=3)) def _down(*a, **k): raise httpx.ConnectError("network down") @@ -105,7 +118,7 @@ def _down(*a, **k): assert set(result["safety"]) == set(policy_mod.SAFE_SAFETY_DEFAULTS) def test_http_error_status_falls_back(self, cache_path: Path, monkeypatch) -> None: - cache_path.write_text(json.dumps(_full_policy(policy_version=9))) + _write_bound_cache(_full_policy(policy_version=9)) monkeypatch.setattr( "engine.policy.httpx.get", lambda *a, **k: FakeResponse(500, {}) ) @@ -120,11 +133,203 @@ def test_fetch_raises_on_401(self, cache_path: Path, monkeypatch) -> None: with pytest.raises(policy_mod.PolicyFetchError, match="401"): policy_mod.fetch_effective_policy("https://app.openadapt.ai") + def test_network_policy_requires_a_monotonic_version( + self, cache_path: Path, monkeypatch + ) -> None: + body = _full_policy() + body.pop("policy_version") + monkeypatch.setattr( + "engine.policy.httpx.get", lambda *a, **k: FakeResponse(200, body) + ) + with pytest.raises(policy_mod.PolicyFetchError, match="policy version"): + policy_mod.fetch_effective_policy("https://app.openadapt.ai") + assert not cache_path.exists() + + def test_network_policy_cannot_move_a_bound_version_backwards( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy(policy_version=8)) + monkeypatch.setattr( + "engine.policy.httpx.get", + lambda *a, **k: FakeResponse(200, _full_policy(policy_version=7)), + ) + + with pytest.raises(policy_mod.PolicyFetchError, match="moved backwards"): + policy_mod.fetch_effective_policy("https://app.openadapt.ai") + assert policy_mod.load_cached_policy("https://app.openadapt.ai")[ + "policy_version" + ] == 8 + + def test_network_policy_cannot_change_without_a_new_version( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy(policy_version=8)) + changed = _full_policy(policy_version=8) + changed["safety"] = {**changed["safety"], "halt_on_ambiguous": False} + monkeypatch.setattr( + "engine.policy.httpx.get", lambda *a, **k: FakeResponse(200, changed) + ) + + with pytest.raises(policy_mod.PolicyFetchError, match="without a new version"): + policy_mod.fetch_effective_policy("https://app.openadapt.ai") + + @pytest.mark.parametrize( + ("field", "value"), + [ + ("baseline_version", "2026.08"), + ("org", {"retention_days": 7}), + ("grounding_model", "reviewed-grounding-v2"), + ], + ) + def test_network_policy_authority_cannot_change_without_a_new_version( + self, cache_path: Path, monkeypatch, field: str, value: object + ) -> None: + _write_bound_cache(_full_policy(policy_version=8)) + changed = _full_policy(policy_version=8) + changed[field] = value + monkeypatch.setattr( + "engine.policy.httpx.get", lambda *a, **k: FakeResponse(200, changed) + ) + + with pytest.raises(policy_mod.PolicyFetchError, match="without a new version"): + policy_mod.fetch_effective_policy("https://app.openadapt.ai") + + def test_same_version_allows_request_and_user_projection_changes( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy(policy_version=8)) + refreshed = _full_policy( + policy_version=8, + resolved_at="2026-07-21T00:00:00Z", + role="member", + is_admin=False, + user={"theme": "light"}, + ) + monkeypatch.setattr( + "engine.policy.httpx.get", lambda *a, **k: FakeResponse(200, refreshed) + ) + + assert policy_mod.fetch_effective_policy("https://app.openadapt.ai") == refreshed + assert policy_mod.load_cached_policy("https://app.openadapt.ai") == refreshed + + def test_remote_policy_host_requires_https( + self, cache_path: Path, monkeypatch + ) -> None: + called = False + + def _get(*_args, **_kwargs): + nonlocal called + called = True + return FakeResponse(200, _full_policy()) + + monkeypatch.setattr("engine.policy.httpx.get", _get) + with pytest.raises(policy_mod.PolicyFetchError, match="must use HTTPS"): + policy_mod.fetch_effective_policy("http://policy.example.test") + assert called is False + def test_load_cached_policy_degrades_on_corrupt( self, cache_path: Path ) -> None: cache_path.write_text("{ not json") - assert policy_mod.load_cached_policy() is None + assert policy_mod.load_cached_policy("https://app.openadapt.ai") is None + + def test_offline_cache_rejects_other_host( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy(policy_version=3)) + monkeypatch.setattr( + "engine.policy.httpx.get", + lambda *a, **k: (_ for _ in ()).throw(httpx.ConnectError("down")), + ) + result = policy_mod.resolve_effective_policy("https://other.openadapt.ai") + assert result["source"] == policy_mod.UNCONFIRMED_POLICY_SOURCE + assert result["policy_version"] is None + + def test_offline_cache_rejects_other_credential( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy(policy_version=3)) + monkeypatch.setenv("OPENADAPT_INGEST_TOKEN", "oai_ingest_other_principal") + monkeypatch.setattr( + "engine.policy.httpx.get", + lambda *a, **k: (_ for _ in ()).throw(httpx.ConnectError("down")), + ) + result = policy_mod.resolve_effective_policy("https://app.openadapt.ai") + assert result["source"] == policy_mod.UNCONFIRMED_POLICY_SOURCE + + def test_offline_cache_rejects_other_org( + self, cache_path: Path, monkeypatch, fake_keyring + ) -> None: + from engine.auth.provider import Credential + from engine.auth.store import store_credential + + monkeypatch.delenv("OPENADAPT_INGEST_TOKEN") + credential: Credential = { + "kind": "ingest_token", + "token": "oai_ingest_org_42_token", + "refresh_token": None, + "org_id": "org_42", + "host": "https://app.openadapt.ai", + "expires_at": None, + } + store_credential(credential) + _write_bound_cache(_full_policy(org_id="org_42")) + credential["org_id"] = "org_99" + store_credential(credential) + monkeypatch.setattr( + "engine.policy.httpx.get", + lambda *a, **k: (_ for _ in ()).throw(httpx.ConnectError("down")), + ) + result = policy_mod.resolve_effective_policy("https://app.openadapt.ai") + assert result["source"] == policy_mod.UNCONFIRMED_POLICY_SOURCE + + def test_offline_cache_expires( + self, cache_path: Path, monkeypatch + ) -> None: + _write_bound_cache(_full_policy()) + envelope = json.loads(cache_path.read_text()) + envelope["fetched_at"] = ( + datetime.now(UTC) - timedelta(seconds=policy_mod.DEFAULT_CACHE_MAX_AGE_S + 1) + ).isoformat() + cache_path.write_text(json.dumps(envelope)) + assert policy_mod.load_cached_policy("https://app.openadapt.ai") is None + + def test_legacy_unbound_cache_is_rejected(self, cache_path: Path) -> None: + cache_path.write_text(json.dumps(_full_policy())) + assert policy_mod.load_cached_policy("https://app.openadapt.ai") is None + + def test_cache_without_a_policy_version_is_rejected(self, cache_path: Path) -> None: + policy = _full_policy() + policy.pop("policy_version") + cache_path.write_text( + json.dumps( + { + "schema": policy_mod.CACHE_SCHEMA, + "binding": { + "host_origin": "https://app.openadapt.ai", + "credential_sha256": policy_mod._credential_sha256( + "https://app.openadapt.ai" + ), + "org_id": "org_42", + "policy_version": None, + "policy_sha256": policy_mod._policy_sha256(policy), + }, + "fetched_at": datetime.now(UTC).isoformat(), + "policy": policy, + } + ) + ) + assert policy_mod.load_cached_policy("https://app.openadapt.ai") is None + + def test_cache_rejects_a_policy_body_changed_after_binding( + self, cache_path: Path + ) -> None: + _write_bound_cache(_full_policy(policy_version=3)) + envelope = json.loads(cache_path.read_text()) + envelope["policy"]["safety"]["halt_on_ambiguous"] = False + cache_path.write_text(json.dumps(envelope)) + + assert policy_mod.load_cached_policy("https://app.openadapt.ai") is None def test_atomic_write_leaves_no_temp_files( self, cache_path: Path, monkeypatch diff --git a/tests/test_engine/test_push_result_contract.py b/tests/test_engine/test_push_result_contract.py new file mode 100644 index 0000000..38004cb --- /dev/null +++ b/tests/test_engine/test_push_result_contract.py @@ -0,0 +1,249 @@ +"""Desktop consumption of Flow's exact openadapt.push-result/v1 contract.""" + +from __future__ import annotations + +import json +import stat +from pathlib import Path + +from engine.qualification_lifecycle import ( + parse_flow_push, + persist_deployment_handoff, +) + +SHA_A = "a" * 64 +SHA_B = "b" * 64 +SHA_C = "c" * 64 +SHA_D = "d" * 64 +WORKFLOW_ID = "123e4567-e89b-42d3-a456-426614174000" +INGEST_ID = "223e4567-e89b-42d3-a456-426614174000" +ORG_ID = "323e4567-e89b-42d3-a456-426614174000" +BUNDLE_VERSION_ID = "423e4567-e89b-42d3-a456-426614174000" +RUNTIME_VALIDATION_ID = "523e4567-e89b-42d3-a456-426614174000" + + +def push_document(*, status: str, kind: str = "recording") -> dict: + document = { + "schema": "openadapt.push-result/v1", + "status": status, + "workflow_id": None, + "artifact_ingest_id": None, + "review": None, + "attestation": None, + "binding": { + "kind": kind, + "source_tree_sha256": SHA_A, + "derivative_tree_sha256": SHA_B, + "approved_archive_sha256": None, + "artifact_sha256": None, + "bundle_sha256": None, + "source_recording_sha256": None, + "sanitization_policy": "outbound-phi-v1", + "certification_policy": None, + "certification_evidence_sha256": None, + "governed_authorization_template_sha256": None, + "parameter_schema_sha256": None, + "attested_run_report_sha256": None, + "resolves_run_id": None, + "organization_id": None, + "bundle_version_id": None, + "bundle_version": None, + "runtime_validation_id": None, + }, + "next_action": None, + "dashboard_url": None, + "delivery": {"attempted": False, "certainty": "not_attempted"}, + "error": None, + } + if status == "paused_for_review": + document["review"] = { + "id": SHA_C, + "scope": "local_non_authoritative", + "sanitized_path": "/private/sanitized/artifact", + "command": "openadapt-flow review-sanitized /private/sanitized/artifact", + } + document["next_action"] = "review_local" + elif status == "accepted_for_ingest": + document["artifact_ingest_id"] = INGEST_ID + document["review"] = { + "id": SHA_C, + "scope": "local_non_authoritative", + "sanitized_path": None, + "command": None, + } + document["binding"]["approved_archive_sha256"] = SHA_D + document["binding"]["artifact_sha256"] = SHA_D + document["delivery"] = {"attempted": True, "certainty": "accepted"} + if kind == "recording": + document["next_action"] = "validate_runtime" + else: + document["workflow_id"] = WORKFLOW_ID + document["attestation"] = { + "id": "challenge-7", + "schema": "openadapt.runtime-validation/v3", + } + document["binding"].update( + { + "bundle_sha256": SHA_D, + "source_recording_sha256": SHA_A, + "certification_policy": "regulated", + "certification_evidence_sha256": SHA_B, + "governed_authorization_template_sha256": SHA_C, + "parameter_schema_sha256": SHA_C, + "attested_run_report_sha256": SHA_D, + "organization_id": ORG_ID, + "bundle_version_id": BUNDLE_VERSION_ID, + "bundle_version": 3, + "runtime_validation_id": RUNTIME_VALIDATION_ID, + } + ) + document["next_action"] = "open_dashboard" + document["dashboard_url"] = ( + f"https://app.openadapt.ai/dashboard/workflows/{WORKFLOW_ID}" + ) + elif status == "delivery_uncertain": + document["next_action"] = "reconcile" + document["delivery"] = {"attempted": True, "certainty": "unknown"} + document["error"] = { + "code": "delivery_uncertain", + "message": "Reconcile the exact artifact before any retry.", + } + return document + + +def test_pause_preserves_exact_review_and_binding() -> None: + document = push_document(status="paused_for_review") + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["ok"] is True + assert result["pending_review"] is True + assert result["artifact_ingest_id"] is None + assert result["binding"]["source_tree_sha256"] == SHA_A + assert result["sanitized_path"] == "/private/sanitized/artifact" + + +def test_accepted_recording_keeps_server_ingest_id_without_workflow() -> None: + document = push_document(status="accepted_for_ingest") + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["accepted_for_ingest"] is True + assert result["deployed"] is False + assert result["workflow_id"] is None + assert result["artifact_ingest_id"] == INGEST_ID + assert result["next_action"] == "validate_runtime" + + +def test_accepted_bundle_requires_attested_exact_hashes_and_dashboard() -> None: + document = push_document(status="accepted_for_ingest", kind="bundle") + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["deployed"] is True + assert result["workflow_id"] == WORKFLOW_ID + assert result["artifact_ingest_id"] == INGEST_ID + assert result["binding"]["bundle_sha256"] == SHA_D + assert result["binding"]["organization_id"] == ORG_ID + assert result["binding"]["bundle_version_id"] == BUNDLE_VERSION_ID + assert result["binding"]["runtime_validation_id"] == RUNTIME_VALIDATION_ID + assert result["attestation"]["id"] == "challenge-7" + + +def test_bundle_rejects_invalid_retained_server_binding() -> None: + document = push_document(status="accepted_for_ingest", kind="bundle") + document["binding"]["runtime_validation_id"] = None + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["ok"] is False + assert result["delivery_uncertain"] is True + + +def test_uncertain_state_rejects_retained_server_binding() -> None: + document = push_document(status="delivery_uncertain") + document["binding"]["runtime_validation_id"] = RUNTIME_VALIDATION_ID + + result = parse_flow_push(json.dumps(document), "", ok=False) + + assert result["ok"] is False + assert result["delivery_uncertain"] is True + assert result["error_code"] == "invalid_ingest_response" + + +def test_failed_state_rejects_uncertain_delivery_shape() -> None: + document = push_document(status="delivery_uncertain") + document["status"] = "failed" + document["error"] = { + "code": "push_failed", + "message": "The artifact was not accepted for ingest.", + } + + result = parse_flow_push(json.dumps(document), "", ok=False) + + assert result["ok"] is False + assert result["delivery_uncertain"] is True + assert result["error_code"] == "invalid_ingest_response" + + +def test_missing_server_ingest_id_is_never_success() -> None: + document = push_document(status="accepted_for_ingest") + document["artifact_ingest_id"] = None + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["ok"] is False + assert result["delivery_uncertain"] is True + assert result["next_action"] == "reconcile" + + +def test_server_ids_must_match_the_closed_uuid_schema() -> None: + document = push_document(status="accepted_for_ingest") + document["artifact_ingest_id"] = "223e4567-e89b-02d3-a456-426614174000" + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["ok"] is False + assert result["delivery_uncertain"] is True + + +def test_process_status_conflict_is_never_success() -> None: + document = push_document(status="accepted_for_ingest") + result = parse_flow_push(json.dumps(document), "", ok=False) + assert result["ok"] is False + assert result["error_code"] == "invalid_ingest_response" + + +def test_invalid_output_does_not_reflect_child_diagnostics() -> None: + secret = "Jane Doe /private/captures/raw.sqlite oai_ingest_secret" + result = parse_flow_push("not-json", secret, ok=False) + assert result["delivery_uncertain"] is True + assert secret not in repr(result) + + +def test_bundle_dashboard_query_is_rejected() -> None: + document = push_document(status="accepted_for_ingest", kind="bundle") + document["dashboard_url"] += "?redirect=https://evil.example" + result = parse_flow_push(json.dumps(document), "", ok=True) + assert result["ok"] is False + assert result["delivery_uncertain"] is True + + +def test_bundle_dashboard_must_match_the_requested_host() -> None: + document = push_document(status="accepted_for_ingest", kind="bundle") + result = parse_flow_push( + json.dumps(document), "", ok=True, expected_host="https://other.openadapt.ai" + ) + assert result["ok"] is False + assert result["delivery_uncertain"] is True + + +def test_handoff_persists_exact_server_id_with_private_permissions(tmp_path: Path) -> None: + document = push_document(status="accepted_for_ingest") + result = parse_flow_push(json.dumps(document), "", ok=True) + path = persist_deployment_handoff( + tmp_path, local_workflow_id="local-workflow-1", result=result + ) + saved = json.loads(path.read_text()) + assert saved["state"] == "accepted_recording" + assert saved["push_result"]["artifact_ingest_id"] == INGEST_ID + assert stat.S_IMODE(path.stat().st_mode) == 0o600 + + +def test_invalid_child_result_persists_reconcile_state(tmp_path: Path) -> None: + result = parse_flow_push("bad", "raw local error", ok=False) + path = persist_deployment_handoff( + tmp_path, local_workflow_id="local-workflow-1", result=result + ) + saved = json.loads(path.read_text()) + assert saved["state"] == "delivery_uncertain" + assert saved["push_result"] is None + assert saved["error_code"] == "invalid_ingest_response" diff --git a/tests/test_engine/test_review_state.py b/tests/test_engine/test_review_state.py index 897c426..69eda30 100644 --- a/tests/test_engine/test_review_state.py +++ b/tests/test_engine/test_review_state.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib from pathlib import Path import pytest @@ -19,6 +20,26 @@ ) +def test_derivative_tree_digest_distinguishes_member_type_and_path(tmp_path: Path) -> None: + """A directory name cannot collide with a file path plus its content hash.""" + + from engine.review import derivative_tree_sha256 + + derivative = tmp_path / "capture.scrubbed" + derivative.mkdir() + secret = b"secret bytes that must not pass an old approval" + collision_name = "a" + hashlib.sha256(secret).hexdigest() + directory = derivative / collision_name + directory.mkdir() + directory_digest = derivative_tree_sha256(derivative) + + directory.rmdir() + (derivative / "a").write_bytes(secret) + file_digest = derivative_tree_sha256(derivative) + + assert file_digest != directory_digest + + @pytest.fixture def db(tmp_path: Path) -> IndexDB: """Create a temporary index database.""" diff --git a/tests/test_engine/test_runner_loop.py b/tests/test_engine/test_runner_loop.py index 75f3a39..f8681db 100644 --- a/tests/test_engine/test_runner_loop.py +++ b/tests/test_engine/test_runner_loop.py @@ -12,9 +12,16 @@ from __future__ import annotations +import asyncio import hashlib +import io import json import random +import stat +import threading +import time +import zipfile +from datetime import UTC, datetime, timedelta from pathlib import Path import httpx @@ -43,10 +50,12 @@ assert_phi_free, backoff_delay, bundle_content_digest, + safe_extract_zip, validate_dispatch, ) HOST = "https://cloud.test" +CONTRACT_HASH = f"sha256:{'a' * 64}" # A report whose steps carry PHI booby traps that must NEVER cross the wire. TRAPPED_REPORT = { @@ -56,7 +65,7 @@ { "step_id": "s1", "rung": "structural", - "effect_contract_hashes": ["sha256:aa"], + "effect_contract_hashes": [CONTRACT_HASH], "effect_verified": True, "identity_verified": True, "elapsed_ms": 10, @@ -81,7 +90,7 @@ "kind": "effect_refuted", "substrate": "fhir", "effect_kind": "record_written", - "contract_hash": "sha256:aa", + "contract_hash": CONTRACT_HASH, "verdict": "refuted", "reason": "observed 2 records, expected 1", "suggested_action": "inspect the matched records and remove the duplicate(s)", @@ -198,6 +207,8 @@ def __init__(self) -> None: self.poll_count = 0 self.poll_status: int | None = None # force a status (401/500) when set self.ack_status: int | None = None + self.evidence_status: int | None = None + self.extend_status: int | None = None self.bundles: dict[str, bytes] = {} # url path -> zip bytes def handler(self, request: httpx.Request) -> httpx.Response: @@ -219,6 +230,8 @@ def handler(self, request: httpx.Request) -> httpx.Response: return httpx.Response(204) if path == EXTEND_PATH: self.extends.append(body) + if self.extend_status is not None: + return httpx.Response(self.extend_status) return httpx.Response(200, json={"ok": True}) if path == ACK_PATH: if self.ack_status is not None: @@ -228,6 +241,8 @@ def handler(self, request: httpx.Request) -> httpx.Response: ) return httpx.Response(200, json={"ok": True}) if path.startswith("/api/runs/") and path.endswith("/evidence"): + if self.evidence_status is not None: + return httpx.Response(self.evidence_status) self.evidence.append(body) return httpx.Response(202, json={"ok": True}) if path in self.bundles: @@ -348,6 +363,7 @@ async def test_register_poll_lease_execute_callback_ack(self, rig) -> None: run_dir = config.data_dir / "runner" / "runs" / "run_1" auth_json = json.loads((run_dir / "authorization.json").read_text()) assert auth_json["authorization_id"] == "auth_1" + assert stat.S_IMODE((run_dir / "authorization.json").stat().st_mode) == 0o600 # evidence: started state, one step event per step, terminal summary kinds = [e["kind"] for e in cloud.evidence] @@ -390,7 +406,9 @@ async def test_halt_reports_reconciliation_task_fields(self, rig) -> None: assert halt["kind"] == "effect_refuted" assert halt["substrate"] == "fhir" assert halt["verdict"] == "refuted" - assert halt["contract_hash"] == "sha256:aa" + assert halt["contract_hash"] == CONTRACT_HASH + assert halt["reason"] == "halt at step s1" + assert "suggested_action" not in halt # counts ONLY -- observed/expected VALUES and matched_records stripped assert halt["evidence_digest"] == {"observed_count": 2, "expected_count": 1} assert cloud.evidence[-1]["run_summary"]["status"] == "halted-needs-attention" @@ -402,9 +420,6 @@ async def test_halt_reports_reconciliation_task_fields(self, rig) -> None: async def test_bundle_staged_from_signed_url(self, rig, tmp_path: Path) -> None: svc, cloud, flow, config, _db, _events = rig login() - import io - import zipfile - manifest = json.dumps({"workflow": "wf_remote"}).encode() digest = hashlib.sha256(manifest).hexdigest() buf = io.BytesIO() @@ -423,11 +438,59 @@ async def test_bundle_staged_from_signed_url(self, rig, tmp_path: Path) -> None: staged = config.data_dir / "runner" / "bundles" / digest / "manifest.json" assert staged.is_file() + def test_bundle_archive_rejects_prefix_traversal_and_symlinks( + self, tmp_path: Path + ) -> None: + traversal = tmp_path / "traversal.zip" + with zipfile.ZipFile(traversal, "w") as archive: + archive.writestr("../outside/manifest.json", "{}") + with pytest.raises(Refusal, match="unsafe member path"): + safe_extract_zip(traversal, tmp_path / "bundle-a") + assert not (tmp_path / "outside" / "manifest.json").exists() + + symlink = tmp_path / "symlink.zip" + info = zipfile.ZipInfo("manifest.json") + info.create_system = 3 + info.external_attr = (stat.S_IFLNK | 0o777) << 16 + with zipfile.ZipFile(symlink, "w") as archive: + archive.writestr(info, "/private/target") + with pytest.raises(Refusal, match="unsupported member type"): + safe_extract_zip(symlink, tmp_path / "bundle-b") + # ------------------------------------------------------------------ refusal class TestRefusal: + @pytest.mark.asyncio + async def test_refuses_digest_path_and_remote_cleartext_staging_url( + self, rig + ) -> None: + svc, cloud, flow, _config, _db, _events = rig + login() + unsafe_digest = "../../outside" + job = make_job(unsafe_digest) + job["bundle"]["url"] = "https://cloud.test/bundles/job.zip" + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, job) + + assert flow.calls == [] + assert cloud.acks[-1]["outcome"] == "refused" + assert "digest is invalid" in cloud.acks[-1]["reason"] + + job = make_job("a" * 64, run_id="run_2") + job["bundle"]["url"] = "http://downloads.example/bundle.zip" + job["lease"] = {"job_id": "job_2", "visibility_timeout_s": 900} + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, job) + + assert flow.calls == [] + assert cloud.acks[-1]["outcome"] == "refused" + assert "staging URL is invalid" in cloud.acks[-1]["reason"] + @pytest.mark.asyncio async def test_refuses_on_local_digest_mismatch(self, rig) -> None: svc, cloud, flow, config, _db, _events = rig @@ -842,6 +905,131 @@ async def test_duplicate_lease_of_finished_run_reacks_same_outcome( assert flow.calls == [] assert cloud.acks[-1]["outcome"] == "confirmed" + @pytest.mark.asyncio + async def test_corrupt_journal_refuses_to_reexecute_the_run(self, rig) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + journal_path = svc.journal._path("run_1") + journal_path.parent.mkdir(parents=True) + journal_path.write_text('{"run_id":"run_1","phase":"started"') + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, make_job(digest)) + + assert flow.calls == [] + assert cloud.acks[-1]["outcome"] == "uncertain" + + +class TestLeaseDiscipline: + @pytest.mark.asyncio + async def test_expired_lease_refuses_before_execution(self, rig) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + expired = (datetime.now(UTC) - timedelta(seconds=1)).isoformat() + job = make_job(digest) + job["lease"] = { + "job_id": "job_1", + "visibility_timeout_s": 900, + "expires_at": expired, + } + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, job) + + assert flow.calls == [] + assert cloud.acks[-1]["outcome"] == "refused" + assert "lease expired" in cloud.acks[-1]["reason"] + + @pytest.mark.asyncio + async def test_start_must_be_confirmed_before_any_gui_execution(self, rig) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + cloud.evidence_status = 503 + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, make_job(digest)) + + assert flow.calls == [] + assert cloud.acks[-1]["outcome"] == "uncertain" + entry = svc.journal.get("run_1") + assert entry["phase"] == "finished" + assert entry["outcome"] == "uncertain" + + @pytest.mark.asyncio + async def test_expired_unrenewed_lease_cannot_report_false_success( + self, rig, monkeypatch + ) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + cloud.extend_status = 503 + job = make_job(digest) + job["lease"] = { + "job_id": "job_1", + "visibility_timeout_s": 900, + "expires_at": (datetime.now(UTC) + timedelta(seconds=0.08)).isoformat(), + } + original_run = flow.run + + def slow_run(*args, **kwargs): + time.sleep(0.12) + return original_run(*args, **kwargs) + + monkeypatch.setattr(flow, "run", slow_run) + monkeypatch.setattr("engine.runner_loop.LEASE_EXTEND_INTERVAL_S", 0.01) + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await svc.handle_job(client, job) + + assert len(flow.calls) == 1 + assert cloud.extends + assert cloud.acks[-1]["outcome"] == "uncertain" + assert not any(event["kind"] == "run_summary" for event in cloud.evidence) + entry = svc.journal.get("run_1") + assert entry["outcome"] == "uncertain" + + @pytest.mark.asyncio + async def test_concurrent_ticks_never_actuate_two_jobs_at_once( + self, rig, monkeypatch + ) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + cloud.jobs.extend( + [make_job(digest, run_id="run_1"), make_job(digest, run_id="run_2")] + ) + active = 0 + maximum_active = 0 + guard = threading.Lock() + original_run = flow.run + + def slow_run(*args, **kwargs): + nonlocal active, maximum_active + with guard: + active += 1 + maximum_active = max(maximum_active, active) + try: + time.sleep(0.05) + return original_run(*args, **kwargs) + finally: + with guard: + active -= 1 + + monkeypatch.setattr(flow, "run", slow_run) + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + await asyncio.gather(svc._tick(client), svc._tick(client)) + + assert len(flow.calls) == 2 + assert maximum_active == 1 + # ------------------------------------------------------------------ PHI boundary @@ -863,6 +1051,8 @@ async def test_no_forbidden_field_ever_serializes(self, rig) -> None: assert "SENSITIVE" not in wire assert "123-45-6789" not in wire assert "frame-004.png" not in wire + assert TRAPPED_HALT["reason"] not in wire + assert TRAPPED_HALT["suggested_action"] not in wire def test_assert_phi_free_fails_closed(self) -> None: with pytest.raises(PhiBoundaryError): @@ -896,6 +1086,24 @@ async def test_client_rejects_phi_event_before_wire(self, rig) -> None: class TestTransport: + @pytest.mark.asyncio + async def test_signed_download_error_does_not_expose_url_query(self, rig) -> None: + svc, cloud, flow, _config, _db, _events = rig + login() + secret = "SENSITIVE-SIGNED-QUERY" + job = make_job("a" * 64) + job["bundle"]["url"] = f"{HOST}/missing.zip?signature={secret}" + cloud.jobs.append(job) + + async with svc._http_factory() as http: + client = RunnerClient(http, token="oar_test") + delay = await svc._tick(client) + + assert delay is not None and delay > 0 + assert flow.calls == [] + assert secret not in repr(svc.status()) + assert secret not in all_wire_payloads(cloud) + def test_backoff_is_exponential_jittered_and_capped(self) -> None: rng = random.Random(42) for attempt in range(10): From c930ed57710009eb6618d438be10c6509ee0b5a2 Mon Sep 17 00:00:00 2001 From: abrichr Date: Tue, 18 Aug 2026 15:18:52 -0400 Subject: [PATCH 4/6] fix: fail closed at hosted trust boundaries --- engine/flow_bridge.py | 26 +++++++++++++++++++- engine/policy.py | 35 +++++++++++++++++---------- tests/test_engine/test_flow_bridge.py | 23 +++++++++++++++--- tests/test_engine/test_policy.py | 6 ++++- 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/engine/flow_bridge.py b/engine/flow_bridge.py index aed5659..6fb391d 100644 --- a/engine/flow_bridge.py +++ b/engine/flow_bridge.py @@ -467,6 +467,7 @@ def __init__( self._runner = runner self._popen = popen self._run_auth_support: bool | None = None + self._push_json_support: bool | None = None # --- low-level --- @@ -776,8 +777,15 @@ def supports_command(self, command: str) -> bool: """Best-effort probe for an optional Flow subcommand.""" try: - return self._run([command, "--help"], timeout=15).ok + result = self._run([command, "--help"], timeout=15) + if command == "push": + self._push_json_support = result.ok and "--json" in ( + result.stdout or "" + ) + return result.ok except Exception: + if command == "push": + self._push_json_support = False return False def push( @@ -794,6 +802,11 @@ def push( ) -> FlowResult: """Upload through the same pinned Flow runtime as every other verb.""" + if json_output and not self.push_supports_json(): + raise FlowNotAvailableError( + "The pinned openadapt-flow runtime does not support the structured " + "push-result contract. Update the exact Flow pin before hosted upload." + ) args = ["push", str(path), "--kind", kind, "--host", host] if json_output: args.append("--json") @@ -806,6 +819,17 @@ def push( child_env[_INGEST_TOKEN_ENV] = token return self._run(args, timeout=timeout, env_overrides=child_env or None) + def push_supports_json(self) -> bool: + """Require Flow's machine-readable push contract before any upload.""" + + if self._push_json_support is None: + try: + result = self._run(["push", "--help"], timeout=15) + self._push_json_support = result.ok and "--json" in (result.stdout or "") + except Exception: + self._push_json_support = False + return self._push_json_support + def report_break( self, run_dir: Path, diff --git a/engine/policy.py b/engine/policy.py index 1b30854..98f4ea8 100644 --- a/engine/policy.py +++ b/engine/policy.py @@ -59,6 +59,7 @@ from __future__ import annotations import hashlib +import hmac import json import os import tempfile @@ -93,7 +94,7 @@ # A cached org policy is an offline continuity aid, not permanent authority. # After one day the Desktop must reconnect before it can govern another run. DEFAULT_CACHE_MAX_AGE_S = 24 * 60 * 60 -CACHE_SCHEMA = "openadapt.policy-cache/v2" +CACHE_SCHEMA = "openadapt.policy-cache/v3" # The SAFEST value for every safety key the contract defines. A missing or # unreachable value MUST resolve to the entry here (fail-closed): more checking, @@ -171,16 +172,22 @@ def _policy_cache_path() -> Path: return Path(override) if override else DEFAULT_POLICY_CACHE -def _credential_sha256(host: str) -> str | None: - """Return a non-secret, destination-bound identity for the active bearer.""" +def _credential_binding_hmac(host: str) -> str | None: + """Return a keyed, destination-bound identity for the active bearer. + + The bearer is the HMAC key. It is never stored and it is not passed through + an unkeyed password-hash operation. The public message binds the result to + this cache contract and exact hosted origin. + """ token = token_for_host(host) if not token: return None - digest = hashlib.sha256() - digest.update(b"openadapt.policy-cache-credential/v1\0") - digest.update(token.encode("utf-8")) - return digest.hexdigest() + origin = canonical_host_origin(host) + if not origin: + return None + message = f"openadapt.policy-cache-credential/v2\0{origin}".encode() + return hmac.new(token.encode("utf-8"), message, hashlib.sha256).hexdigest() def _credential_org_id(host: str) -> str | None: @@ -344,12 +351,12 @@ def _write_cache(policy: dict[str, Any], host: str) -> None: """ path = _policy_cache_path() host_origin = canonical_host_origin(host) - credential_sha256 = _credential_sha256(host) + credential_binding_hmac = _credential_binding_hmac(host) org_id = policy.get("org_id") policy_version = _policy_version(policy) if ( not host_origin - or not credential_sha256 + or not credential_binding_hmac or not isinstance(org_id, str) or not org_id or policy_version is None @@ -360,7 +367,7 @@ def _write_cache(policy: dict[str, Any], host: str) -> None: "schema": CACHE_SCHEMA, "binding": { "host_origin": host_origin, - "credential_sha256": credential_sha256, + "credential_binding_hmac": credential_binding_hmac, "org_id": org_id, "policy_version": policy_version, "policy_sha256": _policy_sha256(policy), @@ -425,7 +432,7 @@ def load_cached_policy( return None if set(binding) != { "host_origin", - "credential_sha256", + "credential_binding_hmac", "org_id", "policy_version", "policy_sha256", @@ -434,12 +441,14 @@ def load_cached_policy( if not isinstance(policy, dict): return None expected_origin = canonical_host_origin(host) - expected_credential = _credential_sha256(host) + expected_credential = _credential_binding_hmac(host) if not expected_origin or not expected_credential: return None if binding.get("host_origin") != expected_origin: return None - if binding.get("credential_sha256") != expected_credential: + if not hmac.compare_digest( + str(binding.get("credential_binding_hmac") or ""), expected_credential + ): return None policy_org_id = policy.get("org_id") if not isinstance(policy_org_id, str) or not policy_org_id: diff --git a/tests/test_engine/test_flow_bridge.py b/tests/test_engine/test_flow_bridge.py index 737b5d6..f8ab747 100644 --- a/tests/test_engine/test_flow_bridge.py +++ b/tests/test_engine/test_flow_bridge.py @@ -14,6 +14,7 @@ EMBEDDED_FLOW_MODE, BrowserRuntimeError, FlowBridge, + FlowNotAvailableError, _safe_command_for_log, flow_available, ) @@ -90,7 +91,7 @@ def test_push_keeps_token_and_local_name_out_of_argv( ) -> None: monkeypatch.setattr("engine.flow_bridge.shutil.which", lambda _: "/usr/bin/openadapt-flow") calls: list = [] - bridge = FlowBridge(runner=_runner(calls, stdout="ok")) + bridge = FlowBridge(runner=_runner(calls, stdout="--json\nok")) bridge.push( tmp_path / "bundle", @@ -100,7 +101,7 @@ def test_push_keeps_token_and_local_name_out_of_argv( token="secret-value", ) - command, env = calls[0] + command, env = calls[1] assert "secret-value" not in command assert "Jane Doe patient transfer" not in command assert "--token" not in command @@ -527,7 +528,7 @@ def test_optional_commands_use_same_bundled_runtime(self, monkeypatch) -> None: monkeypatch.setattr("engine.flow_bridge._is_frozen", lambda: True) monkeypatch.setattr("engine.flow_bridge.sys.executable", "/signed/openadapt-engine") calls: list = [] - bridge = FlowBridge(runner=_runner(calls, stdout="wf_123")) + bridge = FlowBridge(runner=_runner(calls, stdout="--json\nwf_123")) assert bridge.supports_command("push") result = bridge.push( @@ -550,6 +551,22 @@ def test_optional_commands_use_same_bundled_runtime(self, monkeypatch) -> None: "push", ] + def test_push_refuses_before_upload_when_structured_result_is_missing( + self, tmp_path: Path, monkeypatch + ) -> None: + monkeypatch.setattr("engine.flow_bridge.shutil.which", lambda _: "/usr/bin/openadapt-flow") + calls: list = [] + bridge = FlowBridge(runner=_runner(calls, stdout="legacy push help")) + + with pytest.raises(FlowNotAvailableError, match="structured push-result"): + bridge.push( + tmp_path / "bundle", + kind="bundle", + host="https://app.openadapt.ai", + ) + + assert [command[1:] for command, _env in calls] == [["push", "--help"]] + class TestReportParsing: def test_read_report_missing(self, tmp_path: Path) -> None: diff --git a/tests/test_engine/test_policy.py b/tests/test_engine/test_policy.py index 2e41212..8b934bb 100644 --- a/tests/test_engine/test_policy.py +++ b/tests/test_engine/test_policy.py @@ -77,6 +77,10 @@ def test_network_success_writes_cache_and_returns_network( assert cached["binding"]["host_origin"] == "https://app.openadapt.ai" assert cached["binding"]["org_id"] == "org_42" assert cached["binding"]["policy_version"] == 7 + assert cached["binding"]["credential_binding_hmac"] == ( + policy_mod._credential_binding_hmac("https://app.openadapt.ai") + ) + assert "credential_sha256" not in cached["binding"] assert cached["binding"]["policy_sha256"] == policy_mod._policy_sha256( cached["policy"] ) @@ -307,7 +311,7 @@ def test_cache_without_a_policy_version_is_rejected(self, cache_path: Path) -> N "schema": policy_mod.CACHE_SCHEMA, "binding": { "host_origin": "https://app.openadapt.ai", - "credential_sha256": policy_mod._credential_sha256( + "credential_binding_hmac": policy_mod._credential_binding_hmac( "https://app.openadapt.ai" ), "org_id": "org_42", From b8d8648f9f78e008f1e60f513c76cf9e89221a84 Mon Sep 17 00:00:00 2001 From: Richard Abrich Date: Tue, 18 Aug 2026 16:48:53 -0400 Subject: [PATCH 5/6] fix(runner): fail closed on exit code zero without signed VERIFIED proof The experimental runner lane mapped FlowBridge exit code 0 straight to a `confirmed` terminal outcome. Exit code zero proves only that the local process returned; it does not prove the governed effect. This lane does not yet consume Flow's shared qualification-v2 verifier, so it cannot bind an exact signed VERIFIED result to the run, authorization, policy, identity, effect, and event sequence. Until it can, a run that completes without a Flow halt terminates as `halted-needs-attention` with the constant reason COMPLETION_PROOF_REQUIRED_REASON. The reason crosses the ack boundary and the local journal verbatim, and the run is mirrored into the operator's local needs-attention list (kind `completion_proof_missing`) so it cannot pass silently. `confirmed` never leaves this path. - A real Flow halt keeps its own structural halt event and reason path. - A non-zero exit stays `failed`. - No halt evidence event is fabricated for the completion-proof case. Tests: the happy-path, signed-URL staging, and admin-toggle tests now assert the fail-closed contract; a new TestCompletionProof class covers the status, reason, journal, local mirror, PHI-free constant, halt precedence, and failed-exit paths. This is a temporary fail-closed boundary, not the final path: the lane still has to move to the Flow Connector contract and the frozen signed terminal-v2 result. Co-Authored-By: Claude Fable 5 --- engine/runner_loop.py | 50 +++++++-- tests/test_engine/test_runner_loop.py | 148 +++++++++++++++++++++++++- 2 files changed, 184 insertions(+), 14 deletions(-) diff --git a/engine/runner_loop.py b/engine/runner_loop.py index efa4361..86c7cd0 100644 --- a/engine/runner_loop.py +++ b/engine/runner_loop.py @@ -34,6 +34,13 @@ authoritatively, carries an unknown value, or cannot be bound REFUSES the dispatch (ack outcome ``refused``) -- the run never falls back to whatever the local deployment config happened to say. +* **Exit code zero is not proof.** A Flow process that returns ``0`` proves only + that the local process returned. It does not prove the governed effect. This + lane does not yet consume Flow's shared qualification-v2 verifier, so it + cannot bind an exact signed ``VERIFIED`` result to the run, authorization, + policy, identity, effect, and event sequence. Until it can, a run that + completes without a halt is acked ``halted-needs-attention`` with + :data:`COMPLETION_PROOF_REQUIRED_REASON`; it is NEVER acked ``confirmed``. The whole lane is experimental and OFF by default (``runner_enabled=false``); the cloud half is built in parallel -- this module codes to the spec's wire @@ -104,6 +111,16 @@ def evidence_path(run_id: str) -> str: _SAFE_ID_RE = re.compile(r"[A-Za-z0-9_.:-]{1,64}") _RUNGS = frozenset({"structural", "template", "ocr", "geometry"}) +# Terminal outcome for a run whose Flow process returned but which carries no +# signed qualification-v2 VERIFIED proof. Constant text: it crosses the ack +# boundary as ``reason`` and lands in the local halt mirror, so it must stay +# free of any run-derived value. +COMPLETION_PROOF_REQUIRED_REASON = ( + "run completed without the required signed qualification-v2 " + "VERIFIED proof; operator reconciliation is required" +) +COMPLETION_PROOF_HALT_KIND = "completion_proof_missing" + # --- PHI boundary (spec section 3) ---------------------------------------------------- # Keys that must NEVER appear anywhere in an outbound evidence/ack payload. @@ -1143,6 +1160,8 @@ async def _handle_job( await self._evidence( client, run_id, authorization_id, seq, "step", _step_event(step, index) ) + completion_proof_error: str | None = None + local_halt = halt if lease_error: status = "uncertain" elif halt: @@ -1161,7 +1180,19 @@ async def _handle_job( ), ) elif exec_ok: - status = "confirmed" + # Exit code zero proves only that the local process returned. It + # does not prove the governed effect. Keep this legacy lane + # fail-closed until it consumes Flow's shared qualification-v2 + # verifier and can bind an exact signed VERIFIED result to this + # run, authorization, policy, identity, effect, and event sequence. + # The operator sees the run in the local needs-attention mirror; + # the ack carries the constant reason, never a run-derived value. + status = "halted-needs-attention" + completion_proof_error = COMPLETION_PROOF_REQUIRED_REASON + local_halt = { + "kind": COMPLETION_PROOF_HALT_KIND, + "reason": COMPLETION_PROOF_REQUIRED_REASON, + } else: status = "failed" if status != "uncertain": @@ -1169,17 +1200,18 @@ async def _handle_job( client, run_id, authorization_id, seq, "run_summary", _run_summary(job, report, status), ) - self._record_local_run(run_id, run_dir, job, halt, status) + self._record_local_run(run_id, run_dir, job, local_halt, status) self.journal.record( run_id, "finished", outcome=status, - reason=(lease_error or exec_error or "")[:200] or None, - ) - await client.ack( - job_id, - status, - run_id=run_id, - reason=lease_error if status == "uncertain" else None, + reason=(lease_error or exec_error or completion_proof_error or "")[:200] or None, ) + if status == "uncertain": + ack_reason = lease_error + elif status == "halted-needs-attention": + ack_reason = completion_proof_error + else: + ack_reason = None + await client.ack(job_id, status, run_id=run_id, reason=ack_reason) self._set_state("polling") async def _extend_loop( diff --git a/tests/test_engine/test_runner_loop.py b/tests/test_engine/test_runner_loop.py index f8681db..0418715 100644 --- a/tests/test_engine/test_runner_loop.py +++ b/tests/test_engine/test_runner_loop.py @@ -36,6 +36,8 @@ from engine.runner_loop import ( ACK_PATH, BACKOFF_CAP_S, + COMPLETION_PROOF_HALT_KIND, + COMPLETION_PROOF_REQUIRED_REASON, EVIDENCE_SCHEMA, EXTEND_PATH, FORBIDDEN_EVIDENCE_KEYS, @@ -374,21 +376,25 @@ async def test_register_poll_lease_execute_callback_ack(self, rig) -> None: seqs = [e["seq"] for e in cloud.evidence] assert seqs == sorted(seqs) and len(set(seqs)) == len(seqs) + # A clean exit (exit code 0, no halt) carries NO signed qualification-v2 + # VERIFIED proof, so the terminal outcome is fail-closed: it is + # halted-needs-attention, never confirmed (see TestCompletionProof). summary = cloud.evidence[-1]["run_summary"] - assert summary["status"] == "confirmed" + assert summary["status"] == "halted-needs-attention" assert summary["bundle_digest"] == digest assert summary["screenshots_may_leave_box"] is False assert summary["effects_confirmed"] == 1 # terminal ack with the runner token assert cloud.acks[-1]["job_id"] == "job_1" - assert cloud.acks[-1]["outcome"] == "confirmed" + assert cloud.acks[-1]["outcome"] == "halted-needs-attention" + assert cloud.acks[-1]["reason"] == COMPLETION_PROOF_REQUIRED_REASON assert cloud.acks[-1]["auth"] == "Bearer oar_test" # journal reached terminal phase entry = svc.journal.get("run_1") assert entry["phase"] == "finished" - assert entry["outcome"] == "confirmed" + assert entry["outcome"] == "halted-needs-attention" @pytest.mark.asyncio async def test_halt_reports_reconciliation_task_fields(self, rig) -> None: @@ -434,7 +440,9 @@ async def test_bundle_staged_from_signed_url(self, rig, tmp_path: Path) -> None: await run_loop(svc, ticks=1) assert len(flow.calls) == 1 - assert cloud.acks[-1]["outcome"] == "confirmed" + # staged + executed; exit 0 alone is not proof, so fail-closed outcome + assert cloud.acks[-1]["outcome"] == "halted-needs-attention" + assert cloud.acks[-1]["reason"] == COMPLETION_PROOF_REQUIRED_REASON staged = config.data_dir / "runner" / "bundles" / digest / "manifest.json" assert staged.is_file() @@ -458,6 +466,133 @@ def test_bundle_archive_rejects_prefix_traversal_and_symlinks( safe_extract_zip(symlink, tmp_path / "bundle-b") +# ------------------------------------------------------------------ completion proof + + +class TestCompletionProof: + """Exit code zero is not proof of the governed effect. + + This legacy lane does not consume Flow's shared qualification-v2 verifier, + so it cannot bind a signed VERIFIED result to the run. A clean process exit + therefore terminates fail-closed: ``halted-needs-attention`` with the + constant completion-proof reason, mirrored into the operator's local + needs-attention list. ``confirmed`` must never reach the wire or the + journal from this path. + """ + + @pytest.mark.asyncio + async def test_exit_zero_without_signed_proof_halts_needs_attention( + self, rig + ) -> None: + svc, cloud, flow, config, db, _events = rig + login() + _bundle, digest = make_bundle(config) + flow.ok = True # exit code 0, no halt.json -- the exact false-success path + cloud.jobs.append(make_job(digest)) + + await run_loop(svc, ticks=1) + + assert len(flow.calls) == 1 + # wire: run_summary + ack say halted-needs-attention with the constant reason + assert cloud.evidence[-1]["kind"] == "run_summary" + assert cloud.evidence[-1]["run_summary"]["status"] == "halted-needs-attention" + assert cloud.acks[-1]["outcome"] == "halted-needs-attention" + assert cloud.acks[-1]["reason"] == COMPLETION_PROOF_REQUIRED_REASON + # no Flow halt existed, so no halt evidence event was fabricated + assert not any(e["kind"] == "halt" for e in cloud.evidence) + # journal: terminal, with the same reason + entry = svc.journal.get("run_1") + assert entry["phase"] == "finished" + assert entry["outcome"] == "halted-needs-attention" + assert entry["reason"] == COMPLETION_PROOF_REQUIRED_REASON + # local mirror: run status + one open needs-attention halt for the operator + assert db.get_run("run_1")["status"] == "halted-needs-attention" + assert db.count_open_halts() == 1 + local_halt = db.get_halt("halt-run_1") + assert local_halt is not None + assert local_halt["reason"] == COMPLETION_PROOF_REQUIRED_REASON + assert local_halt["workflow_id"] == "wf_1" + + @pytest.mark.asyncio + async def test_confirmed_never_leaves_this_lane_on_exit_zero(self, rig) -> None: + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + # a report that LOOKS fully verified is still not a signed proof + flow.report = { + **TRAPPED_REPORT, + "steps": [ + {**TRAPPED_REPORT["steps"][0], "effect_verified": True}, + {**TRAPPED_REPORT["steps"][1], "effect_verified": True}, + ], + } + cloud.jobs.append(make_job(digest)) + + await run_loop(svc, ticks=1) + + assert all(a["outcome"] != "confirmed" for a in cloud.acks) + assert all( + e["run_summary"]["status"] != "confirmed" + for e in cloud.evidence + if e["kind"] == "run_summary" + ) + assert svc.journal.get("run_1")["outcome"] != "confirmed" + + @pytest.mark.asyncio + async def test_completion_proof_reason_is_constant_and_phi_free( + self, rig + ) -> None: + # The reason crosses the ack boundary verbatim: it must be the module + # constant (no run-derived value) and pass the PHI guard. + svc, cloud, flow, config, _db, _events = rig + login() + _bundle, digest = make_bundle(config) + cloud.jobs.append(make_job(digest)) + + await run_loop(svc, ticks=1) + + assert_phi_free({"reason": cloud.acks[-1]["reason"]}) + assert cloud.acks[-1]["reason"] == COMPLETION_PROOF_REQUIRED_REASON + assert COMPLETION_PROOF_HALT_KIND == "completion_proof_missing" + assert "SENSITIVE" not in all_wire_payloads(cloud) + assert "run_1" not in cloud.acks[-1]["reason"] + + @pytest.mark.asyncio + async def test_flow_halt_still_wins_over_completion_proof(self, rig) -> None: + # A real Flow halt keeps its own structural halt event and reason + # path; the completion-proof reason is not attached to it. + svc, cloud, flow, config, db, _events = rig + login() + _bundle, digest = make_bundle(config) + flow.report = {**TRAPPED_REPORT, "halt": TRAPPED_HALT} + cloud.jobs.append(make_job(digest)) + + await run_loop(svc, ticks=1) + + assert sum(1 for e in cloud.evidence if e["kind"] == "halt") == 1 + assert cloud.acks[-1]["outcome"] == "halted-needs-attention" + assert "reason" not in cloud.acks[-1] + assert db.count_open_halts() == 1 + assert db.get_halt("halt-run_1")["reason"] != COMPLETION_PROOF_REQUIRED_REASON + + @pytest.mark.asyncio + async def test_nonzero_exit_stays_failed(self, rig) -> None: + # The fail-closed boundary narrows success only; a failed process is + # still ``failed`` and carries no completion-proof reason. + svc, cloud, flow, config, db, _events = rig + login() + _bundle, digest = make_bundle(config) + flow.ok = False + cloud.jobs.append(make_job(digest)) + + await run_loop(svc, ticks=1) + + assert cloud.acks[-1]["outcome"] == "failed" + assert "reason" not in cloud.acks[-1] + assert svc.journal.get("run_1")["outcome"] == "failed" + assert db.count_open_halts() == 0 + + # ------------------------------------------------------------------ refusal @@ -603,7 +738,10 @@ async def test_admin_toggle_changes_what_the_run_executes(self, rig) -> None: assert len(flow.calls) == 2 assert flow.runtimes[1]["pixel_verify_enabled"] is True - assert cloud.acks[-1]["outcome"] == "confirmed" + # the run executed under the new policy; without a signed VERIFIED + # proof its terminal outcome is still fail-closed + assert cloud.acks[-1]["outcome"] == "halted-needs-attention" + assert cloud.acks[-1]["reason"] == COMPLETION_PROOF_REQUIRED_REASON @pytest.mark.asyncio async def test_model_call_prohibition_overrides_a_permissive_config( From c6a0e79b44ec33f873d95251879d7e0b0e8062a1 Mon Sep 17 00:00:00 2001 From: Richard Abrich Date: Wed, 19 Aug 2026 17:43:18 -0400 Subject: [PATCH 6/6] docs: repair the truncated governed-push paragraph in the README The governed-push section ended a sentence with a dangling article ("now refuses every upload. The") immediately before the next sentence. Close the sentence and start the release-gating statement as its own paragraph. No behaviour change. Co-Authored-By: Claude Opus 5 --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ab9b83f..74538bd 100644 --- a/README.md +++ b/README.md @@ -303,7 +303,8 @@ the runtime-attestation binding, and the exact trusted dashboard path. An unknown child or delivery outcome requires reconciliation and never becomes an automatic retry. The command never falls back to a direct Desktop upload when Flow is missing or returns an error. The former direct hosted-ingest backend -now refuses every upload. The +now refuses every upload. + This path does not enter a native release until the exact pinned Flow artifact and the managed Cloud runtime pass the same live acceptance contract. The legacy customer-owned adapter queue remains paused for this release; its exit