From 95b12c93f8fcfc9fd100001cd55211db49a12953 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 15:12:06 -0500 Subject: [PATCH 1/6] feat(registry): recognise OData v4 bound-action URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `_parse_bound_action` + `_is_unbound_action` helpers and wires them into `_resolve_url_for_target` so a URL of the form `()/.<...>.` is recognised as a bound-action invocation. The registry validator now looks up only the parent entity set and splices the action tail back onto the resolved URL, instead of treating the whole literal string as a missing entity set. The pattern is namespace-agnostic; tests exercise both `Microsoft.NAV.*` and a generic `Custom.Ns.*` namespace. `disable_standard_api` still applies — gated on the parent entity, which is the right grain (a bound action's policy follows the entity it's bound to). Unbound actions at the service root (`/.`) are explicitly rejected with a clear "not yet supported" error. Pass-through would require bypassing the company-id URL builder, which is a much bigger change than the bound-action case the bug report calls out. --- src/bcli/client/_async.py | 93 +++++++++++ tests/test_url/test_bound_action_resolve.py | 174 ++++++++++++++++++++ 2 files changed, 267 insertions(+) create mode 100644 tests/test_url/test_bound_action_resolve.py diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index 835ce14..e731731 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re from pathlib import Path from typing import Any @@ -17,6 +18,56 @@ from bcli.registry._registry import EndpointRegistry +# OData v4 *bound action* (and bound function) invocation pattern: +# +# ()/.<...>. +# +# - ```` is a normal identifier; it's the parent entity set +# that the registry validator looks up. +# - ```` is anything inside the parentheses — a GUID, a single- +# quoted string, an int, or a composite ``k1='a',k2='b'``. We do not +# over-validate it; BC will reject malformed keys server-side and +# passing through preserves the user's spelling for debugging. +# - The tail is a *qualified identifier* — at least one dot separates +# the namespace from the action/function name. We intentionally don't +# hardcode ``Microsoft.NAV`` so the parser works with any tenant's +# custom action namespaces. +_BOUND_ACTION_RE = re.compile( + r"^(?P[A-Za-z_][A-Za-z0-9_]*)" + r"\((?P.+)\)" + r"/(?P[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+)$" +) + +# An unbound action at the OData service root: ``Namespace.action`` +# with no parent entity. v0.1 explicitly rejects these — a future +# revision could route them past the company-id URL builder, but the +# bound-action case is the only one in the bug report this PR fixes. +_UNBOUND_ACTION_RE = re.compile( + r"^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+$" +) + + +def _parse_bound_action(entity_set_name: str) -> tuple[str, str, str] | None: + """Recognise a bound-action invocation in ``entity_set_name``. + + Returns ``(parent_entity_set, key, qualified_action)`` if the + string matches the OData v4 bound-action shape, ``None`` otherwise. + A plain entity-set name (``customers``) or a record URL with no + action tail (``customers(123)``) also returns ``None`` — those go + through the standard validator path unchanged. + """ + m = _BOUND_ACTION_RE.match(entity_set_name) + if m is None: + return None + return m.group("entity"), m.group("key"), m.group("qualified") + + +def _is_unbound_action(entity_set_name: str) -> bool: + """Return ``True`` for a service-root unbound-action invocation + (``Namespace.action`` with no parent entity set).""" + return bool(_UNBOUND_ACTION_RE.match(entity_set_name)) + + class AsyncBCClient: """Async client for Business Central APIs. @@ -461,6 +512,48 @@ def _resolve_url_for_target( "No company_id configured. Run 'bcli config init' or 'bcli company use '." ) + # ─── OData v4 bound action / function invocation ────────────── + # + # Pattern: ``()/.<...>.``. + # The registry validator should look up *only the parent + # entity set*; everything from the ``(`` onward is opaque to + # the registry and gets stitched back onto the resolved URL. + # This keeps ``disable_standard_api`` enforcement intact (gated + # on the parent, which is the security-relevant identity) and + # honours custom-route registry entries on the parent. + bound = _parse_bound_action(entity_set_name) + if bound is not None: + parent, key, qualified = bound + # Composing a parent URL with ``record_id`` here would + # double-append parens — actions encode the key inside the + # bound-action string, so we resolve the parent alone and + # then splice the ``(key)/qualified`` tail. + parent_url = self._resolve_url_for_target( + environment, + company_id, + parent, + record_id=None, + publisher=publisher, + group=group, + version=version, + ) + return f"{parent_url}({key})/{qualified}" + + # Service-root unbound action: ``Namespace.action`` with no + # parent entity. v0.1 rejects these explicitly so the user gets + # a clear "not yet supported" message instead of being routed + # through ``build_url`` (which expects an entity set). + if _is_unbound_action(entity_set_name): + from bcli.errors import RegistryError + + raise RegistryError( + f"Unbound action '{entity_set_name}' is not yet supported. " + f"This release of bcli only handles *bound* actions of the " + f"form '()/.'. " + f"If you need to invoke an unbound action at the service " + f"root, please file a feature request." + ) + # Explicit override takes priority if publisher and group and version: return build_url( diff --git a/tests/test_url/test_bound_action_resolve.py b/tests/test_url/test_bound_action_resolve.py new file mode 100644 index 0000000..ee26703 --- /dev/null +++ b/tests/test_url/test_bound_action_resolve.py @@ -0,0 +1,174 @@ +"""Tests for OData v4 bound-action URL recognition in the registry resolver. + +A URL of the form ``()/.`` is an +OData v4 *bound action invocation* (one example among many: BC's +``Microsoft.NAV.*`` actions on a per-record entity). The registry +validator must: + +1. Recognise the shape and lookup *only the parent entity set* in the + registry — not the whole literal string (which has never been an + entity set name). +2. Still enforce ``disable_standard_api`` based on the parent entity + set's registry status, exactly like a plain ``get`` would. +3. Surface a parent-entity-missing error (not a "whole URL not found" + error) when the parent isn't registered on a locked-down profile. + +The namespace identifier is *agnostic* — these tests deliberately +exercise ``Custom.Ns.doSomething`` alongside ``Microsoft.NAV.archive`` +so any hardcoded namespace check would surface as a failure. +""" + +from __future__ import annotations + +import pytest + +from bcli.client._async import AsyncBCClient +from bcli.config._model import BCConfig, BCDefaults, BCProfile +from bcli.errors import RegistryError +from bcli.registry._schema import EndpointMetadata + + +def _make_client( + *, + disable_standard: bool = False, + custom_endpoints: list[EndpointMetadata] | None = None, + env: str = "Production", + company_id: str = "company-1", +) -> AsyncBCClient: + """Build a client wired to an in-memory profile + registry.""" + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment=env, + company_id=company_id, + disable_standard_api=disable_standard, + ), + }, + ) + client = AsyncBCClient(profile="dev", config=cfg) + if custom_endpoints: + for ep in custom_endpoints: + client._registry._custom[ep.entity_set_name.lower()] = ep + return client + + +class TestBoundActionURLRecognition: + """The resolver should pass bound-action URLs through to the server + once the parent entity set is registered (or allowed by the standard + fallback).""" + + 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") + assert url.endswith( + "/companies(company-1)/customers(abc-123)/Microsoft.NAV.archive" + ), url + + def test_bound_action_resolves_against_parent_custom_entity(self): + """Custom-route parent entities should keep their custom route + (publisher/group/version) in front of the bound-action tail.""" + client = _make_client( + custom_endpoints=[ + EndpointMetadata( + entity_set_name="widgets", + api_publisher="acme", + api_group="custom", + api_version="v1.0", + ), + ], + ) + url = client._resolve_url("widgets(42)/Microsoft.NAV.cancel") + assert "/api/acme/custom/v1.0/" in url + assert url.endswith("/widgets(42)/Microsoft.NAV.cancel") + + 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") + 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") + 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( + "items(k1='a',k2='b')/Microsoft.NAV.doStuff" + ) + assert url.endswith("/items(k1='a',k2='b')/Microsoft.NAV.doStuff") + + def test_bound_action_with_multi_segment_namespace(self): + """Some namespaces are multi-segment (``Foo.Bar.Baz.action``) + — 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") + assert url.endswith("/widgets(7)/A.B.C.doStuff") + + +class TestRegistryGateOnBoundActions: + """The ``disable_standard_api`` profile gate must still apply — but + it should fire on the *parent entity name*, not on the full URL. + """ + + def test_disable_standard_api_blocks_unregistered_parent(self): + """Parent entity ``examples`` is not in the standard registry + and not in the custom registry, and the profile has the standard + catalogue disabled → reject.""" + client = _make_client(disable_standard=True) + with pytest.raises(RegistryError) as exc: + client._resolve_url("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. + msg = str(exc.value) + assert "examples" in msg + # Sanity: the verbose composed string shouldn't dominate the + # error message — the parent name is what the operator looks up. + assert "Microsoft.NAV" not in msg or msg.index("examples") < msg.index( + "Microsoft.NAV" + ) + + def test_disable_standard_api_allows_registered_custom_parent(self): + """Parent ``widgets`` is in the custom registry → action passes + through even with the standard catalogue disabled.""" + client = _make_client( + disable_standard=True, + custom_endpoints=[ + EndpointMetadata( + entity_set_name="widgets", + api_publisher="acme", + api_group="custom", + api_version="v1.0", + ), + ], + ) + url = client._resolve_url("widgets(7)/Custom.Ns.archive") + assert url.endswith("/widgets(7)/Custom.Ns.archive") + + +class TestUnboundActionAtServiceRootRejected: + """An unbound action (``/Microsoft.NAV.refreshAll``) lives at the + service root and has no parent entity set. The v0.1 implementation + refuses these explicitly so the user gets a clear "not yet + supported" error instead of a confusing parent-entity lookup + failure. See PR docstring — out of scope for this iteration.""" + + def test_unbound_action_at_service_root_rejected(self): + client = _make_client() + with pytest.raises(RegistryError) as exc: + client._resolve_url("Microsoft.NAV.refreshAll") + assert "unbound" in str(exc.value).lower() or "not yet supported" in str( + exc.value + ).lower() From a8e52873081170021808e5707e7f91fa67bf34e5 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 15:59:39 -0500 Subject: [PATCH 2/6] fix(batch): accept 'method:' as alias for 'action:' (no silent GET) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A YAML step that wrote 'method: POST' (instead of 'action: post') was silently downgraded to a GET — the runner reads step.get("action") with a default of "get", and 'method' was an undocumented unknown key. The bug surfaces when users copy-paste OData examples, especially bound- action invocations, where the conventional HTTP method key is 'method'. Fix in three places that all read the action key off the raw dict: * StepDef gains a model_validator that translates 'method:' to 'action:' (lowercased) before field validation, and rejects the combination of both keys to keep YAML one-way. * _execute_batch and the dry-run renderer apply the same alias logic when reading the raw step dict. * The disable_writes pre-flight that classifies mutating steps uses a shared _step_action helper so a 'method: POST' step still trips the write gate. Regression test asserts a YAML step with 'method: POST' reaches client.post — the AsyncMock now raises on .get to lock the contract. --- src/bcli/workflow/_models.py | 50 +++++++- src/bcli_cli/commands/batch_cmd.py | 35 +++++- tests/test_workflow/test_method_alias.py | 153 +++++++++++++++++++++++ 3 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 tests/test_workflow/test_method_alias.py diff --git a/src/bcli/workflow/_models.py b/src/bcli/workflow/_models.py index abc7a5a..4a50dea 100644 --- a/src/bcli/workflow/_models.py +++ b/src/bcli/workflow/_models.py @@ -5,7 +5,7 @@ from dataclasses import dataclass, field from typing import Any, Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator # ─── Pydantic models (YAML → validated structure) ──────────────────── @@ -20,7 +20,18 @@ class ParamDef(BaseModel): class StepDef(BaseModel): - """A single step as declared in a workflow YAML file.""" + """A single step as declared in a workflow YAML file. + + Steps may specify the HTTP verb under either of two keys: + + * ``action: post`` — the original spelling, lowercased. + * ``method: POST`` — alias kept because authors copy-pasting OData + examples (especially bound-action invocations) reach for ``method`` + out of habit. The model lowercases the value before assigning it + to ``action``. + + Both keys at once are rejected to keep YAML files unambiguous. + """ name: str = "" action: Literal["get", "post", "patch", "delete"] = "get" @@ -30,6 +41,41 @@ class StepDef(BaseModel): id: str | None = None etag: str = "*" + @model_validator(mode="before") + @classmethod + def _accept_method_alias(cls, data: Any) -> Any: + """Translate ``method:`` → ``action:`` before field validation. + + Runs in ``mode="before"`` so the ``Literal[...]`` validator on + ``action`` sees the normalised lowercase value and the unknown + ``method`` field is consumed (it would otherwise raise an + ``extra inputs`` error under ``model_config = ConfigDict(extra= + "forbid")`` — kept open so existing YAML stays valid, but the + validator still needs to drop the alias). + """ + if not isinstance(data, dict): + return data + if "method" not in data: + return data + method_raw = data["method"] + # Default to checking against the literal set the field accepts. + # Anything else gets normalised and passed through so pydantic's + # ``Literal[...]`` validator raises with a clear message. + if isinstance(method_raw, str): + method_norm = method_raw.lower() + else: + method_norm = method_raw + if "action" in data: + raise ValueError( + "Step declares both 'action' and 'method'. Pick one key — " + "'action' is the canonical spelling, 'method' is an alias " + "accepted for OData copy-paste habits." + ) + # Strip the alias and inject the normalised action. + normalised = {k: v for k, v in data.items() if k != "method"} + normalised["action"] = method_norm + return normalised + class WorkflowDef(BaseModel): """Top-level workflow definition parsed from YAML.""" diff --git a/src/bcli_cli/commands/batch_cmd.py b/src/bcli_cli/commands/batch_cmd.py index 173cb0b..b70d3db 100644 --- a/src/bcli_cli/commands/batch_cmd.py +++ b/src/bcli_cli/commands/batch_cmd.py @@ -353,15 +353,25 @@ def run_batch( # 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.get("action") or "get").lower() in mutating_actions + step for step in steps if _step_action(step) in mutating_actions ] if mutating_steps: from bcli_cli._safety import confirm_write_or_exit preview = ", ".join( - f"{(step.get('action') or 'get').upper()} {step.get('endpoint', '?')}" + f"{_step_action(step).upper()} {step.get('endpoint', '?')}" for step in mutating_steps[:3] ) if len(mutating_steps) > 3: @@ -488,7 +498,11 @@ 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 = (step_to_show.get("action") or "get").upper() + 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() endpoint = step_to_show.get("endpoint", "?") name = step_to_show.get("name", "") data = step_to_show.get("data") @@ -554,7 +568,18 @@ def emit(self, **_: Any) -> None: ... ) continue - action = (step.get("action") or "get").lower() + # 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() endpoint = step.get("endpoint", "") step_name = step.get("name") or endpoint data = step.get("data") diff --git a/tests/test_workflow/test_method_alias.py b/tests/test_workflow/test_method_alias.py new file mode 100644 index 0000000..2adb8d6 --- /dev/null +++ b/tests/test_workflow/test_method_alias.py @@ -0,0 +1,153 @@ +"""Regression: ``method: POST`` in a YAML batch step is silently +downgraded to GET. + +The workflow schema documents ``action: get|post|patch|delete`` but the +batch runner (``_execute_batch``) reads ``step.get("action") or "get"`` +— a step that writes ``method: POST`` instead of ``action: post`` is +silently treated as a GET because ``action`` is absent. The bug surfaces +when users copy-paste OData bound-action examples (where the conventional +HTTP method key is ``method``) into a bcli workflow file. + +These tests pin the fix: ``method`` is accepted as an alias for +``action`` (lowercased + normalised), and the schema validator rejects +the *combination* of both keys to keep YAML files unambiguous. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from bcli.workflow._models import StepDef, WorkflowDef + + +class TestMethodKeyAlias: + def test_method_key_normalises_to_action(self): + """``method: POST`` (no ``action:``) → ``action == 'post'``.""" + step = StepDef(endpoint="examples", method="POST") + assert step.action == "post" + + def test_method_key_lowercased(self): + """Authors write HTTP methods in uppercase (``POST``, ``PATCH``) + out of habit; the model should normalise to the lowercase form + the rest of bcli expects.""" + for raw, expected in [ + ("POST", "post"), + ("Patch", "patch"), + ("delete", "delete"), + ("GET", "get"), + ]: + step = StepDef(endpoint="examples", method=raw) + assert step.action == expected + + def test_unknown_method_rejected(self): + with pytest.raises(ValidationError): + StepDef(endpoint="examples", method="PUT") + + def test_action_and_method_both_set_rejected(self): + """An author who sets both ``action:`` and ``method:`` to the + same value is harmless, but to keep the YAML one-way we reject + the combination — the author should pick one key.""" + with pytest.raises(ValidationError): + StepDef(endpoint="examples", action="post", method="POST") + + def test_action_alone_still_works(self): + """The existing ``action:`` spelling must not regress.""" + step = StepDef(endpoint="examples", action="post") + assert step.action == "post" + + def test_workflow_def_accepts_method_in_step(self): + wf = WorkflowDef( + steps=[{"endpoint": "examples", "method": "POST"}], + ) + assert wf.steps[0].action == "post" + + +class TestBatchRunnerHonoursMethodAlias: + """End-to-end: a YAML step that uses ``method:`` instead of + ``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, + ): + from pathlib import Path + from unittest.mock import AsyncMock + + from bcli.config._model import BCConfig, BCDefaults, BCProfile + from bcli_cli._state import state + from bcli_cli.commands.batch_cmd import run_batch + + # Isolate HOME so the ledger lands in tmp_path. + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment="Sandbox", + company_id="c-1", + disable_writes=False, + ), + }, + ) + state._config = cfg + state._registry = None + state.profile_name = None + 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) + + # Non-interactive: bypass the --yes prompt. + import sys + monkeypatch.setattr(sys.stdin, "isatty", lambda: False) + + yaml_file = tmp_path / "workflow.yaml" + yaml_file.write_text( + "name: method-alias\n" + "steps:\n" + " - name: archive_one\n" + " method: POST\n" + " endpoint: examples\n" + " data: {flag: true}\n", + encoding="utf-8", + ) + + try: + run_batch( + file=yaml_file, + dry_run=False, + output=None, + format=None, + set_params=None, + params_file=None, + yes=True, + result_out=None, + result_fd=None, + progress_fd=None, + ) + except SystemExit: + pass + finally: + 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}" + ) From 9d07cc41421f9c607106047e48fb3d832592e7b7 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 16:01:46 -0500 Subject: [PATCH 3/6] feat(cli): add 'bcli action' verb for OData v4 bound actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A new top-level verb that lets a user invoke an OData v4 bound action without hand-writing the parens-and-dot-escaped URL: bcli action examples 42 archive --no-data bcli action items "'ALFKI'" doSomething --data '{"flag": true}' bcli action widgets 7 cancel --namespace Custom.Ns --data @body.json Internally the verb composes the synthetic '()/.' string and forwards it to the same client.post path that 'bcli post' uses — so the registry validator's new bound-action recognition (previous commit) covers it for free. Design choices: * --data and --no-data are mutually exclusive AND one is required. Defaulting silently to '{}' would mask a forgotten parameter on a real action; making the user choose forces an audit trail. * Default namespace is 'Microsoft.NAV' (BC convention) with --namespace for override. Tests exercise a 'Custom.Ns' namespace to lock in namespace-agnosticism. * Actions are always POST per the OData spec — there is no --method flag; the verb refuses to GET. * Honors --publisher/--group/--version, --idempotency-key, the disable_writes gate, and the result envelope just like 'bcli post'. --- src/bcli_cli/app.py | 5 + src/bcli_cli/commands/action_cmd.py | 224 ++++++++++++++++++++++++++++ tests/test_cli/test_action_cmd.py | 184 +++++++++++++++++++++++ 3 files changed, 413 insertions(+) create mode 100644 src/bcli_cli/commands/action_cmd.py create mode 100644 tests/test_cli/test_action_cmd.py diff --git a/src/bcli_cli/app.py b/src/bcli_cli/app.py index 8cc34a8..a55db5c 100644 --- a/src/bcli_cli/app.py +++ b/src/bcli_cli/app.py @@ -158,6 +158,7 @@ def _emit_command_summary() -> None: # Import and register command groups from bcli_cli.commands import ( # noqa: E402 + action_cmd, attach_cmd, auth_cmd, batch_cmd, @@ -197,6 +198,10 @@ def _emit_command_summary() -> None: app.command(name="post")(post_cmd.post_command) app.command(name="patch")(patch_cmd.patch_command) app.command(name="delete")(delete_cmd.delete_command) +app.command( + name="action", + help="Invoke an OData v4 bound action on a record", +)(action_cmd.action_command) app.command(name="q", help="Run a saved query (no OData required)")(query_cmd.query_command) app.command(name="ai-context")(context_cmd.ai_context_command) app.command(name="doctor", help="Diagnose your bcli install (self-rescue for team users)")(doctor_cmd.doctor_command) diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py new file mode 100644 index 0000000..1109dde --- /dev/null +++ b/src/bcli_cli/commands/action_cmd.py @@ -0,0 +1,224 @@ +"""bcli action — invoke an OData v4 bound action. + +OData v4 *actions* are POST requests against a URL of the form + + ()/. + +where ``.`` is a server-side declared action +bound to the record at ``()``. Hand-writing that string +is error-prone (the parentheses + dot escaping in particular trips up +shells), so this verb composes the URL from named arguments and +forwards to the same client.post path that ``bcli post`` uses. + +Spec-conformance notes: + +* Actions are always POST regardless of any caller intent — OData v4 + does not allow GET on an action invocation. +* The default namespace is ``Microsoft.NAV`` (the BC convention); the + ``--namespace`` flag overrides it for non-BC tenants. The registry + validator itself is namespace-agnostic — see + ``bcli.client._async._parse_bound_action``. +* ``--data`` and ``--no-data`` are mutually exclusive; one of the two + is required, to force a deliberate decision (a silent default to ``{}`` + would mask a forgotten parameter on a real action). +""" + +from __future__ import annotations + +import asyncio +import json +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._state import state +from bcli_cli.output import format_output, print_context_banner + +console = Console(stderr=True) + + +DEFAULT_NAMESPACE = "Microsoft.NAV" + + +def action_command( + entity_set: str = typer.Argument( + ..., help="Parent entity set name (e.g. 'examples')", + ), + key: str = typer.Argument( + ..., + help=( + "Record key inside the parens. Pass a GUID/int as bare text " + "or a string key with explicit single quotes: \"'ALFKI'\"." + ), + ), + action_name: str = typer.Argument( + ..., help="Action identifier (e.g. 'archive')", + ), + data: Optional[str] = typer.Option( + None, + "--data", + "-d", + help="JSON body for the action (literal or @filename).", + ), + no_data: bool = typer.Option( + False, + "--no-data", + help="Send an empty body. Mutually exclusive with --data.", + ), + namespace: Optional[str] = typer.Option( + None, + "--namespace", + "-n", + help=f"Action namespace (default: {DEFAULT_NAMESPACE}).", + ), + format: Optional[str] = typer.Option( + None, "--format", "-f", + help="Output format: table, json, csv, ndjson, raw", + ), + publisher: Optional[str] = typer.Option(None, "--publisher", hidden=True), + group: Optional[str] = typer.Option(None, "--group", hidden=True), + version: Optional[str] = typer.Option(None, "--version", hidden=True), + yes: bool = typer.Option( + False, "--yes", "-y", + help="Skip the read-only-profile warning prompt", + ), + result_out: Optional[Path] = typer.Option( + None, "--result-out", + help="Write a JSON result envelope to this path (atomic). See AIP §Phase 2.", + ), + result_fd: Optional[int] = typer.Option( + None, "--result-fd", + help="Write the JSON result envelope to this file descriptor and close it.", + ), + idempotency_key: Optional[str] = typer.Option( + None, "--idempotency-key", + help="Opaque token forwarded as Idempotency-Key header (AIP §Phase 4d).", + ), +) -> None: + """Invoke an OData v4 bound action on a record. + + \b + Examples: + bcli action examples 42 archive --no-data + bcli action items "'ALFKI'" doSomething --data '{"flag": true}' + bcli action widgets 7 cancel --namespace Custom.Ns --data @payload.json + """ + validate_flags(result_out, result_fd) + + # ``--data`` and ``--no-data`` are mutually exclusive; one of the + # two is required. A silent default of ``{}`` would mask a forgotten + # parameter, so we force the caller to make the choice explicit. + if data is not None and no_data: + raise typer.BadParameter( + "--data and --no-data are mutually exclusive. " + "Pass one or the other.", + ) + if data is None and not no_data: + raise typer.BadParameter( + "Pass --data for a body, or --no-data to send an empty " + "body. Refusing to default silently — actions usually take " + "parameters.", + ) + + body: dict + if no_data: + body = {} + else: + body = _parse_data(data) # type: ignore[arg-type] + + ns = namespace or DEFAULT_NAMESPACE + # Compose the synthetic bound-action string. The registry validator + # downstream (``_parse_bound_action``) will split it back into + # parent + key + qualified-action; we don't try to second-guess that + # parse here — keeping the bound-action shape the single source of + # truth. + composed = f"{entity_set}({key})/{ns}.{action_name}" + + output_format = format or state.format + state.format = output_format + if output_format in ("json", "csv", "ndjson", "raw"): + state.quiet = True + + 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) + cap.emit_success() + if 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"), + ) + 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) + + +def _parse_data(data: str) -> dict: + """Parse --data argument: JSON string or @filename.""" + if data.startswith("@"): + path = Path(data[1:]) + if not path.is_file(): + raise typer.BadParameter(f"File not found: {path}") + return json.loads(path.read_text(encoding="utf-8")) + return json.loads(data) diff --git a/tests/test_cli/test_action_cmd.py b/tests/test_cli/test_action_cmd.py new file mode 100644 index 0000000..59dc3b2 --- /dev/null +++ b/tests/test_cli/test_action_cmd.py @@ -0,0 +1,184 @@ +"""Tests for the ``bcli action`` verb (OData v4 bound action invocation). + +The action verb is a thin sugar layer over ``bcli post``. Internally it +composes the bound-action URL string +``()/.`` and forwards it to the +same client.post path. The verb takes care of: + +* default namespace ``Microsoft.NAV`` (the BC convention; the registry + validator itself remains namespace-agnostic). +* ``--namespace`` override for non-BC tenants. +* ``--data`` (JSON literal or @file) versus ``--no-data`` (empty body), + which are mutually exclusive. +* honouring ``--profile``/``--company``/``--publisher``/``--group``/ + ``--version`` overrides via the standard CLI plumbing. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import AsyncMock + +import pytest +import typer + +from bcli.config._model import BCConfig, BCDefaults, BCProfile +from bcli_cli._state import state +from bcli_cli.commands import action_cmd + + +@pytest.fixture +def cli_state(): + 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.env_override = None + state.company_override = None + state.format = "table" + state.dry_run = False + state.quiet = True + yield state + state._config = None + state._registry = None + state.profile_name = None + state.dry_run = False + + +@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 + + +@pytest.fixture(autouse=True) +def non_interactive(monkeypatch): + import sys + monkeypatch.setattr(sys.stdin, "isatty", lambda: False) + + +def _run( + *, + entity="examples", + key="42", + action="archive", + data='{"flag": true}', + no_data=False, + namespace=None, + yes=True, + **kwargs, +): + kwargs.setdefault("format", None) + kwargs.setdefault("publisher", None) + kwargs.setdefault("group", None) + kwargs.setdefault("version", None) + kwargs.setdefault("result_out", None) + kwargs.setdefault("result_fd", None) + kwargs.setdefault("idempotency_key", None) + return action_cmd.action_command( + entity_set=entity, + key=key, + action_name=action, + data=data, + no_data=no_data, + namespace=namespace, + yes=yes, + **kwargs, + ) + + +class TestURLComposition: + def test_default_namespace_is_microsoft_nav(self, cli_state, fake_client): + _run() + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples(42)/Microsoft.NAV.archive" + + def test_namespace_override(self, cli_state, fake_client): + _run(namespace="Custom.Ns") + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples(42)/Custom.Ns.archive" + + def test_quoted_string_key_passed_through(self, cli_state, fake_client): + _run(key="'ALFKI'") + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples('ALFKI')/Microsoft.NAV.archive" + + def test_action_uses_post_not_get(self, cli_state, fake_client): + """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 + + +class TestDataHandling: + def test_data_json_literal_parsed(self, cli_state, fake_client): + _run(data='{"a": 1, "b": "two"}') + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {"a": 1, "b": "two"} + + def test_data_from_file(self, cli_state, fake_client, 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] + assert body_arg == {"loaded": "from-file"} + + def test_no_data_sends_empty_body(self, cli_state, fake_client): + _run(data=None, no_data=True) + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {} + + def test_data_and_no_data_mutually_exclusive(self, cli_state, fake_client): + with pytest.raises(typer.BadParameter): + _run(data='{"x": 1}', no_data=True) + + def test_neither_data_nor_no_data_is_an_error(self, cli_state, fake_client): + """The verb must force the user to make a deliberate choice + between sending a body or none — silently defaulting to ``{}`` + could mask a forgotten parameter on a real action.""" + with pytest.raises(typer.BadParameter): + _run(data=None, no_data=False) + + +class TestProfileOverrides: + def test_publisher_group_version_forwarded(self, cli_state, fake_client): + _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" + + def test_idempotency_key_forwarded(self, cli_state, fake_client): + _run(idempotency_key="k-123") + kwargs = fake_client.post.await_args.kwargs + assert kwargs.get("idempotency_key") == "k-123" + + +class TestEnvelope: + def test_envelope_written_on_success( + self, cli_state, fake_client, 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" + # Envelope's endpoint field captures the synthetic bound-action + # string so an agent can audit *which* action was invoked. + assert env["endpoint"].endswith("/Microsoft.NAV.archive") From 401f79cae3569eed94311f35a10ec174321de5c8 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 16:25:19 -0500 Subject: [PATCH 4/6] feat(action): default empty body and route unbound actions at service root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the bound-action work that came out of design review: 1. ``bcli action`` no longer requires an explicit ``--data`` or ``--no-data`` flag. An empty body is the default — matching ``bcli post`` semantics — so an AI agent invoking ``bcli action examples 42 archive`` for a no-parameter action no longer hits a ``BadParameter``. ``--no-data`` is retained as an explicit no-op alias; ``--data`` and ``--no-data`` together is still an error. 2. Unbound actions at the OData service root (``Namespace.action`` with no parent entity set) are now routed instead of rejected. The resolver composes ``companies()/.`` under either the standard ``/api/v2.0/`` path or an explicit ``--publisher/--group/--version`` custom route. The ``disable_standard_api`` security gate still applies when no explicit override is supplied — locked-down profiles must opt into a specific custom API path, mirroring the protection on standard entity sets. Tests: - ``test_neither_flag_defaults_to_empty_body`` replaces the previous "neither flag is an error" assertion. - ``TestUnboundActionAtServiceRoot`` covers: standard-route routing, custom-route override, ``disable_standard_api`` enforcement, and namespace agnosticism. Full suite: 862 passed, 5 skipped (+3 net from the prior 859 baseline). --- src/bcli/client/_async.py | 41 +++++++++++----- src/bcli_cli/commands/action_cmd.py | 31 +++++------- tests/test_cli/test_action_cmd.py | 24 +++++---- tests/test_url/test_bound_action_resolve.py | 54 ++++++++++++++++----- 4 files changed, 97 insertions(+), 53 deletions(-) diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index e731731..830be3b 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -540,18 +540,37 @@ def _resolve_url_for_target( return f"{parent_url}({key})/{qualified}" # Service-root unbound action: ``Namespace.action`` with no - # parent entity. v0.1 rejects these explicitly so the user gets - # a clear "not yet supported" message instead of being routed - # through ``build_url`` (which expects an entity set). + # parent entity. Resolves to ``companies()/.`` + # under the chosen API route. Registry lookup is skipped (unbound + # actions aren't entity sets and can't be registered), but the + # ``disable_standard_api`` security gate still applies when no + # explicit publisher/group/version override is supplied. if _is_unbound_action(entity_set_name): - from bcli.errors import RegistryError - - raise RegistryError( - f"Unbound action '{entity_set_name}' is not yet supported. " - f"This release of bcli only handles *bound* actions of the " - f"form '()/.'. " - f"If you need to invoke an unbound action at the service " - f"root, please file a feature request." + if publisher and group and version: + return build_url( + environment=environment, + company_id=company_id, + entity_set_name=entity_set_name, + record_id=None, + publisher=publisher, + group=group, + version=version, + ) + if self._profile.disable_standard_api: + from bcli.errors import RegistryError + + raise RegistryError( + f"Unbound action '{entity_set_name}' cannot be routed: " + f"'disable_standard_api = true' blocks the standard v2.0 " + f"fallback, and unbound actions are not registry entries. " + f"Pass --publisher/--group/--version to target a custom " + f"API route." + ) + return build_url( + environment=environment, + company_id=company_id, + entity_set_name=entity_set_name, + record_id=None, ) # Explicit override takes priority diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py index 1109dde..bc40cb4 100644 --- a/src/bcli_cli/commands/action_cmd.py +++ b/src/bcli_cli/commands/action_cmd.py @@ -18,9 +18,10 @@ ``--namespace`` flag overrides it for non-BC tenants. The registry validator itself is namespace-agnostic — see ``bcli.client._async._parse_bound_action``. -* ``--data`` and ``--no-data`` are mutually exclusive; one of the two - is required, to force a deliberate decision (a silent default to ``{}`` - would mask a forgotten parameter on a real action). +* ``--data`` defaults to an empty body when omitted (matching + ``bcli post`` semantics). ``--no-data`` is retained as an explicit + no-op alias for callers who want to spell the intent out; passing + both ``--data`` and ``--no-data`` is still an error. """ from __future__ import annotations @@ -66,7 +67,8 @@ def action_command( no_data: bool = typer.Option( False, "--no-data", - help="Send an empty body. Mutually exclusive with --data.", + help="Explicitly send an empty body. Same as omitting --data; " + "kept for callers who want to spell the intent out.", ), namespace: Optional[str] = typer.Option( None, @@ -102,32 +104,21 @@ def action_command( \b Examples: - bcli action examples 42 archive --no-data + bcli action examples 42 archive bcli action items "'ALFKI'" doSomething --data '{"flag": true}' bcli action widgets 7 cancel --namespace Custom.Ns --data @payload.json """ validate_flags(result_out, result_fd) - # ``--data`` and ``--no-data`` are mutually exclusive; one of the - # two is required. A silent default of ``{}`` would mask a forgotten - # parameter, so we force the caller to make the choice explicit. + # ``--data`` and ``--no-data`` may not both be set. Either alone, or + # neither (defaults to ``{}``), is fine — matches ``bcli post``. if data is not None and no_data: raise typer.BadParameter( "--data and --no-data are mutually exclusive. " - "Pass one or the other.", - ) - if data is None and not no_data: - raise typer.BadParameter( - "Pass --data for a body, or --no-data to send an empty " - "body. Refusing to default silently — actions usually take " - "parameters.", + "Pass one or the other (or neither — empty body is the default).", ) - body: dict - if no_data: - body = {} - else: - body = _parse_data(data) # type: ignore[arg-type] + body: dict = {} if data is None else _parse_data(data) ns = namespace or DEFAULT_NAMESPACE # Compose the synthetic bound-action string. The registry validator diff --git a/tests/test_cli/test_action_cmd.py b/tests/test_cli/test_action_cmd.py index 59dc3b2..d16e63c 100644 --- a/tests/test_cli/test_action_cmd.py +++ b/tests/test_cli/test_action_cmd.py @@ -8,8 +8,10 @@ * default namespace ``Microsoft.NAV`` (the BC convention; the registry validator itself remains namespace-agnostic). * ``--namespace`` override for non-BC tenants. -* ``--data`` (JSON literal or @file) versus ``--no-data`` (empty body), - which are mutually exclusive. +* ``--data`` (JSON literal or @file) for a body, or no flag at all for + an empty body (matching ``bcli post``). ``--no-data`` is retained as + an explicit no-op alias; ``--data`` and ``--no-data`` may not both be + supplied. * honouring ``--profile``/``--company``/``--publisher``/``--group``/ ``--version`` overrides via the standard CLI plumbing. """ @@ -139,22 +141,24 @@ def test_data_from_file(self, cli_state, fake_client, tmp_path: Path): body_arg = fake_client.post.await_args.args[1] assert body_arg == {"loaded": "from-file"} - def test_no_data_sends_empty_body(self, cli_state, fake_client): + def test_no_data_flag_sends_empty_body(self, cli_state, fake_client): + """Explicit --no-data sends an empty body.""" _run(data=None, no_data=True) body_arg = fake_client.post.await_args.args[1] assert body_arg == {} + def test_neither_flag_defaults_to_empty_body(self, cli_state, fake_client): + """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] + assert body_arg == {} + def test_data_and_no_data_mutually_exclusive(self, cli_state, fake_client): with pytest.raises(typer.BadParameter): _run(data='{"x": 1}', no_data=True) - def test_neither_data_nor_no_data_is_an_error(self, cli_state, fake_client): - """The verb must force the user to make a deliberate choice - between sending a body or none — silently defaulting to ``{}`` - could mask a forgotten parameter on a real action.""" - with pytest.raises(typer.BadParameter): - _run(data=None, no_data=False) - class TestProfileOverrides: def test_publisher_group_version_forwarded(self, cli_state, fake_client): diff --git a/tests/test_url/test_bound_action_resolve.py b/tests/test_url/test_bound_action_resolve.py index ee26703..25421c6 100644 --- a/tests/test_url/test_bound_action_resolve.py +++ b/tests/test_url/test_bound_action_resolve.py @@ -158,17 +158,47 @@ def test_disable_standard_api_allows_registered_custom_parent(self): assert url.endswith("/widgets(7)/Custom.Ns.archive") -class TestUnboundActionAtServiceRootRejected: - """An unbound action (``/Microsoft.NAV.refreshAll``) lives at the - service root and has no parent entity set. The v0.1 implementation - refuses these explicitly so the user gets a clear "not yet - supported" error instead of a confusing parent-entity lookup - failure. See PR docstring — out of scope for this iteration.""" - - def test_unbound_action_at_service_root_rejected(self): - client = _make_client() +class TestUnboundActionAtServiceRoot: + """An unbound action (``Namespace.action`` with no parent entity) + resolves to ``companies()/.`` under the + chosen API route. Unbound actions aren't entity sets and cannot be + registered — the ``disable_standard_api`` security gate therefore + still applies when no explicit publisher/group/version override is + supplied, mirroring the protection on standard-v2.0 entity sets.""" + + def test_unbound_action_resolves_via_standard_route(self): + client = _make_client(disable_standard=False) + url = client._resolve_url("Microsoft.NAV.refreshAll") + assert url.endswith( + "/companies(company-1)/Microsoft.NAV.refreshAll" + ), url + assert "/api/v2.0/" in url + + 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( + "Custom.Ns.refreshAll", + publisher="acme", group="custom", version="v1.0", + ) + assert "/api/acme/custom/v1.0/" in url + assert url.endswith("/companies(company-1)/Custom.Ns.refreshAll") + + def test_unbound_action_blocked_when_disable_standard_api(self): + """A locked-down profile without an explicit custom-route + override must refuse the unbound action — there's nowhere to + route it that doesn't fall through to the suppressed standard + v2.0 surface.""" + client = _make_client(disable_standard=True) with pytest.raises(RegistryError) as exc: client._resolve_url("Microsoft.NAV.refreshAll") - assert "unbound" in str(exc.value).lower() or "not yet supported" in str( - exc.value - ).lower() + assert "disable_standard_api" in str(exc.value) + assert "publisher" in str(exc.value).lower() + + 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") + assert url.endswith("/companies(company-1)/Custom.Ns.doStuff") From 04bcd2be59efe97bea4b1c1ca8fb63db33298514 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 16:30:41 -0500 Subject: [PATCH 5/6] fix(batch): skip rollback URL composition for action endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OData actions (bound and unbound) have no inverse operation — there is no DELETE that undoes an archive, supersede, or refreshAll. If a step records an action POST in the batch ledger and the action returns a body with an ``id`` field, the previous ``_compose_rollback_url`` logic would naïvely append ``(id)`` to the action endpoint, producing nonsense like ``examples(42)/Microsoft.NAV.archive(new-id-789)``. Subsequent ``bcli batch rollback`` would then send a DELETE to that path and fail in a confusing way. Detect action endpoints (both bound ``entitySet(key)/Ns.action`` and unbound ``Ns.action``) early and return ``None`` so the ledger records ``rollback_skipped`` — the correct semantics for a non-invertible step. Regression coverage: ``tests/test_workflow/test_rollback_url_bound_action.py``. --- src/bcli_cli/commands/batch_cmd.py | 9 +++ .../test_rollback_url_bound_action.py | 61 +++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 tests/test_workflow/test_rollback_url_bound_action.py diff --git a/src/bcli_cli/commands/batch_cmd.py b/src/bcli_cli/commands/batch_cmd.py index b70d3db..e3439ff 100644 --- a/src/bcli_cli/commands/batch_cmd.py +++ b/src/bcli_cli/commands/batch_cmd.py @@ -194,6 +194,15 @@ def _compose_rollback_url(client: Any, entity: str, post_result: Any) -> str | N """ if not isinstance(post_result, dict): return None + # OData actions (bound or unbound) have no inverse — there's no + # "DELETE the archiving" of ``examples(42)/Microsoft.NAV.archive``, + # nor of ``Microsoft.NAV.refreshAll``. If an action happens to + # return a body with an ``id`` field, composing a rollback URL by + # appending ``(id)`` would yield a nonsense path. The batch ledger + # should record ``rollback_skipped`` instead. + from bcli.client._async import _is_unbound_action, _parse_bound_action + if _parse_bound_action(entity) is not None or _is_unbound_action(entity): + return None record_id = post_result.get("id") or post_result.get("systemId") if not record_id: return None diff --git a/tests/test_workflow/test_rollback_url_bound_action.py b/tests/test_workflow/test_rollback_url_bound_action.py new file mode 100644 index 0000000..d2f6158 --- /dev/null +++ b/tests/test_workflow/test_rollback_url_bound_action.py @@ -0,0 +1,61 @@ +"""Regression test for ``_compose_rollback_url`` and bound-action endpoints. + +When the batch runner records the outcome of a POST step, it tries to +compose a rollback URL (``DELETE ()``) so a subsequent +``bcli batch rollback`` can undo the create. Bound-action endpoints +have no such inverse — there is no "DELETE the archiving" of +``examples(42)/Microsoft.NAV.archive``. If the action happened to +return a body containing an ``id`` (unusual but legal for OData +actions), naïve URL composition would yield a nonsensical path like +``examples(42)/Microsoft.NAV.archive(new-id-789)``. + +The composer must detect bound-action endpoints and return ``None``, +which causes the ledger to record ``rollback_skipped`` instead. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from bcli_cli.commands.batch_cmd import _compose_rollback_url + + +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, + ) + 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"}, + ) + 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" + ) + 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)" + ) From 239d66cf38b459b4a988c9b9b846ba6682867495 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Fri, 22 May 2026 16:56:58 -0500 Subject: [PATCH 6/6] chore: sync uv.lock with pyproject.toml (bc-cli 0.3.0 -> 0.4.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI ran ``uv sync --locked --extra dev --extra etl`` and refused with ``The lockfile at 'uv.lock' needs to be updated, but '--locked' was provided``. The 0.4.0 release commit bumped pyproject.toml's project version but did not regenerate uv.lock, so every Tests run since 2026-05-18 (including this PR's, before any of the bound-action changes are even installed) errors out before pytest is invoked. Regenerating with ``uv lock`` yields the same two-character diff — the editable ``bc-cli`` entry catches up to its declared version. --- uv.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uv.lock b/uv.lock index 22160c6..7d9389e 100644 --- a/uv.lock +++ b/uv.lock @@ -321,7 +321,7 @@ wheels = [ [[package]] name = "bc-cli" -version = "0.3.0" +version = "0.4.0" source = { editable = "." } dependencies = [ { name = "httpx" },