An empty record_id must be an error, not a collection request

CHANGELOG.md

@@ -39,6 +39,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   genuinely part of a key. The same validation applies to the key inside a
   bound-action invocation, whose resolver checks the registry for the parent
   entity set only.
+- `--dry-run` no longer reports success for a request the real command refuses.
+  `try_resolve_url` never raises by design, so an invalid `record_id` was
+  swallowed to `resolved_url: null` and the preview still rendered and exited 0
+  while the real run exited 1 — the opposite of what a preview is for, and its
+  documented consumers are agents parsing that envelope to decide whether to
+  proceed. It now takes `strict=True` from the dry-run path, which re-raises
+  invalid *input* while still swallowing *incidental* resolution failures (a
+  registry miss keeps previewing with a null URL, as documented). `BCLIError`
+  subclasses don't inherit `ValueError`, which is what makes that split clean.
+  The failure is presented as the same `Error:` line the real run prints, not a
+  traceback.
+- An **empty** `record_id` is now an error instead of silently addressing the
+  collection. `None` still means "operate on the entity set" (`bcli get
+  <entity>` with no id is a collection read), but `""` previously took the same
+  path — so a caller that meant one record and supplied nothing got a request
+  against the whole set. `delete` and `patch` take `record_id` as a required
+  positional, so `bcli delete <entity> ""` composed a DELETE against the entity
+  set rather than a row.
 - `test_no_context_policy_path` no longer asserts against the developer's real
   `~/.config/bcli`, so it passes on a machine that has recorded a bcli error
   rather than only on a clean CI home.

src/bcli/_url.py

