fix(updater): re-detect ansible_dir per execute_playbook call, not once at construction (#17150)

[mrveiss]

Refs #17150 (not Closes: the host-confirmation criterion below needs the owner to run a real sync)

Thinking Path

PlaybookExecutor is a module-level lazy singleton (get_playbook_executor): constructed once per SLM backend process, on whichever API call first touches it. __init__ resolved ansible_dir once — preferring code_source's ansible directory, falling back to the deployed copy when that directory did not exist YET — and never looked again. A process that started under the fallback stayed pinned to it for its entire lifetime, even after code_source was brought fully current by the code-sync API (the actual mechanism that populates code_source; _update_code_source here is only a pre-playbook freshness safety net, not what creates it). Restarting the process was the only way to clear it.

This is not a hypothetical. Per the owner's drift check on the live host (read-only): origin/main and /opt/autobot/code_source are both at 358606d25 — code_source is byte-current — while .deployed_commit sits at 1b9f51432840 (last night's #17048 merge), six merges behind. Verified per-file: ai_api_server.py has the #16428 _validation_error_without_input handler in code_source and not in the deployed copy; worker_settings.py has 7 matches for the #17087 weight pins in code_source and 0 deployed. The live ai-stack container is missing #16428's handler, so its 422 responses echo the submitted payload verbatim — a merged security fix sitting on disk, not running, for a day. This is the mechanism, not a coincidence: the git sync step works and is silently syncing the wrong directory while the process stays pinned to the fallback.

Why the fix runs the re-check BEFORE _update_code_source, not after: _update_code_source derives its own sync target from self.ansible_dir (two directories up). While pinned to the fallback, that target is /opt/autobot (no .git), not /opt/autobot/code_source (which has one) — so _update_code_source sees no .git and reports a no-op success, without ever touching the real checkout. Re-checking after syncing would leave this half of the bug in place; a naive fix that only reorders "check, then sync, in that order" without noticing this dependency would look correct and still fail.

What Changed

• playbook_executor.py: extracted the resolution logic into _auto_detect_ansible_dir() (module-level DEFAULT_CODE_SOURCE_ANSIBLE_DIR/FALLBACK_ANSIBLE_DIR constants, so a test can monkeypatch either candidate without touching real /opt/autobot) and _refresh_ansible_dir(), called at the top of execute_playbook() before _update_code_source(). An explicit ansible_dir passed to the constructor (tests, wizard provisioning) is never auto-refreshed — a deliberate caller choice, not the auto-detected default this instance would otherwise track.
• New test file playbook_executor_ansible_dir_self_heal_17150_test.py (4 tests): module loaded from disk like its playbook_executor_timeout_14524_test.py sibling (the package conftest stubs services.*). The core test calls the REAL execute_playbook() end-to-end, not just the extracted resolver, so a future change that moves the refresh call to the wrong spot is also caught.

Sibling candidates (from the filed issue) — handled, not dropped

• The silently non-fatal git fetch/reset failure, _update_code_source's no-.git early-return: this fix directly addresses it — once ansible_dir is re-detected before _update_code_source runs, that method's own directory derivation is correct too, so it stops silently no-opping against the wrong tree.
• AUTOBOT_GIT_BRANCH pointing at a branch other than main: left alone, deliberately. A legitimate, working operator override (same philosophy as SLM_ANSIBLE_DIR) for tracking a non-default branch; a misconfigured value is an operational concern, not a code defect in this issue's scope. Not fixing it here.

Acceptance Criteria

• A PlaybookExecutor instance constructed while code_source's ansible directory does not yet exist, then given a following execute_playbook() call after that directory has since appeared, reads the playbook from the code_source path on that later call — not the cached fallback. Proven via test_execute_playbook_self_heals_before_syncing_and_before_reading_paths.
• SLM_ANSIBLE_DIR env-var override still takes priority unconditionally, unchanged from current behavior — the resolution logic itself is untouched, only extracted and re-invoked; _auto_detect_ansible_dir() is byte-identical logic to the old __init__ block.
• A regression test proves the OLD behavior (permanent fallback caching) would have failed this exact scenario, via a negative control — verified by hand: temporarily removed the self._refresh_ansible_dir() call, re-ran, confirmed test_execute_playbook_self_heals_before_syncing_and_before_reading_paths fails with FileNotFoundError naming the STALE fallback path, restored the fix, confirmed green again.
• Host confirmation — explicitly NOT verifiable from this PR. The observable that actually matters is that a real code-sync run advances .deployed_commit past 1b9f5143 and the deployed files gain the content code_source already has. That requires running an actual sync through the builtin updater on the live host, which only the owner can do and confirm; this PR's own testing proves the code-level self-heal, not the host-level outcome. Left unchecked deliberately — a guessed "yes" here would be exactly the fabrication this repo's rules forbid.

Verification

• 18 tests pass locally (4 new + 14 in the untouched sibling playbook_executor_timeout_14524_test.py, confirming no regression). Environment is below CI's declared dependency floors, so this is not CI evidence — CI is authoritative.
• black/isort/flake8 clean.
• Reach floors measured directly (bug(guards): reach_declarations_test passes locally on a commit CI fails — a local pass is not evidence #17144, not via a local guard pass): prompt-injection-detector-strict-mode unaffected (scoped to autobot-backend/*.py only; this change is entirely under autobot-slm-backend/) — confirmed 3245, unchanged from main. hooks-path-override confirmed safe at its current 6488 (population 6886 with the new test file staged, ceiling 6889) — no re-pin needed.
• File-size ratchet: playbook_executor.py re-pinned 1362 → 1406 for this fix's own lines (both baselines updated).

Model Used

Claude Sonnet 5 (claude-sonnet-5)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Playbook execution now automatically re-detects the Ansible directory when no directory was explicitly configured.
  • Code synchronisation and playbook path resolution now use the current Ansible directory, avoiding stale fallback paths.
  • Explicitly configured Ansible directories remain unchanged.

• Tests

  • Added regression coverage for directory re-detection, explicit path preservation, and execution using refreshed paths.

[coderabbitai]

Warning

Review limit reached

Next included review available in 46 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: 57783dff-0141-4705-ad6b-25b7d7d430d9

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9a0ae and a3f7263.

📒 Files selected for processing (1)

• autobot-slm-backend/tests/services/test_self_update_systemd_detach_11492.py

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Advanced

Run ID: fd95c017-5d45-4cf4-9217-43a5c9296e85

📥 Commits

Reviewing files that changed from the base of the PR and between 3941a06 and 2d9a0ae.

⛔ Files ignored due to path filters (2)

• repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
• scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (3)

• autobot-slm-backend/services/playbook_executor.py
• autobot-slm-backend/services/playbook_executor_ansible_dir_self_heal_17150_test.py
• autobot-slm-backend/tests/services/test_self_update_systemd_detach_11492.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

PlaybookExecutor now refreshes automatically detected Ansible directories before execution. Explicit directories remain unchanged. New regression tests cover detection, synchronisation, path resolution, and existing test helper behaviour.

Changes

Ansible directory self-healing

Layer / File(s)	Summary
Directory detection and refresh autobot-slm-backend/services/playbook_executor.py	The executor records whether ansible_dir was explicit. Automatic detection uses configurable and fallback paths. Automatic paths refresh before code-source synchronisation.
Directory refresh validation autobot-slm-backend/services/playbook_executor_ansible_dir_self_heal_17150_test.py, autobot-slm-backend/tests/services/test_self_update_systemd_detach_11492.py	Tests verify real-module loading, explicit-directory preservation, stale fallback behaviour, refreshed path resolution, and compatibility with the existing executor helper.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PlaybookExecutor
  participant DirectoryDetection
  participant CodeSourceSync
  PlaybookExecutor->>DirectoryDetection: Refresh ansible_dir and inventory_path
  DirectoryDetection-->>PlaybookExecutor: Return detected directory
  PlaybookExecutor->>CodeSourceSync: Update code source using refreshed directory

Loading

Merge Risk: ⚪ Minimal · up to 2d9a0

The updated directory refresh behavior preserves explicitly configured paths and intentionally rechecks automatic selections before execution. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: re-detecting ansible_dir for each execute_playbook call.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.