fix(slm): shared TLS cert on vnc/redis-only nodes + persistent service-restart cap (#16020, #16712)

[mrveiss]

Closes #16020, Closes #16712, Refs #16970

Thinking Path

66 handed me a stale local branch covering #16020 and #16712, with instructions to verify against CURRENT main first rather than revive the branch. Doing that changed the shape of both:

#16020: 5 of the issue's 6 acceptance criteria were already satisfied independently on current main (the shared ensure_node_tls_cert.yml task, every provisioning role including it, nginx not starting before the cert exists, the role-sweep guard test, the diagnosable failure mode). The stale branch's cherry-pick would have reintroduced an OLDER, already-superseded version of roles/backend/tasks/main.yml over main's own newer consolidation -- caught by diffing against origin/main before committing anything, not by re-reading the issue. The one real remaining gap: update-all-nodes.yml (the only path a GUI/self-update reaches) never ensures the shared cert for a node whose only cert-consuming role is vnc (no group_names entry to gate on) or for a database node reached only through Play 2b (redis's role task file has the shared task, but this playbook applies it via tasks_from: code_only, which skips it). Fixed with two small, correctly-gated tasks -- no changes to roles/backend/tasks/main.yml needed at all, so this never touched the ansible vehicle #17062's files.

#16712: re-investigated fresh against current main too. _create_max_attempts_service_event already exists and fires a NodeEvent on exhaustion, so the "surfaced to an operator" criterion turned out to already be satisfied -- not something I needed to add. The real gap was narrower than the stale branch assumed: the tracker itself (self._service_remediation_tracker, process memory) and the fact that reset_service_remediation_tracker had zero callers anywhere in the codebase, so "a service that recovers resets its count" was never actually true even though the method existed.

#16970: 66 asked only for the narrow SSOT Configuration Compliance fix to ride in this PR, not #16970's full staged-swap-publish scope (which stays on its own PR). provision-fleet-roles.yml's co-location detection had a hardcoded /opt/autobot/autobot-frontend/dist fallback for a bare hosts: all play that never loads roles/frontend's own frontend_dist_dir default -- replaced with base_dir (the existing group_vars/all.yml global), same resolved path, no baseline entry added. Verified against the actual gate script (check_baseline_no_growth.sh), not just eyeballed.

The push itself was blocked by a pre-existing, general infrastructure defect, not by anything in the three issues above: tools/git-hooks/pre-push selected services/reconciler_playbook_timeout_14524_test.py and api/nodes_test.py together (both under autobot-slm-backend/), and repo_tests/sys_modules_leak_guard.py failed the run. Root cause, confirmed empirically by reading _pytest/pathlib.py: pytest.ini's --import-mode=importlib collects a services/*_test.py file as the dotted module services.<name>, and before it can, _pytest.pathlib._import_module_using_spec checks hasattr(sys.modules["services"], "__path__") to decide whether conftest's MagicMock stub already "looks like" a real package. It doesn't (__path__ isn't a magic method MagicMock pre-configures), so pytest reimports services/__init__.py for real, permanently replacing the stub for the rest of the process. Confirmed this is general, not specific to the touched file, by reproducing the identical leak against reconciler_remediation_outcome_test.py and a2a_card_fetcher_test.py -- both untouched by this branch. Fixed at the source in conftest.py, not worked around in a test file: give the stub a real __path__ (the same technique the file already uses for its api stub), plus pin its setUpModule/setup_module/tearDownModule/teardown_module/pytest_plugins so pytest's now-triggered Package.setup() (which reads those off it) doesn't choke on a MagicMock auto-vivifying them instead of leaving them absent.

What Changed

