fix(slm): declaration-gate every group the update playbook deploys to (#14552)

[mrveiss]

Thinking Path

The fleet update failed again on the same [vnc, slm-agent] node, with a different task:

TASK [[PLAY 2] Frontend | Rebuild production dist]
fatal: cmd "npx vite build" -> [Errno 2] No such file or directory: b'npx'

No Node.js, because it is not a frontend node. It was handed the frontend deploy anyway.

This is #14513 recurring, and the reason is my scoping. That fix gated slm_server, backend and main because I had checked what those plays do. I read one range of update-all-nodes.yml, saw two gates, and gated one of them — leaving the other half of the same play open.

Deriving the set rather than reading it immediately showed the problem was bigger than the report:

deploy gates in the playbook: aiml, backend, browser, frontend, npu
declaration-gated (#14513):   backend, main, slm_server
missing:                      aiml, browser, frontend, npu

Three more than the failure that prompted this. Hand-maintaining a list that mirrors a playbook is what let frontend through, and it would have let the next one through too.

What Changed

_DECLARED_ONLY_GROUPS now covers every group the update playbook deploys to: {slm_server, backend, main, frontend, aiml, npu, browser}.

services/inventory_deploy_groups_test.py derives the expected set from update-all-nodes.yml — the when: "'x' in group_names" gates plus Play 1's hosts: slm_server — and fails if the runtime constant omits any of them. Add a component to that play without listing it here and the guard fails.

The constant stays in code: production must not parse a playbook to build an inventory. Only the assertion is derived.

Verification

Not run from the checkout, by instruction — CI verifies correctness, the deployment verifies runtime.

gates: [aiml, backend, browser, frontend, npu]   gated: [aiml, backend, browser, frontend, main, npu, slm_server]   MISSING: none

vnc node gets: ai, ai_stack, autobot, autobot_cluster, browser_worker, database, infrastructure,
               llm_nodes, npu_worker, npu_workers, production_vms, redis, slm, slm_nodes
               -> no frontend, no backend, no aiml, no npu, no browser, no slm_server

declared frontend        keeps frontend : True
declared ai-stack        keeps aiml     : True
declared npu-worker      keeps npu      : True
declared browser-service keeps browser  : True
declared backend         keeps backend  : True

Mutation: with #14513's constant the guard reports missing: [aiml, browser, frontend, npu] and fails.

Deliberate boundary

The node still joins sibling group names the deploy gates do not test — ai_stack, npu_worker, browser_worker, llm_nodes. Those are not gated here because the criterion is what update-all-nodes.yml actually deploys to, and widening to every similarly-named group would be a guess rather than a derivation. If another playbook gates deploys on those names, that is a separate derivation and a separate change.

redis/database remain reachable by detection on purpose — a node genuinely running redis should keep receiving redis updates, pinned by test_detected_roles_merged_with_roles.

Model Used

Claude Opus 5

Closes #14552

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review — BLOCK

The runtime fix is correct and the reach claim is accurate for the file it covers. What blocks is the mechanism that is supposed to stop a fourth recurrence: the "derivation" is not a derivation.

Reach, re-derived independently: 6/6 for this playbook

Deploy-gated groups in update-all-nodes.yml, counted semantically rather than by quoting style: backend, frontend, npu, browser, aiml from group_names gates, plus slm_server targeted directly by Play 1's hosts:. _DECLARED_ONLY_GROUPS covers all six. database (Play 2b) runs the redis role code-only and is not a component-tree deploy — matching your stated boundary. So the claim holds for its stated subject.

Critical — the guard matches 33 of 60 occurrences and cannot detect its own under-derivation

inventory_deploy_groups_test.py:49-58:

return set(re.findall(r"when:\s*\"'([a-z_]+)' in group_names\"", text))

That matches only the inline double-quoted spelling. Measured against the playbook:

'... in group_names' occurrences : 60
matched by the PR regex          : 33
matched by a style-agnostic scan : 60

Both scans return the same five names today — but only by accident of file history. The majority of the npu/browser/aiml gates are block-scalar and invisible to the regex; those three names survive solely because one later differently-styled occurrence of each exists in the role-owned-files section added by #13460.

The two tests cannot catch this:

• test_the_derivation_finds_the_playbook_and_its_gates pins only that backend and frontend are present. aiml, npu and browser could vanish from gates entirely and it stays green.
• test_every_deploy_gated_group_is_declaration_gated asserts gates ⊆ _DECLARED_ONLY_GROUPS. Since the constant is a static superset, a shrunk gates passes trivially.

So a future PR adding a sixth deploy-gated group in the block-scalar style — the dominant style in this very file, and the natural thing to copy — is not seen by the derivation, and both tests report green while #14513/#14552 recur a fourth time.

This is the "guard narrower than its own subject" shape, and it matters more here than usual because the PR's whole argument is "hand-maintaining a list that mirrors a playbook is what let frontend through". The list is still hand-maintained; only the check on it was supposed to be derived, and it is not.

Fix before merge:

1. Match the gate regardless of YAML scalar style — resolve the when: value (yaml.safe_load, or at minimum scan for '([a-z_]+)'\s+in\s+group_names across the gated-task text rather than requiring the quoted when: line).
2. Make the sanity check require the full known set or a minimum count, not two names. A derivation that can silently shrink is the failure being guarded against.
3. Keep it loud: if the playbook path is wrong or the scan yields zero gates, error rather than pass.

High — file this, do not fold it in: the same defect class is still open one playbook over

provision-fleet-roles.yml:346,388,545,578,650 gates real component provisioning on role_backend_active / role_frontend_active / role_ai_stack_active / role_npu_worker_active / role_browser_active. Each fact (vars/role_active_facts.yml:31-60) is an OR:

inventory_hostname in groups.get('x', [])     # the ansible group — filtered by this PR
OR  'x' in (node_roles | default([]))          # NOT filtered

node_roles is stamped from _union_roles(node) at inventory_builder.py:307 — declared and detected, never passed through _strip_undeclared_privileged_groups. So a node that merely detected as frontend still trips role_frontend_active, and provisioning runs the frontend role's npm/vite install on it through a route this PR does not touch. Same signature, same five components, different file.

Your title says "the update playbook", singular, and that is honest. But the body closes as though the bug class is shut, and it is not — please open the follow-up before this merges so the closure of #14552 does not read as closure of the class.

Low — main is not a deploy gate

update-all-nodes.yml never tests 'main' in group_names; main exists only as a co-emitted alias of backend (inventory_builder.py:101-104). Including it is harmless and arguably right for alias consistency, but "every group the update playbook deploys to" is slightly imprecise. One comment line saying it is a defensive alias strip fixes that.

Verified clean

No over-tight regression. inventory_builder.py:411 computes candidates from declared+detected, then strips only members of undeclared - declared_groups where declared_groups is declared-only (:277). A node that genuinely declares frontend keeps it. The quiet failure mode of an over-tight gate — a legitimate node silently dropped from a deploy it needs — does not occur.

The hand-listed constant is also the right layer: the file's own comment that production must not parse a playbook to build an inventory is a sound boundary. The architecture is fine; only the guard on it needs to be real.

[mrveiss]

Follow-up filed for the provisioning bypass: #14560. That keeps closing #14552 from reading as closing the bug class. The derivation regex is still blocking here.

[mrveiss]

Review response

Finding 1 accepted and fixed — this was the real risk, and it was mine to catch.

Verified before acting: _upsert_node_role creates a NodeRole row and commits, and never touches Node.roles. So "Assign Role Manually" left the role undeclared, _strip_undeclared_privileged_groups stripped the group, the update playbook's tasks never fired, and the component silently never installed — while the admin saw the assignment succeed.

That is worse than the failure this PR fixes: a loud playbook error is recoverable, a silent no-op after an explicit operator action is not. And I had flagged exactly this shape as "the real risk in this change" when dispatching the review, then did not check the write path.

Fixed at the source rather than by exempting the path: _declare_role_on_node now records the role on Node.roles in both the create and update branches, matching what api/npu.py already does for the NPU modal — which is precisely why npu-worker never exhibited it.

nodes_role_declaration_test.py drives the real function (AST-extracted, since importing api/nodes.py registers routes): assignment reaches Node.roles without discarding existing ones, repeat assignment does not duplicate, a None roles column is handled, and a missing node is a no-op rather than a crash.

Finding 3 fixed while the file was open. You are right that it worked only by coincidence. The regex anchored on when: and double quotes, missing the folded-scalar and list forms that are already the dominant style for the npu and browser gates in that same file. Every gated group happened to also appear once in the single-line form, so the derived set was correct today — and a new gate written in the folded style would have been dropped silently, with the rule reporting "missing: none" while the hole it exists to catch was open. That is the failure mode this PR is about, inside the guard for it.

Now matches '([a-z_]+)'\s+in\s+group_names regardless of the syntax carrying it. Same five groups today, confirming the coincidence; robust to the style going forward.

Finding 2 filed as #14567. Confirmed live rather than hypothetical: setup-ai-stack.yml:19 targets hosts: ai_stack and setup-npu-worker.yml:31 targets hosts: npu_worker — sibling names _ROLE_TO_GROUPS grants alongside the gated aiml/npu, reachable from the same /infrastructure/execute surface with limit_hosts optional. The issue records that browser-service does not have the gap (setup-browser-worker.yml targets the already-gated browser, despite the target_hosts=["browser_worker"] metadata saying otherwise), and that site.yml's sibling targets appear dead but should be confirmed rather than assumed.

I did not fold it into this PR: the right fix there is probably to collapse the sibling aliases rather than add four more names to a gate list — which is the same enumerate-versus-derive lesson, and a larger change than unblocking the fleet update.

Finding 4 noted, no change. Agreed importorskip makes a skip indistinguishable from a pass for a correctness-critical guard, but it is the repo convention for optional test deps and changing it here would be a policy decision rather than a fix.

[mrveiss]

Re-review — block cleared, APPROVE

The derivation is fixed at head (eae055a06):

return set(re.findall(r"'([a-z_]+)'\s+in\s+group_names", text))

Re-derived against the playbook at PR head: ['aiml', 'backend', 'browser', 'frontend', 'npu'] — all five, from all 60 occurrences rather than the 33 the anchored form matched. _DECLARED_ONLY_GROUPS covers those plus slm_server and the main alias. The Critical is closed.

test_ordinary_groups_are_not_swept_in is a better addition than anything I asked for. I had checked the over-tight direction by hand and reported it clean; you have now pinned it, so redis/database staying detection-granted is a property the suite defends rather than something the next reviewer re-derives.

Two residual nits, neither blocking:

1. _plays_targeting_a_group_directly() & {"slm_server"} narrows a general derivation to one literal. The function returns every play's hosts:, but the intersection means a future play targeting, say, hosts: aiml directly — with no group_names gate — is not checked. Stronger form: assert every play-targeted group that is not a known carrier (localhost, infrastructure, database) is in _DECLARED_ONLY_GROUPS.

2. The sanity check still requires only backend and frontend. That mattered a great deal when the regex could silently under-match; with the derivation now style-agnostic it is much weaker. Still, requiring the full known set costs one line and makes a shrinking derivation impossible to miss.

The follow-up for the provisioning half is #14560 — role_active_facts.yml activates privileged roles from unfiltered node_roles, so the same failure signature is still reachable through provision-fleet-roles.yml. That keeps closing #14552 from reading as closing the bug class.

Approving. I'll merge once the required contexts are confirmed green against the current head.