@@ -87,7 +87,13 @@ def build_url(
         https://api.businesscentral.dynamics.com/v2.0/{env}/api/{pub}/{grp}/{ver}/companies({id})/{entity}
     """
     validate_record_key("entity_set_name", entity_set_name)
-    if record_id:
+    # `None` means "address the collection" and is a supported call — `bcli get
+    # <entity>` with no id reads the set. An *empty* key is not the same thing:
+    # it means the caller meant one record and supplied nothing, and silently
+    # dropping it would retarget the request at the whole collection (a DELETE
+    # or PATCH against an entity set rather than a row). Validate anything that
+    # was actually passed, including "".
+    if record_id is not None:
         validate_record_key("record_id", record_id)
 
     if publisher and group and version:
@@ -100,7 +106,7 @@ def build_url(
 
     url = f"{BC_BASE_URL}/{environment}/{api_path}/companies({company_id})/{entity_set_name}"
 
-    if record_id:
+    if record_id is not None:
         url = f"{url}({record_id})"
 
     return url

src/bcli_cli/_dry_run.py

@@ -57,14 +57,29 @@ def render_dry_run(
     profile = state.profile
     profile_name = state.active_profile_name
 
-    resolved_url = try_resolve_url(
-        endpoint,
-        record_id=record_id,
-        publisher=publisher,
-        group=group,
-        version=version,
-        force_standard=force_standard,
-    )
+    # strict=True: a preview must not report success for a request the real
+    # command would refuse. An unresolvable endpoint still previews with a null
+    # URL (that is the documented behaviour); invalid input is fatal, so
+    # `--dry-run` fails exactly where the real run fails.
+    #
+    # Presented here rather than propagated, for two reasons: the dry-run branch
+    # in each write command sits above that command's own try/except, so a raw
+    # raise surfaces as a traceback instead of the `Error: ...` line the real run
+    # prints; and doing it in one place keeps post/patch/delete/attach/batch
+    # consistent without five edits.
+    try:
+        resolved_url = try_resolve_url(
+            endpoint,
+            record_id=record_id,
+            publisher=publisher,
+            group=group,
+            version=version,
+            force_standard=force_standard,
+            strict=True,
+        )
+    except ValueError as exc:
+        _console.print(f"[red]Error:[/red] {exc}")
+        raise typer.Exit(1) from exc
 
     payload: dict[str, Any] = {
         "dry_run": True,

src/bcli_cli/_url_resolve.py

@@ -26,13 +26,26 @@ def try_resolve_url(
     group: str | None = None,
     version: str | None = None,
     force_standard: bool = False,
+    strict: bool = False,
 ) -> str | None:
     """Resolve ``endpoint`` to a full URL using the active profile.
 
     Returns ``None`` on any failure (registry miss with
     ``disable_standard_api``, missing company id, malformed profile,
     etc.). Callers should treat ``None`` as "preview the rest, the user
     will see the gap and correct".
+
+    ``strict=True`` keeps that for *incidental* failures but re-raises invalid
+    **input**. The two are not the same thing, and conflating them made
+    ``--dry-run`` report success for a request the real command refuses: an empty
+    or path-bearing ``record_id`` raises ``ValueError`` in the URL builder, this
+    helper swallowed it to ``None``, and the preview rendered a clean DELETE and
+    exited 0 while the real run exited 1. A preview whose entire job is to
+    predict must not disagree with what it predicts.
+
+    ``BCLIError`` subclasses (``RegistryError``, ``ConfigError``) are the
+    incidental kind and stay swallowed even under ``strict`` — they do not
+    inherit ``ValueError``, which is what makes the split clean.
     """
     try:
         if force_standard:
@@ -53,5 +66,12 @@ def try_resolve_url(
             group=group,
             version=version,
         )
+    except ValueError:
+        # Invalid input, not an incidental resolution failure. The real request
+        # validates the same value and fails, so a strict caller (the dry-run
+        # preview) must fail too rather than reporting a request it cannot make.
+        if strict:
+            raise
+        return None
     except Exception:
         return None

tests/test_cli/test_dry_run.py

@@ -225,3 +225,110 @@ def test_exits_clean_for_human_format_too(self, configured_state):
         with pytest.raises(typer.Exit) as excinfo:
             render_dry_run("DELETE", "items", record_id="x")
         assert excinfo.value.exit_code == 0
+
+
+@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.
+    """
+    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:
+    """A preview that reports success for input the real request refuses is worse
+    than no preview: ``--dry-run`` exists so an agent can decide whether to
+    proceed, and its documented consumers parse the JSON envelope.
+
+    ``try_resolve_url`` deliberately never raises, so a resolution failure
+    records ``resolved_url: null`` and the preview continues. That is right for
+    an incidental failure (registry miss, no company id) but wrong for invalid
+    *input*, because the real command validates the same value and exits 1. An
+    empty ``record_id`` used to render a clean DELETE preview and exit 0 while
+    ``bcli delete <entity> ""`` exited 1.
+    """
+
+    @pytest.mark.parametrize("empty", ["", "   "])
+    def test_empty_record_id_fails_the_preview(self, real_resolver_state, empty):
+        state.format = "json"
+        with pytest.raises(typer.Exit) as excinfo:
+            render_dry_run("DELETE", "items", record_id=empty)
+        assert excinfo.value.exit_code == 1
+
+    def test_traversing_record_id_fails_the_preview(self, real_resolver_state):
+        state.format = "json"
+        with pytest.raises(typer.Exit) as excinfo:
+            render_dry_run("DELETE", "items", record_id="1)/../../glEntries('X'")
+        assert excinfo.value.exit_code == 1
+
+    def test_the_failure_is_reported_not_traced(self, real_resolver_state, capsys):
+        """The dry-run branch sits above each command's own try/except, so a raw
+        raise would surface as a traceback rather than the ``Error:`` line the
+        real run prints."""
+        state.format = "json"
+        with pytest.raises(typer.Exit):
+            render_dry_run("DELETE", "items", record_id="")
+        assert "must not be empty" in capsys.readouterr().err
+
+    def test_none_record_id_still_previews_cleanly(self, real_resolver_state):
+        """A collection-targeted write is a real thing; don't break it."""
+        state.format = "json"
+        with pytest.raises(typer.Exit) as excinfo:
+            render_dry_run("POST", "items", body={"x": 1}, record_id=None)
+        assert excinfo.value.exit_code == 0
+
+    def test_a_valid_key_still_previews_cleanly(self, real_resolver_state):
+        state.format = "json"
+        with pytest.raises(typer.Exit) as excinfo:
+            render_dry_run("DELETE", "items", record_id="'V00010'")
+        assert excinfo.value.exit_code == 0
+
+
+class TestTryResolveUrlStrictMode:
+    def test_strict_re_raises_input_validation(self, real_resolver_state):
+        from bcli_cli._url_resolve import try_resolve_url
+
+        with pytest.raises(ValueError, match="must not be empty"):
+            try_resolve_url("items", record_id="", strict=True)
+
+    def test_non_strict_still_swallows_input_validation(self, real_resolver_state):
+        """The audit path must never break a command that already ran."""
+        from bcli_cli._url_resolve import try_resolve_url
+
+        assert try_resolve_url("items", record_id="") is None
+
+    def test_strict_still_swallows_incidental_failures(self, real_resolver_state):
+        """A registry miss is not the caller's input being wrong, so even strict
+        mode returns None — both the preview and audit paths want that. This
+        profile has disable_standard_api unset, so an unknown entity falls
+        through to the standard route and resolves; force the registry gate on
+        to get a genuine incidental failure."""
+        from bcli_cli._url_resolve import try_resolve_url
+
+        state.profile.disable_standard_api = True
+        state._registry = None
+        assert try_resolve_url("definitelyNotAnEndpoint", record_id="x", strict=True) is None

tests/test_url/test_record_key_validation.py

@@ -90,6 +90,35 @@ def test_none_record_id_is_unchanged(self):
         assert url.endswith("engineOverviews")
 
 
+class TestEmptyRecordIdIsNotTheSameAsNone:
+    """``None`` means "operate on the collection" and is legitimate — ``bcli get
+    <entity>`` with no id is a collection read. An *empty* key is different: it
+    means the caller meant to address one record and supplied nothing.
+
+    The first version of this validation guarded with ``if record_id:``, so a
+    falsy key skipped validation *and* skipped appending the key — silently
+    turning a single-record operation into a collection one. ``delete`` and
+    ``patch`` take ``record_id`` as a required positional with no default, so
+    ``bcli delete engineOverviews ""`` composed a DELETE against the whole
+    entity set. Whether BC would honour that is not the point; the client must
+    not build it.
+    """
+
+    @pytest.mark.parametrize("empty", ["", "   ", "\t", "\n"])
+    def test_empty_record_id_is_rejected(self, empty: str):
+        with pytest.raises(ValueError, match="must not be empty"):
+            _build(entity_set_name="engineOverviews", record_id=empty)
+
+    def test_empty_record_id_does_not_silently_become_a_collection_url(self):
+        with pytest.raises(ValueError):
+            _build(entity_set_name="engineOverviews", record_id="")
+
+    def test_none_still_means_collection(self):
+        url = _build(entity_set_name="engineOverviews", record_id=None)
+        assert url.endswith("engineOverviews")
+        assert "(" not in url.rsplit("/", 1)[-1]
+
+
 class TestEntitySetNameIsValidatedToo:
     @pytest.mark.parametrize("bad", ["a/b", "..", ".", "a\\b"])
     def test_rejects_path_syntax(self, bad: str):