• autobot-slm-backend/ansible/playbooks/update-all-nodes.yml -- a role_vnc_active-gated TLS-ensure task in Play 2 (vnc has no group_names entry -- services/inventory_builder.py::_ROLE_TO_GROUPS -- so the existing backend/frontend group_names gate pattern can't reach it), and a role_redis_active-gated one in Play 2b, both before any task that could restart the service reading the cert.
• autobot-slm-backend/ansible/playbooks/provision-fleet-roles.yml -- the SSOT hardcode fix described above.
• repo_tests/update_all_nodes_ensures_shared_tls_cert_16020_test.py (new) -- reads the playbook as text (no live inventory to parse it as a real playbook) and asserts both new tasks exist, are correctly gated, and run before any task that could restart the reading service. Includes the "vnc has no _ROLE_TO_GROUPS entry" pin so the guard can tell "gated correctly" from "gated by luck."
• autobot-slm-backend/services/reconciler.py -- _service_remediation_tracker (process memory) removed. _remediate_failed_service, reset_service_remediation_tracker (now DB-backed, async) and _update_existing_service read/write the tracker via Service.extra_data["remediation"] instead -- the row already read and written every heartbeat. _update_existing_service clears it the moment a heartbeat observes the service as no longer FAILED.
• autobot-slm-backend/services/service_remediation_tracker.py (new) -- read_service_remediation/write_service_remediation/clear_service_remediation, extracted out of reconciler.py (grandfathered at 2222 lines, no room to grow) rather than added inline.
• autobot-slm-backend/api/nodes.py -- acknowledge-remediation now also calls the (now-wired) service-level reset, since it previously had zero callers.
• autobot-slm-backend/services/reconciler_playbook_timeout_14524_test.py -- its hand-written SimpleNamespace Service stand-in needed extra_data added, or the new read path AttributeErrors.
• autobot-slm-backend/services/reconciler_service_remediation_persistence_16712_test.py (new) -- persistence round-trip, survives a fresh ReconcilerService() instance (simulating a restart), auto-reset on recovery with a same-heartbeat-keeps-it-while-still-failed control, and the operator-reset path with its own no-op control.
• autobot-slm-backend/conftest.py -- the pre-push-blocking sys.modules leak fix described above (services stub: real __path__, setUpModule/setup_module/tearDownModule/teardown_module/pytest_plugins pinned).
• repo_tests/sys_modules_leak_baseline.txt -- deleted services/a2a_card_fetcher_test.py's line: the same fix incidentally stopped it leaking too, and the baseline is shrink-only by the guard's own rule.
• autobot-slm-backend/ansible/playbooks/update-all-nodes.yml -- also rewords the nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020 VNC-task comment (prose only, no logic change): it contained the literal text 'vnc' in group_names as an example of what the task does not do, which services/inventory_deploy_groups_test.py's regex-over-raw-text gate derivation matched as if it were a real Jinja gate -- a self-inflicted false-positive test failure, caught by running the broader services/ suite, not just the two files the hook selects.

Verification

• Re-checked every nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020 acceptance criterion against current origin/main (not the stale branch) before writing anything; 5 of 6 were already met independently.
• pytest (SLM backend + repo_tests, CI-parity venv), each in its own invocation matching CI's actual two-invocation split: 65 passed (services/ -k reconciler), 9 passed (the two nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020 guard files), 51 passed (the leak guard's own test suite plus the two nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020 repo_tests files).
• The exact combination tools/git-hooks/pre-push selects for this diff (services/reconciler_playbook_timeout_14524_test.py, services/reconciler_service_remediation_persistence_16712_test.py, api/nodes_test.py): 20 passed, exit 0, no leak warning. Broader sweep for confidence beyond what the hook selects -- the whole services/ api/ tree together (729 tests): also 0 new leaks, only the 2 pre-existing baseline entries the fix didn't touch (api/monitoring_applogs_test.py, services/reconciler_check_node_health_test.py).
• Caught one real non-vacuous-test trap in my own new bug(slm): the reconciler's service-restart cap lives in memory, so it resets on every SLM restart and retries a failing service forever #16712 test file before trusting it: the package conftest stubs models.database to a bare MagicMock for the whole session, which silently made status != ServiceStatus.FAILED.value evaluate True unconditionally (a MagicMock never equals a plain string) -- one of the four assertion tests was passing for the wrong reason until the loader bound the real service_status.ServiceStatus onto the stub first.
• Reverted reconciler.py, update-all-nodes.yml and provision-fleet-roles.yml to their pre-fix origin/main content in turn and reran the corresponding new test files: update_all_nodes_ensures_shared_tls_cert_16020_test.py -- 3 of 4 correctly fail; reconciler_service_remediation_persistence_16712_test.py -- 6 of 7 correctly fail (mostly AttributeError, the functions don't exist pre-fix).
• black --check / isort --check / flake8 / bandit: clean on all 7 touched Python files (bandit: only the expected B101 assert-used in test files).
• yamllint on both touched playbooks: only pre-existing, repo-wide line-length warnings; none land on any changed line.
• scripts/check_python_file_size.py --audit-ceilings: clean. reconciler.py sits exactly at its grandfathered 2222-line ceiling (trimmed comments and extracted the new tracker helpers to their own module to get there); api/nodes.py at its 2926-line ceiling.
• pipeline-scripts/check_baseline_no_growth.sh run directly against BASE_SHA=$(git rev-parse origin/main): "no key added and no count increased" -- the actual gate script, not just a visual diff.
• jscpd against the SLM scope (autobot_shared autobot-slm-backend autobot-slm-frontend/src, its own 2945-line pin): 2907, unchanged from before this PR.
• detect-secrets scan (non-destructive, no --baseline write): no findings on any touched file.
• pr-graph who on every touched file before pushing: update-all-nodes.yml/provision-fleet-roles.yml overlap fix(deploy): the user-frontend build sites publish through a staged swap (#15603) #16970 (expected -- 66 directed the SSOT fix here); reconciler.py overlaps chore(train): batch vehicle — 11 approved independent PRs, one CI run #17086, the batch vehicle carrying my own already-approved fix(reconciler): stop a DB-level failure poisoning the heartbeat session (#17070) #17075/bug(slm): one DB timeout in heartbeat service sync poisons the session — every remaining service fails with PendingRollbackError, and logins time out #17070 -- not a collision with another agent's work, just sequencing between two of my own changes; a routine rebase once either lands.

Model Used

Claude Sonnet 5

[coderabbitai]

Warning

Review limit reached

Next included review available in 32 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6d53825b-a897-4916-9306-001e1bee85e6

📥 Commits

Reviewing files that changed from the base of the PR and between 928573e and c5962cf.

⛔ Files ignored due to path filters (3)

• repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
• repo_tests/sys_modules_leak_baseline.txt is excluded by !**/*_baseline.txt
• scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (11)

• autobot-slm-backend/ansible/playbooks/provision-fleet-roles.yml
• autobot-slm-backend/ansible/playbooks/update-all-nodes.yml
• autobot-slm-backend/api/nodes.py
• autobot-slm-backend/conftest.py
• autobot-slm-backend/services/reconciler.py
• autobot-slm-backend/services/reconciler_playbook_timeout_14524_test.py
• autobot-slm-backend/services/reconciler_remediation_tracker_expiry_14465_test.py
• autobot-slm-backend/services/reconciler_service_remediation_persistence_16712_test.py
• autobot-slm-backend/services/service_remediation_tracker.py
• repo_tests/provision_fleet_roles_ssot_default_16970_test.py
• repo_tests/update_all_nodes_ensures_shared_tls_cert_16020_test.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• test(audit): commit the filing-token leak-check, add rotation doc (#13859) #17100 test(audit): commit the filing-token leak-check, add rotation doc (audit worker files issues via ambient gh CLI auth — no vault-owned credential, no revocation path #13859)
• chore(train): batch vehicle 2 — 4 approved PRs (#17093, #17094, #16896, #16899), one CI run #17095 chore(train): batch vehicle 2 — 4 approved PRs (fix(kb): share_facts routes through the canonical ownership index (#16709) #17093, fix(i18n): route TranscriberLayout's settings labels through vue-i18n (#16335) #17094, security(api): gate captcha, metrics and diagnostics routers (#16375) #16896, security(ai-ml): pin HuggingFace model revisions and verify weight integrity (#13034) #16899), one CI run
• security(models): pin 3 unpinned HF loads, widen bandit to npu-worker (#17087) #17088 security(models): pin 3 unpinned HF loads, widen bandit to npu-worker (security(models): 3 more unpinned HF from_pretrained() call sites missed by #13034 and invisible to CI's bandit gate #17087)
• chore(train): batch vehicle — 11 approved independent PRs, one CI run #17086 chore(train): batch vehicle — 11 approved independent PRs, one CI run
• refactor(approvals): unify the general and LLC approval systems (#17043, #17056) #17072 refactor(approvals): unify the general and LLC approval systems (refactor(approvals): consolidate the general and LLC approval systems into one platform approval service with optional company scope (#17038) #17043, fix(approvals): approval-gate comments are recorded as author_type=human whoever the caller is #17056)
• chore(deps): bump the uv group across 1 directory with 2 updates #17066 chore(deps): bump the uv group across 1 directory with 2 updates
• feat(backend): orphan-storage detector framework + code-source clone detector (#17038, #17039) #17050 feat(backend): orphan-storage detector framework + code-source clone detector (Umbrella: SLM Data hygiene page — detect and safely clean up orphaned data across services #17038, feat(backend): orphan-storage detector framework + code-source clone detector, delete through the owning service (#17038) #17039)
• chore(vehicle): land WebSocket auth stack — 4 approved PRs (#16891, #16939, #17018, #17027) #17048 chore(vehicle): land WebSocket auth stack — 4 approved PRs (security(websocket): auth token travels via Sec-WebSocket-Protocol, not the URL (#16457) #16891, security(websocket): every authenticated WebSocket echoes the bearer subprotocol through one helper (#16457) #16939, fix(security): authenticate nine WebSocket endpoints before accept, owner-scope the overseer and workflow sockets, admin-only /api/long-running (#17009, #17010) #17018, fix(ops): long-running operations — create works and records its creator, creator-or-admin access, the panel reads real data, unimplemented starts answer 501 (#17017) #17027)
• chore(vehicle): land CI & hooks infra — 6 approved PRs (#16938, #17032, #16952, #16958, #16882, #16959) #17047 chore(vehicle): land CI & hooks infra — 6 approved PRs (fix(ci): chain formatter dispatch after the branch guard in pre-commit (#16923) #16938, chore(hooks): track the commit-msg hook, install it, and enforce the subject convention (#17029) #17032, fix(tests): prune SKIP dirs during the walk, not after rglob returns (#16601) #16952, test(security): a delegated subagent runs a write its parent is held from (#16950) #16958, security(infra): stop binding ChromaDB to every interface by default (#15317) #16882, fix(infra): gate doc-sync to main-at-origin-tip, move it off post-commit (#16934) #16959)
• security(kb): re-scope #16985 to a guard -- every connector already reaches store_fact #17016 security(kb): re-scope security(secrets): wire credential redaction into the remaining 11 KB connectors (#13708 follow-up) #16985 to a guard -- every connector already reaches store_fact
• feat(agents): one-shot idle notice (#16949) #17015 feat(agents): one-shot idle notice (feat(agents): subscribe once to be told when a peer is next idle #16949)
• feat(ci): add an open-PR cap to the pre-push gate (#17006) #17008 feat(ci): add an open-PR cap to the pre-push gate (Open-PR cap: refuse new-branch pushes at/above AUTOBOT_OPEN_PR_CAP (default 40) #17006)
• fix(agents): the peer channel delivers to the recipient, not the sender; broadcast reaches every other agent (#16986) #17001 fix(agents): the peer channel delivers to the recipient, not the sender; broadcast reaches every other agent (agents: the peer channel never delivers to the recipient — a request lands in the sender's own inbox and the sender answers itself #16986)
• feat(agents): peer-to-peer messaging and idle notice (#16948, #16949) #16996 feat(agents): peer-to-peer messaging and idle notice (feat(agents): deliver peer messages at the recipient's next turn, not mid-task #16948, feat(agents): subscribe once to be told when a peer is next idle #16949)
• fix(test): give result.metadata a real dict in 3 multimodal perf/scalability mocks (#16990) #16995 fix(test): give result.metadata a real dict in 3 multimodal perf/scalability mocks (bug(test): 3 multimodal performance/scalability tests fail — metadata Mock lacks __setitem__ since #15234 #16990)
• feat(auth): SLM API keys only on declared routes, 403 elsewhere, every key request audited; main backend accepts none (#16040, #16294, #16999) #16989 feat(auth): SLM API keys only on declared routes, 403 elsewhere, every key request audited; main backend accepts none (API key scopes are stored and displayed but never enforced — keys inherit full user authority #16040, feat(auth): decide which routes accept API keys, and wire X-API-Key through the #16040 mechanism #16294, docs(api): the public API docs tell integrators to send a user API key to the main backend, which accepts none #16999)
• feat(agents): capture a terminal session's tenant at creation (#16975) #16978 feat(agents): capture a terminal session's tenant at creation (feat(agents): capture a terminal session's tenant at creation — it cannot be recovered afterwards #16975)
• feat(security): bound every Company OS run by its org role, per adapter (#16950) #16974 feat(security): bound every Company OS run by its org role, per adapter (security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950)
• fix(deploy): the user-frontend build sites publish through a staged swap (#15603) #16970 fix(deploy): the user-frontend build sites publish through a staged swap (bug(deploy): the user-frontend build sites still publish their own failures, and three SLM build sites sit outside the shared staged publish #15603)
• fix(security): A2A trust keyed on the verified pair, capabilities enforced where they apply (#16957, #16950) #16969 fix(security): A2A trust keyed on the verified pair, capabilities enforced where they apply (security(a2a): three of four trust-matrix capabilities are enforced nowhere — a level that denies them denies nothing #16957, security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950)
• feat(agents): live presence registry, one query across all three agent kinds (#16947) #16965 feat(agents): live presence registry, one query across all three agent kinds (feat(agents): live presence — one registry of named agents with busy/idle state, across all three kinds #16947)
• fix(security): match injection patterns against what a model reads, not the raw text (#16354) #16937 fix(security): match injection patterns against what a model reads, not the raw text (security(detector): detect_injection matches its patterns before stripping invisible Unicode, so a zero-width character inside a phrase defeats the match #16354)
• fix(ci): python-paths covers the shell wrappers python-suite tests (#16249) #16892 fix(ci): python-paths covers the shell wrappers python-suite tests (python-paths: a change to only check-pre-commit-hook-pr.sh skips the tests that exercise it #16249)
• fix(ci): give the SLM backend's tests their own venv in CI (#16394) #16889 fix(ci): give the SLM backend's tests their own venv in CI (ci: run SLM backend tests in their own venv, so CI stops forcing the backend's websockets<16 cap onto the SLM #16394)
• fix(control): emergency-stop reports what it actually paused instead of a fixed success string #16854 fix(control): emergency-stop reports what it actually paused instead of a fixed success string
• fix(billing): record LLM spend from the chat path and both compat gateways (#16845) #16853 fix(billing): record LLM spend from the chat path and both compat gateways (bug(billing): LLMCostTracker.record() doesn't exist — main chat path's cost tracking and budget enforcement silently broken since April #16845)
• chore(release): promote main to release — 682 commits since v0.8.0 #16801 chore(release): promote main to release — 682 commits since v0.8.0
• fix(hooks,tests): pre-push runs changed test_*.py, and a services/ test no longer swaps the real package over the conftest stub (#16711, #16722) #16728 fix(hooks,tests): pre-push runs changed test_*.py, and a services/ test no longer swaps the real package over the conftest stub (ci(pre-push): a changed test file named test_*.py never runs in the pre-push hook #16711, sys.modules leak guard: reconciler_status_restart_delta_14465_test.py leaks 'services' into a later autobot-slm-backend/tests file #16722)
• feat(secrets): bridge connector credential templates to ConnectorCredentialStore (#16428) #16444 feat(secrets): bridge connector credential templates to ConnectorCredentialStore (secrets: connector credential templates must write where knowledge connectors read #16428)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

[mrveiss]

Carried into vehicle #17119 (member head c5962cfb4 merged server-side, vehicle head 29a976e286). Closing here; acceptance-criteria evidence and CI ride with the vehicle.