Skip to content

chore(ansible): the native-services playbook states the deployed source root as a literal twenty times #15687

Description

@mrveiss

Raised in review of #15686 against one task, declined there as out of scope, and filed here as the whole finding.

What is repeated

autobot-slm-backend/ansible/playbooks/deploy-native-services.yml states the deployed source root as a literal 20 times, across five plays, in six different shapes:

shape occurrences
synchronize: dest: 4
file: path: for the follow-up chown 4
systemd WorkingDirectory= 4
shell cd 2
pip: requirements: and its -c 2
nginx root, PYTHONPATH=, and one more dest: 4

Each play writes the destination itself with synchronize before reading from it, so there is no correctness bug here today: the path cannot be absent unless the sync failed, and a variable would not change that. This is the SSOT rule applied to a value that is genuinely one value, not a defect report.

Why it was not done in #15686

Two reasons, both worth carrying into whoever picks this up.

It is not a local repetition. #15686 touched two of the twenty. Naming an NPU-only variable for those two would leave one play spelling one path two ways while the other four plays kept the literal — worse for a reader than the uniform literal it replaced. The fix is per playbook, not per task.

It costs guard coverage unless that guard is taught first. scripts/check_ansible_file_references.py resolves ansible values to repo paths only where the value is literally prefixed /opt/autobot/src/, under _FILE_KEYS = ("requirements", "src", "chdir", "creates"). That guard exists for #13744 — which was this playbook's NPU requirements: line pointing at docker/npu-worker/requirements.txt, a path that never existed in the repo, so the documented native NPU deploy installed nothing and still reported success. Templating those values removes them from the guard's population, and its only vacuity floor is plays_read >= 1, so the loss would be silent.

So the guard change is a prerequisite, not a follow-up: it must resolve {{ some_var }}/autobot-npu-worker/requirements.txt before the literals move, or #13744 becomes undetectable again.

Acceptance criteria

  • check_ansible_file_references.py resolves a templated deploy-root reference to its repo path, with a test that a wrong path behind the template is still caught — the fix(ansible): NPU play installs from docker/npu-worker/requirements.txt, which does not exist #13744 case, restated against the templated form
  • The playbook names the deployed source root once and every one of the 20 sites reads it
  • The count of references that guard reports does not fall — quoted in the PR, before and after, since it is the only evidence that coverage survived
  • repo_tests/ansible_manifest_resolution.py still derives autobot-npu-worker/requirements.txt for /opt/autobot/autobot-npu-worker/venv (it resolves the templated form through _existing_suffix, so this should hold — assert it rather than assume it)

Related

Activity

  1. mrveiss commented on Sep 7, 2026

    @mrveiss
    OwnerAuthor

    Closed by #15901, merged as 539873f75. Verified against merged base.

    AC Evidence
    check_ansible_file_references.py resolves a templated deploy-root reference to its repo path, with a test that a wrong path behind the template is still caught The #13744 defect was restored behind the template and confirmed still caught — verified end to end rather than by inspection
    The playbook names the deployed source root once and every one of the 20 sites reads it Single definition, 20 sites templated
    The count of references the guard reports does not fall Held at 1 — and the criterion turned out to be weaker than it reads. See below
    ansible_manifest_resolution.py still derives autobot-npu-worker/requirements.txt for the npu-worker venv NPU = "autobot-npu-worker/requirements.txt" at :67, resolved through _existing_suffix; asserted rather than assumed

    The third criterion could not have failed, and fixing that is what this issue actually bought

    It reads as the coverage guarantee — "the only evidence that coverage survived". Review found two defects in it:

    1. _resolve() returns None for anything unresolvable and the caller does continue. A reference that stops resolving leaves the population rather than failing. The floor was >= 1 against 218 host patterns: it detected the walk reading nothing, and nothing else.
    2. The measured and templated populations were never the same set. The guard tracks _FILE_KEYS assignments, of which exactly one is a deployed-src reference, while this change moved roughly twenty literals. An unchanged 1 is a statement about one reference offered as evidence about twenty. In the author's words: "not a weak number, a number about a different thing."

    Both fixed. _referenced_repo_paths now returns (resolved, unresolvable), the summary reports 1 resolves / 168 unresolvable where it previously hid 168 entirely, and the floor is asserted in the test file — a fact about this repository — rather than in main(), which must work on an arbitrary tree.

    Two wrong turns on the way, both caught by the existing suite: failing on all 168 was over-fire ({{ item }} in a loop, {{ cert_file }} at run time, network subnets in group_vars are legitimately unresolvable), and putting the floor in main() broke test_a_play_resolving_nothing_is_still_a_pass. That test exists to protect a legitimate empty case, and it is what stopped a reach floor becoming a false positive.

    A criterion that names a single number as its only evidence is worth re-reading before it is ticked. This one was satisfiable by a number that could not move in the direction the failure would move it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions