From ff5788d2a1188153177990ecd41b9e010902f90d Mon Sep 17 00:00:00 2001 From: Igor <62580819+igor-ctrl@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:29:14 -0500 Subject: [PATCH] refactor: one SDK write operation for every write path Every mutation now goes through AsyncBCClient.plan() (resolve once) and AsyncBCClient.write() (send, audit, return status + correlation id): CLI post/patch/delete/action, batch steps and rollback, SafeContext, MCP record writes, and plain SDK client.post(). - Transport returns BCResponse(body, status, correlation_id); the BC correlation id is read from `request-id` (BC never sent the x-ms-correlation-request-id header we looked for, so it was always empty). - Public client.resolve() replaces private _resolve_url everywhere; `get --company all` no longer lets the registry override --publisher and pages with --all. - client.pages(entity, query, company_id=) keeps top/skip/count for `get --all` and saved queries. - Batch writes and rollback deletes are audited; ledger rows carry the real correlation id; steps accept publisher/group/version. - MCP bcli_post/patch/delete run in-process via the SDK; remaining subprocess tools run off the event loop. - The dlt source runs on bcli's transport and honours disable_standard_api. - Tests drive the real client against a shared FakeTransport instead of AsyncMock clients. Verified against the SBEnvQD sandbox (reads, dry-runs, CLI/batch/MCP writes, rollback, audit and envelopes). Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- CHANGELOG.md | 43 +++ docs/configuration.md | 8 +- docs/mcp-server.md | 34 ++- src/bcli/audit/_protocol.py | 2 +- src/bcli/client/__init__.py | 3 +- src/bcli/client/_async.py | 267 +++++++++++++--- src/bcli/client/_safety.py | 44 +-- src/bcli/client/_sync.py | 14 + src/bcli/client/_transport.py | 41 ++- src/bcli/client/_write.py | 65 ++++ src/bcli/context/_protocol.py | 2 +- src/bcli/etl/_bridge.py | 52 ++-- src/bcli/etl/_client.py | 170 +++-------- src/bcli/etl/_generic.py | 6 +- src/bcli_cli/_audit_wrap.py | 12 +- src/bcli_cli/_dry_run.py | 37 ++- src/bcli_cli/_state.py | 1 + src/bcli_cli/_url_resolve.py | 2 +- src/bcli_cli/_write_cmd.py | 115 +++++++ src/bcli_cli/commands/action_cmd.py | 115 ++----- src/bcli_cli/commands/batch_cmd.py | 289 +++++++----------- src/bcli_cli/commands/delete_cmd.py | 75 +---- src/bcli_cli/commands/get_cmd.py | 61 +--- src/bcli_cli/commands/patch_cmd.py | 80 +---- src/bcli_cli/commands/post_cmd.py | 80 +---- src/bcli_cli/commands/query_cmd.py | 12 +- src/bcli_mcp/_runner.py | 4 +- src/bcli_mcp/_server.py | 116 ++++++- tests/conftest.py | 89 ++++++ .../test_batch_cmd_ledger.py | 209 +++++++------ .../test_batch_write_path.py | 87 ++++++ .../test_idempotency_replay.py | 73 ++--- tests/test_cli/test_action_cmd.py | 108 +++---- tests/test_cli/test_audit_wrap.py | 74 ++--- tests/test_cli/test_batch_safety.py | 157 ++++------ tests/test_cli/test_dry_run.py | 113 +++---- tests/test_cli/test_get_paging.py | 97 ++++++ tests/test_cli/test_patch_cmd.py | 47 ++- tests/test_cli/test_post_cmd.py | 47 +-- tests/test_cli/test_state.py | 6 +- tests/test_client/test_resolve_url.py | 10 +- tests/test_client/test_safety.py | 173 ++++------- tests/test_client/test_transport.py | 11 + tests/test_client/test_write_operation.py | 140 +++++++++ tests/test_envelope/conftest.py | 2 +- .../test_batch_envelope_with_ledger.py | 83 ++--- .../test_envelope_other_verbs.py | 47 ++- .../test_envelope_policy_violation.py | 15 +- tests/test_envelope/test_envelope_post.py | 86 +++--- tests/test_etl/test_bridge.py | 69 +++-- tests/test_etl/test_generic.py | 144 +++++---- tests/test_idempotency/test_cli_flags.py | 136 ++------- .../test_idempotency/test_idempotency_key.py | 95 ++---- tests/test_mcp/test_server_tools.py | 217 +++++++++---- tests/test_progress/test_progress_events.py | 2 +- tests/test_url/test_bound_action_resolve.py | 24 +- tests/test_url/test_origin_allowlist.py | 34 +-- tests/test_workflow/test_batch_integration.py | 80 +++-- tests/test_workflow/test_method_alias.py | 27 +- .../test_rollback_url_bound_action.py | 34 +-- 61 files changed, 2342 insertions(+), 1946 deletions(-) create mode 100644 src/bcli/client/_write.py create mode 100644 src/bcli_cli/_write_cmd.py create mode 100644 tests/test_batch_ledger/test_batch_write_path.py create mode 100644 tests/test_cli/test_get_paging.py create mode 100644 tests/test_client/test_write_operation.py diff --git a/AGENTS.md b/AGENTS.md index b76d018..423ac05 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -311,7 +311,7 @@ When the user has `[audit] enabled = true` in `~/.config/bcli/config.toml`, every write you trigger appends a JSON line to `~/.config/bcli/audit/.jsonl` (or whatever the user configured). Each entry includes `outcome` (`completed` / `failed` / `dry_run`), -`correlation_id` (BC's `x-ms-correlation-request-id`), `latency_ms`, and the +`correlation_id` (BC's `request-id` header, the CorrelationId BC quotes in errors), `latency_ms`, and the redacted request body. Useful for: * Showing the user "here's what just happened" after a multi-step task. diff --git a/CHANGELOG.md b/CHANGELOG.md index 35f1e3e..6b5867a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,49 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- One write operation in the SDK: `client.plan(...)` resolves a mutation once + (`WritePlan`), `client.write(plan)` sends it and returns a `WriteResult` with + the body, HTTP status and BC correlation id. `post` / `patch` / `delete` / + `action`, `batch run` steps, batch rollback, `SafeContext`, MCP record writes + and plain `client.post(...)` all go through it. +- Public `client.resolve(...)` (replaces the private `_resolve_url`), + `client.pages(entity, query, company_id=...)`, `client.auth`, and + `plan` / `write` / `resolve` / `count()` on the sync `BCClient`. +- Batch steps accept `publisher` / `group` / `version`; batch dry-run shows each + write step's resolved URL. + +### Changed + +- Audit entries are emitted by the SDK. Profile-based SDK clients now audit + when `[audit] enabled = true` (previously CLI only); `batch run` writes and + rollback deletes are now audited; `caller` is `cli` / `mcp` / `sdk`. +- `--dry-run` on `post` / `patch` / `delete` / `action` now fails (exit 1) for + an endpoint the real run cannot route, instead of previewing a null URL. +- MCP `bcli_post` / `bcli_patch` / `bcli_delete` run in-process via the SDK and + return the envelope plus `body`; other subprocess-backed tools no longer + block the server's event loop. +- The dlt source uses bcli's transport and URL validation, and + `bcli_profile` / `load_entities_from_bcli_registry` honour the profile's + `disable_standard_api` and allow-lists. An unknown profile now raises + `ConfigError` instead of returning no entities. +- A batch patch/delete step without `id` reports `missing 'id' field`. + +### Fixed + +- The BC correlation id was read from `x-ms-correlation-request-id`, an Azure + Resource Manager header BC never sends, so it was always empty — on errors + too. It is now read from BC's `request-id` header, the value BC's own error + messages quote as `CorrelationId` (verified against SBEnvQD). +- Success audit entries, result envelopes and batch ledger rows recorded + `status=200, correlation_id=None` regardless of the response; they now carry + the real status and BC correlation id. +- `get --company all` let a registry route override an explicit `--publisher` + and read only the first page even with `--all`. +- `get --all` and saved queries with `all_pages` dropped `--top`, `--skip` and + `--count`. + ## [0.8.3] - 2026-10-30 ### Fixed diff --git a/docs/configuration.md b/docs/configuration.md index 1baa77b..921ad64 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -186,9 +186,11 @@ Each entry is one JSON object with the following keys: - `dry_run` — the user passed `--dry-run`; no HTTP call fired but the intent is recorded. -The SDK (`AsyncBCClient`) does NOT auto-emit. Audit is a CLI-layer ergonomic; -programmatic SDK users get unfiltered access by design and can wire their own -logging. +Entries are emitted by the SDK's write operation (`client.write`), so every +write path is covered: CLI verbs, `batch run` and rollback, MCP record writes, +`SafeContext`, and profile-based SDK clients. `caller` says which (`cli`, +`mcp`, `sdk`). A programmatic client (`AsyncBCClient(tenant_id=...)`) has no +config file and so no audit log. ## File Locations diff --git a/docs/mcp-server.md b/docs/mcp-server.md index b307a17..35ecf61 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -8,16 +8,21 @@ Desktop, but it also works with the official [MCP Inspector](https://github.com/ and any other client that speaks the spec. The server generates its tool list **dynamically** by subprocessing -`bcli describe --format json` once on startup, and delegates every call to the -bcli CLI as a subprocess. Profile resolution, auth, retry, telemetry, and the -read-only `disable_writes` gate are inherited from the CLI for free. New CLI -commands light up automatically as MCP tools; deprecated ones disappear. - -Read commands return parsed stdout JSON. Mutating commands (`bcli_post`, -`bcli_patch`, `bcli_delete`, `bcli_attach_upload`, `bcli_batch_run`) pass -`--result-out ` and return the AIP §Phase 2 result envelope content as -the tool result. A `status="failed"` envelope surfaces as an MCP `ToolError` -with the BC correlation id quoted so the agent can cite it. +`bcli describe --format json` once on startup. New CLI commands light up +automatically as MCP tools; deprecated ones disappear. + +Record writes (`bcli_post`, `bcli_patch`, `bcli_delete`) run in-process +through the SDK's write operation (`client.plan` + `client.write`): the same +URL resolution, audit (`caller = "mcp"`) and BC correlation id as the CLI. +They return the AIP §Phase 2 result envelope fields plus the response `body`. +A `disable_writes` profile is refused unless the agent passes `yes=true`; any +failure raises an MCP `ToolError` quoting the correlation id. + +Read commands, `bcli_attach_upload` and `bcli_batch_run` still delegate to the +bcli CLI as a subprocess (run off the event loop): reads return parsed stdout +JSON; the two mutating ones pass `--result-out ` and return the envelope, +and a `status="failed"` envelope surfaces as a `ToolError` with the +correlation id quoted. ## Install @@ -203,7 +208,8 @@ Two separate MCP processes, two `mcpServers` blocks in crash the public MCP, and an OSS install never needs to skip optional private-package imports. -If exposing mutating tools from a private MCP, the package must either -subprocess `bcli post / patch / delete` (so the CLI's `disable_writes` gate -applies) or reimplement that gate locally — the SDK alone does not enforce -it. See `src/bcli_cli/_safety.py` for the canonical helper. +If exposing mutating tools from a private MCP, use the SDK's `client.plan` + +`client.write` (resolution and audit come with it) and apply the +`disable_writes` rule yourself, as `bcli_mcp._server._make_sdk_write_handler` +does — `write()` does not enforce it. The CLI's interactive version is +`src/bcli_cli/_safety.py`. diff --git a/src/bcli/audit/_protocol.py b/src/bcli/audit/_protocol.py index 2404223..ec00504 100644 --- a/src/bcli/audit/_protocol.py +++ b/src/bcli/audit/_protocol.py @@ -38,7 +38,7 @@ class AuditEntry: * ``request_body`` — redacted body that was sent; ``None`` for DELETE. * ``status`` — HTTP status code; ``None`` if the call never fired (dry-run, pre-call failure). - * ``correlation_id`` — BC ``x-ms-correlation-request-id`` header; useful + * ``correlation_id`` — BC ``request-id`` header (BC's CorrelationId); useful when grepping BC server-side logs. * ``latency_ms`` — round-trip latency; ``None`` if no call fired. * ``cli_version`` — bcli version string. diff --git a/src/bcli/client/__init__.py b/src/bcli/client/__init__.py index 590c1f2..e76dcb0 100644 --- a/src/bcli/client/__init__.py +++ b/src/bcli/client/__init__.py @@ -2,5 +2,6 @@ from bcli.client._async import AsyncBCClient from bcli.client._sync import BCClient +from bcli.client._write import WritePlan, WriteResult -__all__ = ["AsyncBCClient", "BCClient"] +__all__ = ["AsyncBCClient", "BCClient", "WritePlan", "WriteResult"] diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index a53050f..3793c1a 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -3,6 +3,8 @@ from __future__ import annotations import re +import time +from datetime import datetime, timezone from pathlib import Path from typing import Any from urllib.parse import quote @@ -12,6 +14,7 @@ from bcli.auth._credentials import ClientCredentialsAuth from bcli.client._safety import SafeContext from bcli.client._transport import BCTransport +from bcli.client._write import WRITE_METHODS, WritePlan, WriteResult from bcli.config import BCConfig, BCProfile, load_config from bcli.errors import BCLIError, ConfigError from bcli.odata._pagination import PageIterator @@ -117,7 +120,9 @@ def __init__( # Shared options timeout: int | None = None, auth: AuthProvider | None = None, + caller: str = "sdk", ) -> None: + self._caller = caller if tenant_id is not None: # Programmatic mode — build a synthetic profile self._config = BCConfig() @@ -128,14 +133,16 @@ def __init__( client_id=client_id, ) self._programmatic_secret = client_secret + self._profile_name = "programmatic" self._registry = EndpointRegistry() else: # Profile-based mode — read from config self._config = config or load_config() self._profile = self._config.get_profile(profile) self._programmatic_secret = None + self._profile_name = profile or self._config.defaults.profile self._registry = EndpointRegistry( - profile_name=profile or self._config.defaults.profile, + profile_name=self._profile_name, disable_standard=self._profile.disable_standard_api, allowed_categories=self._profile.allowed_categories or None, allowed_endpoints=self._profile.allowed_endpoints or None, @@ -144,17 +151,24 @@ def __init__( self._transport: BCTransport | None = None self._timeout = timeout or self._config.defaults.timeout self._injected_auth = auth + self._audit_sink = None - def _ensure_transport(self) -> BCTransport: - if self._transport is None: + @property + def auth(self) -> AuthProvider: + """The provider this client authenticates with (built once, on first use).""" + if self._injected_auth is None: # An injected provider wins outright: the caller already holds a token, # so the profile's auth_method is never consulted. That is what lets a # `browser` profile work server-side, where there is no local browser # and no loopback listener to bind. - auth = self._injected_auth or self._build_auth( + self._injected_auth = self._build_auth( self._profile, self._programmatic_secret, self._config ) - self._transport = BCTransport(auth, timeout=self._timeout) + return self._injected_auth + + def _ensure_transport(self) -> BCTransport: + if self._transport is None: + self._transport = BCTransport(self.auth, timeout=self._timeout) return self._transport @staticmethod @@ -224,7 +238,7 @@ async def get( """Execute a GET request.""" transport = self._ensure_transport() - url = self._resolve_url( + url = self.resolve( entity_set_name, record_id=record_id, publisher=publisher, @@ -236,6 +250,29 @@ async def get( data = await transport.get(url, params=params) return ODataResponse(data) + def pages( + self, + entity_set_name: str, + query: Query | None = None, + *, + publisher: str | None = None, + group: str | None = None, + version: str | None = None, + company_id: str | None = None, + ) -> PageIterator: + """Iterate every page of ``entity_set_name``, following ``@odata.nextLink``. + + ``query`` is sent whole on the first request (filter, select, expand, + orderby, top, skip, count). ``company_id`` queries another company in + the same environment. + """ + url = self.resolve( + entity_set_name, publisher=publisher, group=group, version=version, + company_id=company_id, + ) + params = query.to_params() if query else {} + return PageIterator(self._ensure_transport(), url, params) + async def post( self, entity_set_name: str, @@ -253,11 +290,12 @@ async def post( it lets reverse proxies / gateways apply replay protection and keeps the ledger key consistent with what was wired out. """ - transport = self._ensure_transport() - url = self._resolve_url(entity_set_name, publisher=publisher, group=group, version=version) - return await transport.post( - url, json_body=body, idempotency_key=idempotency_key, + plan = self.plan( + "POST", entity_set_name, body=body, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, ) + return (await self.write(plan)).body async def patch( self, @@ -272,14 +310,12 @@ async def patch( idempotency_key: str | None = None, ) -> dict[str, Any]: """PATCH (update) a record.""" - transport = self._ensure_transport() - url = self._resolve_url( - entity_set_name, record_id=record_id, + plan = self.plan( + "PATCH", entity_set_name, body=body, record_id=record_id, etag=etag, publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, ) - return await transport.patch( - url, json_body=body, etag=etag, idempotency_key=idempotency_key, - ) + return (await self.write(plan)).body async def delete( self, @@ -293,13 +329,140 @@ async def delete( idempotency_key: str | None = None, ) -> dict[str, Any]: """DELETE a record.""" - transport = self._ensure_transport() - url = self._resolve_url( + plan = self.plan( + "DELETE", entity_set_name, record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + ) + return (await self.write(plan)).body + + # ─── Write operation ───────────────────────────────────────── + + def plan( + self, + method: str, + entity_set_name: str, + *, + body: dict[str, Any] | None = None, + record_id: str | None = None, + etag: str = "*", + idempotency_key: str | None = None, + publisher: str | None = None, + group: str | None = None, + version: str | None = None, + environment: str | None = None, + company_id: str | None = None, + ) -> WritePlan: + """Resolve a mutation once, without sending it. + + Raises the same errors the real write would (``ValueError`` for an + invalid key or route segment, ``RegistryError`` / ``ConfigError`` for + an endpoint that cannot be routed), so a dry-run built from the plan + fails exactly where the real run fails. ``environment`` / ``company_id`` + retarget the write away from the profile's defaults. + """ + method = method.upper() + if method not in WRITE_METHODS: + raise ValueError(f"Unsupported write method {method!r}; expected one of {sorted(WRITE_METHODS)}") + environment = environment or self._profile.environment + company_id = company_id or self._profile.company_id + url = self.resolve( entity_set_name, record_id=record_id, publisher=publisher, group=group, version=version, + environment=environment, company_id=company_id, ) - return await transport.delete( - url, etag=etag, idempotency_key=idempotency_key, + return WritePlan( + method=method, + endpoint=entity_set_name, + url=url, + environment=environment, + company_id=company_id, + record_id=record_id, + body=None if method == "DELETE" else body, + etag=etag, + idempotency_key=idempotency_key, + ) + + async def write(self, plan: WritePlan) -> WriteResult: + """Send a planned mutation and record the outcome in the audit log.""" + transport = self._ensure_transport() + start = time.perf_counter() + try: + response = await transport.request( + plan.method, + plan.url, + json_body=plan.body, + etag=None if plan.method == "POST" else plan.etag, + idempotency_key=plan.idempotency_key, + ) + except Exception as exc: + self._audit( + plan, "failed", + status=getattr(exc, "status_code", None), + correlation_id=getattr(exc, "correlation_id", None), + latency_ms=_elapsed_ms(start), + error=str(exc), + ) + raise + result = WriteResult( + plan=plan, + body=response.body, + status=response.status, + correlation_id=response.correlation_id, + latency_ms=_elapsed_ms(start), + ) + self._audit( + plan, "completed", + status=result.status, + correlation_id=result.correlation_id, + latency_ms=result.latency_ms, + ) + return result + + def record_dry_run(self, plan: WritePlan, *, method: str | None = None) -> None: + """Record that ``plan`` was previewed but not sent.""" + self._audit(plan, "dry_run", method=method) + + def _audit( + self, + plan: WritePlan, + outcome: str, + *, + method: str | None = None, + status: int | None = None, + correlation_id: str | None = None, + latency_ms: int | None = None, + error: str | None = None, + ) -> None: + from bcli._version import __version__ + from bcli.audit import AuditEntry, get_audit_sink, redact + + if self._audit_sink is None: + self._audit_sink = get_audit_sink(self._config.audit, profile_name=self._profile_name) + if not self._audit_sink.is_active: + return + self._audit_sink.emit( + AuditEntry( + ts=datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), + profile=self._profile_name, + environment=plan.environment, + company_id=plan.company_id, + method=(method or plan.method).upper(), + endpoint=plan.endpoint, + resolved_url=plan.url, + record_id=plan.record_id, + request_body=( + None if plan.body is None + else redact(plan.body, self._config.audit.redact_keys) + ), + status=status, + correlation_id=correlation_id, + latency_ms=latency_ms, + cli_version=__version__, + caller=self._caller, + outcome=outcome, + error=error, + ) ) async def delete_url(self, url: str, *, etag: str = "*") -> dict[str, Any]: @@ -311,8 +474,15 @@ async def delete_url(self, url: str, *, etag: str = "*") -> dict[str, Any]: endpoint may have moved between runs); we trust the captured URL. """ - transport = self._ensure_transport() - return await transport.delete(url, etag=etag) + plan = WritePlan( + method="DELETE", + endpoint=url.rsplit("/", 1)[-1], + url=url, + environment=self._profile.environment, + company_id=self._profile.company_id, + etag=etag, + ) + return (await self.write(plan)).body async def upload_attachment( self, @@ -397,10 +567,14 @@ async def upload_attachment( idempotency_key=idempotency_key, ) else: - metadata = await self.post( - "documentAttachments", - metadata_body, - publisher=publisher, group=group, version=version, + # Straight to the transport, not self.post: the caller audits the + # upload as one UPLOAD, not as an extra POST. + metadata = await transport.post( + self.resolve( + "documentAttachments", + publisher=publisher, group=group, version=version, + ), + json_body=metadata_body, idempotency_key=idempotency_key, ) attachment_id = metadata.get("id") or metadata.get("systemId") @@ -419,7 +593,7 @@ async def upload_attachment( record_id=attachment_id, ) else: - base_url = self._resolve_url( + base_url = self.resolve( "documentAttachments", record_id=attachment_id, publisher=publisher, group=group, version=version, @@ -481,7 +655,7 @@ async def get_media( straight to a file. Two requests. First the record itself, resolved through the same - ``_resolve_url`` every other read uses — so registry routing, an + ``resolve`` every other read uses — so registry routing, an explicit ``publisher``/``group``/``version`` override and the ``disable_standard_api`` lockdown all apply here exactly as they do to ``get``. A media download is not a side door around the profile's @@ -507,7 +681,7 @@ async def get_media( """ transport = self._ensure_transport() - record_url = self._resolve_url( + record_url = self.resolve( entity_set_name, record_id=record_id, publisher=publisher, @@ -633,8 +807,8 @@ def _resolve_url_for_target( ) -> str: """Resolve entity to a full URL bound to an explicit target. - Used by ``SafeContext`` so writes go to the environment + company the - caller passed to ``client.safe_write(...)``, not the client's + Called through :meth:`resolve`; ``SafeContext`` passes its explicit + environment + company so writes never fall back to the client's profile-bound target. See vuln-0004. Registry lookup, custom-route binding, ``disable_standard_api`` @@ -774,7 +948,7 @@ def _resolve_url_for_target( record_id=record_id, ) - def _resolve_url( + def resolve( self, entity_set_name: str, *, @@ -782,11 +956,20 @@ def _resolve_url( publisher: str | None = None, group: str | None = None, version: str | None = None, + environment: str | None = None, + company_id: str | None = None, ) -> str: - """Resolve entity to full URL using the client's profile-bound target.""" + """Resolve an entity (or bound action) to its full URL. + + The one place that decides which route an entity uses: explicit + ``publisher/group/version`` override, then the custom registry, then + standard v2.0 unless ``disable_standard_api`` forbids it. Targets the + profile's environment and company unless ``environment`` / + ``company_id`` say otherwise (e.g. ``get --company all``). + """ return self._resolve_url_for_target( - self._profile.environment, - self._profile.company_id or "", + environment or self._profile.environment, + company_id or self._profile.company_id or "", entity_set_name, record_id=record_id, publisher=publisher, @@ -803,6 +986,10 @@ def profile(self) -> BCProfile: return self._profile +def _elapsed_ms(start: float) -> int: + return int((time.perf_counter() - start) * 1000) + + class BoundQuery: """A query bound to a specific client and entity, supporting fluent chaining.""" @@ -866,11 +1053,7 @@ async def get(self) -> list[dict]: async def pages(self) -> PageIterator: """Return a page iterator for streaming large result sets.""" - url = self._client._resolve_url( - self._entity, - publisher=self._publisher, - group=self._group, - version=self._version, + return self._client.pages( + self._entity, self._query, + publisher=self._publisher, group=self._group, version=self._version, ) - transport = self._client._ensure_transport() - return PageIterator(transport, url, self._query.to_params()) diff --git a/src/bcli/client/_safety.py b/src/bcli/client/_safety.py index ab7f33c..2cd3f04 100644 --- a/src/bcli/client/_safety.py +++ b/src/bcli/client/_safety.py @@ -124,16 +124,12 @@ async def post( f"Writes are not allowed for domain '{domain}'." ) safe_body = self._apply_draft_if_needed(body, domain) - transport = self._client._ensure_transport() - url = self._client._resolve_url_for_target( - self._environment, - self._company_id, - entity_set_name, - publisher=publisher, - group=group, - version=version, + plan = self._client.plan( + "POST", entity_set_name, body=safe_body, + publisher=publisher, group=group, version=version, + environment=self._environment, company_id=self._company_id, ) - return await transport.post(url, json_body=safe_body) + return (await self._client.write(plan)).body async def patch( self, @@ -157,17 +153,12 @@ async def patch( raise SafetyError( f"Writes are not allowed for domain '{domain}'." ) - transport = self._client._ensure_transport() - url = self._client._resolve_url_for_target( - self._environment, - self._company_id, - entity_set_name, - record_id=record_id, - publisher=publisher, - group=group, - version=version, + plan = self._client.plan( + "PATCH", entity_set_name, body=body, record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + environment=self._environment, company_id=self._company_id, ) - return await transport.patch(url, json_body=body, etag=etag) + return (await self._client.write(plan)).body async def delete( self, @@ -190,17 +181,12 @@ async def delete( raise SafetyError( f"Writes are not allowed for domain '{domain}'." ) - transport = self._client._ensure_transport() - url = self._client._resolve_url_for_target( - self._environment, - self._company_id, - entity_set_name, - record_id=record_id, - publisher=publisher, - group=group, - version=version, + plan = self._client.plan( + "DELETE", entity_set_name, record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + environment=self._environment, company_id=self._company_id, ) - return await transport.delete(url, etag=etag) + return (await self._client.write(plan)).body @property def environment(self) -> str: diff --git a/src/bcli/client/_sync.py b/src/bcli/client/_sync.py index 44a0b97..a189d78 100644 --- a/src/bcli/client/_sync.py +++ b/src/bcli/client/_sync.py @@ -7,6 +7,7 @@ from typing import Any from bcli.client._async import AsyncBCClient +from bcli.client._write import WritePlan, WriteResult from bcli.config import BCConfig from bcli.odata._query import Query from bcli.odata._response import ODataResponse @@ -81,6 +82,15 @@ def patch( def delete(self, entity_set_name: str, record_id: str, **kwargs) -> dict[str, Any]: return self._run(self._async.delete(entity_set_name, record_id, **kwargs)) + def resolve(self, entity_set_name: str, **kwargs) -> str: + return self._async.resolve(entity_set_name, **kwargs) + + def plan(self, method: str, entity_set_name: str, **kwargs) -> WritePlan: + return self._async.plan(method, entity_set_name, **kwargs) + + def write(self, plan: WritePlan) -> WriteResult: + return self._run(self._async.write(plan)) + def upload_attachment( self, parent_type: str, @@ -156,6 +166,10 @@ def skip(self, n: int) -> SyncBoundQuery: self._query.skip(n) return self + def count(self, enabled: bool = True) -> SyncBoundQuery: + self._query.count(enabled) + return self + def route(self, publisher: str, group: str, version: str) -> SyncBoundQuery: self._publisher = publisher self._group = group diff --git a/src/bcli/client/_transport.py b/src/bcli/client/_transport.py index 0d390e3..27663fe 100644 --- a/src/bcli/client/_transport.py +++ b/src/bcli/client/_transport.py @@ -8,6 +8,7 @@ import re import tempfile import time +from dataclasses import dataclass from pathlib import Path from typing import Any @@ -95,6 +96,15 @@ def _hint_for_bc_error(status: int, bc_message: str | None, url: str) -> str | N return None +@dataclass(frozen=True) +class BCResponse: + """A successful response: the parsed body plus what BC said about the call.""" + + body: dict[str, Any] + status: int + correlation_id: str | None + + class BCTransport: """HTTP transport with auth injection, retry, and BC error parsing.""" @@ -124,7 +134,11 @@ async def _inject_auth(self) -> dict[str, str]: token = await self._auth.get_access_token() return {"Authorization": f"Bearer {token}"} - async def _request( + async def _request(self, method: str, url: str, **kwargs: Any) -> dict[str, Any]: + """Execute a request and return only the parsed body.""" + return (await self.request(method, url, **kwargs)).body + + async def request( self, method: str, url: str, @@ -136,7 +150,7 @@ async def _request( etag: str | None = None, idempotency_key: str | None = None, log_context: dict[str, str] | None = None, - ) -> dict[str, Any]: + ) -> BCResponse: """Execute an HTTP request with retry and error handling. Request body handling: @@ -148,7 +162,7 @@ async def _request( - Passing both is an error — the caller should pick one. """ if json_body is not None and content is not None: - raise ValueError("BCTransport._request: pass either json_body or content, not both") + raise ValueError("BCTransport.request: pass either json_body or content, not both") last_error: Exception | None = None backoff = INITIAL_BACKOFF @@ -204,16 +218,18 @@ async def _request( headers=headers, ) - correlation_id = response.headers.get("x-ms-correlation-request-id") + correlation_id = _correlation_id(response) if response.is_success: self._emit_request_log( method, url, response.status_code, attempt, time.monotonic() - t0, correlation_id, log_context, ) - if response.status_code == 204 or not response.content: - return {} - return response.json() + body = ( + {} if response.status_code == 204 or not response.content + else response.json() + ) + return BCResponse(body, response.status_code, correlation_id) # Parse BC error response bc_message, correlation_id = _parse_bc_error(response) @@ -567,10 +583,19 @@ async def delete( ) +def _correlation_id(response: httpx.Response) -> str | None: + """BC's correlation id: the ``request-id`` header, the value BC's own error + messages quote as ``CorrelationId``. ``x-ms-correlation-request-id`` is an + Azure Resource Manager header BC does not send; kept as a fallback.""" + return response.headers.get("request-id") or response.headers.get( + "x-ms-correlation-request-id" + ) + + def _parse_bc_error(response: httpx.Response) -> tuple[str | None, str | None]: """Extract error message and correlation ID from a BC error response.""" bc_message = None - correlation_id = response.headers.get("x-ms-correlation-request-id") + correlation_id = _correlation_id(response) try: body = response.json() diff --git a/src/bcli/client/_write.py b/src/bcli/client/_write.py new file mode 100644 index 0000000..e496f2c --- /dev/null +++ b/src/bcli/client/_write.py @@ -0,0 +1,65 @@ +"""Write operations: plan once, then execute. + +Every mutation — ``bcli post/patch/delete/action``, a ``bcli batch run`` step, +an MCP tool call, ``SafeContext``, or plain SDK ``client.post(...)`` — goes +through the same two steps on :class:`~bcli.client.AsyncBCClient`: + +1. ``client.plan(...)`` resolves the URL exactly once and validates the input. + The plan is what a dry-run previews and what the real run sends, so the + two cannot disagree. +2. ``client.write(plan)`` sends it and records the outcome in the profile's + audit log, returning the body together with the HTTP status and BC's + correlation id. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +#: Methods ``plan()`` accepts. Bound actions are POSTs to a composed URL. +WRITE_METHODS = frozenset({"POST", "PATCH", "DELETE"}) + + +@dataclass(frozen=True) +class WritePlan: + """A fully-resolved mutation, ready to preview or send.""" + + method: str + endpoint: str + url: str + environment: str + company_id: str | None + record_id: str | None = None + body: dict[str, Any] | None = None + etag: str = "*" + idempotency_key: str | None = None + + def preview(self) -> dict[str, Any]: + """The stable ``--dry-run`` payload for this plan.""" + payload: dict[str, Any] = { + "dry_run": True, + "method": self.method, + "endpoint": self.endpoint, + "resolved_url": self.url, + "environment": self.environment, + "company_id": self.company_id, + } + if self.record_id is not None: + payload["record_id"] = self.record_id + if self.body is not None: + payload["body"] = self.body + if self.method != "POST": + payload["etag"] = self.etag + return payload + + +@dataclass(frozen=True) +class WriteResult: + """What came back from sending a :class:`WritePlan`.""" + + plan: WritePlan + body: dict[str, Any] + status: int + correlation_id: str | None + latency_ms: int diff --git a/src/bcli/context/_protocol.py b/src/bcli/context/_protocol.py index fa7a70f..2d3293c 100644 --- a/src/bcli/context/_protocol.py +++ b/src/bcli/context/_protocol.py @@ -130,7 +130,7 @@ class HttpEvent: ``url`` arrives here already URL-scrubbed (query params replaced with ``?key=[REDACTED]``). ``correlation_id`` is the BC - ``x-ms-correlation-request-id`` header, useful for cross-checking + ``request-id`` header (BC's CorrelationId), useful for cross-checking against an Azure Application Insights record. """ diff --git a/src/bcli/etl/_bridge.py b/src/bcli/etl/_bridge.py index 6aae958..0a3709d 100644 --- a/src/bcli/etl/_bridge.py +++ b/src/bcli/etl/_bridge.py @@ -1,15 +1,15 @@ """bcli-aware adapter for the generic ETL source. -This is the ONLY module in bcli.etl that may import from bcli.*. -It translates bcli-specific concepts (profile, registry, token cache) into -the generic source's abstractions (AuthProvider, EntityDef list). +Translates bcli-specific concepts (profile, registry, auth provider) into +the generic source's abstractions (AuthProvider, EntityDef list). The +generic source itself never sees a profile; its HTTP goes through bcli's +transport (see ``bcli.etl._client``). """ from __future__ import annotations from typing import Any -from bcli.etl._auth import StaticTokenAuth from bcli.etl._generic import EntityDef, business_central as _generic_business_central from bcli.etl._stamper_factory import build_stampers from bcli.etl._stampers import Stamper @@ -20,13 +20,16 @@ def load_entities_from_bcli_registry( ) -> list[EntityDef]: """Translate bcli's EndpointRegistry into a list of EntityDef. - Only entities supporting GET are included. + Only entities supporting GET are included. The registry is the profile's + own, so ``disable_standard_api`` and the allowed category/endpoint lists + apply exactly as they do to ``bcli get``. """ - from bcli.registry._registry import EndpointRegistry + from bcli import AsyncBCClient - registry = EndpointRegistry(profile_name=profile) - endpoints = registry.list_all(custom_only=custom_only) + return _entities(AsyncBCClient(profile=profile).registry, custom_only) + +def _entities(registry: Any, custom_only: bool) -> list[EntityDef]: return [ EntityDef( name=ep.entity_set_name, @@ -35,25 +38,11 @@ def load_entities_from_bcli_registry( api_group=ep.api_group, api_version=ep.api_version, ) - for ep in endpoints + for ep in registry.list_all(custom_only=custom_only) if "GET" in ep.supports ] -def _build_token_provider(profile: str): - """Return an async callable that yields a fresh bearer token via bcli.""" - - async def _token() -> str: - from bcli import AsyncBCClient - - async with AsyncBCClient(profile=profile) as client: - transport = client._ensure_transport() - # bcli's transport handles caching + refresh internally - return await transport._auth.get_access_token() - - return _token - - def bcli_profile( profile: str, *, @@ -87,17 +76,15 @@ def bcli_profile( Returns: A dlt source ready to pass to ``pipeline.run(...)``. """ - # Resolve environment from the profile + from bcli import AsyncBCClient from bcli.config import load_config config = load_config() - bc_profile = config.get_profile(profile) - environment = bc_profile.environment + client = AsyncBCClient(profile=profile, config=config) + environment = client.profile.environment # Translate registry → EntityDef list - all_entities = load_entities_from_bcli_registry( - profile, custom_only=not include_standard - ) + all_entities = _entities(client.registry, custom_only=not include_standard) if entities is not None: name_set = set(entities) available = {e.name for e in all_entities} @@ -115,11 +102,10 @@ def bcli_profile( if extra_stampers: resolved_stampers.extend(extra_stampers) - # Wrap bcli's auth as an AuthProvider - auth = StaticTokenAuth(_build_token_provider(profile)) - + # bcli's own provider: one instance, so its token cache and refresh + # carry across every page and company. return _generic_business_central( - auth=auth, + auth=client.auth, environment=environment, entities=all_entities, multi_company=multi_company, diff --git a/src/bcli/etl/_client.py b/src/bcli/etl/_client.py index b9ae652..19f1932 100644 --- a/src/bcli/etl/_client.py +++ b/src/bcli/etl/_client.py @@ -1,58 +1,21 @@ -"""Minimal async BC client for the generic ETL layer. +"""BC reads for the dlt source, on bcli's own transport. -Uses httpx directly with an injectable AuthProvider. Handles OData -pagination (``@odata.nextLink``), 429/503/504 retry with exponential -backoff, and structured URL building. - -This module is part of the generic layer and must not import from bcli.*. +Retry/backoff, BC error mapping, URL validation and the off-origin +``@odata.nextLink`` guard all come from :class:`bcli.client._transport.BCTransport` +and :func:`bcli._url.build_url` — the ETL layer only adapts its token-based +:class:`~bcli.etl._auth.AuthProvider` and pages through entities. """ from __future__ import annotations -import asyncio from typing import Any, AsyncIterator -from urllib.parse import urlparse - -import httpx - -from bcli.etl._auth import AuthProvider - -_BASE_URL = "https://api.businesscentral.dynamics.com/v2.0" -_STANDARD_API_PATH = "api/v2.0" -_MAX_RETRIES = 3 -_INITIAL_BACKOFF = 1.0 -_RETRY_STATUSES = (429, 503, 504) - -# Allowed host suffixes for absolute URLs that get a BC bearer token attached. -# Mirrors ``bcli._url._ALLOWED_HOST_SUFFIXES`` — kept inline because this -# module is part of the dlt-friendly "generic" layer that must avoid -# reaching into ``bcli.*`` outside of ``bcli.etl.*``. If a malicious BC -# response ever returns an off-origin ``@odata.nextLink``, the paginator -# below would otherwise leak the bearer token to that host. -_ALLOWED_HOST_SUFFIXES: tuple[str, ...] = ( - "businesscentral.dynamics.com", - "bc.dynamics.com", -) - - -def _assert_bc_origin(url: str) -> None: - """Raise ``ValueError`` if ``url`` isn't relative or a BC host.""" - parsed = urlparse(url) - if not parsed.scheme: - return # relative URL, joined to base by httpx - if parsed.scheme != "https": - # Bearer tokens never ride cleartext; BC always serves https. - raise ValueError(f"Refusing non-HTTPS URL with auth: {url!r}") - host = (parsed.hostname or "").lower() - if not host: - raise ValueError(f"Refusing URL with no host: {url!r}") - for suffix in _ALLOWED_HOST_SUFFIXES: - if host == suffix or host.endswith("." + suffix): - return - raise ValueError( - f"Refusing to attach BC credentials to off-origin URL: {url!r}. " - f"Allowed host suffixes: {list(_ALLOWED_HOST_SUFFIXES)}." - ) + +from bcli._url import build_companies_url, build_url +from bcli.client._transport import BCTransport +from bcli.errors import NotFoundError +from bcli.odata._pagination import PageIterator + +__all__ = ["BCClient", "NotFoundError", "build_entity_url"] def build_entity_url( @@ -64,25 +27,32 @@ def build_entity_url( group: str | None = None, version: str | None = None, ) -> str: - """Build the full BC URL for an entity.""" - if publisher and group and version: - api_path = f"api/{publisher}/{group}/{version}" - else: - api_path = _STANDARD_API_PATH - return f"{_BASE_URL}/{environment}/{api_path}/companies({company_id})/{entity_set_name}" + """Build the full BC URL for an entity (validated like every other bcli URL).""" + return build_url( + environment=environment, + company_id=company_id, + entity_set_name=entity_set_name, + publisher=publisher, + group=group, + version=version, + ) -def build_companies_url(environment: str) -> str: - """Build the URL to list companies in an environment.""" - return f"{_BASE_URL}/{environment}/{_STANDARD_API_PATH}/companies" +class _TokenAuth: + """Adapts an ETL ``get_token()`` provider to bcli's ``get_access_token()``.""" + def __init__(self, auth: Any) -> None: + self._auth = auth -class NotFoundError(Exception): - """Raised when BC returns 404 (entity missing in a company, etc.).""" + async def get_access_token(self) -> str: + return await self._auth.get_token() class BCClient: - """Minimal async BC client. + """Async BC reader for the dlt source. + + ``auth`` is either an ETL provider (``get_token``) or a bcli + :class:`~bcli.auth.AuthProvider` (``get_access_token``). Example: >>> async with BCClient(auth=my_auth, environment="Production") as client: @@ -90,89 +60,43 @@ class BCClient: ... process(page) """ - def __init__( - self, - *, - auth: AuthProvider, - environment: str, - timeout: float = 60.0, - ) -> None: + def __init__(self, *, auth: Any, environment: str, timeout: float = 60.0) -> None: + if not hasattr(auth, "get_access_token"): + auth = _TokenAuth(auth) self._auth = auth self._environment = environment self._timeout = timeout - self._http: httpx.AsyncClient | None = None + self._transport: BCTransport | None = None @property def environment(self) -> str: return self._environment async def __aenter__(self) -> "BCClient": - self._http = httpx.AsyncClient(timeout=self._timeout) + self._transport = BCTransport(self._auth, timeout=int(self._timeout)) return self async def __aexit__(self, *_exc: Any) -> None: - if self._http is not None: - await self._http.aclose() - self._http = None - - async def _request( - self, - method: str, - url: str, - *, - params: dict[str, str] | None = None, - ) -> dict[str, Any]: - assert self._http is not None, "BCClient must be used as a context manager" - - # Validate origin before attaching the bearer token. The paginator - # below feeds @odata.nextLink straight back through this method, so - # a malicious BC response cannot use that path to redirect auth. - _assert_bc_origin(url) - - backoff = _INITIAL_BACKOFF - for attempt in range(_MAX_RETRIES + 1): - token = await self._auth.get_token() - headers = {"Authorization": f"Bearer {token}"} - response = await self._http.request( - method, url, params=params, headers=headers - ) - - if response.status_code == 404: - raise NotFoundError(f"{method} {url} → 404") - - if response.status_code in _RETRY_STATUSES and attempt < _MAX_RETRIES: - retry_after = response.headers.get("Retry-After") - sleep_for = float(retry_after) if retry_after else backoff - await asyncio.sleep(sleep_for) - backoff *= 2 - continue - - if response.status_code >= 400: - response.raise_for_status() - - if not response.content: - return {} - return response.json() - - raise RuntimeError(f"{method} {url} failed after {_MAX_RETRIES} retries") + if self._transport is not None: + await self._transport.close() + self._transport = None + + def _t(self) -> BCTransport: + assert self._transport is not None, "BCClient must be used as a context manager" + return self._transport async def get(self, url: str, params: dict[str, str] | None = None) -> dict[str, Any]: """Single GET.""" - return await self._request("GET", url, params=params) + return await self._t().get(url, params=params) async def paginate( self, url: str, params: dict[str, str] | None = None ) -> AsyncIterator[list[dict[str, Any]]]: """Follow ``@odata.nextLink`` until exhausted. Yields one page at a time.""" - next_url: str | None = url - current_params: dict[str, str] | None = params - while next_url: - data = await self._request("GET", next_url, params=current_params) - yield data.get("value", []) - next_url = data.get("@odata.nextLink") - current_params = None # nextLink URLs are absolute + async for page in PageIterator(self._t(), url, params or {}): + yield page async def list_companies(self) -> list[dict[str, Any]]: """Return all companies in the environment.""" - data = await self.get(build_companies_url(self._environment)) + data = await self.get(build_companies_url(environment=self._environment)) return data.get("value", []) diff --git a/src/bcli/etl/_generic.py b/src/bcli/etl/_generic.py index 2172d1d..0e16aab 100644 --- a/src/bcli/etl/_generic.py +++ b/src/bcli/etl/_generic.py @@ -1,7 +1,9 @@ """Generic dlt source for Microsoft Business Central. -No bcli coupling. Works with any BC tenant given auth + entity definitions. -This module must not import from bcli.* (enforced by CI grep). +No profile or registry coupling. Works with any BC tenant given auth + entity +definitions; HTTP goes through bcli's transport. +Only bcli.etl.* imports here; bcli.etl._client is the one door to bcli's transport +(enforced by tests/test_etl/test_generic.py). """ from __future__ import annotations diff --git a/src/bcli_cli/_audit_wrap.py b/src/bcli_cli/_audit_wrap.py index 60b7df2..f9f03d3 100644 --- a/src/bcli_cli/_audit_wrap.py +++ b/src/bcli_cli/_audit_wrap.py @@ -1,8 +1,10 @@ -"""CLI-side audit-log integration. +"""Audit for ``bcli attach upload`` — the one write not on the SDK write path. -Two entry points used by the write commands: +Every other write (post / patch / delete / action, batch, MCP, SDK) is audited +inside ``AsyncBCClient.write``. An upload is two requests (metadata POST, then +binary PATCH) recorded as one ``UPLOAD``, so it keeps this wrapper: -* :func:`audited_write` wraps the actual write coroutine, emitting one +* :func:`audited_write` wraps the upload coroutine, emitting one ``completed`` or ``failed`` audit entry per call. * :func:`emit_dry_run_audit` is called by ``render_dry_run`` to record the ``dry_run`` outcome before short-circuiting. @@ -10,10 +12,6 @@ Both fast-path when audit is disabled (no sink construction, no work beyond a property read) so the audit feature has zero overhead until the user opts in. - -The SDK (``AsyncBCClient``) intentionally does NOT auto-emit — programmatic -users get unfiltered access. Audit is a CLI-layer ergonomic, layered on -top of BC's own permission set (the actual security boundary). """ from __future__ import annotations diff --git a/src/bcli_cli/_dry_run.py b/src/bcli_cli/_dry_run.py index 3ca97f5..418139b 100644 --- a/src/bcli_cli/_dry_run.py +++ b/src/bcli_cli/_dry_run.py @@ -1,9 +1,10 @@ """Shared ``--dry-run`` renderer for write commands. -One helper, one shape, every write command. Used by ``bcli post``, -``patch``, ``delete``, ``attach upload``, ``attach test``, and -``batch run`` so an agent driving any of those gets the same machine- -readable preview without each command rolling its own format. +One shape, every write command. ``post`` / ``patch`` / ``delete`` / ``action`` +and ``batch run`` preview a ``WritePlan`` (see ``_write_cmd.render_plan``); +``attach upload`` / ``attach test`` build theirs with :func:`render_dry_run`. +Both end in :func:`render_preview`, so an agent driving any of them gets the +same machine-readable preview. Output format follows the active CLI ``--format``: @@ -55,7 +56,6 @@ def render_dry_run( ``/api/v2.0/`` route, matching what the actual write would hit. """ profile = state.profile - profile_name = state.active_profile_name # strict=True: a preview must not report success for a request the real # command would refuse. An unresolvable endpoint still previews with a null @@ -86,7 +86,6 @@ def render_dry_run( "method": method.upper(), "endpoint": endpoint, "resolved_url": resolved_url, - "profile": profile_name, "environment": profile.environment, "company_id": profile.company_id, } @@ -97,12 +96,6 @@ def render_dry_run( if extra: payload.update(extra) - if state.format in _MACHINE_FORMATS: - indent = 2 if state.format == "json" else None - print(json.dumps(payload, indent=indent, default=str)) - else: - _render_human(payload) - # Record the dry-run in the audit log when the user has it enabled, # so a paper trail captures every "would have done X" intent in # addition to actual writes. @@ -115,6 +108,26 @@ def render_dry_run( resolved_url=resolved_url, ) + render_preview(payload) + + +def render_preview(payload: dict[str, Any]) -> None: + """Print a dry-run ``payload`` in the active format and ``typer.Exit(0)``. + + Adds the active profile name; the caller is responsible for auditing. + """ + keys = list(payload) + at = keys.index("resolved_url") + 1 if "resolved_url" in keys else len(keys) + items = list(payload.items()) + items.insert(at, ("profile", state.active_profile_name)) + payload = dict(items) + + if state.format in _MACHINE_FORMATS: + indent = 2 if state.format == "json" else None + print(json.dumps(payload, indent=indent, default=str)) + else: + _render_human(payload) + raise typer.Exit() diff --git a/src/bcli_cli/_state.py b/src/bcli_cli/_state.py index bb6cd96..7258d14 100644 --- a/src/bcli_cli/_state.py +++ b/src/bcli_cli/_state.py @@ -111,6 +111,7 @@ def make_async_client(self, **kwargs) -> AsyncBCClient: new_profiles = dict(cfg.profiles) new_profiles[profile_name] = self.profile cfg = cfg.model_copy(update={"profiles": new_profiles}) + kwargs.setdefault("caller", "cli") return AsyncBCClient(profile=profile_name, config=cfg, **kwargs) diff --git a/src/bcli_cli/_url_resolve.py b/src/bcli_cli/_url_resolve.py index 4d640eb..6fd58aa 100644 --- a/src/bcli_cli/_url_resolve.py +++ b/src/bcli_cli/_url_resolve.py @@ -59,7 +59,7 @@ def try_resolve_url( record_id=record_id, ) client = state.make_async_client() - return client._resolve_url( + return client.resolve( endpoint, record_id=record_id, publisher=publisher, diff --git a/src/bcli_cli/_write_cmd.py b/src/bcli_cli/_write_cmd.py new file mode 100644 index 0000000..21d063a --- /dev/null +++ b/src/bcli_cli/_write_cmd.py @@ -0,0 +1,115 @@ +"""The one write path behind ``bcli post / patch / delete / action``. + +Each command parses its own arguments, then hands a single call to +:func:`run_write`, which owns the sequence every mutation shares: + +plan (resolve once) → envelope URL → ``disable_writes`` gate → dry-run +preview *or* send → envelope with BC's correlation id. + +Auditing happens inside the SDK's ``client.write`` / ``client.record_dry_run``. +""" + +from __future__ import annotations + +import asyncio +from pathlib import Path +from typing import Any, Callable + +import typer +from rich.console import Console + +from bcli.client import AsyncBCClient, WritePlan, WriteResult +from bcli_cli._envelope_wrap import capture +from bcli_cli._state import state + +_console = Console(stderr=True) + + +def run_write( + method: str, + endpoint: str, + *, + body: dict[str, Any] | None = None, + record_id: str | None = None, + etag: str = "*", + publisher: str | None = None, + group: str | None = None, + version: str | None = None, + idempotency_key: str | None = None, + yes: bool = False, + result_out: Path | None = None, + result_fd: int | None = None, + gate_label: str | None = None, + check: Callable[[WriteResult], None] | None = None, +) -> WriteResult: + """Plan, gate, preview-or-send one write and return its result. + + ``check`` runs after the write and before the success envelope, so a + result the caller cannot use (``action --out`` with no payload) is + recorded as failed. Dry-run exits via ``typer.Exit(0)``; any error prints + ``Error: …`` and exits 1 (the gate exits 8). + """ + from bcli_cli._safety import confirm_write_or_exit + + with capture( + method=method, endpoint=endpoint, result_out=result_out, result_fd=result_fd, + ) as cap: + client = state.make_async_client() + try: + plan = client.plan( + method, endpoint, body=body, record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + ) + except Exception as e: + _fail(cap, e) + cap.set_resolved_url(plan.url) + cap.set_record_id(record_id) + + # Inside the capture block so a refused write still emits a failed + # envelope. See PR #15 review. + confirm_write_or_exit(gate_label or method, endpoint, yes=yes) + + if state.dry_run: + cap.mark_dry_run() + cap.emit_success() + render_plan(client, plan) + + try: + result = asyncio.run(_send(client, plan)) + if method == "POST": + # A create learns its id from the response; PATCH / DELETE keep + # the key the caller addressed (an API keyed on No. still + # returns a different systemId). + cap.extract_record_id_from(result.body) + if check is not None: + check(result) + except Exception as e: + _fail(cap, e) + cap.emit_success(bc_correlation_id=result.correlation_id) + return result + + +def render_plan(client: AsyncBCClient, plan: WritePlan, *, method: str | None = None, + extra: dict[str, Any] | None = None) -> None: + """Print ``plan``'s dry-run preview, audit it, and ``typer.Exit(0)``.""" + from bcli_cli._dry_run import render_preview + + payload = plan.preview() + if method is not None: + payload["method"] = method + if extra: + payload.update(extra) + client.record_dry_run(plan, method=method) + render_preview(payload) + + +async def _send(client: AsyncBCClient, plan: WritePlan) -> WriteResult: + async with client: + return await client.write(plan) + + +def _fail(cap: Any, e: Exception) -> None: + cap.emit_failure(e) + _console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) from e diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py index c156f8a..7c88d9c 100644 --- a/src/bcli_cli/commands/action_cmd.py +++ b/src/bcli_cli/commands/action_cmd.py @@ -26,18 +26,19 @@ from __future__ import annotations -import asyncio from pathlib import Path from typing import Optional import typer from rich.console import Console +from bcli.client import WriteResult from bcli.errors import BCLIError from bcli_cli._data_arg import parse_data_argument -from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._envelope_wrap import validate_flags from bcli_cli._out_path import atomic_write_bytes, prepare_out_path from bcli_cli._state import state +from bcli_cli._write_cmd import run_write from bcli_cli.output import format_output, print_context_banner console = Console(stderr=True) @@ -159,89 +160,37 @@ def action_command( print_context_banner() - with capture( - method="POST", - endpoint=composed, - result_out=result_out, - result_fd=result_fd, - ) as cap: - from bcli_cli._safety import confirm_write_or_exit - from bcli_cli._url_resolve import try_resolve_url - - cap.set_resolved_url(try_resolve_url( - composed, - publisher=publisher, group=group, version=version, - )) - - # disable_writes / read-only-profile gate. Same policy as ``post`` - # — actions are mutating writes from the agent's perspective. - confirm_write_or_exit("ACTION", composed, yes=yes) - - if state.dry_run: - from bcli_cli._dry_run import render_dry_run - cap.mark_dry_run() - cap.emit_success() - render_dry_run( - "POST", composed, body=body, - publisher=publisher, group=group, version=version, - ) - - try: - result = asyncio.run(_audited_post( - composed, body, - publisher=publisher, group=group, version=version, - idempotency_key=idempotency_key, - )) - cap.extract_record_id_from(result) - - written: int | None = None - if dest is not None: - # Decode *before* emit_success: a return value we can't turn - # into bytes means the caller didn't get what they asked for, - # and the envelope has to say failed. The POST already - # happened either way — that's what the envelope records. - written = _write_decoded_payload(result, dest, overwrite=overwrite) - - cap.emit_success() - - if dest is not None: - # Deliberately no format_output here: the payload is base64, - # and dumping it to stdout after writing the decoded file is - # noise at best and a wrecked pipe at worst. - console.print(f"[green]✓[/green] Decoded {written:,} bytes to {dest}") - elif result: - format_output([result], output_format) - else: - # 204 No Content — common for actions that mutate but - # don't return a payload. Stay quiet on machine-readable - # formats, print a friendly line on table/markdown. - if output_format in (None, "table", "markdown"): - console.print("[green]✓[/green] Action invoked (204 No Content)") - except Exception as e: - cap.emit_failure(e) - console.print(f"[red]Error:[/red] {e}") - raise typer.Exit(1) - - -async def _audited_post(endpoint, body, **kwargs): - from bcli_cli._audit_wrap import audited_write - from bcli_cli._url_resolve import try_resolve_url - resolved_url = try_resolve_url( - endpoint, - publisher=kwargs.get("publisher"), - group=kwargs.get("group"), - version=kwargs.get("version"), + written: list[int] = [] + + def _decode_to_out(result: WriteResult) -> None: + # Decode *before* the success envelope: a return value we can't turn + # into bytes means the caller didn't get what they asked for, and the + # envelope has to say failed. The POST already happened either way — + # that's what the envelope records. + if dest is not None: + written.append(_write_decoded_payload(result.body, dest, overwrite=overwrite)) + + # Same disable_writes gate as ``post`` — actions are mutating writes from + # the agent's perspective — labelled ACTION in the prompt. + result = run_write( + "POST", composed, body=body, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + yes=yes, result_out=result_out, result_fd=result_fd, + gate_label="ACTION", check=_decode_to_out, ) - return await audited_write( - _execute_post(endpoint, body, **kwargs), - method="POST", endpoint=endpoint, body=body, - resolved_url=resolved_url, - ) - -async def _execute_post(endpoint, body, **kwargs): - async with state.make_async_client() as client: - return await client.post(endpoint, body, **kwargs) + if dest is not None: + # Deliberately no format_output here: the payload is base64, and + # dumping it to stdout after writing the decoded file is noise at + # best and a wrecked pipe at worst. + console.print(f"[green]✓[/green] Decoded {written[0]:,} bytes to {dest}") + elif result.body: + format_output([result.body], output_format) + elif output_format in (None, "table", "markdown"): + # 204 No Content — common for actions that mutate but don't return a + # payload. Stay quiet on machine-readable formats. + console.print("[green]✓[/green] Action invoked (204 No Content)") def _decode_base64_payload(result: dict | str) -> bytes: diff --git a/src/bcli_cli/commands/batch_cmd.py b/src/bcli_cli/commands/batch_cmd.py index e3439ff..c81f184 100644 --- a/src/bcli_cli/commands/batch_cmd.py +++ b/src/bcli_cli/commands/batch_cmd.py @@ -146,21 +146,6 @@ def _validate_step_names(steps: list[dict]) -> None: seen.add(name) -def _close_if_coroutine(obj: Any) -> None: - """Close a stray coroutine to suppress the ResourceWarning that fires - when an ``AsyncMock``'s synchronously-accessed attribute returns a - coroutine the production code never awaits. No-op for non-coroutines. - """ - if hasattr(obj, "close") and callable(obj.close): - # ``inspect.iscoroutine`` is stricter than necessary — duck-type - # on ``send`` / ``close`` to also cover generator-coroutines. - if hasattr(obj, "send") or type(obj).__name__ in {"coroutine"}: - try: - obj.close() - except Exception: - pass - - def _body_hash(body: Any) -> str | None: """sha256 of a JSON-serialised body, or None if there's no body. @@ -207,14 +192,48 @@ def _compose_rollback_url(client: Any, entity: str, post_result: Any) -> str | N if not record_id: return None try: - base = client._resolve_url(entity) # noqa: SLF001 + return client.resolve(entity, record_id=str(record_id)) except Exception: return None - if not isinstance(base, str): - _close_if_coroutine(base) - return None - # OData record URL convention: () - return f"{base}({record_id})" + + +_WRITE_ACTIONS = frozenset({"post", "patch", "delete"}) +_DONE_LABEL = {"post": "created", "patch": "updated", "delete": "deleted"} + + +def _step_action(step: dict) -> str: + """A step's lower-case action, accepting the ``method:`` alias. + + Authors who copy-paste OData examples (especially bound-action + invocations) reach for ``method`` — silently treating that as a missing + ``action`` and downgrading to GET would let a mutating POST step land as a + GET round-trip. See ``bcli.workflow._models.StepDef`` for the equivalent + normalisation at schema-validation time. + """ + raw = step.get("action") + if raw is None and "method" in step: + m = step.get("method") + raw = m.lower() if isinstance(m, str) else None + return (raw or "get").lower() + + +def _plan_step(client: Any, step: dict, action: str) -> Any: + """Plan a write step exactly as ``bcli post/patch/delete`` would.""" + from bcli.errors import BCLIError + + record_id = step.get("id") + if action != "post" and not record_id: + raise BCLIError("missing 'id' field") + return client.plan( + action.upper(), step.get("endpoint", ""), + body=None if action == "delete" else (step.get("data") or {}), + record_id=record_id, + etag=step.get("etag", "*"), + idempotency_key=step.get("idempotency_key"), + publisher=step.get("publisher"), + group=step.get("group"), + version=step.get("version"), + ) # ─── Command: batch run ────────────────────────────────────────────── @@ -348,7 +367,7 @@ def run_batch( # zombie ``running`` row that never gets resolved). The envelope # records ``dry_run=true`` for disambiguation. if dry_run or state.dry_run: - _print_dry_run(steps, context) + _print_dry_run(steps, context, state.make_async_client()) ledger.finish_run(run_id, "completed") ledger.close() cap.mark_dry_run() @@ -361,20 +380,8 @@ def run_batch( # review). Lives after ``start_run`` so the ledger has a row to # update; the BaseException branch below derives a truthful # state from the (empty) step table when the gate fires. - mutating_actions = {"post", "patch", "delete"} - - def _step_action(step: dict) -> str: - """Mirror the alias logic from ``_execute_batch`` so the - disable_writes gate doesn't miss a step that uses - ``method: POST`` instead of ``action: post``.""" - raw = step.get("action") - if raw is None and "method" in step: - m = step.get("method") - raw = m.lower() if isinstance(m, str) else None - return (raw or "get").lower() - mutating_steps = [ - step for step in steps if _step_action(step) in mutating_actions + step for step in steps if _step_action(step) in _WRITE_ACTIONS ] if mutating_steps: from bcli_cli._safety import confirm_write_or_exit @@ -492,8 +499,13 @@ def _step_action(step: dict) -> str: # ─── Dry run ───────────────────────────────────────────────────────── -def _print_dry_run(steps: list[dict], context: Any | None) -> None: - """Print resolved (or partially resolved) requests without executing.""" +def _print_dry_run(steps: list[dict], context: Any | None, client: Any) -> None: + """Preview every step without executing. + + Write steps are planned exactly as the real run plans them, so the + preview shows the URL that would be hit (and records a ``dry_run`` audit + entry). + """ from bcli.workflow._resolver import resolve_references for i, step in enumerate(steps, 1): @@ -507,21 +519,30 @@ def _print_dry_run(steps: list[dict], context: Any | None) -> None: # Step references can't resolve at dry-run time — show raw step_to_show = step - action_raw = step_to_show.get("action") - if action_raw is None and "method" in step_to_show: - method_raw = step_to_show.get("method") - action_raw = method_raw.lower() if isinstance(method_raw, str) else None - action = (action_raw or "get").upper() + action = _step_action(step_to_show) endpoint = step_to_show.get("endpoint", "?") name = step_to_show.get("name", "") data = step_to_show.get("data") params = step_to_show.get("params", {}) - label = f" [dim]Step {i}:[/dim] {action} {endpoint}" + url = error = None + if action in _WRITE_ACTIONS: + try: + plan = _plan_step(client, step_to_show, action) + url = plan.url + client.record_dry_run(plan) + except Exception as e: + error = str(e) + + label = f" [dim]Step {i}:[/dim] {action.upper()} {endpoint}" if name and name != endpoint: label += f" [dim]({name})[/dim]" console.print(label) + if url: + console.print(f" [dim]URL: {url}[/dim]") + if error: + console.print(f" [red]✗ {error}[/red]") if params: console.print(f" [dim]Params: {params}[/dim]") if data: @@ -577,24 +598,12 @@ def emit(self, **_: Any) -> None: ... ) continue - # Accept the ``method:`` alias for ``action:``. Authors who - # copy-paste OData examples (especially bound-action - # invocations) reach for ``method`` — silently treating that - # as a missing ``action`` and downgrading to GET would let a - # mutating POST step land as a GET round-trip. See - # ``bcli.workflow._models.StepDef`` for the equivalent - # normalisation at schema-validation time. - action_raw = step.get("action") - if action_raw is None and "method" in step: - method_raw = step.get("method") - action_raw = method_raw.lower() if isinstance(method_raw, str) else None - action = (action_raw or "get").lower() + action = _step_action(step) endpoint = step.get("endpoint", "") step_name = step.get("name") or endpoint data = step.get("data") params = step.get("params", {}) record_id = step.get("id") - etag = step.get("etag", "*") # AIP §Phase 4d — optional per-step idempotency token; same-run # replay protection is enforced below. idempotency_key = step.get("idempotency_key") @@ -659,25 +668,27 @@ def emit(self, **_: Any) -> None: ... ) continue + # Plan a write once: its URL goes into the ledger intent and is + # the URL the write hits. A planning error (bad key, unroutable + # endpoint, missing id) is recorded as this step's failure below. + plan = plan_error = None + if action in _WRITE_ACTIONS: + try: + plan = _plan_step(client, step, action) + except Exception as e: + plan_error = e + # ── Ledger: write intent BEFORE the HTTP call. ── - # The resolved URL is computed lazily via the client's - # registry. If resolution fails (or the client is a mock - # whose ``_resolve_url`` returns something non-string), fall - # back to the entity name so the operator has *something* to - # look at and the SQLite TEXT column accepts the value. + # Falls back to the entity name when the URL can't be resolved, + # so the operator has *something* to look at. ledger_step_id: int | None = None if ledger is not None: intent_url = endpoint - if endpoint: + if plan is not None: + intent_url = plan.url + elif endpoint and plan_error is None: try: - candidate = client._resolve_url(endpoint, record_id=record_id) # noqa: SLF001 - if isinstance(candidate, str): - intent_url = candidate - else: - # AsyncMock returns a coroutine when treated - # as sync — close it to avoid the resource - # warning at GC time. - _close_if_coroutine(candidate) + intent_url = client.resolve(endpoint, record_id=record_id) except Exception: intent_url = endpoint ledger_step_id = ledger.write_intent( @@ -717,7 +728,11 @@ def emit(self, **_: Any) -> None: ... if params.get("orderby"): query.orderby(params["orderby"]) - response = await client.get(endpoint, record_id, query=query) + response = await client.get( + endpoint, record_id, query=query, + publisher=step.get("publisher"), group=step.get("group"), + version=step.get("version"), + ) records = response.value if not record_id else [response.raw] if response.raw else [] console.print(f"[green]✓[/green] {len(records)} record(s)") result_entry = {"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "ok", "count": len(records), "data": records} @@ -742,123 +757,41 @@ def emit(self, **_: Any) -> None: ... ) step_terminal_status = "committed" - elif action == "post": - result = await client.post( - endpoint, data or {}, - idempotency_key=idempotency_key, - ) - console.print("[green]✓[/green] created") - result_entry = {"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "ok", "data": [result] if result else []} - results.append(result_entry) - - if isinstance(context, WorkflowContext): - context.set_result( - step_name, - StepResult(name=step_name, action=action, endpoint=endpoint, status="ok", data=result or {}), - ) - - if output_format and result: - format_output([result], output_format) - - if ledger is not None and ledger_step_id is not None: - rb_url = _compose_rollback_url(client, endpoint, result) - ledger.write_outcome( - step_id=ledger_step_id, status="committed", - bc_correlation_id=None, error_message=None, - rollback_url=rb_url, - ) - step_terminal_status = "committed" - - elif action == "patch": - if not record_id: - console.print("[red]✗ missing 'id' field[/red]") - results.append({"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "error", "error": "missing id"}) - if ledger is not None and ledger_step_id is not None: - ledger.write_outcome( - step_id=ledger_step_id, status="failed", - bc_correlation_id=None, - error_message="missing id", - rollback_url=None, - ) - step_terminal_status = "failed" - step_terminal_error = "missing id" - # Emit step_completed before the early ``continue`` - # so the agent sees a complete pair. - prog.emit( - event="step_completed", - seq=i, name=step_name, method=step_method, - endpoint=endpoint, status=step_terminal_status, - error=step_terminal_error, - duration_ms=max(0, int((_time.monotonic_ns() - step_started_ns) / 1_000_000)), - ) - continue - result = await client.patch( - endpoint, record_id, data or {}, - etag=etag, - idempotency_key=idempotency_key, - ) - console.print("[green]✓[/green] updated") - result_entry = {"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "ok", "data": [result] if result else []} - results.append(result_entry) - - if isinstance(context, WorkflowContext): - context.set_result( - step_name, - StepResult(name=step_name, action=action, endpoint=endpoint, status="ok", data=result or {}), - ) - - if ledger is not None and ledger_step_id is not None: - ledger.write_outcome( - step_id=ledger_step_id, status="committed", - bc_correlation_id=None, error_message=None, - # PATCH rollback would need a pre-image snapshot - # we don't keep in v0.1 — see help text on - # ``bcli batch rollback``. - rollback_url=None, - ) - step_terminal_status = "committed" - - elif action == "delete": - if not record_id: - console.print("[red]✗ missing 'id' field[/red]") - results.append({"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "error", "error": "missing id"}) - if ledger is not None and ledger_step_id is not None: - ledger.write_outcome( - step_id=ledger_step_id, status="failed", - bc_correlation_id=None, - error_message="missing id", - rollback_url=None, - ) - step_terminal_status = "failed" - step_terminal_error = "missing id" - prog.emit( - event="step_completed", - seq=i, name=step_name, method=step_method, - endpoint=endpoint, status=step_terminal_status, - error=step_terminal_error, - duration_ms=max(0, int((_time.monotonic_ns() - step_started_ns) / 1_000_000)), - ) - continue - await client.delete( - endpoint, record_id, - etag=etag, - idempotency_key=idempotency_key, - ) - console.print("[green]✓[/green] deleted") + elif action in _WRITE_ACTIONS: + if plan_error is not None: + raise plan_error + written = await client.write(plan) + body = written.body + console.print(f"[green]✓[/green] {_DONE_LABEL[action]}") result_entry = {"step": i, "name": step_name, "action": action, "endpoint": endpoint, "status": "ok"} + if action != "delete": + result_entry["data"] = [body] if body else [] results.append(result_entry) if isinstance(context, WorkflowContext): context.set_result( step_name, - StepResult(name=step_name, action=action, endpoint=endpoint, status="ok", data={}), + StepResult( + name=step_name, action=action, endpoint=endpoint, status="ok", + data={} if action == "delete" else (body or {}), + ), ) + if output_format and body and action == "post": + format_output([body], output_format) + if ledger is not None and ledger_step_id is not None: ledger.write_outcome( step_id=ledger_step_id, status="committed", - bc_correlation_id=None, error_message=None, - rollback_url=None, # DELETE has no clean inverse + bc_correlation_id=written.correlation_id, error_message=None, + # Only a POST has a clean inverse (DELETE the new + # record). PATCH would need a pre-image snapshot + # we don't keep; DELETE has none — see help text + # on ``bcli batch rollback``. + rollback_url=( + _compose_rollback_url(client, endpoint, body) + if action == "post" else None + ), ) step_terminal_status = "committed" @@ -885,7 +818,7 @@ def emit(self, **_: Any) -> None: ... if ledger is not None and ledger_step_id is not None: ledger.write_outcome( step_id=ledger_step_id, status="failed", - bc_correlation_id=None, + bc_correlation_id=getattr(e, "correlation_id", None), error_message=str(e), rollback_url=None, ) diff --git a/src/bcli_cli/commands/delete_cmd.py b/src/bcli_cli/commands/delete_cmd.py index 523e37e..5cce5b0 100644 --- a/src/bcli_cli/commands/delete_cmd.py +++ b/src/bcli_cli/commands/delete_cmd.py @@ -2,15 +2,15 @@ from __future__ import annotations -import asyncio from pathlib import Path from typing import Optional import typer from rich.console import Console -from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._envelope_wrap import validate_flags from bcli_cli._state import state +from bcli_cli._write_cmd import run_write from bcli_cli.output import print_context_banner console = Console(stderr=True) @@ -51,69 +51,10 @@ def delete_command( print_context_banner() - with capture( - method="DELETE", - endpoint=endpoint, - result_out=result_out, - result_fd=result_fd, - ) as cap: - from bcli_cli._safety import confirm_write_or_exit - from bcli_cli._url_resolve import try_resolve_url - - cap.set_resolved_url(try_resolve_url( - endpoint, - record_id=record_id, - publisher=publisher, - group=group, - version=version, - )) - cap.set_record_id(record_id) - - # Policy gate runs *inside* the capture block so a refused write - # still emits a failed envelope. See PR #15 review. - confirm_write_or_exit("DELETE", endpoint, yes=yes) - - if state.dry_run: - from bcli_cli._dry_run import render_dry_run - cap.mark_dry_run() - cap.emit_success() - render_dry_run( - "DELETE", endpoint, record_id=record_id, - publisher=publisher, group=group, version=version, - extra={"etag": etag}, - ) - - try: - asyncio.run(_audited_delete( - endpoint, record_id, - etag=etag, publisher=publisher, group=group, version=version, - idempotency_key=idempotency_key, - )) - cap.emit_success() - console.print(f"[green]✓[/green] Deleted {endpoint}({record_id})") - except Exception as e: - cap.emit_failure(e) - console.print(f"[red]Error:[/red] {e}") - raise typer.Exit(1) - - -async def _audited_delete(endpoint, record_id, **kwargs): - from bcli_cli._audit_wrap import audited_write - from bcli_cli._url_resolve import try_resolve_url - resolved_url = try_resolve_url( - endpoint, - record_id=record_id, - publisher=kwargs.get("publisher"), - group=kwargs.get("group"), - version=kwargs.get("version"), - ) - return await audited_write( - _execute_delete(endpoint, record_id, **kwargs), - method="DELETE", endpoint=endpoint, record_id=record_id, - resolved_url=resolved_url, + run_write( + "DELETE", endpoint, record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + yes=yes, result_out=result_out, result_fd=result_fd, ) - - -async def _execute_delete(endpoint, record_id, **kwargs): - async with state.make_async_client() as client: - return await client.delete(endpoint, record_id, **kwargs) + console.print(f"[green]✓[/green] Deleted {endpoint}({record_id})") diff --git a/src/bcli_cli/commands/get_cmd.py b/src/bcli_cli/commands/get_cmd.py index 445eb1b..88ff308 100644 --- a/src/bcli_cli/commands/get_cmd.py +++ b/src/bcli_cli/commands/get_cmd.py @@ -118,7 +118,7 @@ def get_command( try: records = asyncio.run( _execute_get_all_companies( - endpoint, query, top, + endpoint, query, top, all_pages=all_pages, publisher=publisher, group=group, version=version, ) ) @@ -173,6 +173,7 @@ async def _execute_get_all_companies( query: Query, top: int | None, *, + all_pages: bool = False, publisher: str | None = None, group: str | None = None, version: str | None = None, @@ -192,39 +193,16 @@ async def _execute_get_all_companies( display = alias or company_name or company_id[:8] console.print(f"[dim]Querying {endpoint} in {display}...[/dim]") - # Override the company for this query - url = client._resolve_url( - endpoint, - publisher=publisher, - group=group, - version=version, + records: list[dict] = [] + pages = client.pages( + endpoint, query, + publisher=publisher, group=group, version=version, + company_id=company_id, ) - # Replace the company_id in the URL - from bcli._url import build_url - ep = client.registry.get(endpoint) - if ep and ep.is_custom: - url = build_url( - environment=profile.environment, - company_id=company_id, - entity_set_name=endpoint, - publisher=ep.api_publisher, - group=ep.api_group, - version=ep.api_version, - ) - else: - url = build_url( - environment=profile.environment, - company_id=company_id, - entity_set_name=endpoint, - publisher=publisher, - group=group, - version=version, - ) - - transport = client._ensure_transport() - params = query.to_params() - data = await transport.get(url, params=params) - records = data.get("value", []) + async for page in pages: + records.extend(page) + if not all_pages: + break # Tag each record with _company for record in records: @@ -398,20 +376,9 @@ async def _execute_get( async with state.make_async_client() as client: if all_pages: all_records: list[dict] = [] - bound = client.query(endpoint) - if publisher and group and version: - bound.route(publisher, group, version) - if not query.is_empty: - for f in query._params.filters: - bound.filter(f) - for s in query._params.selects: - bound.select(s) - for e in query._params.expands: - bound.expand(e) - if query._params.orderby: - bound.orderby(query._params.orderby) - - pages = await bound.pages() + pages = client.pages( + endpoint, query, publisher=publisher, group=group, version=version, + ) async for page in pages: all_records.extend(page) return all_records diff --git a/src/bcli_cli/commands/patch_cmd.py b/src/bcli_cli/commands/patch_cmd.py index c8ef0e2..392ebdf 100644 --- a/src/bcli_cli/commands/patch_cmd.py +++ b/src/bcli_cli/commands/patch_cmd.py @@ -2,20 +2,17 @@ from __future__ import annotations -import asyncio from pathlib import Path from typing import Optional import typer -from rich.console import Console from bcli_cli._data_arg import parse_data_argument -from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._envelope_wrap import validate_flags from bcli_cli._state import state +from bcli_cli._write_cmd import run_write from bcli_cli.output import format_output, print_context_banner -console = Console(stderr=True) - def patch_command( endpoint: str = typer.Argument(help="Entity set name"), @@ -53,71 +50,10 @@ def patch_command( print_context_banner() - body = parse_data_argument(data) - - with capture( - method="PATCH", - endpoint=endpoint, - result_out=result_out, - result_fd=result_fd, - ) as cap: - from bcli_cli._safety import confirm_write_or_exit - from bcli_cli._url_resolve import try_resolve_url - - cap.set_resolved_url(try_resolve_url( - endpoint, - record_id=record_id, - publisher=publisher, - group=group, - version=version, - )) - cap.set_record_id(record_id) - - # Policy gate runs *inside* the capture block so a refused write - # still emits a failed envelope. See PR #15 review. - confirm_write_or_exit("PATCH", endpoint, yes=yes) - - if state.dry_run: - from bcli_cli._dry_run import render_dry_run - cap.mark_dry_run() - cap.emit_success() - render_dry_run( - "PATCH", endpoint, body=body, record_id=record_id, - publisher=publisher, group=group, version=version, - extra={"etag": etag}, - ) - - try: - result = asyncio.run(_audited_patch( - endpoint, record_id, body, - etag=etag, publisher=publisher, group=group, version=version, - idempotency_key=idempotency_key, - )) - cap.emit_success() - format_output([result] if result else [], output_format) - except Exception as e: - cap.emit_failure(e) - console.print(f"[red]Error:[/red] {e}") - raise typer.Exit(1) - - -async def _audited_patch(endpoint, record_id, body, **kwargs): - from bcli_cli._audit_wrap import audited_write - from bcli_cli._url_resolve import try_resolve_url - resolved_url = try_resolve_url( - endpoint, - record_id=record_id, - publisher=kwargs.get("publisher"), - group=kwargs.get("group"), - version=kwargs.get("version"), - ) - return await audited_write( - _execute_patch(endpoint, record_id, body, **kwargs), - method="PATCH", endpoint=endpoint, body=body, record_id=record_id, - resolved_url=resolved_url, + result = run_write( + "PATCH", endpoint, body=parse_data_argument(data), record_id=record_id, etag=etag, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + yes=yes, result_out=result_out, result_fd=result_fd, ) - - -async def _execute_patch(endpoint, record_id, body, **kwargs): - async with state.make_async_client() as client: - return await client.patch(endpoint, record_id, body, **kwargs) + format_output([result.body] if result.body else [], output_format) diff --git a/src/bcli_cli/commands/post_cmd.py b/src/bcli_cli/commands/post_cmd.py index 6fae9b2..be754a6 100644 --- a/src/bcli_cli/commands/post_cmd.py +++ b/src/bcli_cli/commands/post_cmd.py @@ -2,20 +2,17 @@ from __future__ import annotations -import asyncio from pathlib import Path from typing import Optional import typer -from rich.console import Console from bcli_cli._data_arg import parse_data_argument -from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._envelope_wrap import validate_flags from bcli_cli._state import state +from bcli_cli._write_cmd import run_write from bcli_cli.output import format_output, print_context_banner -console = Console(stderr=True) - def post_command( endpoint: str = typer.Argument(help="Entity set name"), @@ -51,71 +48,10 @@ def post_command( print_context_banner() - body = parse_data_argument(data) - - with capture( - method="POST", - endpoint=endpoint, - result_out=result_out, - result_fd=result_fd, - ) as cap: - from bcli_cli._safety import confirm_write_or_exit - from bcli_cli._url_resolve import try_resolve_url - - cap.set_resolved_url(try_resolve_url( - endpoint, - publisher=publisher, - group=group, - version=version, - )) - - # Policy gate runs *inside* the capture block so a refused write - # still emits a failed envelope. The ``capture()`` salvage path - # turns the ``typer.Exit(1)`` into a status="failed" envelope. - # See PR #15 review. - confirm_write_or_exit("POST", endpoint, yes=yes) - - if state.dry_run: - from bcli_cli._dry_run import render_dry_run - cap.mark_dry_run() - cap.emit_success() - render_dry_run( - "POST", endpoint, body=body, - publisher=publisher, group=group, version=version, - ) - - try: - result = asyncio.run(_audited_post( - endpoint, body, - publisher=publisher, group=group, version=version, - idempotency_key=idempotency_key, - )) - cap.extract_record_id_from(result) - cap.emit_success() - format_output([result] if result else [], output_format) - except Exception as e: - cap.emit_failure(e) - console.print(f"[red]Error:[/red] {e}") - raise typer.Exit(1) - - -async def _audited_post(endpoint, body, **kwargs): - from bcli_cli._audit_wrap import audited_write - from bcli_cli._url_resolve import try_resolve_url - resolved_url = try_resolve_url( - endpoint, - publisher=kwargs.get("publisher"), - group=kwargs.get("group"), - version=kwargs.get("version"), - ) - return await audited_write( - _execute_post(endpoint, body, **kwargs), - method="POST", endpoint=endpoint, body=body, - resolved_url=resolved_url, + result = run_write( + "POST", endpoint, body=parse_data_argument(data), + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + yes=yes, result_out=result_out, result_fd=result_fd, ) - - -async def _execute_post(endpoint, body, **kwargs): - async with state.make_async_client() as client: - # ``client.post`` accepts ``idempotency_key``; passthrough. - return await client.post(endpoint, body, **kwargs) + format_output([result.body] if result.body else [], output_format) diff --git a/src/bcli_cli/commands/query_cmd.py b/src/bcli_cli/commands/query_cmd.py index f44151f..a02adaa 100644 --- a/src/bcli_cli/commands/query_cmd.py +++ b/src/bcli_cli/commands/query_cmd.py @@ -491,17 +491,7 @@ async def _run_saved_query(endpoint: str, resolved: ResolvedQuery) -> list[dict] async with state.make_async_client() as client: if resolved.all_pages: records: list[dict] = [] - bound = client.query(endpoint) - for f in query._params.filters: - bound.filter(f) - for s in query._params.selects: - bound.select(s) - for ex in query._params.expands: - bound.expand(ex) - if query._params.orderby: - bound.orderby(query._params.orderby) - pages = await bound.pages() - async for page in pages: + async for page in client.pages(endpoint, query): records.extend(page) return records diff --git a/src/bcli_mcp/_runner.py b/src/bcli_mcp/_runner.py index e044892..596943f 100644 --- a/src/bcli_mcp/_runner.py +++ b/src/bcli_mcp/_runner.py @@ -16,8 +16,8 @@ back via :func:`bcli.result_envelope.read_envelope`. The envelope is the source of truth for outcome; stdout is mostly noise. -We deliberately do NOT import ``bcli`` Python modules here. Subprocess -delegation is the design — see ``docs/mcp-server.md``. +Record writes (post / patch / delete) don't come through here — the server +calls the SDK for those. See ``bcli_mcp._server``. """ from __future__ import annotations diff --git a/src/bcli_mcp/_server.py b/src/bcli_mcp/_server.py index 78ded55..d66ed0a 100644 --- a/src/bcli_mcp/_server.py +++ b/src/bcli_mcp/_server.py @@ -6,26 +6,34 @@ :func:`bcli_mcp._tool_generator.generate_tools`. New CLI commands light up automatically; deprecated ones disappear. -Two invocation paths: +Three invocation paths: +* **Record writes** (``bcli_post`` / ``bcli_patch`` / ``bcli_delete``) — + in-process through the SDK's write operation (``client.plan`` + + ``client.write``): the same planning, audit (``caller="mcp"``) and + correlation id as the CLI, with typed errors instead of scraped stderr. * **Read** (``effects: ["read"]``) — append ``--format json``, parse stdout, return the parsed JSON. -* **Mutating** (``emits_result_envelope: True``) — pass - ``--result-out ``, read the envelope back, return its content - as the tool result. A ``status="failed"`` envelope surfaces as an - MCP ``ToolError`` with the envelope's correlation id quoted for the - agent to cite. +* **Other mutating** (``emits_result_envelope: True``: attach upload, + batch run) — pass ``--result-out ``, read the envelope back, return + its content as the tool result. A ``status="failed"`` envelope surfaces + as an MCP ``ToolError`` with the envelope's correlation id quoted for + the agent to cite. + +Subprocess calls run in a worker thread so one slow command never stalls +the server's event loop. If ``bcli describe`` fails on startup (bcli not on PATH, broken install, etc.), the server still starts — with zero tools registered and a stderr warning. The operator can fix the install and reconnect. -The subprocess boundary inherits auth, retry, telemetry, profile gates -for free — that's the whole point. No Python imports from ``bcli`` core. +Every path returns the same envelope shape (``bcli.result_envelope``) +for writes, so agents parse one format. """ from __future__ import annotations +import asyncio import inspect import json import os @@ -129,7 +137,7 @@ async def handler(**inputs: Any) -> Any: inputs = {k: v for k, v in inputs.items() if v is not None} profile = inputs.pop("profile", None) args = tool.build_argv(inputs) - result = run_bcli_json(*args, profile=profile) + result = await asyncio.to_thread(run_bcli_json, *args, profile=profile) # Single-record dict from ``bcli get `` is wrapped # so the return type stays stable for the agent. if tool.name == "bcli_get" and isinstance(result, dict): @@ -159,7 +167,8 @@ async def handler(**inputs: Any) -> dict[str, Any]: fd, env_path = tempfile.mkstemp(prefix="bcli-mcp-", suffix=".json") os.close(fd) try: - exit_code, _stdout, stderr = run_bcli_with_envelope( + exit_code, _stdout, stderr = await asyncio.to_thread( + run_bcli_with_envelope, args, env=env, capture_envelope_path=env_path, ) try: @@ -194,6 +203,89 @@ async def handler(**inputs: Any) -> dict[str, Any]: return handler +#: Tools served in-process by the SDK write operation rather than a subprocess. +_SDK_WRITE_METHODS = {"bcli_post": "POST", "bcli_patch": "PATCH", "bcli_delete": "DELETE"} + + +def _make_sdk_write_handler(tool: GeneratedTool, method: str): + """Record write through ``client.plan`` + ``client.write``, in-process. + + Mirrors the CLI's non-interactive rules: a ``disable_writes`` profile is + refused unless the agent passes ``yes=true`` (after the user confirmed). + Returns a ``ResultEnvelope`` dict plus the response ``body``. + """ + async def handler(**inputs: Any) -> dict[str, Any]: + import time + import uuid + from dataclasses import asdict + from datetime import datetime, timezone + + from bcli import AsyncBCClient + from bcli._version import __version__ + from bcli.exit_codes import EXIT_OK + from bcli.result_envelope import ENVELOPE_VERSION, ResultEnvelope + from bcli_cli._data_arg import parse_data_argument + from bcli_cli._envelope_wrap import record_id_from + + inputs = {k: v for k, v in inputs.items() if v is not None} + endpoint = inputs.get("endpoint", "") + started_at = datetime.now(timezone.utc).isoformat().replace("+00:00", "Z") + t0 = time.monotonic() + try: + client = AsyncBCClient(profile=inputs.get("profile"), caller="mcp") + if client.profile.disable_writes and not inputs.get("yes"): + raise ToolError( + f"Refusing {method} {endpoint}: profile is read-only " + "(disable_writes = true). Pass yes=true once the user has " + "confirmed this write." + ) + data = inputs.get("data") + plan = client.plan( + method, endpoint, + body=parse_data_argument(data) if data is not None else None, + record_id=inputs.get("record_id"), + etag=inputs.get("etag", "*"), + idempotency_key=inputs.get("idempotency_key"), + publisher=inputs.get("publisher"), + group=inputs.get("group"), + version=inputs.get("version"), + ) + async with client: + result = await client.write(plan) + except ToolError: + raise + except Exception as exc: # noqa: BLE001 — every failure goes back to the agent + corr = getattr(exc, "correlation_id", None) or "n/a" + raise ToolError(f"{method} {endpoint} failed: {exc} (correlation_id={corr})") from exc + + envelope = ResultEnvelope( + version=ENVELOPE_VERSION, + invocation_id=uuid.uuid4().hex, + tool_version=__version__, + profile=client._profile_name, + environment=plan.environment, + company=plan.company_id, + method=method, + endpoint=endpoint, + resolved_url=plan.url, + record_id=record_id_from(result.body) or plan.record_id, + dry_run=False, + status="succeeded", + exit_code=EXIT_OK, + bc_correlation_id=result.correlation_id, + telemetry_event_id=None, + audit_log_offset=None, + started_at=started_at, + duration_ms=int((time.monotonic() - t0) * 1000), + ) + return {**asdict(envelope), "body": result.body} + + handler.__name__ = tool.name + handler.__doc__ = tool.description + _apply_dynamic_signature(handler, tool) + return handler + + def _register_tool(mcp: FastMCP, tool: GeneratedTool) -> None: """Register one generated tool with FastMCP. @@ -201,7 +293,9 @@ def _register_tool(mcp: FastMCP, tool: GeneratedTool) -> None: envelope-reading flow. Profile is always available as an extra kwarg the agent can pass to scope the call. """ - if tool.emits_envelope: + if tool.name in _SDK_WRITE_METHODS: + fn = _make_sdk_write_handler(tool, _SDK_WRITE_METHODS[tool.name]) + elif tool.emits_envelope: fn = _make_mutating_handler(tool) else: fn = _make_read_handler(tool) diff --git a/tests/conftest.py b/tests/conftest.py index c898f5f..f7c82e9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,9 @@ from __future__ import annotations +from dataclasses import dataclass +from typing import Any + import pytest @@ -24,3 +27,89 @@ def _isolate_msal_cache(tmp_path, monkeypatch): msal_cache_mod, "MSAL_CACHE_FILE", tmp_path / "msal_cache.json" ) yield + + +# ─── Fake BC transport ─────────────────────────────────────────────────── + + +@dataclass +class Sent: + """One request the code under test sent to BC.""" + + method: str + url: str + json_body: Any = None + params: Any = None + etag: str | None = None + idempotency_key: str | None = None + + +class FakeTransport: + """Stands in for ``BCTransport``: records every request, sends nothing. + + Replies come from ``replies`` in order (a dict body, or an exception to + raise), then ``default``. Tests drive the real ``AsyncBCClient`` — planning, + URL resolution, audit — and assert on what was sent. + """ + + def __init__(self) -> None: + self.sent: list[Sent] = [] + self.replies: list[Any] = [] + self.default: dict[str, Any] = {} + self.status = 200 + self.correlation_id: str | None = "corr-test" + + def reply(self, *bodies: Any) -> FakeTransport: + self.replies.extend(bodies) + return self + + @property + def last(self) -> Sent: + return self.sent[-1] + + async def request(self, method, url, *, params=None, json_body=None, content=None, + content_type=None, etag=None, idempotency_key=None, log_context=None): + from bcli.client._transport import BCResponse + + self.sent.append(Sent(method, url, json_body, params, etag, idempotency_key)) + nxt = self.replies.pop(0) if self.replies else self.default + if isinstance(nxt, BaseException): + raise nxt + return BCResponse(nxt, self.status, self.correlation_id) + + async def _request(self, method, url, **kwargs): + return (await self.request(method, url, **kwargs)).body + + async def get(self, url, *, params=None): + return await self._request("GET", url, params=params) + + async def get_absolute(self, url): + return await self._request("GET", url) + + async def post(self, url, *, json_body, idempotency_key=None): + return await self._request("POST", url, json_body=json_body, idempotency_key=idempotency_key) + + async def patch(self, url, *, json_body, etag="*", idempotency_key=None): + return await self._request("PATCH", url, json_body=json_body, etag=etag, + idempotency_key=idempotency_key) + + async def patch_binary(self, url, *, content, content_type="application/octet-stream", + etag="*", idempotency_key=None): + return await self._request("PATCH", url, content=content, etag=etag, + idempotency_key=idempotency_key) + + async def delete(self, url, *, etag="*", idempotency_key=None): + return await self._request("DELETE", url, etag=etag, idempotency_key=idempotency_key) + + async def close(self) -> None: + pass + + +@pytest.fixture +def fake_bc(monkeypatch) -> FakeTransport: + """Every ``AsyncBCClient`` in the test talks to one ``FakeTransport``.""" + from bcli.client._async import AsyncBCClient + + transport = FakeTransport() + monkeypatch.setattr(AsyncBCClient, "_ensure_transport", lambda self: transport) + return transport diff --git a/tests/test_batch_ledger/test_batch_cmd_ledger.py b/tests/test_batch_ledger/test_batch_cmd_ledger.py index 4929a50..92145e4 100644 --- a/tests/test_batch_ledger/test_batch_cmd_ledger.py +++ b/tests/test_batch_ledger/test_batch_cmd_ledger.py @@ -2,8 +2,8 @@ outcome row after, surviving SIGKILL in the gap. These are integration tests against ``run_batch`` — they patch HOME so the -ledger lands in tmp_path, patch ``make_async_client`` to inject a fake -async client, and assert on the resulting SQLite file. +ledger lands in tmp_path, drive the real client against the ``fake_bc`` +transport, and assert on what was sent and on the resulting SQLite file. """ from __future__ import annotations @@ -11,7 +11,6 @@ import sqlite3 import textwrap from pathlib import Path -from unittest.mock import AsyncMock, patch import pytest @@ -59,44 +58,41 @@ def ledger_home(tmp_path, monkeypatch): yield ledger_dir -@pytest.fixture -def fake_client(): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"id": "rec-1", "@odata.context": "x"}) - c.patch = AsyncMock(return_value={"id": "rec-1"}) - c.delete = AsyncMock(return_value=None) - c.get = AsyncMock( - return_value=type("R", (), {"value": [], "raw": None})() - ) - # Used so the ledger can compose a rollback URL for POSTs. - c._resolve_url = lambda entity, record_id=None, **_: ( - f"https://x/{entity}({record_id})" if record_id else f"https://x/{entity}" - ) - return c - - def _write_yaml(tmp_path: Path, name: str, content: str) -> Path: f = tmp_path / name f.write_text(textwrap.dedent(content).strip(), encoding="utf-8") return f +def _run(f: Path) -> None: + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=True, + ) + + +def _steps(ledger_home: Path) -> list[dict]: + db_files = list(ledger_home.glob("*.db")) + assert len(db_files) == 1 + with sqlite3.connect(db_files[0]) as conn: + conn.row_factory = sqlite3.Row + return [dict(r) for r in conn.execute("SELECT * FROM step ORDER BY seq")] + + # ─── Intent before HTTP ────────────────────────────────────────────── class TestIntentBeforeHttp: def test_intent_written_before_post( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, monkeypatch, ): - """When the fake client's POST is invoked, an intent row must - already exist in the ledger DB. The test enforces ordering via a - side_effect that reads the DB at the moment of the call. + """When the POST reaches the transport, an intent row must + already exist in the ledger DB. The test enforces ordering by + reading the DB at the moment of the request. """ - captured = {} + send = fake_bc.request - async def post_side_effect(entity, body, **kwargs): + async def checking_request(method, url, **kwargs): # Scan the batch ledger dir for any *.db that contains an # intent row for this POST. Done at call time so we catch a # write-order regression. @@ -117,10 +113,9 @@ async def post_side_effect(entity, body, **kwargs): assert all(r["outcome_ts"] is None for r in rows), ( "outcome must NOT be written before HTTP returns" ) - captured["body"] = body - return {"id": "created-1"} + return await send(method, url, **kwargs) - fake_client.post.side_effect = post_side_effect + monkeypatch.setattr(fake_bc, "request", checking_request) f = _write_yaml(tmp_path, "p.yaml", """ steps: @@ -130,49 +125,29 @@ async def post_side_effect(entity, body, **kwargs): data: displayName: "ok" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - ) + _run(f) - # Post-run: outcome row populated. - db_files = list(ledger_home.glob("*.db")) - assert len(db_files) == 1 - with sqlite3.connect(db_files[0]) as conn: - conn.row_factory = sqlite3.Row - rows = [ - dict(r) for r in conn.execute( - "SELECT * FROM step ORDER BY seq" - ) - ] + assert [(s.method, s.json_body) for s in fake_bc.sent] == [ + ("POST", {"displayName": "ok"}), + ] + # Post-run: outcome row populated; intent URL is the URL sent. + rows = _steps(ledger_home) assert len(rows) == 1 assert rows[0]["status"] == "committed" assert rows[0]["outcome_ts"] is not None assert rows[0]["method"] == "POST" - assert "items" in rows[0]["url"] + assert rows[0]["url"] == fake_bc.last.url + assert rows[0]["url"].endswith("/items") def test_partially_committed_after_simulated_crash( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): """Step 1 succeeds; step 2 'crashes' (raises mid-flight) — the run ledger surfaces ``partially_committed`` on read. """ - call_count = {"n": 0} - - async def post_side_effect(entity, body, **kwargs): - call_count["n"] += 1 - if call_count["n"] == 2: - # Simulate SIGKILL between intent + outcome by raising a - # BaseException that batch_cmd cannot catch with `except - # Exception:`. - raise BaseException("simulated SIGKILL") - return {"id": f"rec-{call_count['n']}"} - - fake_client.post.side_effect = post_side_effect + # Simulate SIGKILL between intent + outcome on step 2 by raising a + # BaseException that batch_cmd cannot catch with `except Exception:`. + fake_bc.reply({"id": "rec-1"}, BaseException("simulated SIGKILL")) f = _write_yaml(tmp_path, "two.yaml", """ steps: @@ -185,15 +160,8 @@ async def post_side_effect(entity, body, **kwargs): endpoint: items data: {x: 2} """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(BaseException, match="simulated SIGKILL"): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - ) + with pytest.raises(BaseException, match="simulated SIGKILL"): + _run(f) # Read the ledger after the crash. from bcli.batch.ledger import Ledger @@ -205,7 +173,7 @@ async def post_side_effect(entity, body, **kwargs): assert rows[0]["state"] == "partially_committed" def test_normal_completion_marks_completed( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "ok.yaml", """ steps: @@ -214,14 +182,7 @@ def test_normal_completion_marks_completed( endpoint: items data: {x: 1} """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - ) + _run(f) from bcli.batch.ledger import Ledger rows = Ledger.list_runs(base_dir=ledger_home) @@ -234,10 +195,10 @@ def test_normal_completion_marks_completed( class TestRollbackUrlCapture: def test_post_step_stores_rollback_url( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): """POST that returns an ``id`` → rollback_url = entity_url(id).""" - fake_client.post.return_value = {"id": "VND-1"} + fake_bc.reply({"id": "VND-1"}) f = _write_yaml(tmp_path, "p.yaml", """ steps: - name: create_vendor @@ -245,21 +206,71 @@ def test_post_step_stores_rollback_url( endpoint: vendors data: {name: "AAR"} """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - ) + _run(f) - db_files = list(ledger_home.glob("*.db")) - with sqlite3.connect(db_files[0]) as conn: - conn.row_factory = sqlite3.Row - row = dict( - conn.execute("SELECT * FROM step LIMIT 1").fetchone() - ) - assert row["rollback_url"] - assert "vendors" in row["rollback_url"] - assert "VND-1" in row["rollback_url"] + row = _steps(ledger_home)[0] + assert row["rollback_url"] == f"{fake_bc.last.url}(VND-1)" + + +# ─── Batch writes go through the SDK write operation ───────────────── + + +class TestBatchWriteOperation: + def test_post_outcome_records_bc_correlation_id( + self, writable_state, ledger_home, fake_bc, tmp_path, + ): + f = _write_yaml(tmp_path, "p.yaml", """ + steps: + - name: create + action: post + endpoint: items + data: {x: 1} + """) + _run(f) + + assert _steps(ledger_home)[0]["bc_correlation_id"] == "corr-test" + + def test_post_step_writes_one_audit_entry( + self, writable_state, ledger_home, fake_bc, tmp_path, + ): + import json + + from bcli.config._model import AuditConfig + + audit_file = tmp_path / "audit.jsonl" + state._config = state._config.model_copy( + update={"audit": AuditConfig(enabled=True, path=str(audit_file))}, + ) + f = _write_yaml(tmp_path, "p.yaml", """ + steps: + - name: create + action: post + endpoint: items + data: {x: 1} + """) + _run(f) + + entries = [json.loads(line) for line in audit_file.read_text().splitlines()] + assert len(entries) == 1 + assert entries[0]["outcome"] == "completed" + assert entries[0]["method"] == "POST" + assert entries[0]["resolved_url"] == fake_bc.last.url + assert entries[0]["correlation_id"] == "corr-test" + + def test_step_route_override_sends_to_custom_api( + self, writable_state, ledger_home, fake_bc, tmp_path, + ): + f = _write_yaml(tmp_path, "p.yaml", """ + steps: + - name: create + action: post + endpoint: widgets + publisher: acme + group: ops + version: v1.0 + data: {x: 1} + """) + _run(f) + + assert "/api/acme/ops/v1.0/" in fake_bc.last.url + assert fake_bc.last.url.endswith("/widgets") diff --git a/tests/test_batch_ledger/test_batch_write_path.py b/tests/test_batch_ledger/test_batch_write_path.py new file mode 100644 index 0000000..1ad4b44 --- /dev/null +++ b/tests/test_batch_ledger/test_batch_write_path.py @@ -0,0 +1,87 @@ +"""Batch dry-run and rollback go through the SDK write operation. + +Dry-run plans each write step exactly as the real run would (URL shown, a +``dry_run`` audit entry, nothing sent); rollback's DELETE is audited like any +other write. +""" + +from __future__ import annotations + +import json +import textwrap +from pathlib import Path + +import pytest + +from bcli.config._model import AuditConfig, BCConfig, BCDefaults, BCProfile +from bcli_cli._state import state +from bcli_cli.commands.batch_cmd import rollback_cmd, run_batch + + +@pytest.fixture +def audit_path(tmp_path, monkeypatch): + monkeypatch.setattr(Path, "home", lambda: tmp_path) + path = tmp_path / "audit.jsonl" + state._config = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={"dev": BCProfile(tenant_id="t1", environment="Sandbox", company_id="c-1")}, + audit=AuditConfig(enabled=True, path=str(path)), + ) + state._registry = None + state.profile_name = None + state.dry_run = False + state.quiet = True + yield path + state._config = None + state._registry = None + + +def _entries(path: Path) -> list[dict]: + return [json.loads(line) for line in path.read_text().splitlines()] if path.exists() else [] + + +def _batch(tmp_path: Path, steps: str) -> Path: + f = tmp_path / "b.yaml" + f.write_text("name: t\nsteps:\n" + textwrap.indent(textwrap.dedent(steps).strip(), " ")) + return f + + +def _run(f: Path, *, dry_run: bool = False) -> None: + run_batch(file=f, dry_run=dry_run, output=None, format=None, + set_params=None, params_file=None, yes=True) + + +def test_dry_run_plans_writes_and_sends_nothing(audit_path, fake_bc, tmp_path, capsys): + f = _batch(tmp_path, """ + - action: post + endpoint: items + data: {displayName: x} + - action: patch + endpoint: items + data: {displayName: y} + """) + _run(f, dry_run=True) + + assert fake_bc.sent == [] + out = capsys.readouterr().out + assert "URL:" in out and "/companies(c-1)/items" in out.replace("\n", "") + assert "missing 'id' field" in out + assert [e["outcome"] for e in _entries(audit_path)] == ["dry_run"] + + +def test_rollback_delete_is_audited(audit_path, fake_bc, tmp_path): + fake_bc.reply({"id": "rec-1"}) + _run(_batch(tmp_path, """ + - action: post + endpoint: items + data: {displayName: x} + """)) + run_id = next((tmp_path / ".config" / "bcli" / "batch").glob("*.db")).stem + + rollback_cmd(run_id=run_id, dry_run=False, yes=True) + + assert fake_bc.last.method == "DELETE" + assert fake_bc.last.url.endswith("/items(rec-1)") + assert [(e["method"], e["outcome"]) for e in _entries(audit_path)] == [ + ("POST", "completed"), ("DELETE", "completed"), + ] diff --git a/tests/test_batch_ledger/test_idempotency_replay.py b/tests/test_batch_ledger/test_idempotency_replay.py index 1be8704..efd3066 100644 --- a/tests/test_batch_ledger/test_idempotency_replay.py +++ b/tests/test_batch_ledger/test_idempotency_replay.py @@ -19,7 +19,6 @@ import sqlite3 import textwrap from pathlib import Path -from unittest.mock import AsyncMock, patch import pytest @@ -64,20 +63,6 @@ def ledger_home(tmp_path, monkeypatch): yield tmp_path / ".config" / "bcli" / "batch" -@pytest.fixture -def fake_client(): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"id": "rec-1", "systemId": "rec-1"}) - c.patch = AsyncMock(return_value={"id": "rec-1"}) - c.delete = AsyncMock(return_value=None) - c._resolve_url = lambda entity, record_id=None, **_: ( - f"https://x/{entity}({record_id})" if record_id else f"https://x/{entity}" - ) - return c - - def _write_yaml(tmp_path: Path, name: str, content: str) -> Path: f = tmp_path / name f.write_text(textwrap.dedent(content).strip(), encoding="utf-8") @@ -96,7 +81,7 @@ class TestSameRunIdempotencyReplay: """ def test_duplicate_key_replays_second_step( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): yaml_file = _write_yaml(tmp_path, "dup.yaml", """ name: dup-idempotent @@ -113,22 +98,18 @@ def test_duplicate_key_replays_second_step( idempotency_key: op-shared """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - - # The fake POST was awaited exactly once (the first step). The - # second step short-circuited on the replay path. - assert fake_client.post.await_count == 1, ( - f"second step must replay (no second POST); " - f"got await_count={fake_client.post.await_count}" + run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, ) + # Exactly one POST was sent (the first step), carrying the key as + # the Idempotency-Key header. The second step short-circuited on the + # replay path. + assert [(s.method, s.idempotency_key) for s in fake_bc.sent] == [ + ("POST", "op-shared"), + ], "second step must replay (no second POST)" + # The first step's idempotency_key landed in the ledger and was # committed; the second step's intent row does NOT exist. dbs = list(ledger_home.glob("*.db")) @@ -144,7 +125,7 @@ def test_duplicate_key_replays_second_step( assert rows == [(1, "committed", "op-shared")], rows def test_distinct_keys_both_fire_http( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): """Sanity: different keys don't trigger replay.""" yaml_file = _write_yaml(tmp_path, "two-keys.yaml", """ @@ -162,16 +143,12 @@ def test_distinct_keys_both_fire_http( idempotency_key: op-B """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) + run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) - assert fake_client.post.await_count == 2, ( + assert [s.idempotency_key for s in fake_bc.sent] == ["op-A", "op-B"], ( "distinct keys must not trigger replay" ) @@ -186,7 +163,7 @@ def test_distinct_keys_both_fire_http( ], rows def test_no_key_no_replay( - self, writable_state, ledger_home, fake_client, tmp_path, + self, writable_state, ledger_home, fake_bc, tmp_path, ): """Steps without an idempotency_key keep existing behavior: every step fires HTTP and lands an intent + outcome row.""" @@ -203,16 +180,12 @@ def test_no_key_no_replay( data: {displayName: "Beta"} """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) + run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) - assert fake_client.post.await_count == 2 + assert [s.method for s in fake_bc.sent] == ["POST", "POST"] dbs = list(ledger_home.glob("*.db")) with sqlite3.connect(dbs[0]) as conn: diff --git a/tests/test_cli/test_action_cmd.py b/tests/test_cli/test_action_cmd.py index fea7196..e491074 100644 --- a/tests/test_cli/test_action_cmd.py +++ b/tests/test_cli/test_action_cmd.py @@ -21,7 +21,6 @@ import base64 import json from pathlib import Path -from unittest.mock import AsyncMock import pytest import typer @@ -60,14 +59,10 @@ def cli_state(): @pytest.fixture -def fake_client(monkeypatch): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"result": "ok"}) - c._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" - monkeypatch.setattr(state, "make_async_client", lambda **_: c) - return c +def bc(fake_bc): + """The real client against the fake transport; BC answers ``{"result": "ok"}``.""" + fake_bc.default = {"result": "ok"} + return fake_bc @pytest.fixture(autouse=True) @@ -109,71 +104,68 @@ def _run( class TestURLComposition: - def test_default_namespace_is_microsoft_nav(self, cli_state, fake_client): + def test_default_namespace_is_microsoft_nav(self, cli_state, bc): _run() - endpoint_arg = fake_client.post.await_args.args[0] - assert endpoint_arg == "examples(42)/Microsoft.NAV.archive" + assert bc.last.url.endswith("/companies(c-123)/examples(42)/Microsoft.NAV.archive") - def test_namespace_override(self, cli_state, fake_client): + def test_namespace_override(self, cli_state, bc): _run(namespace="Custom.Ns") - endpoint_arg = fake_client.post.await_args.args[0] - assert endpoint_arg == "examples(42)/Custom.Ns.archive" + assert bc.last.url.endswith("/examples(42)/Custom.Ns.archive") - def test_quoted_string_key_passed_through(self, cli_state, fake_client): + def test_quoted_string_key_passed_through(self, cli_state, bc): _run(key="'ALFKI'") - endpoint_arg = fake_client.post.await_args.args[0] - assert endpoint_arg == "examples('ALFKI')/Microsoft.NAV.archive" + assert bc.last.url.endswith("/examples('ALFKI')/Microsoft.NAV.archive") - def test_action_uses_post_not_get(self, cli_state, fake_client): + def test_action_uses_post_not_get(self, cli_state, bc): """OData actions are always POST. The verb must NOT call .get, even if a future flag suggests otherwise.""" _run() - assert fake_client.post.await_count == 1 + assert [r.method for r in bc.sent] == ["POST"] class TestDataHandling: - def test_data_json_literal_parsed(self, cli_state, fake_client): + def test_data_json_literal_parsed(self, cli_state, bc): _run(data='{"a": 1, "b": "two"}') - body_arg = fake_client.post.await_args.args[1] + body_arg = bc.last.json_body assert body_arg == {"a": 1, "b": "two"} - def test_data_from_file(self, cli_state, fake_client, tmp_path: Path): + def test_data_from_file(self, cli_state, bc, tmp_path: Path): f = tmp_path / "payload.json" f.write_text('{"loaded": "from-file"}', encoding="utf-8") _run(data=f"@{f}") - body_arg = fake_client.post.await_args.args[1] + body_arg = bc.last.json_body assert body_arg == {"loaded": "from-file"} - def test_no_data_flag_sends_empty_body(self, cli_state, fake_client): + def test_no_data_flag_sends_empty_body(self, cli_state, bc): """Explicit --no-data sends an empty body.""" _run(data=None, no_data=True) - body_arg = fake_client.post.await_args.args[1] + body_arg = bc.last.json_body assert body_arg == {} - def test_neither_flag_defaults_to_empty_body(self, cli_state, fake_client): + def test_neither_flag_defaults_to_empty_body(self, cli_state, bc): """Omitting both flags is equivalent to --no-data — matches ``bcli post`` semantics so AI agents calling ``bcli action examples 42 archive`` don't get a BadParameter.""" _run(data=None, no_data=False) - body_arg = fake_client.post.await_args.args[1] + body_arg = bc.last.json_body assert body_arg == {} - def test_data_and_no_data_mutually_exclusive(self, cli_state, fake_client): + def test_data_and_no_data_mutually_exclusive(self, cli_state, bc): with pytest.raises(typer.BadParameter): _run(data='{"x": 1}', no_data=True) def test_malformed_inline_json_raises_bad_parameter_not_traceback( - self, cli_state, fake_client, + self, cli_state, bc, ): """Regression: a mangled inline literal used to surface a raw json.JSONDecodeError. It must come back as a usage error, and the call must never reach the network.""" with pytest.raises(typer.BadParameter): _run(data="{not valid json") - fake_client.post.assert_not_called() + assert bc.sent == [] def test_bare_file_path_without_at_prefix_hints_the_fix( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "payload.json" f.write_text('{"x": 1}', encoding="utf-8") @@ -181,31 +173,27 @@ def test_bare_file_path_without_at_prefix_hints_the_fix( _run(data=str(f)) assert "looks like a file path" in str(exc_info.value) assert f"-d @{f}" in str(exc_info.value) - fake_client.post.assert_not_called() + assert bc.sent == [] def test_malformed_at_file_names_the_file( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "bad.json" f.write_text("{not valid json", encoding="utf-8") with pytest.raises(typer.BadParameter) as exc_info: _run(data=f"@{f}") assert str(f) in str(exc_info.value) - fake_client.post.assert_not_called() + assert bc.sent == [] class TestProfileOverrides: - def test_publisher_group_version_forwarded(self, cli_state, fake_client): + def test_publisher_group_version_forwarded(self, cli_state, bc): _run(publisher="acme", group="custom", version="v1.0") - kwargs = fake_client.post.await_args.kwargs - assert kwargs.get("publisher") == "acme" - assert kwargs.get("group") == "custom" - assert kwargs.get("version") == "v1.0" + assert "/api/acme/custom/v1.0/companies(c-123)/examples(42)/" in bc.last.url - def test_idempotency_key_forwarded(self, cli_state, fake_client): + def test_idempotency_key_forwarded(self, cli_state, bc): _run(idempotency_key="k-123") - kwargs = fake_client.post.await_args.kwargs - assert kwargs.get("idempotency_key") == "k-123" + assert bc.last.idempotency_key == "k-123" class TestOutDecode: @@ -216,10 +204,10 @@ class TestOutDecode: """ def test_base64_payload_decoded_to_file( - self, cli_state, fake_client, tmp_path: Path, capsys, + self, cli_state, bc, tmp_path: Path, capsys, ): payload = b"%PDF-1.4\nfake\n%%EOF\n" - fake_client.post.return_value = { + bc.default = { "value": base64.b64encode(payload).decode("ascii"), } dest = tmp_path / "document.pdf" @@ -233,9 +221,9 @@ def test_base64_payload_decoded_to_file( assert base64.b64encode(payload).decode("ascii") not in stdout def test_204_no_content_fails_rather_than_writing_an_empty_file( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): - fake_client.post.return_value = {} + bc.default = {} dest = tmp_path / "document.pdf" with pytest.raises(typer.Exit): @@ -244,10 +232,10 @@ def test_204_no_content_fails_rather_than_writing_an_empty_file( assert not dest.exists() def test_204_marks_the_envelope_failed( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): """The POST happened; the caller still didn't get what they asked for.""" - fake_client.post.return_value = {} + bc.default = {} envelope = tmp_path / "env.json" with pytest.raises(typer.Exit): @@ -256,9 +244,9 @@ def test_204_marks_the_envelope_failed( assert json.loads(envelope.read_text())["status"] == "failed" def test_dict_without_value_lists_the_keys( - self, cli_state, fake_client, tmp_path: Path, capsys, + self, cli_state, bc, tmp_path: Path, capsys, ): - fake_client.post.return_value = {"status": "ok", "recordId": "42"} + bc.default = {"status": "ok", "recordId": "42"} dest = tmp_path / "document.pdf" with pytest.raises(typer.Exit): @@ -269,8 +257,8 @@ def test_dict_without_value_lists_the_keys( assert "--result-out" in err assert not dest.exists() - def test_invalid_base64_fails(self, cli_state, fake_client, tmp_path: Path, capsys): - fake_client.post.return_value = {"value": "not base64 at all !!!"} + def test_invalid_base64_fails(self, cli_state, bc, tmp_path: Path, capsys): + bc.default = {"value": "not base64 at all !!!"} dest = tmp_path / "document.pdf" with pytest.raises(typer.Exit): @@ -280,7 +268,7 @@ def test_invalid_base64_fails(self, cli_state, fake_client, tmp_path: Path, caps assert not dest.exists() def test_existing_file_refused_before_the_post( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): """An action can mutate BC — the destination check has to come first.""" dest = tmp_path / "document.pdf" @@ -290,14 +278,14 @@ def test_existing_file_refused_before_the_post( _run(out=dest) assert exc.value.exit_code == 1 - assert fake_client.post.await_count == 0 + assert bc.sent == [] assert dest.read_bytes() == b"do not clobber me" def test_overwrite_allows_replacing_the_file( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): payload = b"fresh bytes" - fake_client.post.return_value = { + bc.default = { "value": base64.b64encode(payload).decode("ascii"), } dest = tmp_path / "document.pdf" @@ -308,10 +296,10 @@ def test_overwrite_allows_replacing_the_file( assert dest.read_bytes() == payload def test_out_and_result_out_compose( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): payload = b"both channels" - fake_client.post.return_value = { + bc.default = { "value": base64.b64encode(payload).decode("ascii"), } dest = tmp_path / "document.pdf" @@ -325,7 +313,7 @@ def test_out_and_result_out_compose( class TestEnvelope: def test_envelope_written_on_success( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): out = tmp_path / "env.json" _run(result_out=out) diff --git a/tests/test_cli/test_audit_wrap.py b/tests/test_cli/test_audit_wrap.py index cb70ab1..b0587ae 100644 --- a/tests/test_cli/test_audit_wrap.py +++ b/tests/test_cli/test_audit_wrap.py @@ -212,54 +212,60 @@ async def _boom(): assert entry["resolved_url"] == url -class TestCommandLevelAuditing: - """The command-side wrappers (_audited_post / _audited_patch / - _audited_delete) must thread resolved_url through to the audit entry. - A null URL there breaks the documented audit contract.""" - - def test_audited_post_records_resolved_url(self, audit_state: Path, monkeypatch): - from bcli_cli.commands import post_cmd +_CMD_DEFAULTS = dict( + format=None, publisher=None, group=None, version=None, yes=True, + result_out=None, result_fd=None, idempotency_key=None, +) - captured_url = "https://api.example.test/api/v2.0/companies(c-123)/customers" - async def _fake_execute(endpoint, body, **kwargs): - return {"id": "abc"} - - class _StubClient: - def _resolve_url(self, entity, **_): - return captured_url +class TestCommandLevelAuditing: + """``bcli post/patch/delete`` audit inside the SDK's ``client.write``: + exactly one entry per write, carrying the URL that was actually sent, + BC's real status and correlation id, and ``caller="cli"``. A null URL + there breaks the documented audit contract.""" - monkeypatch.setattr(post_cmd, "_execute_post", _fake_execute) - monkeypatch.setattr(state, "make_async_client", lambda **_: _StubClient()) + def test_post_writes_one_completed_entry(self, audit_state: Path, fake_bc): + from bcli_cli.commands import post_cmd - asyncio.run(post_cmd._audited_post("customers", {"x": 1})) + post_cmd.post_command(endpoint="customers", data='{"x": 1}', **_CMD_DEFAULTS) - entry = _read_entries(audit_state)[0] - assert entry["resolved_url"] == captured_url - assert entry["outcome"] == "completed" + entries = _read_entries(audit_state) + assert len(entries) == 1 + e = entries[0] + assert e["outcome"] == "completed" + assert e["status"] == 200 + assert e["correlation_id"] == "corr-test" + assert e["caller"] == "cli" + assert e["resolved_url"] == fake_bc.last.url + assert e["resolved_url"].endswith("/companies(c-123)/customers") - def test_audited_delete_records_resolved_url(self, audit_state: Path, monkeypatch): + def test_delete_records_resolved_url_and_record_id(self, audit_state: Path, fake_bc): from bcli_cli.commands import delete_cmd - captured_url = ( - "https://api.example.test/api/v2.0/companies(c-123)/items(rec-1)" + delete_cmd.delete_command( + endpoint="items", record_id="rec-1", etag="*", **_CMD_DEFAULTS, ) - async def _fake_execute(endpoint, record_id, **kwargs): - return {} + entry = _read_entries(audit_state)[0] + assert entry["resolved_url"] == fake_bc.last.url + assert entry["resolved_url"].endswith("/items(rec-1)") + assert entry["record_id"] == "rec-1" - class _StubClient: - def _resolve_url(self, entity, **_): - return captured_url + def test_failed_post_records_bc_error_metadata(self, audit_state: Path, fake_bc): + import typer - monkeypatch.setattr(delete_cmd, "_execute_delete", _fake_execute) - monkeypatch.setattr(state, "make_async_client", lambda **_: _StubClient()) + from bcli_cli.commands import post_cmd - asyncio.run(delete_cmd._audited_delete("items", "rec-1")) + fake_bc.reply(ValidationError("nope", status_code=400, correlation_id="err-corr")) + with pytest.raises(typer.Exit): + post_cmd.post_command(endpoint="customers", data='{"x": 1}', **_CMD_DEFAULTS) - entry = _read_entries(audit_state)[0] - assert entry["resolved_url"] == captured_url - assert entry["record_id"] == "rec-1" + entries = _read_entries(audit_state) + assert len(entries) == 1 + assert entries[0]["outcome"] == "failed" + assert entries[0]["status"] == 400 + assert entries[0]["correlation_id"] == "err-corr" + assert entries[0]["resolved_url"] == fake_bc.last.url class TestDryRunAudit: diff --git a/tests/test_cli/test_batch_safety.py b/tests/test_cli/test_batch_safety.py index b823998..2fce0df 100644 --- a/tests/test_cli/test_batch_safety.py +++ b/tests/test_cli/test_batch_safety.py @@ -9,8 +9,7 @@ These tests assert that: * a non-interactive batch with a mutating step on a read-only profile - aborts with ``typer.Exit(1)`` and does NOT call any client write - methods; + aborts with ``typer.Exit(8)`` and sends nothing to BC; * the same batch with ``--yes`` proceeds (scripted use); * a batch composed exclusively of GET steps is unaffected by the disable_writes flag (read-only is fine on read-only profile); @@ -22,7 +21,6 @@ import sys import textwrap from pathlib import Path -from unittest.mock import AsyncMock, patch import pytest import typer @@ -91,19 +89,6 @@ def writable_state(): state._registry = None -@pytest.fixture -def fake_client(): - """Async client whose write methods would be called if the gate failed.""" - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"id": "created"}) - c.patch = AsyncMock(return_value={"id": "patched"}) - c.delete = AsyncMock(return_value=None) - c.get = AsyncMock(return_value=AsyncMock(value=[])) - return c - - @pytest.fixture def non_interactive(monkeypatch): """Pretend stdin is not a TTY — matches CI/automation.""" @@ -128,7 +113,7 @@ def _isolate_ledger_home(tmp_path, monkeypatch): class TestReadonlyProfileBlocksMutatingSteps: def test_post_step_aborts_without_yes( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "post.yaml", """ name: bypass-post @@ -139,20 +124,16 @@ def test_post_step_aborts_without_yes( data: displayName: "batch write" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit) as excinfo: - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) + with pytest.raises(typer.Exit) as excinfo: + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) assert excinfo.value.exit_code == 8 # AIP §Phase 4a: EXIT_POLICY - fake_client.post.assert_not_awaited() + assert fake_bc.sent == [] def test_patch_step_aborts_without_yes( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "patch.yaml", """ name: bypass-patch @@ -165,19 +146,15 @@ def test_patch_step_aborts_without_yes( data: displayName: "batch patch" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - fake_client.patch.assert_not_awaited() + with pytest.raises(typer.Exit): + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) + assert fake_bc.sent == [] def test_delete_step_aborts_without_yes( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "delete.yaml", """ name: bypass-delete @@ -188,19 +165,15 @@ def test_delete_step_aborts_without_yes( id: "record-2" etag: "*" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - fake_client.delete.assert_not_awaited() + with pytest.raises(typer.Exit): + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) + assert fake_bc.sent == [] def test_mixed_steps_with_one_mutation_aborts( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): """A single mutating step among many gets caught by the gate.""" f = _write_yaml(tmp_path, "mixed.yaml", """ @@ -217,19 +190,14 @@ def test_mixed_steps_with_one_mutation_aborts( data: displayName: "batch write" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - fake_client.post.assert_not_awaited() + with pytest.raises(typer.Exit): + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) # No GET happens either — we abort BEFORE any step runs, so the # whole batch is refused atomically. - fake_client.get.assert_not_awaited() + assert fake_bc.sent == [] # ── --yes overrides the gate (scripted use) ────────────────────────────── @@ -237,7 +205,7 @@ def test_mixed_steps_with_one_mutation_aborts( class TestReadonlyProfileWithYesProceeds: def test_yes_flag_lets_mutating_batch_proceed( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "post.yaml", """ steps: @@ -247,15 +215,11 @@ def test_yes_flag_lets_mutating_batch_proceed( data: displayName: "batch write" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - ) - fake_client.post.assert_awaited_once() + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=True, + ) + assert [s.method for s in fake_bc.sent] == ["POST"] # ── Read-only batches are unaffected ───────────────────────────────────── @@ -263,7 +227,7 @@ def test_yes_flag_lets_mutating_batch_proceed( class TestReadOnlyBatchUnaffected: def test_get_only_batch_runs_on_readonly_profile( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): """A GET-only batch must still work on a disable_writes profile — otherwise read-only profiles become useless for read-heavy automation. @@ -277,20 +241,13 @@ def test_get_only_batch_runs_on_readonly_profile( action: get endpoint: vendors """) - # Make get() return a stub ODataResponse-like with .value - fake_client.get = AsyncMock( - return_value=type("R", (), {"value": [], "raw": None})() + fake_bc.default = {"value": []} + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, ) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) # Both GET calls executed, no prompt/abort. - assert fake_client.get.await_count == 2 + assert [s.method for s in fake_bc.sent] == ["GET", "GET"] # ── Writable profile is unaffected ─────────────────────────────────────── @@ -298,7 +255,7 @@ def test_get_only_batch_runs_on_readonly_profile( class TestWritableProfileUnaffected: def test_mutating_batch_runs_on_writable_profile( - self, writable_state, non_interactive, fake_client, tmp_path, + self, writable_state, non_interactive, fake_bc, tmp_path, ): f = _write_yaml(tmp_path, "post.yaml", """ steps: @@ -308,15 +265,11 @@ def test_mutating_batch_runs_on_writable_profile( data: displayName: "ok" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - fake_client.post.assert_awaited_once() + run_batch( + file=f, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) + assert [s.method for s in fake_bc.sent] == ["POST"] # ── Dry-run skips both execution and the gate ──────────────────────────── @@ -324,7 +277,7 @@ def test_mutating_batch_runs_on_writable_profile( class TestDryRunSkipsGate: def test_dry_run_does_not_prompt_or_abort( - self, readonly_state, non_interactive, fake_client, tmp_path, + self, readonly_state, non_interactive, fake_bc, tmp_path, ): """``--dry-run`` already short-circuits before any execution, so the gate must not trigger either — otherwise read-only users @@ -338,12 +291,8 @@ def test_dry_run_does_not_prompt_or_abort( data: displayName: "preview" """) - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - run_batch( - file=f, dry_run=True, output=None, format=None, - set_params=None, params_file=None, yes=False, - ) - fake_client.post.assert_not_awaited() + run_batch( + file=f, dry_run=True, output=None, format=None, + set_params=None, params_file=None, yes=False, + ) + assert fake_bc.sent == [] diff --git a/tests/test_cli/test_dry_run.py b/tests/test_cli/test_dry_run.py index 98f9b4c..9d4e43b 100644 --- a/tests/test_cli/test_dry_run.py +++ b/tests/test_cli/test_dry_run.py @@ -23,7 +23,7 @@ @pytest.fixture -def configured_state(monkeypatch): +def configured_state(): """Set up a minimal CLIState with one writable profile. Cleans up afterwards so tests don't bleed into each other. @@ -43,15 +43,6 @@ def configured_state(monkeypatch): state._registry = None state.profile_name = None state.format = "table" - - # Default: URL resolution returns a deterministic stub. Individual - # tests can override by re-monkeypatching make_async_client. - class _StubClient: - def _resolve_url(self, entity, **_): - return f"https://api.example.test/v2.0/companies(c-123)/{entity}" - - monkeypatch.setattr(state, "make_async_client", lambda **_: _StubClient()) - yield state._config = None state._registry = None @@ -150,7 +141,7 @@ def test_human_format_includes_resolved_url(self, configured_state, capsys): render_dry_run("POST", "items", body={"x": 1}) captured = capsys.readouterr() # URL should appear somewhere in the human output - assert "api.example.test" in captured.err + assert "api.businesscentral.dynamics.com" in captured.err def test_human_format_renders_body_on_stdout(self, configured_state, capsys): """The body itself is JSON to stdout so users can pipe it; the @@ -169,10 +160,10 @@ class TestForceStandardResolution: about exactly the escape-hatch case the flag exists for.""" def test_force_standard_uses_v2_route(self, configured_state, capsys, monkeypatch): - # Stub make_async_client so the client's _resolve_url would lie if + # Stub make_async_client so the client's resolve would lie if # called. force_standard=True must bypass it entirely. class _LyingClient: - def _resolve_url(self, *_a, **_kw): + def resolve(self, *_a, **_kw): return "https://example.test/CUSTOM/PATH/should-not-be-used" monkeypatch.setattr(state, "make_async_client", lambda **_: _LyingClient()) @@ -191,20 +182,13 @@ def _resolve_url(self, *_a, **_kw): class TestResolutionFailure: def test_failed_url_resolution_does_not_break_dry_run( - self, configured_state, capsys, monkeypatch + self, configured_state, capsys ): """If the registry can't resolve the entity (e.g. typo, missing - custom registry entry), dry-run still emits — with resolved_url=None - — so the user sees what they asked for and can correct.""" - - def _failing_client(**_): - class _C: - def _resolve_url(self, *_a, **_kw): - raise RuntimeError("registry boom") - - return _C() - - monkeypatch.setattr(state, "make_async_client", _failing_client) + custom registry entry), ``render_dry_run`` (still used by + ``attach upload``) emits — with resolved_url=None — so the user sees + what they asked for and can correct.""" + state.profile.disable_standard_api = True state.format = "json" with suppress(typer.Exit): render_dry_run("POST", "totallyMadeUpEntity", body={"x": 1}) @@ -228,35 +212,14 @@ def test_exits_clean_for_human_format_too(self, configured_state): @pytest.fixture -def real_resolver_state(): - """Like ``configured_state`` but WITHOUT stubbing ``make_async_client``. - - ``configured_state``'s ``_StubClient._resolve_url`` ignores ``record_id`` - entirely and always returns a URL, which is fine for testing the renderer's - output shape — but it means the existing dry-run suite never exercises real - URL resolution. That is exactly why an empty ``record_id`` could render a - clean preview: nothing here ever built a real URL. These tests use the real - resolver. +def real_resolver_state(configured_state): + """``configured_state`` under the name these tests were written against. + + The suite once stubbed the resolver, which is why an empty ``record_id`` + could render a clean preview: nothing built a real URL. Every test here + now uses the real resolver. """ - cfg = BCConfig( - defaults=BCDefaults(profile="dev"), - profiles={ - "dev": BCProfile( - tenant_id="t1", - environment="Sandbox", - company_id="c-123", - disable_writes=False, - ), - }, - ) - state._config = cfg - state._registry = None - state.profile_name = None - state.format = "table" yield - state._config = None - state._registry = None - state.format = "table" class TestDryRunMustNotSucceedWhereTheRealRunFails: @@ -332,3 +295,49 @@ def test_strict_still_swallows_incidental_failures(self, real_resolver_state): state.profile.disable_standard_api = True state._registry = None assert try_resolve_url("definitelyNotAnEndpoint", record_id="x", strict=True) is None + + +def _post_dry_run(endpoint: str, **kwargs) -> None: + from bcli_cli.commands import post_cmd + + state.dry_run = True + try: + post_cmd.post_command( + endpoint=endpoint, data='{"x": 1}', format="json", publisher=None, + group=None, version=None, yes=True, result_out=None, result_fd=None, + idempotency_key=None, **kwargs, + ) + finally: + state.dry_run = False + + +class TestWriteCommandDryRun: + """``post/patch/delete/action --dry-run`` preview the SDK's write plan.""" + + def test_post_previews_the_plan_and_sends_nothing( + self, real_resolver_state, fake_bc, capsys, + ): + with pytest.raises(typer.Exit) as excinfo: + _post_dry_run("items") + assert excinfo.value.exit_code == 0 + assert fake_bc.sent == [] + + payload = _capture_json_payload(capsys) + assert payload["method"] == "POST" + assert payload["resolved_url"].endswith("/companies(c-123)/items") + assert payload["body"] == {"x": 1} + assert "etag" not in payload + keys = list(payload) + assert keys[keys.index("resolved_url") + 1] == "profile" + + def test_post_to_endpoint_blocked_by_disable_standard_api_fails( + self, real_resolver_state, fake_bc, capsys, + ): + """Previously the preview printed ``resolved_url: null`` and exited 0; + the plan now fails exactly where the real run would.""" + state.profile.disable_standard_api = True + with pytest.raises(typer.Exit) as excinfo: + _post_dry_run("definitelyNotAnEndpoint") + assert excinfo.value.exit_code == 1 + assert fake_bc.sent == [] + assert "Error:" in capsys.readouterr().err diff --git a/tests/test_cli/test_get_paging.py b/tests/test_cli/test_get_paging.py new file mode 100644 index 0000000..c129373 --- /dev/null +++ b/tests/test_cli/test_get_paging.py @@ -0,0 +1,97 @@ +"""``bcli get --company all`` and ``--all`` resolve and page through the client. + +Both used to rebuild URLs or queries by hand: ``--company all`` let the +registry route override an explicit ``--publisher`` and fetched only the first +page; ``--all`` silently dropped ``--top`` / ``--skip`` / ``--count``. +""" + +from __future__ import annotations + +import asyncio +import json + +import pytest + +import bcli.registry._registry as registry_mod +from bcli.config._model import BCConfig, BCDefaults, BCProfile, CompanyAlias +from bcli.odata._query import Query +from bcli_cli._state import state +from bcli_cli.commands.get_cmd import _execute_get, _execute_get_all_companies + + +@pytest.fixture +def cli_state(tmp_path, monkeypatch): + # One custom endpoint, routed through the registry. + monkeypatch.setattr(registry_mod, "REGISTRIES_DIR", tmp_path) + (tmp_path / "dev.json").write_text(json.dumps({"endpoints": [{ + "entity_set_name": "engines", + "api_publisher": "beautech", "api_group": "technical", "api_version": "v1.0", + }]})) + state._config = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={"dev": BCProfile( + tenant_id="t1", environment="Sandbox", company_id="c-default", + companies={"LLC": CompanyAlias(id="c-llc", name="LLC"), + "GMBH": CompanyAlias(id="c-gmbh", name="GmbH")}, + )}, + ) + state._registry = None + state.profile_name = None + state.env_override = None + state.company_override = None + yield state + state._config = None + state._registry = None + + +def _pages(fake_bc, *pages): + """Queue paged replies: every page but the last carries a nextLink.""" + for i, records in enumerate(pages): + body = {"value": records} + if i < len(pages) - 1: + body["@odata.nextLink"] = "https://api.businesscentral.dynamics.com/next" + fake_bc.reply(body) + + +def test_company_all_queries_each_company_on_the_registry_route(cli_state, fake_bc): + fake_bc.reply(*({"value": [{"no": str(i)}]} for i in range(3))) + records = asyncio.run(_execute_get_all_companies("engines", Query(), None)) + + urls = [s.url for s in fake_bc.sent] + assert len(urls) == 3 # LLC, GMBH, default + assert all("/api/beautech/technical/v1.0/" in u for u in urls) + assert {u.split("companies(")[1].split(")")[0] for u in urls} == {"c-llc", "c-gmbh", "c-default"} + assert {r["_company"] for r in records} == {"LLC", "GMBH", "default"} + + +def test_company_all_explicit_route_wins_over_registry(cli_state, fake_bc): + fake_bc.default = {"value": []} + asyncio.run(_execute_get_all_companies( + "engines", Query(), None, publisher="other", group="grp", version="v2.0", + )) + assert all("/api/other/grp/v2.0/" in s.url for s in fake_bc.sent) + + +def test_company_all_without_all_reads_first_page_only(cli_state, fake_bc): + state._config.profiles["dev"].companies = {} + _pages(fake_bc, [{"no": "1"}], [{"no": "2"}]) + records = asyncio.run(_execute_get_all_companies("engines", Query(), None)) + assert [r["no"] for r in records] == ["1"] + + +def test_company_all_with_all_follows_next_link(cli_state, fake_bc): + state._config.profiles["dev"].companies = {} + _pages(fake_bc, [{"no": "1"}], [{"no": "2"}]) + records = asyncio.run(_execute_get_all_companies("engines", Query(), None, all_pages=True)) + assert [r["no"] for r in records] == ["1", "2"] + + +def test_all_pages_keeps_top_skip_and_count(cli_state, fake_bc): + fake_bc.default = {"value": [{"no": "1"}]} + query = Query().filter("no eq '1'").top(5).skip(10).count() + asyncio.run(_execute_get("engines", None, query, True)) + params = fake_bc.sent[0].params + assert params["$top"] == "5" + assert params["$skip"] == "10" + assert params["$count"] == "true" + assert "no eq '1'" in params["$filter"] diff --git a/tests/test_cli/test_patch_cmd.py b/tests/test_cli/test_patch_cmd.py index bbb47fa..6f53172 100644 --- a/tests/test_cli/test_patch_cmd.py +++ b/tests/test_cli/test_patch_cmd.py @@ -11,7 +11,6 @@ import json from pathlib import Path -from unittest.mock import AsyncMock import pytest import typer @@ -50,14 +49,10 @@ def cli_state(): @pytest.fixture -def fake_client(monkeypatch): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.patch = AsyncMock(return_value={"result": "ok"}) - c._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" - monkeypatch.setattr(state, "make_async_client", lambda **_: c) - return c +def bc(fake_bc): + """The real client against the fake transport; BC answers ``{"result": "ok"}``.""" + fake_bc.default = {"result": "ok"} + return fake_bc @pytest.fixture(autouse=True) @@ -88,27 +83,29 @@ def _run( class TestValidData: - def test_inline_json_literal_parsed(self, cli_state, fake_client): - _run(data='{"a": 1, "b": "two"}') - body_arg = fake_client.patch.await_args.args[2] - assert body_arg == {"a": 1, "b": "two"} - - def test_data_from_file(self, cli_state, fake_client, tmp_path: Path): + def test_inline_json_literal_parsed(self, cli_state, bc): + _run(data='{"a": 1, "b": "two"}', etag='W/"abc"') + assert bc.last.method == "PATCH" + assert bc.last.url.endswith("/companies(c-123)/vendors(vnd-1)") + assert bc.last.etag == 'W/"abc"' + assert bc.last.json_body == {"a": 1, "b": "two"} + + def test_data_from_file(self, cli_state, bc, tmp_path: Path): f = tmp_path / "payload.json" f.write_text('{"loaded": "from-file"}', encoding="utf-8") _run(data=f"@{f}") - body_arg = fake_client.patch.await_args.args[2] + body_arg = bc.last.json_body assert body_arg == {"loaded": "from-file"} class TestMalformedData: - def test_malformed_inline_json_raises_bad_parameter(self, cli_state, fake_client): + def test_malformed_inline_json_raises_bad_parameter(self, cli_state, bc): with pytest.raises(typer.BadParameter): _run(data="{not valid json") - fake_client.patch.assert_not_called() + assert bc.sent == [] def test_bare_file_path_without_at_prefix_hints_the_fix( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "payload.json" f.write_text('{"x": 1}', encoding="utf-8") @@ -116,27 +113,27 @@ def test_bare_file_path_without_at_prefix_hints_the_fix( _run(data=str(f)) assert "looks like a file path" in str(exc_info.value) assert f"-d @{f}" in str(exc_info.value) - fake_client.patch.assert_not_called() + assert bc.sent == [] - def test_missing_at_file_keeps_file_not_found(self, cli_state, fake_client): + def test_missing_at_file_keeps_file_not_found(self, cli_state, bc): with pytest.raises(typer.BadParameter) as exc_info: _run(data="@/no/such/file.json") assert "File not found" in str(exc_info.value) - fake_client.patch.assert_not_called() + assert bc.sent == [] def test_malformed_at_file_names_the_file( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "bad.json" f.write_text("{not valid json", encoding="utf-8") with pytest.raises(typer.BadParameter) as exc_info: _run(data=f"@{f}") assert str(f) in str(exc_info.value) - fake_client.patch.assert_not_called() + assert bc.sent == [] class TestEnvelope: - def test_envelope_written_on_success(self, cli_state, fake_client, tmp_path: Path): + def test_envelope_written_on_success(self, cli_state, bc, tmp_path: Path): out = tmp_path / "env.json" _run(result_out=out) env = json.loads(out.read_text()) diff --git a/tests/test_cli/test_post_cmd.py b/tests/test_cli/test_post_cmd.py index 33f4421..dd5d845 100644 --- a/tests/test_cli/test_post_cmd.py +++ b/tests/test_cli/test_post_cmd.py @@ -11,7 +11,6 @@ import json from pathlib import Path -from unittest.mock import AsyncMock import pytest import typer @@ -50,14 +49,10 @@ def cli_state(): @pytest.fixture -def fake_client(monkeypatch): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"result": "ok"}) - c._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" - monkeypatch.setattr(state, "make_async_client", lambda **_: c) - return c +def bc(fake_bc): + """The real client against the fake transport; BC answers ``{"result": "ok"}``.""" + fake_bc.default = {"result": "ok"} + return fake_bc @pytest.fixture(autouse=True) @@ -78,28 +73,29 @@ def _run(*, endpoint="vendors", data='{"displayName": "Acme"}', yes=True, **kwar class TestValidData: - def test_inline_json_literal_parsed(self, cli_state, fake_client): + def test_inline_json_literal_parsed(self, cli_state, bc): _run(data='{"a": 1, "b": "two"}') - body_arg = fake_client.post.await_args.args[1] - assert body_arg == {"a": 1, "b": "two"} + assert bc.last.method == "POST" + assert bc.last.url.endswith("/companies(c-123)/vendors") + assert bc.last.json_body == {"a": 1, "b": "two"} - def test_data_from_file(self, cli_state, fake_client, tmp_path: Path): + def test_data_from_file(self, cli_state, bc, tmp_path: Path): f = tmp_path / "payload.json" f.write_text('{"loaded": "from-file"}', encoding="utf-8") _run(data=f"@{f}") - body_arg = fake_client.post.await_args.args[1] + body_arg = bc.last.json_body assert body_arg == {"loaded": "from-file"} class TestMalformedData: - def test_malformed_inline_json_raises_bad_parameter(self, cli_state, fake_client): + def test_malformed_inline_json_raises_bad_parameter(self, cli_state, bc): with pytest.raises(typer.BadParameter): _run(data="{not valid json") # Must fail before any network call goes out. - fake_client.post.assert_not_called() + assert bc.sent == [] def test_bare_file_path_without_at_prefix_hints_the_fix( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "payload.json" f.write_text('{"x": 1}', encoding="utf-8") @@ -107,30 +103,35 @@ def test_bare_file_path_without_at_prefix_hints_the_fix( _run(data=str(f)) assert "looks like a file path" in str(exc_info.value) assert f"-d @{f}" in str(exc_info.value) - fake_client.post.assert_not_called() + assert bc.sent == [] - def test_missing_at_file_keeps_file_not_found(self, cli_state, fake_client): + def test_missing_at_file_keeps_file_not_found(self, cli_state, bc): with pytest.raises(typer.BadParameter) as exc_info: _run(data="@/no/such/file.json") assert "File not found" in str(exc_info.value) - fake_client.post.assert_not_called() + assert bc.sent == [] def test_malformed_at_file_names_the_file( - self, cli_state, fake_client, tmp_path: Path, + self, cli_state, bc, tmp_path: Path, ): f = tmp_path / "bad.json" f.write_text("{not valid json", encoding="utf-8") with pytest.raises(typer.BadParameter) as exc_info: _run(data=f"@{f}") assert str(f) in str(exc_info.value) - fake_client.post.assert_not_called() + assert bc.sent == [] class TestEnvelope: - def test_envelope_written_on_success(self, cli_state, fake_client, tmp_path: Path): + def test_envelope_written_on_success(self, cli_state, bc, tmp_path: Path): out = tmp_path / "env.json" _run(result_out=out) env = json.loads(out.read_text()) assert env["status"] == "succeeded" assert env["method"] == "POST" assert env["endpoint"] == "vendors" + + def test_envelope_carries_bc_correlation_id(self, cli_state, bc, tmp_path: Path): + out = tmp_path / "env.json" + _run(result_out=out) + assert json.loads(out.read_text())["bc_correlation_id"] == "corr-test" diff --git a/tests/test_cli/test_state.py b/tests/test_cli/test_state.py index ee7b2af..dd5a855 100644 --- a/tests/test_cli/test_state.py +++ b/tests/test_cli/test_state.py @@ -55,7 +55,7 @@ def test_make_async_client_applies_company_override(state_with_config): assert client.profile.company_name == "LLC Co" # URL resolution uses the overridden company_id. - url = client._resolve_url("customers") + url = client.resolve("customers") assert "companies(llc-guid)" in url finally: # No transport was built (lazy), but calling close is still safe. @@ -68,7 +68,7 @@ def test_make_async_client_no_override_uses_default(state_with_config): client = state_with_config.make_async_client() try: assert client.profile.company_id == "default-guid" - url = client._resolve_url("customers") + url = client.resolve("customers") assert "companies(default-guid)" in url finally: import asyncio @@ -82,7 +82,7 @@ def test_make_async_client_env_override(state_with_config): client = state_with_config.make_async_client() try: assert client.profile.environment == "Sandbox" - url = client._resolve_url("customers") + url = client.resolve("customers") assert "/Sandbox/" in url finally: import asyncio diff --git a/tests/test_client/test_resolve_url.py b/tests/test_client/test_resolve_url.py index 12e95df..0ead599 100644 --- a/tests/test_client/test_resolve_url.py +++ b/tests/test_client/test_resolve_url.py @@ -34,13 +34,13 @@ class TestDisableStandardApiLockdown: def test_unknown_entity_blocked_when_standard_disabled(self): client = _make_client(disable_standard=True) with pytest.raises(RegistryError, match="disable_standard_api"): - client._resolve_url("salesInvoices") + client.resolve("salesInvoices") def test_error_message_actionable(self): """The error should point the user at concrete next commands.""" client = _make_client(disable_standard=True) with pytest.raises(RegistryError) as excinfo: - client._resolve_url("vendors") + client.resolve("vendors") msg = str(excinfo.value) assert "bcli endpoint list" in msg assert "bcli registry import" in msg @@ -49,7 +49,7 @@ def test_error_message_actionable(self): def test_explicit_override_still_allowed(self): """Explicit publisher/group/version is the documented escape hatch.""" client = _make_client(disable_standard=True) - url = client._resolve_url( + url = client.resolve( "salesInvoices", publisher="acme", group="finance", version="v1.5", ) @@ -59,12 +59,12 @@ def test_explicit_override_still_allowed(self): def test_unknown_entity_routes_to_v20_when_standard_enabled(self): """Default profile (standard catalog enabled): legacy fallback still works.""" client = _make_client(disable_standard=False) - url = client._resolve_url("vendors") + url = client.resolve("vendors") assert "/api/v2.0/" in url assert url.endswith("vendors") def test_known_standard_entity_routes_correctly(self): """Standard v2.0 catalog → /api/v2.0/ when not disabled.""" client = _make_client(disable_standard=False) - url = client._resolve_url("customers") + url = client.resolve("customers") assert "/api/v2.0/" in url diff --git a/tests/test_client/test_safety.py b/tests/test_client/test_safety.py index fd7323f..d4b79d7 100644 --- a/tests/test_client/test_safety.py +++ b/tests/test_client/test_safety.py @@ -2,8 +2,6 @@ from __future__ import annotations -from unittest.mock import AsyncMock, MagicMock - import pytest from bcli.client._async import AsyncBCClient @@ -11,31 +9,19 @@ from bcli.errors import SafetyError -def _mock_client() -> MagicMock: - """Create a mock AsyncBCClient with the surface SafeContext touches. +def _mock_client() -> AsyncBCClient: + """A real programmatic client bound to Production + company-PROD. - Post v0004, SafeContext talks to the underlying transport directly via - ``client._ensure_transport()`` and resolves URLs via - ``client._resolve_url_for_target(env, company_id, ...)``. The mock has - to expose both, plus return a transport with async post/patch/delete. + Tests that send pair it with ``fake_bc`` so the wire URL can be inspected; + the profile target is deliberately *not* what SafeContext binds to. """ - client = MagicMock() - transport = MagicMock() - transport.post = AsyncMock(return_value={"id": "new-1", "status": "Draft"}) - transport.patch = AsyncMock(return_value={"id": "1", "name": "Updated"}) - transport.delete = AsyncMock(return_value={}) - client._ensure_transport.return_value = transport - # Default URL resolver returns a sentinel string per call so tests can - # distinguish post/patch/delete URLs. - client._resolve_url_for_target = MagicMock( - side_effect=lambda env, cid, entity, **kw: ( - f"https://api.test/{env}/{cid}/{entity}" - + (f"({kw['record_id']})" if kw.get("record_id") else "") - ) + return AsyncBCClient( + tenant_id="tenant-1", + client_id="client-1", + client_secret="secret-1", + environment="Production", + company_id="company-PROD", ) - # Keep a handle to transport for assertions. - client._mock_transport = transport - return client # ── Construction safety checks ──────────────────────────────────────────── @@ -97,18 +83,18 @@ async def test_async_context_manager(self): # ── Domain rules ────────────────────────────────────────────────────────── class TestDomainRules: - async def test_finance_domain_adds_draft_status(self): + async def test_finance_domain_adds_draft_status(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-1", ) as sw: await sw.post("salesInvoices", body={"customerNumber": "10000"}, domain="finance") - # The body passed to transport.post should have status=Draft. - body = client._mock_transport.post.call_args.kwargs["json_body"] + # The body sent to BC should have status=Draft. + body = fake_bc.last.json_body assert body["status"] == "Draft" - async def test_finance_domain_preserves_explicit_status(self): + async def test_finance_domain_preserves_explicit_status(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-1", @@ -119,20 +105,20 @@ async def test_finance_domain_preserves_explicit_status(self): domain="finance", ) - body = client._mock_transport.post.call_args.kwargs["json_body"] + body = fake_bc.last.json_body assert body["status"] == "Open" - async def test_standard_domain_no_draft(self): + async def test_standard_domain_no_draft(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-1", ) as sw: await sw.post("items", body={"name": "Widget"}, domain="standard") - body = client._mock_transport.post.call_args.kwargs["json_body"] + body = fake_bc.last.json_body assert "status" not in body - async def test_custom_domain_rules(self): + async def test_custom_domain_rules(self, fake_bc): client = _mock_client() custom_rules = { "hr": DomainRule(allow_write=False), @@ -145,9 +131,9 @@ async def test_custom_domain_rules(self): await sw.post("employees", body={"name": "Test"}, domain="hr") # Disallowed domain must not reach the transport. - client._mock_transport.post.assert_not_called() + assert fake_bc.sent == [] - async def test_unknown_domain_uses_default_rule(self): + async def test_unknown_domain_uses_default_rule(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-1", @@ -155,7 +141,7 @@ async def test_unknown_domain_uses_default_rule(self): await sw.post("customEntities", body={"key": "val"}, domain="custom") # Unknown domain gets DomainRule() defaults — allow_write=True, no draft. - client._mock_transport.post.assert_called_once() + assert [r.method for r in fake_bc.sent] == ["POST"] # ── Target binding (vuln-0004 regression) ───────────────────────────────── @@ -166,63 +152,55 @@ async def test_unknown_domain_uses_default_rule(self): # went to whatever the client profile pointed at — defeating the safety # guarantee documented in the README and changelog. # -# These tests assert that the URL resolver is invoked with the SafeContext -# target and that the transport sees the resulting URL. +# These tests drive the real client and assert on the URL BC receives. class TestSafeContextTargetBinding: - async def test_post_uses_explicit_target(self): + async def test_post_uses_explicit_target(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-SANDBOX", ) as sw: await sw.post("items", body={"name": "Widget"}) - client._resolve_url_for_target.assert_called_once_with( - "Sandbox", "c-SANDBOX", "items", - publisher=None, group=None, version=None, + sent = fake_bc.last + assert sent.method == "POST" + assert sent.url == client.resolve( + "items", environment="Sandbox", company_id="c-SANDBOX", ) - url = client._mock_transport.post.call_args.args[0] - assert "Sandbox" in url and "c-SANDBOX" in url - assert client._mock_transport.post.call_args.kwargs == { - "json_body": {"name": "Widget"}, - } + assert sent.json_body == {"name": "Widget"} + assert sent.etag is None - async def test_patch_uses_explicit_target(self): + async def test_patch_uses_explicit_target(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-SANDBOX", ) as sw: await sw.patch("items", "item-1", body={"name": "Updated"}) - client._resolve_url_for_target.assert_called_once_with( - "Sandbox", "c-SANDBOX", "items", - record_id="item-1", - publisher=None, group=None, version=None, + sent = fake_bc.last + assert sent.method == "PATCH" + assert sent.url == client.resolve( + "items", record_id="item-1", environment="Sandbox", company_id="c-SANDBOX", ) - url = client._mock_transport.patch.call_args.args[0] - assert "Sandbox" in url and "c-SANDBOX" in url - assert client._mock_transport.patch.call_args.kwargs == { - "json_body": {"name": "Updated"}, - "etag": "*", - } + assert sent.json_body == {"name": "Updated"} + assert sent.etag == "*" - async def test_delete_uses_explicit_target(self): + async def test_delete_uses_explicit_target(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-SANDBOX", ) as sw: await sw.delete("items", "item-1") - client._resolve_url_for_target.assert_called_once_with( - "Sandbox", "c-SANDBOX", "items", - record_id="item-1", - publisher=None, group=None, version=None, + sent = fake_bc.last + assert sent.method == "DELETE" + assert sent.url == client.resolve( + "items", record_id="item-1", environment="Sandbox", company_id="c-SANDBOX", ) - url = client._mock_transport.delete.call_args.args[0] - assert "Sandbox" in url and "c-SANDBOX" in url - assert client._mock_transport.delete.call_args.kwargs == {"etag": "*"} + assert sent.json_body is None + assert sent.etag == "*" - async def test_route_override_is_propagated(self): + async def test_route_override_is_propagated(self, fake_bc): client = _mock_client() async with SafeContext( client=client, environment="Sandbox", company_id="c-SANDBOX", @@ -232,21 +210,22 @@ async def test_route_override_is_propagated(self): publisher="contoso", group="sales", version="v2.0", ) - client._resolve_url_for_target.assert_called_once_with( - "Sandbox", "c-SANDBOX", "customEntities", - publisher="contoso", group="sales", version="v2.0", - ) + url = fake_bc.last.url + assert "/api/contoso/sales/v2.0/" in url + assert "/Sandbox/" in url and "c-SANDBOX" in url - async def test_does_not_call_profile_bound_write_methods(self): + async def test_does_not_call_profile_bound_write_methods(self, fake_bc, monkeypatch): """Defense-in-depth: the profile-bound client.post/patch/delete must never be invoked from SafeContext, otherwise wrong-environment writes could regress silently. """ client = _mock_client() - # Make profile-bound methods loud. - client.post = AsyncMock(side_effect=AssertionError("client.post called")) - client.patch = AsyncMock(side_effect=AssertionError("client.patch called")) - client.delete = AsyncMock(side_effect=AssertionError("client.delete called")) + + async def boom(*a, **kw): + raise AssertionError("profile-bound write method called") + + for name in ("post", "patch", "delete"): + monkeypatch.setattr(client, name, boom) async with SafeContext( client=client, environment="Sandbox", company_id="c-SANDBOX", @@ -255,67 +234,49 @@ async def test_does_not_call_profile_bound_write_methods(self): await sw.patch("items", "item-1", body={"name": "X"}) await sw.delete("items", "item-1") + assert [r.method for r in fake_bc.sent] == ["POST", "PATCH", "DELETE"] + # ── End-to-end target binding through the real AsyncBCClient ────────────── # # The strix PoC built a real ``AsyncBCClient`` bound to Production + # company-PROD, opened ``client.safe_write("Sandbox", "company-SANDBOX")``, # and observed that the URLs handed to the transport still contained the -# Production target. This class exercises the same path with a stub -# transport so the URL used at the wire level can be inspected. +# Production target. This class exercises the same path through ``fake_bc`` +# so the URL used at the wire level can be inspected. class TestEndToEndTargetBinding: - @pytest.fixture - def stub_transport(self) -> MagicMock: - t = MagicMock() - t.post = AsyncMock(return_value={"id": "created"}) - t.patch = AsyncMock(return_value={"id": "updated"}) - t.delete = AsyncMock(return_value={}) - return t - - def _make_client(self, transport: MagicMock) -> AsyncBCClient: - client = AsyncBCClient( - tenant_id="tenant-1", - client_id="client-1", - client_secret="secret-1", - environment="Production", - company_id="company-PROD", - ) - # Don't make a real auth/HTTP call. - client._ensure_transport = lambda: transport # type: ignore[method-assign] - return client - - async def test_post_url_targets_safe_context_not_profile(self, stub_transport): - client = self._make_client(stub_transport) + async def test_post_url_targets_safe_context_not_profile(self, fake_bc): + client = _mock_client() async with client.safe_write( "Sandbox", "company-SANDBOX", confirm_production=False, ) as sw: await sw.post("items", {"displayName": "poc"}) - url = stub_transport.post.call_args.args[0] + url = fake_bc.last.url assert "/Sandbox/" in url, f"expected Sandbox in URL, got: {url}" assert "company-SANDBOX" in url, f"expected company-SANDBOX in URL, got: {url}" assert "/Production/" not in url, f"profile env leaked into URL: {url}" assert "company-PROD" not in url, f"profile company leaked into URL: {url}" - async def test_patch_url_targets_safe_context_not_profile(self, stub_transport): - client = self._make_client(stub_transport) + async def test_patch_url_targets_safe_context_not_profile(self, fake_bc): + client = _mock_client() async with client.safe_write("Sandbox", "company-SANDBOX") as sw: await sw.patch("items", "record-1", {"displayName": "patched"}) - url = stub_transport.patch.call_args.args[0] + url = fake_bc.last.url assert "/Sandbox/" in url assert "company-SANDBOX" in url assert url.endswith("items(record-1)") assert "/Production/" not in url assert "company-PROD" not in url - async def test_delete_url_targets_safe_context_not_profile(self, stub_transport): - client = self._make_client(stub_transport) + async def test_delete_url_targets_safe_context_not_profile(self, fake_bc): + client = _mock_client() async with client.safe_write("Sandbox", "company-SANDBOX") as sw: await sw.delete("items", "record-1") - url = stub_transport.delete.call_args.args[0] + url = fake_bc.last.url assert "/Sandbox/" in url assert "company-SANDBOX" in url assert url.endswith("items(record-1)") diff --git a/tests/test_client/test_transport.py b/tests/test_client/test_transport.py index 20a37b3..4a36858 100644 --- a/tests/test_client/test_transport.py +++ b/tests/test_client/test_transport.py @@ -66,6 +66,17 @@ def test_extracts_message_and_correlation_id(self): assert msg == "Bad filter syntax" assert cid == "abc-123" + def test_bc_request_id_is_the_correlation_id(self): + # What BC actually sends (seen on SBEnvQD): no x-ms-correlation-request-id; + # the error message's "CorrelationId: …" equals the request-id header. + resp = _make_response( + 404, + json_body={"error": {"message": "Not found. CorrelationId: feb0b37c."}}, + headers={"request-id": "feb0b37c", "ms-correlation-x": "8931dbed"}, + ) + _, cid = _parse_bc_error(resp) + assert cid == "feb0b37c" + def test_empty_body(self): resp = _make_response(500, content=b"") msg, cid = _parse_bc_error(resp) diff --git a/tests/test_client/test_write_operation.py b/tests/test_client/test_write_operation.py new file mode 100644 index 0000000..4749b29 --- /dev/null +++ b/tests/test_client/test_write_operation.py @@ -0,0 +1,140 @@ +"""The SDK write operation: ``client.plan(...)`` then ``await client.write(plan)``. + +Also covers the read-side helpers that share its resolution path: +``client.resolve`` and ``client.pages``. +""" + +from __future__ import annotations + +import json + +import pytest + +import bcli.registry._registry as registry_mod +from bcli.client._async import AsyncBCClient +from bcli.config._model import AuditConfig, BCConfig, BCDefaults, BCProfile +from bcli.errors import ServerError +from bcli.odata._query import Query + + +@pytest.fixture +def client(tmp_path, monkeypatch) -> AsyncBCClient: + """Profile ``dev`` with one custom route (``engines``) and audit on.""" + monkeypatch.setattr(registry_mod, "REGISTRIES_DIR", tmp_path) + (tmp_path / "dev.json").write_text(json.dumps({"endpoints": [{ + "entity_set_name": "engines", + "api_publisher": "beautech", "api_group": "technical", "api_version": "v1.0", + "supports": ["GET", "POST", "PATCH", "DELETE"], + }]})) + config = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={"dev": BCProfile(tenant_id="t1", environment="Sandbox", company_id="c-dev")}, + audit=AuditConfig(enabled=True, path=str(tmp_path / "audit-{profile}.jsonl")), + ) + return AsyncBCClient(profile="dev", config=config) + + +def _audit(tmp_path) -> list[dict]: + path = tmp_path / "audit-dev.jsonl" + return [json.loads(line) for line in path.read_text().splitlines()] if path.exists() else [] + + +# ── plan / write ────────────────────────────────────────────────────── + + +async def test_plan_resolves_once_and_write_sends_that_url(client, fake_bc, monkeypatch): + calls = [] + real_resolve = client.resolve + monkeypatch.setattr( + client, "resolve", lambda *a, **kw: calls.append(a) or real_resolve(*a, **kw), + ) + + plan = client.plan("PATCH", "engines", record_id="e-1", body={"x": 1}, etag="W/1") + await client.write(plan) + + assert len(calls) == 1 + assert fake_bc.last.url == plan.url + assert "/api/beautech/technical/v1.0/" in plan.url + assert plan.url.endswith("engines(e-1)") + assert (fake_bc.last.method, fake_bc.last.json_body, fake_bc.last.etag) == ( + "PATCH", {"x": 1}, "W/1", + ) + + +async def test_write_returns_status_and_correlation_id(client, fake_bc): + fake_bc.reply({"id": "e-9"}) + fake_bc.status = 201 + fake_bc.correlation_id = "corr-201" + + result = await client.write(client.plan("POST", "engines", body={"esn": "1"})) + + assert result.body == {"id": "e-9"} + assert result.status == 201 + assert result.correlation_id == "corr-201" + assert fake_bc.last.etag is None # POST carries no If-Match + + +async def test_write_emits_one_completed_audit_entry(client, fake_bc, tmp_path): + fake_bc.status = 201 + fake_bc.correlation_id = "corr-ok" + + plan = client.plan("POST", "engines", body={"esn": "1"}) + await client.write(plan) + + entries = _audit(tmp_path) + assert len(entries) == 1 + e = entries[0] + assert e["outcome"] == "completed" + assert e["status"] == 201 + assert e["correlation_id"] == "corr-ok" + assert e["resolved_url"] == plan.url + assert e["method"] == "POST" + assert e["caller"] == "sdk" + + +async def test_failed_write_emits_one_failed_audit_entry(client, fake_bc, tmp_path): + fake_bc.reply(ServerError("boom", status_code=500, correlation_id="corr-fail")) + + with pytest.raises(ServerError): + await client.write(client.plan("DELETE", "engines", record_id="e-1")) + + entries = _audit(tmp_path) + assert len(entries) == 1 + e = entries[0] + assert e["outcome"] == "failed" + assert e["status"] == 500 + assert e["correlation_id"] == "corr-fail" + assert "boom" in e["error"] + + +# ── pages / resolve ─────────────────────────────────────────────────── + + +async def test_pages_sends_top_skip_count(client, fake_bc): + fake_bc.reply({"value": [{"id": "1"}]}) + + pages = [p async for p in client.pages("engines", Query().top(5).skip(10).count())] + + assert pages == [[{"id": "1"}]] + params = fake_bc.last.params + assert params["$top"] == "5" + assert params["$skip"] == "10" + assert params["$count"] == "true" + + +async def test_pages_company_id_changes_company_in_url(client, fake_bc): + fake_bc.default = {"value": []} + + [p async for p in client.pages("engines", company_id="c-other")] + + assert "companies(c-other)" in fake_bc.last.url + assert "c-dev" not in fake_bc.last.url + + +def test_explicit_route_override_wins_over_registry(client): + assert "/api/beautech/technical/v1.0/" in client.resolve("engines") + + url = client.resolve("engines", publisher="other", group="grp", version="v2.0") + + assert "/api/other/grp/v2.0/" in url + assert "beautech" not in url diff --git a/tests/test_envelope/conftest.py b/tests/test_envelope/conftest.py index 9ca9ed6..03dfa31 100644 --- a/tests/test_envelope/conftest.py +++ b/tests/test_envelope/conftest.py @@ -113,7 +113,7 @@ def fake_client(): c.upload_attachment = AsyncMock( return_value={"id": "att-9", "fileName": "x.pdf", "byteSize": 12} ) - c._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" + c.resolve = lambda entity, **kw: f"https://example.test/{entity}" return c diff --git a/tests/test_envelope/test_batch_envelope_with_ledger.py b/tests/test_envelope/test_batch_envelope_with_ledger.py index 046328d..e3be0b3 100644 --- a/tests/test_envelope/test_batch_envelope_with_ledger.py +++ b/tests/test_envelope/test_batch_envelope_with_ledger.py @@ -9,7 +9,7 @@ 2. **Partial commit.** One step succeeds, the next raises → envelope ``status="failed"``, ledger ``state="partially_committed"``. 3. **Policy refusal.** ``disable_writes=true`` + non-interactive + - no ``--yes`` → envelope ``status="failed"`` with ``exit_code=1``; + no ``--yes`` → envelope ``status="failed"`` with ``exit_code=8`` (EXIT_POLICY); the ledger row exists (``start_run`` ran) and is finalized as ``failed`` (no committed steps). @@ -25,7 +25,6 @@ import sys import textwrap from pathlib import Path -from unittest.mock import AsyncMock, patch import pytest import typer @@ -95,21 +94,6 @@ def readonly_state(): state._telemetry = None -@pytest.fixture -def fake_client(): - c = AsyncMock() - c.__aenter__ = AsyncMock(return_value=c) - c.__aexit__ = AsyncMock(return_value=False) - c.post = AsyncMock(return_value={"id": "rec-1", "@odata.context": "x"}) - c.patch = AsyncMock(return_value={"id": "rec-1"}) - c.delete = AsyncMock(return_value=None) - c.get = AsyncMock(return_value=type("R", (), {"value": [], "raw": None})()) - c._resolve_url = lambda entity, record_id=None, **_: ( - f"https://x/{entity}({record_id})" if record_id else f"https://x/{entity}" - ) - return c - - def _write_yaml(tmp_path: Path, name: str, content: str) -> Path: f = tmp_path / name f.write_text(textwrap.dedent(content).strip(), encoding="utf-8") @@ -137,7 +121,7 @@ class TestEnvelopeAndLedgerAgreeOnSuccess: uses to drill into per-step detail).""" def test_success_envelope_matches_ledger_run_id( - self, writable_state, isolated_home, fake_client, tmp_path, + self, writable_state, isolated_home, fake_bc, tmp_path, ): yaml_file = _write_yaml(tmp_path, "ok.yaml", """ name: happy @@ -149,15 +133,11 @@ def test_success_envelope_matches_ledger_run_id( """) out = tmp_path / "envelope.json" - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - batch_cmd.run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - result_out=out, result_fd=None, - ) + batch_cmd.run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=True, + result_out=out, result_fd=None, + ) env = json.loads(out.read_text()) assert env["status"] == "succeeded" @@ -170,6 +150,7 @@ def test_success_envelope_matches_ledger_run_id( assert derived["state"] == "completed" # The envelope's record_id is the ledger run id — agents pivot here. assert env["record_id"] == run_row["run_id"] + assert [s.method for s in fake_bc.sent] == ["POST"] class TestEnvelopeAndLedgerAgreeOnPartialCommit: @@ -178,17 +159,9 @@ class TestEnvelopeAndLedgerAgreeOnPartialCommit: landed before the crash.""" def test_partial_commit_envelope_failed_ledger_partial( - self, writable_state, isolated_home, fake_client, tmp_path, + self, writable_state, isolated_home, fake_bc, tmp_path, ): - call_count = {"n": 0} - - async def post_side_effect(entity, body, **kwargs): - call_count["n"] += 1 - if call_count["n"] == 2: - raise RuntimeError("boom on step 2") - return {"id": f"rec-{call_count['n']}"} - - fake_client.post.side_effect = post_side_effect + fake_bc.reply({"id": "rec-1"}, RuntimeError("boom on step 2")) yaml_file = _write_yaml(tmp_path, "partial.yaml", """ name: partial @@ -204,16 +177,12 @@ async def post_side_effect(entity, body, **kwargs): """) out = tmp_path / "envelope.json" - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit) as excinfo: - batch_cmd.run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=True, - result_out=out, result_fd=None, - ) + with pytest.raises(typer.Exit) as excinfo: + batch_cmd.run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=True, + result_out=out, result_fd=None, + ) assert excinfo.value.exit_code == 1 env = json.loads(out.read_text()) @@ -238,7 +207,7 @@ class TestEnvelopeOnPolicyRefusalForBatch: """ def test_policy_refusal_emits_failed_envelope_and_failed_ledger( - self, readonly_state, isolated_home, fake_client, tmp_path, monkeypatch, + self, readonly_state, isolated_home, fake_bc, tmp_path, monkeypatch, ): # Force non-interactive stdin so confirm_write_or_exit refuses. monkeypatch.setattr(sys.stdin, "isatty", lambda: False) @@ -253,16 +222,12 @@ def test_policy_refusal_emits_failed_envelope_and_failed_ledger( """) out = tmp_path / "envelope.json" - with patch( - "bcli_cli.commands.batch_cmd.state.make_async_client", - return_value=fake_client, - ): - with pytest.raises(typer.Exit) as excinfo: - batch_cmd.run_batch( - file=yaml_file, dry_run=False, output=None, format=None, - set_params=None, params_file=None, yes=False, - result_out=out, result_fd=None, - ) + with pytest.raises(typer.Exit) as excinfo: + batch_cmd.run_batch( + file=yaml_file, dry_run=False, output=None, format=None, + set_params=None, params_file=None, yes=False, + result_out=out, result_fd=None, + ) assert excinfo.value.exit_code == 8 # AIP §Phase 4a: EXIT_POLICY env = json.loads(out.read_text()) @@ -283,4 +248,4 @@ def test_policy_refusal_emits_failed_envelope_and_failed_ledger( assert steps == [] # And the actual write was never issued. - fake_client.post.assert_not_awaited() + assert fake_bc.sent == [] diff --git a/tests/test_envelope/test_envelope_other_verbs.py b/tests/test_envelope/test_envelope_other_verbs.py index c65d289..895f208 100644 --- a/tests/test_envelope/test_envelope_other_verbs.py +++ b/tests/test_envelope/test_envelope_other_verbs.py @@ -24,6 +24,13 @@ def force_non_interactive(monkeypatch): monkeypatch.setattr(sys.stdin, "isatty", lambda: False) +@pytest.fixture +def bc(cli_state, fake_bc): + """Writable profile + the real client against the fake transport.""" + fake_bc.default = {"systemId": "vnd-9", "displayName": "Acme2"} + return fake_bc + + # ── PATCH ────────────────────────────────────────────────────────────── @@ -46,20 +53,28 @@ def _run_patch(**kwargs): class TestPatchEnvelope: - def test_patch_writes_envelope(self, stub_client, tmp_path: Path): + def test_patch_writes_envelope(self, bc, tmp_path: Path): + # An API keyed on No. returns a systemId that differs from the key we sent; + # the envelope keeps the key the caller addressed. + bc.default = {"systemId": "vnd-9", "displayName": "Renamed"} out = tmp_path / "out.json" _run_patch(result_out=out) + assert bc.last.method == "PATCH" + assert bc.last.url.endswith("/companies(c-123)/vendors(vnd-1)") + assert bc.last.json_body == {"displayName": "Renamed"} env = json.loads(out.read_text()) + assert env["resolved_url"] == bc.last.url + assert env["bc_correlation_id"] == "corr-test" assert env["method"] == "PATCH" assert env["endpoint"] == "vendors" assert env["record_id"] == "vnd-1" assert env["status"] == "succeeded" assert env["exit_code"] == 0 - def test_patch_envelope_on_failure(self, stub_client, tmp_path: Path): - stub_client.patch.side_effect = ValidationError( + def test_patch_envelope_on_failure(self, bc, tmp_path: Path): + bc.reply(ValidationError( "etag mismatch", status_code=412, correlation_id="corr-patch", - ) + )) out = tmp_path / "out.json" with pytest.raises(typer.Exit): _run_patch(result_out=out) @@ -90,19 +105,24 @@ def _run_delete(**kwargs): class TestDeleteEnvelope: - def test_delete_writes_envelope(self, stub_client, tmp_path: Path): + def test_delete_writes_envelope(self, bc, tmp_path: Path): + bc.default = {} out = tmp_path / "out.json" _run_delete(result_out=out) + assert bc.last.method == "DELETE" + assert bc.last.url.endswith("/companies(c-123)/vendors(vnd-1)") + assert bc.last.json_body is None + assert bc.last.etag == "*" env = json.loads(out.read_text()) assert env["method"] == "DELETE" assert env["endpoint"] == "vendors" assert env["record_id"] == "vnd-1" assert env["status"] == "succeeded" - def test_delete_envelope_on_failure(self, stub_client, tmp_path: Path): - stub_client.delete.side_effect = ValidationError( + def test_delete_envelope_on_failure(self, bc, tmp_path: Path): + bc.reply(ValidationError( "not found", status_code=404, correlation_id="corr-del", - ) + )) out = tmp_path / "out.json" with pytest.raises(typer.Exit): _run_delete(result_out=out) @@ -195,7 +215,7 @@ def _run_batch(file: Path, **kwargs): class TestBatchRunEnvelope: def test_batch_run_writes_envelope_all_ok( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): yaml_file = _write_batch_yaml(tmp_path, """ name: ok @@ -207,6 +227,9 @@ def test_batch_run_writes_envelope_all_ok( """) out = tmp_path / "out.json" _run_batch(yaml_file, result_out=out) + assert [(r.method, r.json_body) for r in bc.sent] == [ + ("POST", {"displayName": "Acme"}), + ] env = json.loads(out.read_text()) assert env["method"] == "BATCH_RUN" # Batch envelope identifies the manifest stem, not a single endpoint @@ -222,11 +245,11 @@ def test_batch_run_writes_envelope_all_ok( assert env["resolved_url"] is None def test_batch_run_envelope_failed_when_any_step_fails( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): - stub_client.post.side_effect = ValidationError( + bc.reply(ValidationError( "boom", status_code=400, correlation_id="corr-batch", - ) + )) yaml_file = _write_batch_yaml(tmp_path, """ name: oops steps: diff --git a/tests/test_envelope/test_envelope_policy_violation.py b/tests/test_envelope/test_envelope_policy_violation.py index 32ce748..be25724 100644 --- a/tests/test_envelope/test_envelope_policy_violation.py +++ b/tests/test_envelope/test_envelope_policy_violation.py @@ -56,9 +56,8 @@ def _assert_policy_failure_envelope(envelope_path: Path, *, method: str, endpoin class TestPostEnvelopeOnPolicyViolation: def test_post_emits_failed_envelope_when_disable_writes_blocks( - self, readonly_state, fake_client, monkeypatch, tmp_path: Path, + self, readonly_state, fake_bc, tmp_path: Path, ): - monkeypatch.setattr(state, "make_async_client", lambda **_: fake_client) out = tmp_path / "post-refused.json" with pytest.raises(typer.Exit) as excinfo: post_cmd.post_command( @@ -74,14 +73,13 @@ def test_post_emits_failed_envelope_when_disable_writes_blocks( ) assert excinfo.value.exit_code == 8 _assert_policy_failure_envelope(out, method="POST", endpoint="vendors") - fake_client.post.assert_not_awaited() + assert fake_bc.sent == [] class TestPatchEnvelopeOnPolicyViolation: def test_patch_emits_failed_envelope_when_disable_writes_blocks( - self, readonly_state, fake_client, monkeypatch, tmp_path: Path, + self, readonly_state, fake_bc, tmp_path: Path, ): - monkeypatch.setattr(state, "make_async_client", lambda **_: fake_client) out = tmp_path / "patch-refused.json" with pytest.raises(typer.Exit) as excinfo: patch_cmd.patch_command( @@ -103,14 +101,13 @@ def test_patch_emits_failed_envelope_when_disable_writes_blocks( # is now inside the capture block. env = json.loads(out.read_text()) assert env["record_id"] == "vnd-1" - fake_client.patch.assert_not_awaited() + assert fake_bc.sent == [] class TestDeleteEnvelopeOnPolicyViolation: def test_delete_emits_failed_envelope_when_disable_writes_blocks( - self, readonly_state, fake_client, monkeypatch, tmp_path: Path, + self, readonly_state, fake_bc, tmp_path: Path, ): - monkeypatch.setattr(state, "make_async_client", lambda **_: fake_client) out = tmp_path / "delete-refused.json" with pytest.raises(typer.Exit) as excinfo: delete_cmd.delete_command( @@ -129,7 +126,7 @@ def test_delete_emits_failed_envelope_when_disable_writes_blocks( _assert_policy_failure_envelope(out, method="DELETE", endpoint="vendors") env = json.loads(out.read_text()) assert env["record_id"] == "vnd-1" - fake_client.delete.assert_not_awaited() + assert fake_bc.sent == [] class TestAttachUploadEnvelopeOnPolicyViolation: diff --git a/tests/test_envelope/test_envelope_post.py b/tests/test_envelope/test_envelope_post.py index 4cfafba..6c0f213 100644 --- a/tests/test_envelope/test_envelope_post.py +++ b/tests/test_envelope/test_envelope_post.py @@ -18,7 +18,6 @@ import json import os from pathlib import Path -from unittest.mock import AsyncMock import pytest import typer @@ -37,6 +36,16 @@ def force_non_interactive(monkeypatch): monkeypatch.setattr(sys.stdin, "isatty", lambda: False) +@pytest.fixture +def bc(cli_state, fake_bc): + """Writable profile + the real client against the fake transport. + + BC answers with a ``systemId`` so the envelope can pick a record_id. + """ + fake_bc.default = {"systemId": "vnd-9", "displayName": "Acme"} + return fake_bc + + def _run_post(endpoint="vendors", data='{"displayName": "Acme"}', **kwargs): kwargs.setdefault("format", None) kwargs.setdefault("publisher", None) @@ -53,7 +62,7 @@ def _run_post(endpoint="vendors", data='{"displayName": "Acme"}', **kwargs): class TestEnvelopeOnSuccess: - def test_writes_envelope_to_path(self, stub_client, tmp_path: Path): + def test_writes_envelope_to_path(self, bc, tmp_path: Path): out = tmp_path / "out.json" _run_post(result_out=out) assert out.is_file() @@ -64,7 +73,7 @@ def test_writes_envelope_to_path(self, stub_client, tmp_path: Path): assert env["status"] == "succeeded" assert env["exit_code"] == 0 - def test_envelope_has_all_spec_fields(self, stub_client, tmp_path: Path): + def test_envelope_has_all_spec_fields(self, bc, tmp_path: Path): out = tmp_path / "out.json" _run_post(result_out=out) env = json.loads(out.read_text()) @@ -80,7 +89,7 @@ def test_envelope_has_all_spec_fields(self, stub_client, tmp_path: Path): assert len(env["invocation_id"]) >= 16 def test_envelope_captures_profile_environment_company( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): out = tmp_path / "out.json" _run_post(result_out=out) @@ -90,43 +99,43 @@ def test_envelope_captures_profile_environment_company( assert env["company"] == "c-123" def test_record_id_extracted_from_systemid( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): - stub_client.post.return_value = {"systemId": "vnd-12345", "x": 1} + bc.default = {"systemId": "vnd-12345", "x": 1} out = tmp_path / "out.json" _run_post(result_out=out) env = json.loads(out.read_text()) assert env["record_id"] == "vnd-12345" def test_record_id_falls_back_to_id_field( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): - stub_client.post.return_value = {"id": "vnd-fallback"} + bc.default = {"id": "vnd-fallback"} out = tmp_path / "out.json" _run_post(result_out=out) env = json.loads(out.read_text()) assert env["record_id"] == "vnd-fallback" def test_record_id_null_when_response_has_no_id_field( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): - stub_client.post.return_value = {"opaqueResult": True} + bc.default = {"opaqueResult": True} out = tmp_path / "out.json" _run_post(result_out=out) env = json.loads(out.read_text()) assert env["record_id"] is None def test_resolved_url_populated_on_success( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): out = tmp_path / "out.json" _run_post(result_out=out) env = json.loads(out.read_text()) - # try_resolve_url -> AsyncBCClient._resolve_url stub returned this - assert env["resolved_url"] == "https://example.test/vendors" + assert env["resolved_url"] == bc.last.url + assert env["resolved_url"].endswith("/companies(c-123)/vendors") def test_started_at_and_duration_present( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): out = tmp_path / "out.json" _run_post(result_out=out) @@ -136,7 +145,7 @@ def test_started_at_and_duration_present( assert env["duration_ms"] >= 0 def test_dry_run_field_false_on_real_write( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): out = tmp_path / "out.json" _run_post(result_out=out) @@ -145,7 +154,7 @@ def test_dry_run_field_false_on_real_write( class TestAtomicity: - def test_uses_os_replace(self, stub_client, tmp_path: Path, monkeypatch): + def test_uses_os_replace(self, bc, tmp_path: Path, monkeypatch): out = tmp_path / "out.json" real_replace = os.replace calls: list[tuple[str, str]] = [] @@ -163,7 +172,7 @@ def spy(src, dst): class TestStdoutUnaffected: def test_stdout_does_not_contain_envelope( - self, stub_client, tmp_path: Path, capsys, + self, bc, tmp_path: Path, capsys, ): """The envelope writes to its own channel — stdout still follows ``--format`` (here: default table render). Critically, none of the @@ -178,7 +187,7 @@ def test_stdout_does_not_contain_envelope( class TestResultFdMode: - def test_writes_envelope_to_fd(self, stub_client, tmp_path: Path): + def test_writes_envelope_to_fd(self, bc, tmp_path: Path): r, w = os.pipe() try: _run_post(result_fd=w) @@ -197,19 +206,19 @@ def test_writes_envelope_to_fd(self, stub_client, tmp_path: Path): class TestMutualExclusion: def test_result_out_and_result_fd_mutually_exclusive( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): with pytest.raises(typer.BadParameter): _run_post(result_out=tmp_path / "x.json", result_fd=2) class TestEnvelopeOnFailure: - def test_envelope_on_http_4xx(self, stub_client, tmp_path: Path): - stub_client.post.side_effect = ValidationError( + def test_envelope_on_http_4xx(self, bc, tmp_path: Path): + bc.reply(ValidationError( "field 'displayName' missing", status_code=400, correlation_id="corr-400", - ) + )) out = tmp_path / "out.json" with pytest.raises(typer.Exit): _run_post(result_out=out) @@ -219,12 +228,12 @@ def test_envelope_on_http_4xx(self, stub_client, tmp_path: Path): assert env["exit_code"] == EXIT_REMOTE_4XX assert env["bc_correlation_id"] == "corr-400" - def test_envelope_on_http_5xx(self, stub_client, tmp_path: Path): - stub_client.post.side_effect = ServerError( + def test_envelope_on_http_5xx(self, bc, tmp_path: Path): + bc.reply(ServerError( "boom", status_code=502, correlation_id="corr-5xx", - ) + )) out = tmp_path / "out.json" with pytest.raises(typer.Exit): _run_post(result_out=out) @@ -233,8 +242,8 @@ def test_envelope_on_http_5xx(self, stub_client, tmp_path: Path): assert env["exit_code"] == EXIT_REMOTE_5XX assert env["bc_correlation_id"] == "corr-5xx" - def test_envelope_on_generic_exception(self, stub_client, tmp_path: Path): - stub_client.post.side_effect = RuntimeError("oops") + def test_envelope_on_generic_exception(self, bc, tmp_path: Path): + bc.reply(RuntimeError("oops")) out = tmp_path / "out.json" with pytest.raises(typer.Exit): _run_post(result_out=out) @@ -245,41 +254,26 @@ def test_envelope_on_generic_exception(self, stub_client, tmp_path: Path): class TestEnvelopeOnDryRun: - def test_dry_run_writes_envelope_no_http( - self, cli_state, stub_resolve_url, tmp_path: Path, monkeypatch, - ): + def test_dry_run_writes_envelope_no_http(self, bc, tmp_path: Path): """``--dry-run`` short-circuits HTTP but still drops an envelope so an agent driving the CLI can verify the request shape was correct without parsing stdout prose.""" - called = {"post": False} - - async def _no_call(*a, **kw): - called["post"] = True - return {} - - fake = AsyncMock() - fake.__aenter__ = AsyncMock(return_value=fake) - fake.__aexit__ = AsyncMock(return_value=False) - fake.post = _no_call - fake._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" - monkeypatch.setattr(state, "make_async_client", lambda **_: fake) - state.dry_run = True out = tmp_path / "out.json" with pytest.raises(typer.Exit) as exc: _run_post(result_out=out) - # render_dry_run uses typer.Exit(); exit_code attr will be 0/None assert (exc.value.exit_code or 0) == 0 - assert not called["post"], "Dry-run must NOT issue the HTTP call" + assert bc.sent == [], "Dry-run must NOT issue the HTTP call" env = json.loads(out.read_text()) assert env["dry_run"] is True assert env["status"] == "succeeded" assert env["exit_code"] == 0 + assert env["resolved_url"].endswith("/companies(c-123)/vendors") class TestAddedFlagIsOptional: def test_envelope_not_written_when_neither_flag_set( - self, stub_client, tmp_path: Path, + self, bc, tmp_path: Path, ): """Backward-compat — calling post_command without the envelope flags leaves no envelope file anywhere. The flag is strictly opt-in.""" diff --git a/tests/test_etl/test_bridge.py b/tests/test_etl/test_bridge.py index 3a2747a..d1133f6 100644 --- a/tests/test_etl/test_bridge.py +++ b/tests/test_etl/test_bridge.py @@ -2,10 +2,14 @@ from __future__ import annotations +import json + import pytest pytest.importorskip("dlt") +import bcli.registry._registry as registry_mod +from bcli.errors import ConfigError from bcli.etl._bridge import ( bcli_profile, load_entities_from_bcli_registry, @@ -13,46 +17,73 @@ from bcli.etl._generic import EntityDef +@pytest.fixture(autouse=True) +def _hermetic_config(tmp_path, monkeypatch): + """Two profiles with one custom endpoint each; never the developer's real config. + + ``sandbox`` sees standard v2.0 plus its custom route; ``locked`` has + ``disable_standard_api = true``. + """ + config_file = tmp_path / "config.toml" + config_file.write_text( + '[defaults]\nprofile = "sandbox"\n\n' + '[profiles.sandbox]\ntenant_id = "t1"\nenvironment = "Sandbox"\n\n' + '[profiles.locked]\ntenant_id = "t1"\nenvironment = "Sandbox"\n' + "disable_standard_api = true\n" + ) + monkeypatch.setattr("bcli.config._loader.CONFIG_FILE", config_file) + monkeypatch.setattr("bcli.config._loader._find_project_config", lambda: None) + registries = tmp_path / "registries" + registries.mkdir() + monkeypatch.setattr(registry_mod, "REGISTRIES_DIR", registries) + endpoint = { + "entity_set_name": "engines", + "api_publisher": "beautech", "api_group": "technical", "api_version": "v1.0", + } + for name in ("sandbox", "locked"): + (registries / f"{name}.json").write_text(json.dumps({"endpoints": [endpoint]})) + + class TestLoadEntitiesFromBcliRegistry: def test_loads_custom_endpoints(self): - """Smoke test — loads from sandbox registry if it exists on this machine.""" entities = load_entities_from_bcli_registry("sandbox", custom_only=True) - if not entities: - pytest.skip("No sandbox registry present — developer-local smoke test") + assert [e.name for e in entities] == ["engines"] assert all(isinstance(e, EntityDef) for e in entities) # Custom-only → all should have a publisher for e in entities: assert e.api_publisher is not None - def test_nonexistent_profile_returns_empty(self): - entities = load_entities_from_bcli_registry("nonexistent_xyz_profile", custom_only=True) - assert entities == [] + def test_nonexistent_profile_raises(self): + # The registry is now the profile's own (via AsyncBCClient), so an + # unknown profile fails loudly instead of yielding an empty list. + with pytest.raises(ConfigError, match="nonexistent_xyz_profile"): + load_entities_from_bcli_registry("nonexistent_xyz_profile", custom_only=True) def test_entities_have_systemModifiedAt_cursor(self): entities = load_entities_from_bcli_registry("sandbox", custom_only=True) - if not entities: - pytest.skip("No sandbox registry") + assert entities for e in entities: assert e.cursor_field == "systemModifiedAt" + def test_standard_included_when_not_custom_only(self): + entities = load_entities_from_bcli_registry("sandbox", custom_only=False) + assert len(entities) > 1 + assert any(e.api_publisher is None for e in entities) + + def test_disable_standard_api_yields_only_custom(self): + entities = load_entities_from_bcli_registry("locked", custom_only=False) + assert [e.name for e in entities] == ["engines"] + class TestBcliProfileSource: def test_requires_valid_profile(self): - with pytest.raises(Exception): + with pytest.raises(ConfigError): bcli_profile("nonexistent_xyz_profile") def test_unknown_entity_raises(self): - # Use a real profile if available, otherwise skip - entities = load_entities_from_bcli_registry("sandbox", custom_only=True) - if not entities: - pytest.skip("No sandbox registry") with pytest.raises(ValueError, match="Unknown entities"): bcli_profile("sandbox", entities=["definitely_not_an_entity_xyz"]) def test_filters_to_subset(self): - entities = load_entities_from_bcli_registry("sandbox", custom_only=True) - if not entities: - pytest.skip("No sandbox registry") - name = entities[0].name - source = bcli_profile("sandbox", entities=[name]) - assert name in {r.name for r in source.resources.values()} + source = bcli_profile("sandbox", entities=["engines"]) + assert "engines" in {r.name for r in source.resources.values()} diff --git a/tests/test_etl/test_generic.py b/tests/test_etl/test_generic.py index fa033b2..7ce2bb8 100644 --- a/tests/test_etl/test_generic.py +++ b/tests/test_etl/test_generic.py @@ -1,11 +1,11 @@ -"""Tests for the generic ETL layer — no bcli coupling.""" +"""Tests for the generic ETL layer — no profile/registry coupling.""" from __future__ import annotations -import json import sys -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock +import httpx import pytest pytest.importorskip("dlt") @@ -80,51 +80,95 @@ async def _token(): # ─── BCClient ──────────────────────────────────────────────────────── +_BASE = ( + "https://api.businesscentral.dynamics.com/v2.0/Sandbox/api/v2.0/" + "companies(00000000-0000-0000-0000-000000000000)/customers" +) + + +def _serve(client: BCClient, *responses: httpx.Response) -> list[httpx.Request]: + """Answer the client's requests from ``responses`` in order; return what was sent.""" + seen: list[httpx.Request] = [] + queue = list(responses) + + def handle(request: httpx.Request) -> httpx.Response: + seen.append(request) + return queue.pop(0) + + client._t()._client = httpx.AsyncClient(transport=httpx.MockTransport(handle)) + return seen + + @pytest.mark.asyncio async def test_bcclient_get_injects_bearer(): auth = StaticTokenAuth(lambda: _async_val("tok-1")) async with BCClient(auth=auth, environment="Sandbox") as client: - mock_response = AsyncMock() - mock_response.status_code = 200 - mock_response.content = b'{"value": []}' - mock_response.json = lambda: {"value": []} - with patch.object(client._http, "request", return_value=mock_response) as mock_req: - await client.get("https://api.businesscentral.dynamics.com/v2.0/Sandbox/api/v2.0/companies(00000000-0000-0000-0000-000000000000)/customers") - call = mock_req.await_args - assert call.kwargs["headers"]["Authorization"] == "Bearer tok-1" + seen = _serve(client, httpx.Response(200, json={"value": []})) + await client.get(_BASE) + assert seen[0].headers["Authorization"] == "Bearer tok-1" + + +@pytest.mark.asyncio +async def test_bcclient_accepts_bcli_auth_provider(): + """A bcli provider (``get_access_token``) is used as-is, no adapter.""" + auth = AsyncMock() + auth.get_access_token = AsyncMock(return_value="tok-bcli") + + async with BCClient(auth=auth, environment="Sandbox") as client: + seen = _serve(client, httpx.Response(200, json={"value": []})) + await client.get(_BASE) + assert seen[0].headers["Authorization"] == "Bearer tok-bcli" @pytest.mark.asyncio async def test_bcclient_404_raises_not_found(): auth = StaticTokenAuth(lambda: _async_val("tok")) async with BCClient(auth=auth, environment="Sandbox") as client: - mock_response = AsyncMock() - mock_response.status_code = 404 - mock_response.content = b"" - with patch.object(client._http, "request", return_value=mock_response): - with pytest.raises(NotFoundError): - await client.get("https://api.businesscentral.dynamics.com/v2.0/Sandbox/api/v2.0/companies(00000000-0000-0000-0000-000000000000)/customers") + _serve(client, httpx.Response(404, json={"error": {"code": "NotFound", "message": "x"}})) + with pytest.raises(NotFoundError): + await client.get(_BASE) @pytest.mark.asyncio async def test_bcclient_paginates_nextlink(): auth = StaticTokenAuth(lambda: _async_val("tok")) async with BCClient(auth=auth, environment="Sandbox") as client: - base = ( - "https://api.businesscentral.dynamics.com/v2.0/Sandbox/api/v2.0/" - "companies(00000000-0000-0000-0000-000000000000)/customers" + seen = _serve( + client, + httpx.Response(200, json={ + "value": [{"id": "1"}, {"id": "2"}], + "@odata.nextLink": f"{_BASE}?$skiptoken=p2", + }), + httpx.Response(200, json={"value": [{"id": "3"}]}), + ) + results = [page async for page in client.paginate(_BASE, params={"$top": "2"})] + assert results == [[{"id": "1"}, {"id": "2"}], [{"id": "3"}]] + assert seen[0].url.params["$top"] == "2" + assert seen[1].url.params["$skiptoken"] == "p2" + + +@pytest.mark.asyncio +async def test_bcclient_refuses_off_origin_nextlink(): + """An off-origin ``@odata.nextLink`` never receives the bearer token.""" + auth = StaticTokenAuth(lambda: _async_val("tok")) + async with BCClient(auth=auth, environment="Sandbox") as client: + seen = _serve(client, httpx.Response(200, json={ + "value": [{"id": "1"}], + "@odata.nextLink": "https://attacker.example/leak", + })) + with pytest.raises(ValueError, match="off-origin"): + [page async for page in client.paginate(_BASE)] + assert [str(r.url.host) for r in seen] == ["api.businesscentral.dynamics.com"] + + +def test_build_entity_url_rejects_bad_segment(): + with pytest.raises(ValueError): + build_entity_url( + environment="Sandbox", + company_id="00000000-0000-0000-0000-000000000000", + entity_set_name="customers/../../evil", ) - page1 = _mock_ok_response({ - "value": [{"id": "1"}, {"id": "2"}], - "@odata.nextLink": f"{base}?$skiptoken=p2", - }) - page2 = _mock_ok_response({"value": [{"id": "3"}]}) - with patch.object(client._http, "request", side_effect=[page1, page2]): - results = [page async for page in client.paginate(f"{base}?$skiptoken=p1")] - assert len(results) == 2 - assert results[0] == [{"id": "1"}, {"id": "2"}] - assert results[1] == [{"id": "3"}] # ─── business_central source ───────────────────────────────────────── @@ -165,11 +209,13 @@ def test_builds_with_credentials(self): class TestGenericHasNoBcliCoupling: - """The generic layer must be importable with only bcli.etl modules loaded. + """The generic layer never sees a profile, registry, config or bcli auth. - If any of _generic.py, _client.py, _auth.py, _stampers.py, - _stamper_factory.py imports from bcli.registry, bcli.client, - bcli.config, or bcli.auth, this test fails. + ``_client.py`` runs on bcli's transport, so it alone may import the + transport-level modules (``bcli._url``, ``bcli.client._transport``, + ``bcli.errors``, ``bcli.odata``). Nothing in the generic layer may import + ``bcli.registry``, ``bcli.config`` or ``bcli.auth``, nor the client + package beyond its transport. """ def test_generic_does_not_import_bcli_sdk(self): @@ -180,21 +226,20 @@ def test_generic_does_not_import_bcli_sdk(self): etl_dir = pathlib.Path( sys.modules["bcli.etl"].__file__ # type: ignore[arg-type] ).parent - forbidden_prefixes = ("bcli.registry", "bcli.client", "bcli.config", "bcli.auth", "bcli.errors") + transport_only = ("bcli._url", "bcli.client._transport", "bcli.errors", "bcli.odata") for module_name in ("_generic.py", "_client.py", "_auth.py", "_stampers.py", "_stamper_factory.py"): - path = etl_dir / module_name - tree = ast.parse(path.read_text()) + allowed = ("bcli.etl",) + (transport_only if module_name == "_client.py" else ()) + tree = ast.parse((etl_dir / module_name).read_text()) for node in ast.walk(tree): + names = [] if isinstance(node, ast.ImportFrom) and node.module: - assert not node.module.startswith(forbidden_prefixes), ( - f"{module_name} imports forbidden {node.module}" - ) - if isinstance(node, ast.Import): - for alias in node.names: - assert not alias.name.startswith(forbidden_prefixes), ( - f"{module_name} imports forbidden {alias.name}" - ) + names = [node.module] + elif isinstance(node, ast.Import): + names = [alias.name for alias in node.names] + for name in names: + if name == "bcli" or name.startswith("bcli."): + assert name.startswith(allowed), f"{module_name} imports forbidden {name}" # ─── Helpers ───────────────────────────────────────────────────────── @@ -202,12 +247,3 @@ def test_generic_does_not_import_bcli_sdk(self): async def _async_val(v): return v - - -def _mock_ok_response(body: dict): - from unittest.mock import AsyncMock - m = AsyncMock() - m.status_code = 200 - m.content = json.dumps(body).encode() - m.json = lambda: body - return m diff --git a/tests/test_idempotency/test_cli_flags.py b/tests/test_idempotency/test_cli_flags.py index e6825bd..b29dac4 100644 --- a/tests/test_idempotency/test_cli_flags.py +++ b/tests/test_idempotency/test_cli_flags.py @@ -3,7 +3,7 @@ Pins that the four single-mutation commands (``post``, ``patch``, ``delete``, ``attach upload``) accept the flag and forward it to :class:`AsyncBCClient`. The transport-level header injection is -covered separately in ``test_idempotency_key.py``. +covered in ``tests/test_client/test_retry_idempotency.py``. Same-run replay protection through the batch ledger lives in ``tests/test_batch_ledger/test_ledger_idempotency.py``; cross-command @@ -12,124 +12,44 @@ from __future__ import annotations -from unittest.mock import AsyncMock +_COMMON = dict( + format=None, publisher=None, group=None, version=None, yes=True, + result_out=None, result_fd=None, +) -import typer - -from bcli_cli._state import state +def test_post_command_forwards_idempotency_key(writable_state, fake_bc): + from bcli_cli.commands import post_cmd -def _fake_client(monkeypatch): - """Stand up an AsyncBCClient stub that records every kwargs dict.""" - client = AsyncMock() + post_cmd.post_command( + endpoint="vendors", data='{"displayName": "Acme"}', + idempotency_key="op-key-1", **_COMMON, + ) - async def _ctx_enter(): - return client + assert fake_bc.last.method == "POST" + assert fake_bc.last.idempotency_key == "op-key-1" - async def _ctx_exit(*_): - return False - fake_cm = AsyncMock() - fake_cm.__aenter__ = AsyncMock(side_effect=_ctx_enter) - fake_cm.__aexit__ = AsyncMock(side_effect=_ctx_exit) - client.post = AsyncMock(return_value={"systemId": "rec-1", "id": "rec-1"}) - client.patch = AsyncMock(return_value={"systemId": "rec-1"}) - client.delete = AsyncMock(return_value={}) +def test_patch_command_forwards_idempotency_key(writable_state, fake_bc): + from bcli_cli.commands import patch_cmd - monkeypatch.setattr( - state, "make_async_client", lambda **_: fake_cm, + patch_cmd.patch_command( + endpoint="vendors", record_id="vnd-1", + data='{"displayName": "Renamed"}', etag="*", + idempotency_key="op-patch-1", **_COMMON, ) - # Make typer.Exit short-circuit cleanly; tests inspect the client. - return client - -def test_post_command_forwards_idempotency_key(writable_state, monkeypatch, tmp_path): - from bcli_cli.commands import post_cmd + assert fake_bc.last.method == "PATCH" + assert fake_bc.last.idempotency_key == "op-patch-1" - client = _fake_client(monkeypatch) - # Bypass print_context_banner / audit wrap noise. - monkeypatch.setattr("bcli_cli.commands.post_cmd.print_context_banner", lambda: None) - monkeypatch.setattr( - "bcli_cli.commands.post_cmd._audited_post", - lambda endpoint, body, **kw: client.post(endpoint, body, **kw), - ) - monkeypatch.setattr("bcli_cli.commands.post_cmd.format_output", lambda *a, **k: None) - monkeypatch.setattr(state, "dry_run", False) - - try: - post_cmd.post_command( - endpoint="vendors", - data='{"displayName": "Acme"}', - format=None, - publisher=None, group=None, version=None, - yes=True, - result_out=None, result_fd=None, - idempotency_key="op-key-1", - ) - except typer.Exit: - pass - - # Last call to the fake client carries the idempotency_key kw. - client.post.assert_awaited() - args, kwargs = client.post.call_args - assert kwargs.get("idempotency_key") == "op-key-1" - - -def test_patch_command_forwards_idempotency_key(writable_state, monkeypatch, tmp_path): - from bcli_cli.commands import patch_cmd - client = _fake_client(monkeypatch) - monkeypatch.setattr("bcli_cli.commands.patch_cmd.print_context_banner", lambda: None) - monkeypatch.setattr( - "bcli_cli.commands.patch_cmd._audited_patch", - lambda endpoint, record_id, body, **kw: client.patch(endpoint, record_id, body, **kw), - ) - monkeypatch.setattr("bcli_cli.commands.patch_cmd.format_output", lambda *a, **k: None) - monkeypatch.setattr(state, "dry_run", False) - - try: - patch_cmd.patch_command( - endpoint="vendors", record_id="vnd-1", - data='{"displayName": "Renamed"}', - etag="*", - format=None, - publisher=None, group=None, version=None, - yes=True, - result_out=None, result_fd=None, - idempotency_key="op-patch-1", - ) - except typer.Exit: - pass - - client.patch.assert_awaited() - args, kwargs = client.patch.call_args - assert kwargs.get("idempotency_key") == "op-patch-1" - - -def test_delete_command_forwards_idempotency_key(writable_state, monkeypatch): +def test_delete_command_forwards_idempotency_key(writable_state, fake_bc): from bcli_cli.commands import delete_cmd - client = _fake_client(monkeypatch) - monkeypatch.setattr("bcli_cli.commands.delete_cmd.print_context_banner", lambda: None) - monkeypatch.setattr( - "bcli_cli.commands.delete_cmd._audited_delete", - lambda endpoint, record_id, **kw: client.delete(endpoint, record_id, **kw), + delete_cmd.delete_command( + endpoint="vendors", record_id="vnd-1", etag="*", + idempotency_key="op-del-1", **_COMMON, ) - monkeypatch.setattr(state, "dry_run", False) - - try: - delete_cmd.delete_command( - endpoint="vendors", record_id="vnd-1", - etag="*", - format=None, - publisher=None, group=None, version=None, - yes=True, - result_out=None, result_fd=None, - idempotency_key="op-del-1", - ) - except typer.Exit: - pass - - client.delete.assert_awaited() - args, kwargs = client.delete.call_args - assert kwargs.get("idempotency_key") == "op-del-1" + + assert fake_bc.last.method == "DELETE" + assert fake_bc.last.idempotency_key == "op-del-1" diff --git a/tests/test_idempotency/test_idempotency_key.py b/tests/test_idempotency/test_idempotency_key.py index 04e1ded..a3dba73 100644 --- a/tests/test_idempotency/test_idempotency_key.py +++ b/tests/test_idempotency/test_idempotency_key.py @@ -1,4 +1,4 @@ -"""Transport sends the Idempotency-Key header when set on the client method. +"""The client hands the idempotency key to the transport on every write verb. This is the http-layer half of AIP §Phase 4d. The CLI half — flag plumbing, ledger persistence, same-run replay protection — lives in @@ -16,102 +16,45 @@ import asyncio -import httpx import pytest from bcli.client._async import AsyncBCClient -class _FakeTransport: - """Tiny stub that records request kwargs for assertion.""" - - def __init__(self) -> None: - self.calls: list[dict] = [] - - async def request(self, method, url, **kwargs): - self.calls.append( - {"method": method, "url": url, "headers": dict(kwargs.get("headers", {}))} - ) - # Return a minimal valid OData response. - return httpx.Response( - 201, json={"systemId": "rec-1", "id": "rec-1"}, request=httpx.Request(method, url), - ) +@pytest.fixture +def client(fake_bc): + """A real programmatic client whose transport is ``fake_bc``.""" + fake_bc.default = {"systemId": "rec-1", "id": "rec-1"} + return AsyncBCClient(tenant_id="t1", environment="Sandbox", company_id="c-123") -@pytest.fixture -def fake_client(monkeypatch): - """Build an AsyncBCClient wired to a fake httpx layer.""" - client = AsyncBCClient.__new__(AsyncBCClient) - # The minimal attributes the post/patch/delete paths touch. - client._closed = False # type: ignore[attr-defined] - transport_stub = _FakeTransport() - - # Patch _ensure_transport to return our stub for the post path. - class _StubTransport: - def __init__(self): - self._client = transport_stub - self.calls = transport_stub.calls - - async def post(self, url, *, json_body, idempotency_key=None): - headers = {} - if idempotency_key is not None: - headers["Idempotency-Key"] = idempotency_key - transport_stub.calls.append( - {"method": "POST", "url": url, "headers": headers, "json": json_body} - ) - return {"systemId": "rec-1", "id": "rec-1"} - - async def patch(self, url, *, json_body, etag="*", idempotency_key=None): - headers = {"If-Match": etag} - if idempotency_key is not None: - headers["Idempotency-Key"] = idempotency_key - transport_stub.calls.append( - {"method": "PATCH", "url": url, "headers": headers, "json": json_body} - ) - return {"systemId": "rec-1"} - - async def delete(self, url, *, etag="*", idempotency_key=None): - headers = {"If-Match": etag} - if idempotency_key is not None: - headers["Idempotency-Key"] = idempotency_key - transport_stub.calls.append( - {"method": "DELETE", "url": url, "headers": headers} - ) - return {} - - stub = _StubTransport() - monkeypatch.setattr(client, "_ensure_transport", lambda: stub, raising=False) - monkeypatch.setattr(client, "_resolve_url", lambda *a, **kw: "https://example.com/api/x", raising=False) - return client, stub - - -def test_post_passes_idempotency_key_to_transport(fake_client): - client, stub = fake_client +def test_post_passes_idempotency_key_to_transport(client, fake_bc): asyncio.run( client.post("vendors", {"name": "Acme"}, idempotency_key="op-abc") ) - assert stub.calls[-1]["method"] == "POST" - assert stub.calls[-1]["headers"].get("Idempotency-Key") == "op-abc" + assert fake_bc.last.method == "POST" + assert fake_bc.last.url.endswith("/companies(c-123)/vendors") + assert fake_bc.last.idempotency_key == "op-abc" -def test_post_without_idempotency_key_omits_header(fake_client): - client, stub = fake_client +def test_post_without_idempotency_key_omits_header(client, fake_bc): asyncio.run(client.post("vendors", {"name": "Acme"})) - assert "Idempotency-Key" not in stub.calls[-1]["headers"] + assert fake_bc.last.idempotency_key is None -def test_patch_passes_idempotency_key_to_transport(fake_client): - client, stub = fake_client +def test_patch_passes_idempotency_key_to_transport(client, fake_bc): asyncio.run( client.patch("vendors", "rec-1", {"name": "Renamed"}, idempotency_key="op-patch") ) - assert stub.calls[-1]["headers"].get("Idempotency-Key") == "op-patch" + assert fake_bc.last.method == "PATCH" + assert fake_bc.last.url.endswith("/vendors(rec-1)") + assert fake_bc.last.idempotency_key == "op-patch" -def test_delete_passes_idempotency_key_to_transport(fake_client): - client, stub = fake_client +def test_delete_passes_idempotency_key_to_transport(client, fake_bc): asyncio.run( client.delete("vendors", "rec-1", idempotency_key="op-del") ) - assert stub.calls[-1]["headers"].get("Idempotency-Key") == "op-del" + assert fake_bc.last.method == "DELETE" + assert fake_bc.last.idempotency_key == "op-del" diff --git a/tests/test_mcp/test_server_tools.py b/tests/test_mcp/test_server_tools.py index ded92bb..fe24c7a 100644 --- a/tests/test_mcp/test_server_tools.py +++ b/tests/test_mcp/test_server_tools.py @@ -6,12 +6,14 @@ 2. Builds the tool list from the describe output via ``_tool_generator.generate_tools``. 3. Registers each generated tool with FastMCP. Read tools shell out via - ``run_bcli_json`` and return parsed stdout. Mutating tools pass - ``--result-out ``, subprocess the CLI, and return the - envelope content as the tool result. - -These tests mock the describe subprocess + the bcli subprocesses so no -real bcli is invoked. + ``run_bcli_json`` and return parsed stdout. Record writes + (``bcli_post`` / ``bcli_patch`` / ``bcli_delete``) run in-process + through the SDK write operation. Other mutating tools (attach upload, + batch run) pass ``--result-out ``, subprocess the CLI, and + return the envelope content as the tool result. + +These tests mock the describe subprocess + the bcli subprocesses, and +drive in-process writes against ``fake_bc``, so no real bcli or BC is hit. """ from __future__ import annotations @@ -64,6 +66,20 @@ "supported_formats": ["json"], "emits_result_envelope": True, }, + { + "path": ["attach", "upload"], + "summary": "Attach a file to an existing record.", + "options": [ + {"name": "--file", "type": "str"}, + {"name": "--yes", "type": "bool"}, + ], + "positionals": [ + {"name": "endpoint", "type": "str", "required": True}, + ], + "effects": ["mutating"], + "supported_formats": ["json"], + "emits_result_envelope": True, + }, { "path": ["company", "list"], "summary": "List companies on the active environment.", @@ -80,6 +96,36 @@ } +@pytest.fixture +def mcp_config(tmp_path, monkeypatch): + """A hermetic config: ``dev`` (Sandbox), ``prod`` (Production) and a + read-only ``locked`` profile. Never the developer's real config.""" + config_file = tmp_path / "config.toml" + config_file.write_text( + '[defaults]\nprofile = "dev"\n\n' + '[profiles.dev]\ntenant_id = "t1"\nenvironment = "Sandbox"\ncompany_id = "c-dev"\n\n' + '[profiles.prod]\ntenant_id = "t1"\nenvironment = "Production"\ncompany_id = "c-prod"\n\n' + '[profiles.locked]\ntenant_id = "t1"\nenvironment = "Sandbox"\ncompany_id = "c-dev"\n' + "disable_writes = true\n" + ) + monkeypatch.setattr("bcli.config._loader.CONFIG_FILE", config_file) + monkeypatch.setattr("bcli.config._loader._find_project_config", lambda: None) + import bcli.registry._registry as registry_mod + + monkeypatch.setattr(registry_mod, "REGISTRIES_DIR", tmp_path / "registries") + + +@pytest.fixture +def no_subprocess(monkeypatch): + """In-process writes must never shell out.""" + def boom(*a, **kw): + raise AssertionError("record write shelled out to bcli") + + monkeypatch.setattr("subprocess.run", boom) + monkeypatch.setattr("bcli_mcp._server.run_bcli_with_envelope", boom) + monkeypatch.setattr("bcli_mcp._server.run_bcli_json", boom) + + @pytest.fixture def fresh_server(): """Force the server module to rebuild its tool list from a fixture @@ -95,7 +141,7 @@ class TestToolListMirrorsDescribe: def test_one_tool_per_command_in_describe(self, fresh_server): """Every command entry → one MCP tool. No extras, no missing.""" registered = set(fresh_server._tool_manager._tools.keys()) - assert registered == {"bcli_get", "bcli_post", "bcli_company_list"} + assert registered == {"bcli_get", "bcli_post", "bcli_attach_upload", "bcli_company_list"} def test_other_effects_excluded(self): """``effects: ["other"]`` (e.g. ``auth login``, ``config init``) @@ -150,7 +196,92 @@ async def test_profile_kwarg_passes_through(self, fresh_server): assert run.call_args.kwargs["profile"] == "prod" -# ── Tool invocation: mutating path ──────────────────────────────────── +# ── Tool invocation: record writes (in-process SDK) ─────────────────── + + +class TestSdkWriteInvocation: + @pytest.mark.asyncio + async def test_post_runs_in_process_and_returns_correlation_id( + self, fresh_server, mcp_config, fake_bc, no_subprocess, + ): + fake_bc.reply({"id": "vnd-9", "number": "V1"}) + fake_bc.status = 201 + fake_bc.correlation_id = "corr-abc" + + tool = fresh_server._tool_manager._tools["bcli_post"] + result = await tool.fn(endpoint="vendors", data='{"x":1}', yes=True) + + assert [(s.method, s.json_body) for s in fake_bc.sent] == [("POST", {"x": 1})] + assert result["bc_correlation_id"] == "corr-abc" + assert result["status"] == "succeeded" + assert result["exit_code"] == 0 + assert result["method"] == "POST" + assert result["endpoint"] == "vendors" + assert result["resolved_url"] == fake_bc.last.url + assert result["record_id"] == "vnd-9" + assert result["profile"] == "dev" + assert result["environment"] == "Sandbox" + assert result["company"] == "c-dev" + assert result["body"] == {"id": "vnd-9", "number": "V1"} + + @pytest.mark.asyncio + async def test_failed_write_raises_toolerror_with_correlation_id( + self, fresh_server, mcp_config, fake_bc, no_subprocess, + ): + """A BC error surfaces as an MCP ``ToolError`` quoting the + correlation id so the agent can cite it in a support ticket.""" + from mcp.server.fastmcp.exceptions import ToolError + + from bcli.errors import ValidationError + + fake_bc.reply(ValidationError("bad field", status_code=400, correlation_id="corr-xyz")) + tool = fresh_server._tool_manager._tools["bcli_post"] + with pytest.raises(ToolError) as excinfo: + await tool.fn(endpoint="vendors", data='{"x":1}', yes=True) + assert "corr-xyz" in str(excinfo.value) + + @pytest.mark.asyncio + async def test_profile_selects_write_target( + self, fresh_server, mcp_config, fake_bc, no_subprocess, + ): + tool = fresh_server._tool_manager._tools["bcli_post"] + result = await tool.fn(endpoint="vendors", data="{}", yes=True, profile="prod") + assert "/Production/" in fake_bc.last.url + assert "c-prod" in fake_bc.last.url + assert result["profile"] == "prod" + assert result["environment"] == "Production" + + @pytest.mark.asyncio + async def test_disable_writes_profile_requires_yes( + self, fresh_server, mcp_config, fake_bc, no_subprocess, + ): + from mcp.server.fastmcp.exceptions import ToolError + + tool = fresh_server._tool_manager._tools["bcli_post"] + with pytest.raises(ToolError, match="disable_writes"): + await tool.fn(endpoint="vendors", data="{}", profile="locked") + assert fake_bc.sent == [] + + await tool.fn(endpoint="vendors", data="{}", profile="locked", yes=True) + assert [s.method for s in fake_bc.sent] == ["POST"] + + +# ── Tool invocation: other mutating tools (subprocess + envelope) ───── + + +def _envelope(**overrides): + env = { + "version": "0.1", "invocation_id": "inv-1", "tool_version": "0.4.0", + "profile": "dev", "environment": "Sandbox", "company": "c-1", + "method": "UPLOAD", "endpoint": "salesInvoices", + "resolved_url": "https://x/salesInvoices", "record_id": "si-9", + "dry_run": False, "status": "succeeded", "exit_code": 0, + "bc_correlation_id": None, "telemetry_event_id": None, + "audit_log_offset": None, "started_at": "2026-05-17T00:00:00Z", + "duration_ms": 42, + } + env.update(overrides) + return env class TestMutatingToolInvocation: @@ -160,25 +291,16 @@ async def test_mutating_tool_passes_result_out_and_reads_envelope( ): """The server must pass ``--result-out `` and return the envelope content. Stdout is dropped (envelope has everything).""" - envelope = { - "version": "0.1", "invocation_id": "inv-1", "tool_version": "0.4.0", - "profile": "dev", "environment": "Sandbox", "company": "c-1", - "method": "POST", "endpoint": "vendors", - "resolved_url": "https://x/vendors", "record_id": "vnd-9", - "dry_run": False, "status": "succeeded", "exit_code": 0, - "bc_correlation_id": None, "telemetry_event_id": None, - "audit_log_offset": None, "started_at": "2026-05-17T00:00:00Z", - "duration_ms": 42, - } + envelope = _envelope() def _fake_run(argv, env, capture_envelope_path): # Write the envelope where the runner expects it. Path(capture_envelope_path).write_text(json.dumps(envelope)) return 0, "", "" - tool = fresh_server._tool_manager._tools["bcli_post"] + tool = fresh_server._tool_manager._tools["bcli_attach_upload"] with patch("bcli_mcp._server.run_bcli_with_envelope", side_effect=_fake_run): - result = await tool.fn(endpoint="vendors", data='{"x":1}', yes=True) + result = await tool.fn(endpoint="salesInvoices", file="a.pdf", yes=True) assert result == envelope @pytest.mark.asyncio @@ -188,24 +310,19 @@ async def test_failed_envelope_raises_toolerror(self, fresh_server, tmp_path): ``exit_code``, ``bc_correlation_id``, etc.""" from mcp.server.fastmcp.exceptions import ToolError - failed = { - "version": "0.1", "invocation_id": "inv-2", "tool_version": "0.4.0", - "profile": "dev", "environment": "Sandbox", "company": "c-1", - "method": "POST", "endpoint": "vendors", "resolved_url": None, - "record_id": None, "dry_run": False, "status": "failed", - "exit_code": 6, "bc_correlation_id": "corr-xyz", - "telemetry_event_id": None, "audit_log_offset": None, - "started_at": "2026-05-17T00:00:00Z", "duration_ms": 11, - } + failed = _envelope( + invocation_id="inv-2", resolved_url=None, record_id=None, + status="failed", exit_code=6, bc_correlation_id="corr-xyz", + ) def _fake_run(argv, env, capture_envelope_path): Path(capture_envelope_path).write_text(json.dumps(failed)) return 6, "", "" - tool = fresh_server._tool_manager._tools["bcli_post"] + tool = fresh_server._tool_manager._tools["bcli_attach_upload"] with patch("bcli_mcp._server.run_bcli_with_envelope", side_effect=_fake_run): with pytest.raises(ToolError) as excinfo: - await tool.fn(endpoint="vendors", data='{"x":1}', yes=True) + await tool.fn(endpoint="salesInvoices", file="a.pdf", yes=True) # The exception message includes the correlation id so the agent # can quote it back when raising a support ticket. assert "corr-xyz" in str(excinfo.value) @@ -221,61 +338,43 @@ async def test_mutating_tool_passes_clean_argv_to_runner( The runner-side contract is exercised in ``test_runner.py``; here we just pin the server side: it shouldn't be re-adding either flag itself.""" - envelope = { - "version": "0.1", "invocation_id": "x", "tool_version": "0.4.0", - "profile": "dev", "environment": "Sandbox", "company": "c-1", - "method": "POST", "endpoint": "vendors", "resolved_url": None, - "record_id": None, "dry_run": False, "status": "succeeded", - "exit_code": 0, "bc_correlation_id": None, - "telemetry_event_id": None, "audit_log_offset": None, - "started_at": "x", "duration_ms": 0, - } captured_argv = {} def _fake_run(argv, env, capture_envelope_path): captured_argv["v"] = argv - Path(capture_envelope_path).write_text(json.dumps(envelope)) + Path(capture_envelope_path).write_text(json.dumps(_envelope())) return 0, "", "" - tool = fresh_server._tool_manager._tools["bcli_post"] + tool = fresh_server._tool_manager._tools["bcli_attach_upload"] with patch("bcli_mcp._server.run_bcli_with_envelope", side_effect=_fake_run): - await tool.fn(endpoint="vendors", data="{}", yes=True) + await tool.fn(endpoint="salesInvoices", file="a.pdf", yes=True) argv = captured_argv["v"] # Positional first, options after. - assert argv[:2] == ["post", "vendors"] - assert "--data" in argv - assert argv[argv.index("--data") + 1] == "{}" + assert argv[:3] == ["attach", "upload", "salesInvoices"] + assert "--file" in argv + assert argv[argv.index("--file") + 1] == "a.pdf" # The server passes a clean argv — runner adds --result-out / --format. assert "--result-out" not in argv assert "--format" not in argv -# ── Profile passthrough on mutating tools ───────────────────────────── +# ── Profile passthrough on subprocess mutating tools ────────────────── class TestMutatingProfilePassthrough: @pytest.mark.asyncio async def test_profile_propagates_to_subprocess(self, fresh_server, tmp_path): - envelope = { - "version": "0.1", "invocation_id": "x", "tool_version": "0.4.0", - "profile": "prod", "environment": "Production", "company": "c-1", - "method": "POST", "endpoint": "v", "resolved_url": None, - "record_id": None, "dry_run": False, "status": "succeeded", - "exit_code": 0, "bc_correlation_id": None, - "telemetry_event_id": None, "audit_log_offset": None, - "started_at": "x", "duration_ms": 0, - } captured = {} def _fake_run(argv, env, capture_envelope_path): captured["argv"] = argv captured["env"] = env - Path(capture_envelope_path).write_text(json.dumps(envelope)) + Path(capture_envelope_path).write_text(json.dumps(_envelope(profile="prod"))) return 0, "", "" - tool = fresh_server._tool_manager._tools["bcli_post"] + tool = fresh_server._tool_manager._tools["bcli_attach_upload"] with patch("bcli_mcp._server.run_bcli_with_envelope", side_effect=_fake_run): - await tool.fn(endpoint="v", data="{}", yes=True, profile="prod") + await tool.fn(endpoint="v", file="a.pdf", yes=True, profile="prod") assert "--profile" in captured["argv"] assert captured["argv"][captured["argv"].index("--profile") + 1] == "prod" assert captured["env"]["BCLI_PROFILE"] == "prod" diff --git a/tests/test_progress/test_progress_events.py b/tests/test_progress/test_progress_events.py index 8217f05..cc82e3c 100644 --- a/tests/test_progress/test_progress_events.py +++ b/tests/test_progress/test_progress_events.py @@ -117,7 +117,7 @@ def test_batch_run_emits_step_events_to_progress_fd(tmp_path: Path, monkeypatch) # Build a fake async-with client that responds to .post. fake_client = AsyncMock() fake_client.post = AsyncMock(return_value={"systemId": "rec-1"}) - fake_client._resolve_url = lambda *a, **kw: "https://example.com/api/vendors" + fake_client.resolve = lambda *a, **kw: "https://example.com/api/vendors" class _CM: async def __aenter__(self_inner): diff --git a/tests/test_url/test_bound_action_resolve.py b/tests/test_url/test_bound_action_resolve.py index 25421c6..6af3cab 100644 --- a/tests/test_url/test_bound_action_resolve.py +++ b/tests/test_url/test_bound_action_resolve.py @@ -63,7 +63,7 @@ def test_bound_action_resolves_against_parent_standard_entity(self): """``customers()/Microsoft.NAV.archive`` — parent is in the standard v2.0 registry, so the URL should compose cleanly.""" client = _make_client(disable_standard=False) - url = client._resolve_url("customers(abc-123)/Microsoft.NAV.archive") + url = client.resolve("customers(abc-123)/Microsoft.NAV.archive") assert url.endswith( "/companies(company-1)/customers(abc-123)/Microsoft.NAV.archive" ), url @@ -81,7 +81,7 @@ def test_bound_action_resolves_against_parent_custom_entity(self): ), ], ) - url = client._resolve_url("widgets(42)/Microsoft.NAV.cancel") + url = client.resolve("widgets(42)/Microsoft.NAV.cancel") assert "/api/acme/custom/v1.0/" in url assert url.endswith("/widgets(42)/Microsoft.NAV.cancel") @@ -89,21 +89,21 @@ def test_bound_action_namespace_is_agnostic(self): """The validator must not hardcode ``Microsoft.NAV`` — any dotted qualified identifier should pass.""" client = _make_client() - url = client._resolve_url("customers(abc-123)/Custom.Ns.doSomething") + url = client.resolve("customers(abc-123)/Custom.Ns.doSomething") assert url.endswith("/customers(abc-123)/Custom.Ns.doSomething") def test_bound_action_with_quoted_key(self): """OData v4 string keys are single-quoted; the validator should not interpret the quotes — just pass them through.""" client = _make_client() - url = client._resolve_url("customers('ALFKI')/Microsoft.NAV.archive") + url = client.resolve("customers('ALFKI')/Microsoft.NAV.archive") assert url.endswith("/customers('ALFKI')/Microsoft.NAV.archive") def test_bound_action_with_composite_key(self): """OData v4 supports ``(k1='a',k2='b')`` composite keys — the validator should not over-validate the inside of the parens.""" client = _make_client() - url = client._resolve_url( + url = client.resolve( "items(k1='a',k2='b')/Microsoft.NAV.doStuff" ) assert url.endswith("/items(k1='a',k2='b')/Microsoft.NAV.doStuff") @@ -113,7 +113,7 @@ def test_bound_action_with_multi_segment_namespace(self): — the parse should still accept them, since ``Namespace`` is explicitly multi-part in the OData v4 spec.""" client = _make_client() - url = client._resolve_url("widgets(7)/A.B.C.doStuff") + url = client.resolve("widgets(7)/A.B.C.doStuff") assert url.endswith("/widgets(7)/A.B.C.doStuff") @@ -128,7 +128,7 @@ def test_disable_standard_api_blocks_unregistered_parent(self): catalogue disabled → reject.""" client = _make_client(disable_standard=True) with pytest.raises(RegistryError) as exc: - client._resolve_url("examples(99)/Microsoft.NAV.archive") + client.resolve("examples(99)/Microsoft.NAV.archive") # The error message should name the *parent* entity, not the # whole bound-action string. This is the actionable signal — an # operator needs to know which entry to add to the registry. @@ -154,7 +154,7 @@ def test_disable_standard_api_allows_registered_custom_parent(self): ), ], ) - url = client._resolve_url("widgets(7)/Custom.Ns.archive") + url = client.resolve("widgets(7)/Custom.Ns.archive") assert url.endswith("/widgets(7)/Custom.Ns.archive") @@ -168,7 +168,7 @@ class TestUnboundActionAtServiceRoot: def test_unbound_action_resolves_via_standard_route(self): client = _make_client(disable_standard=False) - url = client._resolve_url("Microsoft.NAV.refreshAll") + url = client.resolve("Microsoft.NAV.refreshAll") assert url.endswith( "/companies(company-1)/Microsoft.NAV.refreshAll" ), url @@ -178,7 +178,7 @@ def test_unbound_action_resolves_via_custom_route_override(self): """Explicit publisher/group/version routes the unbound action under the matching custom API path.""" client = _make_client(disable_standard=True) - url = client._resolve_url( + url = client.resolve( "Custom.Ns.refreshAll", publisher="acme", group="custom", version="v1.0", ) @@ -192,7 +192,7 @@ def test_unbound_action_blocked_when_disable_standard_api(self): v2.0 surface.""" client = _make_client(disable_standard=True) with pytest.raises(RegistryError) as exc: - client._resolve_url("Microsoft.NAV.refreshAll") + client.resolve("Microsoft.NAV.refreshAll") assert "disable_standard_api" in str(exc.value) assert "publisher" in str(exc.value).lower() @@ -200,5 +200,5 @@ def test_unbound_action_namespace_agnostic(self): """A non-Microsoft namespace must also route, not be rejected for ``not yet supported``.""" client = _make_client(disable_standard=False) - url = client._resolve_url("Custom.Ns.doStuff") + url = client.resolve("Custom.Ns.doStuff") assert url.endswith("/companies(company-1)/Custom.Ns.doStuff") diff --git a/tests/test_url/test_origin_allowlist.py b/tests/test_url/test_origin_allowlist.py index 90b9f8b..1832577 100644 --- a/tests/test_url/test_origin_allowlist.py +++ b/tests/test_url/test_origin_allowlist.py @@ -97,33 +97,17 @@ def test_case_insensitive_host_check(): ) is True -# ── ETL module's inline copy stays in sync ─────────────────────────────── +# ── ETL path uses the same check ──────────────────────────────────────── -def test_etl_inline_check_matches_canonical(): - """The duplicated check in bcli.etl._client must reject the same URLs. - - The ETL layer keeps an inline copy because ``bcli.etl.*`` deliberately - avoids importing from outer ``bcli.*`` modules. Skipped when the - optional ``dlt`` extra isn't installed (matches the rest of - ``tests/test_etl/``). +def test_etl_has_no_private_origin_check(): + """The ETL client used to keep its own copy of the host allowlist; it now + pages through ``BCTransport.get_absolute``, which calls ``assert_bc_origin``. + The end-to-end refusal of an off-origin nextLink on the ETL path lives in + ``tests/test_etl/test_generic.py::test_bcclient_refuses_off_origin_nextlink``. """ pytest.importorskip("dlt") - from bcli.etl._client import _assert_bc_origin as etl_assert + import bcli.etl._client as etl_client - # Allowed - etl_assert( - f"https://api.businesscentral.dynamics.com/v2.0/Production/api/v2.0/companies({_COMPANY_ID})/customers" - ) - etl_assert(f"/v2.0/Production/api/v2.0/companies({_COMPANY_ID})/customers") - - # Rejected - for bad in [ - "https://attacker.example/leak", - "https://evilbusinesscentral.dynamics.com.attacker.example/leak", - "file:///etc/passwd", - # https-only: cleartext to a BC host is still refused. - "http://api.businesscentral.dynamics.com/leak", - ]: - with pytest.raises(ValueError): - etl_assert(bad) + assert not hasattr(etl_client, "_assert_bc_origin") + assert not hasattr(etl_client, "_ALLOWED_HOST_SUFFIXES") diff --git a/tests/test_workflow/test_batch_integration.py b/tests/test_workflow/test_batch_integration.py index 7853848..96de669 100644 --- a/tests/test_workflow/test_batch_integration.py +++ b/tests/test_workflow/test_batch_integration.py @@ -4,12 +4,11 @@ import textwrap from pathlib import Path -from unittest.mock import AsyncMock, patch - import pytest -from bcli.odata._response import ODataResponse +from bcli.config._model import BCConfig, BCDefaults, BCProfile from bcli.workflow._models import WorkflowContext +from bcli_cli._state import state from bcli_cli.commands.batch_cmd import ( _build_workflow_params, _execute_batch, @@ -22,12 +21,18 @@ # ─── Helpers ───────────────────────────────────────────────────────── -def _mock_client() -> AsyncMock: - """AsyncBCClient mock that returns predictable responses.""" - client = AsyncMock() - client.__aenter__ = AsyncMock(return_value=client) - client.__aexit__ = AsyncMock(return_value=False) - return client +@pytest.fixture +def bc(fake_bc): + """A writable profile whose real client talks to ``fake_bc``.""" + state._config = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={"dev": BCProfile(tenant_id="t1", environment="Sandbox", company_id="c-1")}, + ) + state._registry = None + state.profile_name = None + yield fake_bc + state._config = None + state._registry = None def _write_yaml(tmp_path: Path, content: str) -> Path: @@ -103,14 +108,13 @@ def test_no_refs(self): class TestExecuteBatchWorkflow: @pytest.mark.asyncio - async def test_post_chain_resolves_step_reference(self): + async def test_post_chain_resolves_step_reference(self, bc): """POST step 1 creates a record; step 2 uses its 'no' field.""" - client = _mock_client() # Step 1: POST returns a header with 'no' field - client.post.side_effect = [ + bc.reply( {"no": "PI-001", "id": "header-1"}, {"id": "line-1", "documentNo": "PI-001"}, - ] + ) steps = [ { @@ -128,22 +132,20 @@ async def test_post_chain_resolves_step_reference(self): ] context = WorkflowContext(params={}) - with patch("bcli_cli.commands.batch_cmd.state.make_async_client", return_value=client): - results = await _execute_batch(steps, context=context, output_format=None) + results = await _execute_batch(steps, context=context, output_format=None) assert len(results) == 2 assert results[0]["status"] == "ok" assert results[1]["status"] == "ok" - # Verify second POST was called with resolved documentNo - second_post_body = client.post.call_args_list[1][0][1] - assert second_post_body["documentNo"] == "PI-001" + # Verify second POST was sent with resolved documentNo + assert bc.sent[1].url.endswith("/purchaseLines") + assert bc.sent[1].json_body["documentNo"] == "PI-001" @pytest.mark.asyncio - async def test_param_resolution_in_data(self): + async def test_param_resolution_in_data(self, bc): """Runtime params resolve in step data.""" - client = _mock_client() - client.post.return_value = {"id": "new-1"} + bc.reply({"id": "new-1"}) steps = [ { @@ -155,19 +157,15 @@ async def test_param_resolution_in_data(self): ] context = WorkflowContext(params={"vendor_no": "V00011"}) - with patch("bcli_cli.commands.batch_cmd.state.make_async_client", return_value=client): - results = await _execute_batch(steps, context=context) + results = await _execute_batch(steps, context=context) assert results[0]["status"] == "ok" - post_body = client.post.call_args[0][1] - assert post_body["vendorNo"] == "V00011" + assert bc.last.json_body["vendorNo"] == "V00011" @pytest.mark.asyncio - async def test_get_result_chained(self): + async def test_get_result_chained(self, bc): """GET results can be indexed in subsequent steps.""" - client = _mock_client() - client.get.return_value = ODataResponse({"value": [{"id": "vendor-1", "name": "AAR"}]}) - client.post.return_value = {"id": "new-1"} + bc.reply({"value": [{"id": "vendor-1", "name": "AAR"}]}, {"id": "new-1"}) steps = [ {"name": "fetch_vendor", "action": "get", "endpoint": "vendors", "params": {"top": 1}}, @@ -180,19 +178,17 @@ async def test_get_result_chained(self): ] context = WorkflowContext(params={}) - with patch("bcli_cli.commands.batch_cmd.state.make_async_client", return_value=client): - results = await _execute_batch(steps, context=context) + results = await _execute_batch(steps, context=context) assert results[0]["status"] == "ok" assert results[1]["status"] == "ok" - post_body = client.post.call_args[0][1] - assert post_body["vendorId"] == "vendor-1" + assert [s.method for s in bc.sent] == ["GET", "POST"] + assert bc.last.json_body["vendorId"] == "vendor-1" @pytest.mark.asyncio - async def test_failed_step_skips_dependents(self): + async def test_failed_step_skips_dependents(self, bc): """If step 1 fails, step 2 referencing it gets a resolution error.""" - client = _mock_client() - client.post.side_effect = Exception("API error") + bc.reply(Exception("API error")) steps = [ {"name": "create_header", "action": "post", "endpoint": "items", "data": {"x": 1}}, @@ -205,25 +201,23 @@ async def test_failed_step_skips_dependents(self): ] context = WorkflowContext(params={}) - with patch("bcli_cli.commands.batch_cmd.state.make_async_client", return_value=client): - results = await _execute_batch(steps, context=context) + results = await _execute_batch(steps, context=context) assert results[0]["status"] == "error" assert results[1]["status"] == "error" assert "failed step" in results[1]["error"] + assert len(bc.sent) == 1 # the dependent step never reached BC @pytest.mark.asyncio - async def test_no_context_backward_compat(self): + async def test_no_context_backward_compat(self, bc): """Without context (non-workflow mode), batch works as before.""" - client = _mock_client() - client.get.return_value = ODataResponse({"value": [{"id": "1"}]}) + bc.reply({"value": [{"id": "1"}]}) steps = [ {"action": "get", "endpoint": "items", "params": {}}, ] - with patch("bcli_cli.commands.batch_cmd.state.make_async_client", return_value=client): - results = await _execute_batch(steps, context=None) + results = await _execute_batch(steps, context=None) assert len(results) == 1 assert results[0]["status"] == "ok" diff --git a/tests/test_workflow/test_method_alias.py b/tests/test_workflow/test_method_alias.py index 2adb8d6..99348fb 100644 --- a/tests/test_workflow/test_method_alias.py +++ b/tests/test_workflow/test_method_alias.py @@ -68,11 +68,10 @@ class TestBatchRunnerHonoursMethodAlias: ``action:`` must produce the right HTTP verb on the client, not a silent GET.""" - def test_method_post_calls_client_post( - self, tmp_path, monkeypatch, + def test_method_post_sends_post( + self, tmp_path, monkeypatch, fake_bc, ): from pathlib import Path - from unittest.mock import AsyncMock from bcli.config._model import BCConfig, BCDefaults, BCProfile from bcli_cli._state import state @@ -99,17 +98,7 @@ def test_method_post_calls_client_post( state.dry_run = False state.quiet = True - fake = AsyncMock() - fake.__aenter__ = AsyncMock(return_value=fake) - fake.__aexit__ = AsyncMock(return_value=False) - fake.post = AsyncMock(return_value={"id": "x-1"}) - fake.get = AsyncMock(side_effect=AssertionError( - "client.get must NOT be called — method: POST should reach " - "client.post via the action alias." - )) - fake._resolve_url = lambda entity, **kw: f"https://x/{entity}" - - monkeypatch.setattr(state, "make_async_client", lambda **_: fake) + fake_bc.reply({"id": "x-1"}) # Non-interactive: bypass the --yes prompt. import sys @@ -145,9 +134,7 @@ def test_method_post_calls_client_post( state._config = None state._registry = None - # The fix: client.post is what should have been called. The - # AsyncMock raises in client.get if the runner downgraded. - assert fake.post.await_count == 1, ( - f"Expected client.post to be called once, " - f"got post={fake.post.await_count}, get={fake.get.await_count}" - ) + # The fix: a POST carrying the step body went out — not a GET. + assert [(s.method, s.json_body) for s in fake_bc.sent] == [ + ("POST", {"flag": True}), + ], f"Expected one POST, got {[s.method for s in fake_bc.sent]}" diff --git a/tests/test_workflow/test_rollback_url_bound_action.py b/tests/test_workflow/test_rollback_url_bound_action.py index d2f6158..c0b01b7 100644 --- a/tests/test_workflow/test_rollback_url_bound_action.py +++ b/tests/test_workflow/test_rollback_url_bound_action.py @@ -15,47 +15,41 @@ from __future__ import annotations -from unittest.mock import MagicMock - +from bcli.client._async import AsyncBCClient +from bcli.config._model import BCConfig, BCDefaults, BCProfile from bcli_cli.commands.batch_cmd import _compose_rollback_url +def _client() -> AsyncBCClient: + return AsyncBCClient(profile="dev", config=BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={"dev": BCProfile(tenant_id="t1", environment="Sandbox", company_id="c-1")}, + )) + + def test_rollback_url_is_none_for_bound_action_with_id_in_result(): """Even if a bound action returns ``{"id": ...}``, no rollback URL should be composed — the action has no inverse.""" - client = MagicMock() - client._resolve_url.return_value = ( - "https://api.example.test/api/v2.0/companies(c-1)/" - "examples(42)/Microsoft.NAV.archive" - ) post_result = {"id": "new-id-789"} rb_url = _compose_rollback_url( - client, "examples(42)/Microsoft.NAV.archive", post_result, + _client(), "examples(42)/Microsoft.NAV.archive", post_result, ) assert rb_url is None - # And the early guard means we never even reached the resolver. - client._resolve_url.assert_not_called() def test_rollback_url_is_none_for_unbound_action_with_id_in_result(): """Same guard applies to unbound (service-root) actions.""" - client = MagicMock() rb_url = _compose_rollback_url( - client, "Microsoft.NAV.refreshAll", {"id": "irrelevant"}, + _client(), "Microsoft.NAV.refreshAll", {"id": "irrelevant"}, ) assert rb_url is None - client._resolve_url.assert_not_called() def test_rollback_url_normal_post_unchanged(): """A plain entity-set POST still gets a composed rollback URL.""" - client = MagicMock() - client._resolve_url.return_value = ( - "https://api.example.test/api/v2.0/companies(c-1)/examples" - ) + client = _client() rb_url = _compose_rollback_url( client, "examples", {"id": "new-id-789"}, ) - assert rb_url == ( - "https://api.example.test/api/v2.0/companies(c-1)/examples(new-id-789)" - ) + assert rb_url == client.resolve("examples", record_id="new-id-789") + assert rb_url.endswith("/api/v2.0/companies(c-1)/examples(new-id-789)")