Conversation
Covers fetch_task_output / push_task_output / finish_task_output, revoke_task, get_ansible_vault_password, check_ansible_facts and the first iterator helper from osism/utils/__init__.py. Companion to the connection-init suite added in Closes #2229. AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
| mocker.patch("osism.utils.time.time", side_effect=lambda: next(time_values)) | ||
|
|
||
| with pytest.raises(TimeoutError): | ||
| utils_pkg.fetch_task_output("task-1", timeout=10) |
There was a problem hiding this comment.
Design question: the normal exit path calls r.close() before returning; this path raises TimeoutError without closing r. Is that intentional? If closing on timeout is correct, this test should add mock_r.close.assert_called_once() to guard against it being dropped.
| mock_r.xadd.assert_called_once_with("task-1", {"type": "action", "content": "quit"}) | ||
|
|
||
|
|
||
| def test_finish_task_output_rc_zero_publishes_only_quit(mocker): |
There was a problem hiding this comment.
`if rc:` silently drops `rc=0`, making a successful subprocess exit indistinguishable from "no rc message received" — `fetch_task_output` defaults `rc = 0` and returns it either way. The pre-refactor code (commit `7257aa4`) published the rc message unconditionally; the `if rc:` guard was introduced in that refactor without explanation, suggesting an oversight rather than intent. Consider `if rc is not None:`.
The docstring here ("intentionally truthy") appears to be a post-hoc interpretation of the existing code rather than documented design intent, and would block a correct fix.
ideaship
left a comment
ideaship
left a comment
There was a problem hiding this comment.
Bug in production code not covered by new tests — osism/utils/__init__.py:372
timeout=os.environ.get("OSISM_TASK_TIMEOUT", 300) is evaluated at import time. When OSISM_TASK_TIMEOUT is set in the environment the default becomes a str, and time.time() + timeout crashes with TypeError. The call site in wait.py passes no explicit timeout, so this is reachable in production.
Fix: timeout=int(os.environ.get("OSISM_TASK_TIMEOUT", 300)).
Note: all new tests pass an explicit timeout= kwarg, which is why this went undetected. No test exercises the default-parameter path.
…ask output - fetch_task_output: cast OSISM_TASK_TIMEOUT to int; the env-var read at import time produced a str, crashing time.time() + timeout - fetch_task_output: close redis before raising TimeoutError, matching the quit path - finish_task_output: use "if rc is not None" so rc=0 publishes an rc message, restoring pre-refactor behavior (commit 7257aa4) - tests: cover the env-var default path, assert close() on timeout, and flip the rc=0 expectation AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
3 participants
Covers fetch_task_output / push_task_output / finish_task_output, revoke_task, get_ansible_vault_password, check_ansible_facts and the first iterator helper from osism/utils/init.py. Companion to the connection-init suite added in
Closes #2229.
AI-assisted: Claude Code