Add unit tests for task lifecycle commands

[berendt]

Implements #2362 (part of the Tier 8 meta issue #2199).

Adds unit tests for the nine small task-lifecycle CLI command modules under osism/commands/, following the established pattern from tests/unit/commands/test_reconciler.py / test_wait.py (Cmd(MagicMock(), MagicMock()) + get_parser("test").parse_args([...])).

New test files

• test_task.py — parser contract and Celery revoke with terminate=True; pins the latent bug that the one-element nargs=1 list reaches control.revoke
• test_worker.py — concurrency defaults (CPU count capped at 4, OSISM_CELERY_CONCURRENCY read at parser-build time), worker-type → tasks-module/queue mapping including the osism-ansible special case and suffix stripping, -c propagation, and the unvalidated unknown-type command pinned as current behavior
• test_lock.py — Lock/Unlock/LockStatus flows: already-locked warnings (with/without reason), --user/--reason pass-through, OPERATOR_USER default (also baked into the --user help at parser-build time), unknown-key fallbacks, and non-zero returns when the lock backend fails
• test_service.py — exact subprocess command lines for api, listener, beat (seven schedulers, one per task module), flower, and reconciler (env/CPU concurrency logic); watchdog observer setup on /opt/configuration/inventory, cleanup via finally, on_any_event rebinding, and the inventory event triggering reconciler.run.delay(); unknown service type as silent no-op
• test_container.py — parser host/REMAINDER split, one-shot SSH docker execution (key, known-hosts option, operator user), known-hosts init failure warning, interactive prompt loop, and Exit/exit/EXIT loop termination
• test_configuration.py — REMAINDER forwarding, delay(..., auto_release_time=60), rc propagation via handle_task; pins the latent builtin-format argument
• test_set.py / test_noset.py — state playbook scheduling parametrized over the Maintenance/Bootstrap command pairs (status=True vs. status=False)

Extended

• test_reconciler.py — deprecation warning and concurrency of the deprecated worker runner; Sync wait/no-wait paths, --task-timeout forwarding, and the OSISM_TASK_TIMEOUT parser default (the existing timeout test is kept unchanged)

All Celery task calls, subprocesses, and the Redis task lock are mocked at the command module (lazy imports patched at the source module); the tests also assert that check_task_lock_and_exit runs before any task is scheduled or process is started.

Closes #2362

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've left some high level feedback:

• Several tests explicitly codify latent bugs as current behavior (e.g. parsed_args.task list passed to control.revoke, empty tasks module for unknown worker type, builtin format to handle_task); consider marking these with a clear TODO/xfail or a dedicated test naming convention so they’re easy to find and update when the underlying bugs are fixed.
• The test_lock_user_help_bakes_in_operator_user_at_parser_build_time test relies on parser._actions, which is an internal argparse detail; it would be more robust to locate the --user action via a public API or helper rather than depending on private attributes.
• There is repeated setup logic across command tests for env-based concurrency defaults and check_task_lock_and_exit ordering; consider extracting shared helpers/fixtures (e.g. for building parsers and asserting lock-check-before-schedule semantics) to reduce duplication and keep behavior changes easier to maintain.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Several tests explicitly codify latent bugs as current behavior (e.g. `parsed_args.task` list passed to `control.revoke`, empty tasks module for unknown worker type, builtin `format` to `handle_task`); consider marking these with a clear TODO/xfail or a dedicated test naming convention so they’re easy to find and update when the underlying bugs are fixed.
- The `test_lock_user_help_bakes_in_operator_user_at_parser_build_time` test relies on `parser._actions`, which is an internal argparse detail; it would be more robust to locate the `--user` action via a public API or helper rather than depending on private attributes.
- There is repeated setup logic across command tests for env-based concurrency defaults and `check_task_lock_and_exit` ordering; consider extracting shared helpers/fixtures (e.g. for building parsers and asserting lock-check-before-schedule semantics) to reduce duplication and keep behavior changes easier to maintain.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[ideaship]

This pins an unknown service type passing without error. The test is green only because service.py silently does nothing for an unrecognized type, and latent_bug does not exclude it from the default suite — so whoever adds the error path turns this test red and must rewrite it. Assert the intended behavior (unknown type should error) and fix service.py in a dedicated commit.

[ideaship]

This pins a broken command as intended output. An unvalidated short type makes queue[:-8] empty, producing celery -A osism.tasks. worker …. Reject unknown worker types (or map them explicitly) in a dedicated commit, and assert the error rather than the malformed command.

[ideaship]

This pins a parser that rejects the Ansible options its arguments remainder exists to forward. nargs=REMAINDER only starts capturing at the first positional, so osism configuration sync -e foo=1 is rejected — and since every forwardable option starts with - and there is no natural leading positional here, no option can be passed in practice. Switch to the parse_known_args approach already used in openstack, in a dedicated commit, and assert the option is forwarded.

[ideaship]

This pins a real bug — the wrong value is passed. Only this caller passes the builtin format instead of a format string (manage.py, netbox.py → "script"; apply.py, validate.py → parsed_args.format); it is inert today only because wait=True short-circuits before it is read, but --no-wait (as reconciler.Sync already has) would make the else branch print nothing. Fix production to pass "log" (or wire --format) in a dedicated commit and assert that; do not pin the builtin as intended.

[ideaship]

This is the only nargs=1 positional passed on without [0] unpacking — service.py:25, worker.py:34, and set/noset/container all unpack. control.revoke accepts both a string and a list, so the current call is not broken; this is a sibling-consistency gap, not a bug. If uniformity is the goal, unpack here too — app.control.revoke(task[0], terminate=True) — in a dedicated commit, and drop the latent_bug marker; keep the behavioral assertion.

[ideaship]

The commit message's lock-ordering claim is false because the ordering is missing on this path. reconciler.Run.take_action (osism/commands/reconciler.py:14-27) starts a worker subprocess with no check_task_lock_and_exit() — only Sync calls it (:49) — so the commit body's "before any task is scheduled or process is started" does not hold, and this _run_worker helper neither patches nor asserts the lock. It is a deprecated path (its replacement service.Run guards the lock), so severity is low. Pick one: (a) add the missing check in a dedicated commit — then the commit message is accurate as written and this test should assert the lock; or (b) leave the deprecated path and correct the commit message to name reconciler.Run as the exception rather than claiming universal coverage.

[ideaship]

These assertions would block a safe rewrite. call_args[1] == {"shell": True} plus the exact command string would fail a future hardening to an argv vector (subprocess.call([...]), no shell=True) for no behavioral reason. Assert only the observable contract — ssh targets testuser@node1 and runs docker ps -a. Separately, the shell=True interpolation in osism/commands/container.py:47,55 (already # FIXME'd) is a pre-existing, codebase-wide hardening candidate for its own PR; it is operator-supplied input, so not a security blocker here.