From a386d2919aa11a0e4316623998dafc5c05ad94da Mon Sep 17 00:00:00 2001
From: Roger Luethi <luethi@osism.tech>
Date: Mon, 1 Jun 2026 14:42:08 +0200
Subject: [PATCH] Return non-zero exit from get on query failure

osism get hostvars and osism get hosts logged an error but then
returned None when the underlying ansible-inventory subprocess
failed. cliff turns a take_action return value of None into exit
code 0 (return_code = self.take_action(parsed_args) or 0), so these
commands exited successfully even though the inventory query could
not be run at all. That makes them unsafe to use in set -e scripts
or && chains, where a failed lookup silently looks like success.

Both commands now return 1 when subprocess.CalledProcessError is
raised, i.e. when the query itself could not be executed.

A missing host is one such failure. With the pinned ansible-core
(2.18.9, release latest/base.yml), ansible-inventory --host on a
host that is not in the inventory does not return an empty result;
it refuses the query with "ERROR! You must pass a single valid host
to --host parameter" and exits 5. osism get hostvars for an unknown
host therefore now exits 1, which matches its existing "Host not
found in inventory." error message.

Empty results are deliberately left as exit 0: a host that exists
but has no host vars (ansible-inventory prints {} and exits 0), a
requested variable or fact that is absent from an otherwise
successfully retrieved data set, or an inventory that lists no
matching hosts. These are valid answers to a query that did run.
This keeps the behaviour consistent with the convention used
elsewhere in the CLI, where a non-zero status signals an operational
failure rather than an empty result set.

Adds unit tests for both commands covering the failure path (a
raised CalledProcessError yields exit code 1) and the empty-result
path (a successful query with nothing to show yields exit code 0).

AI-assisted: Claude Code
Signed-off-by: Roger Luethi <luethi@osism.tech>
---
 osism/commands/get.py           |  4 +-
 tests/unit/commands/test_get.py | 91 +++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 tests/unit/commands/test_get.py

diff --git a/osism/commands/get.py b/osism/commands/get.py
index 404f17bf..23f8b4ec 100644
--- a/osism/commands/get.py
+++ b/osism/commands/get.py
@@ -139,7 +139,7 @@ def take_action(self, parsed_args):
             )
         except subprocess.CalledProcessError:
             logger.error(f"Host {host} not found in inventory.")
-            return
+            return 1
 
         data = json.loads(result)
         table = []
@@ -249,7 +249,7 @@ def take_action(self, parsed_args):
             )
         except subprocess.CalledProcessError:
             logger.error("Error loading inventory.")
-            return
+            return 1
 
         data = json.loads(result)
         table = []
diff --git a/tests/unit/commands/test_get.py b/tests/unit/commands/test_get.py
new file mode 100644
index 00000000..c4996a3b
--- /dev/null
+++ b/tests/unit/commands/test_get.py
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: Apache-2.0
+
+"""Tests for the ``osism get`` commands.
+
+These focus on the exit-code contract: a command must return a non-zero exit
+status when the underlying query cannot be run (e.g. the inventory cannot be
+loaded), but must keep returning success when the query runs fine and simply
+yields an empty result.
+"""
+
+import json
+import subprocess
+from unittest.mock import MagicMock, patch
+
+from osism.commands import get
+
+
+def _make(cls):
+    return cls(MagicMock(), MagicMock())
+
+
+# --- Hosts.take_action ---
+
+
+def test_hosts_returns_nonzero_when_inventory_cannot_be_loaded():
+    cmd = _make(get.Hosts)
+    parsed_args = cmd.get_parser("test").parse_args([])
+
+    with patch(
+        "osism.commands.get.get_inventory_path",
+        return_value="/ansible/inventory/hosts.yml",
+    ), patch(
+        "osism.commands.get.subprocess.check_output",
+        side_effect=subprocess.CalledProcessError(1, "ansible-inventory"),
+    ):
+        result = cmd.take_action(parsed_args)
+
+    assert result == 1
+
+
+def test_hosts_returns_success_for_empty_inventory():
+    cmd = _make(get.Hosts)
+    parsed_args = cmd.get_parser("test").parse_args([])
+
+    with patch(
+        "osism.commands.get.get_inventory_path",
+        return_value="/ansible/inventory/hosts.yml",
+    ), patch(
+        "osism.commands.get.subprocess.check_output",
+        return_value=json.dumps({"_meta": {"hostvars": {}}}).encode(),
+    ), patch(
+        "osism.commands.get.get_hosts_from_inventory", return_value=[]
+    ):
+        result = cmd.take_action(parsed_args)
+
+    assert not result
+
+
+# --- Hostvars.take_action ---
+
+
+def test_hostvars_returns_nonzero_when_inventory_query_fails():
+    cmd = _make(get.Hostvars)
+    parsed_args = cmd.get_parser("test").parse_args(["somehost"])
+
+    with patch(
+        "osism.commands.get.get_inventory_path",
+        return_value="/ansible/inventory/hosts.yml",
+    ), patch(
+        "osism.commands.get.subprocess.check_output",
+        side_effect=subprocess.CalledProcessError(1, "ansible-inventory"),
+    ):
+        result = cmd.take_action(parsed_args)
+
+    assert result == 1
+
+
+def test_hostvars_returns_success_when_variable_absent_from_result():
+    cmd = _make(get.Hostvars)
+    parsed_args = cmd.get_parser("test").parse_args(["somehost", "missingvar"])
+
+    with patch(
+        "osism.commands.get.get_inventory_path",
+        return_value="/ansible/inventory/hosts.yml",
+    ), patch(
+        "osism.commands.get.subprocess.check_output",
+        return_value=json.dumps({"present": "value"}).encode(),
+    ):
+        result = cmd.take_action(parsed_args)
+
+    assert not result