fix(code-sync): delete files removed from source through the updater, and check drift across every file (#16310, #16322)

[mrveiss]

Closes #16322. Refs #16310 (AC5 needs host evidence from a real GUI update). Refs #16262 (fixed the root cause -- config.py's external_url socket probe at import time -- and shrunk 4 of the 51 known-offender baseline entries this PR's own diff reached; the remaining 47 need a full sweep or other PRs to clear before #16262 itself closes).

Thinking Path

• The problem. A normal code sync never deleted anything, so files removed from source stayed on hosts. The drift check found 16 on this machine, plus a byte-identical nested npu_workers.yaml left by a fixed cwd bug. The owner ruled: no manual patches, and a drift check that covers every file.
• The first design was blocked in review. It ran deletion from the SLM's Python code and wrote the SLM's own .deployed_commit mid-update (CRITICAL: one-click update never redeploys the SLM control plane — self-update skip reads code_source HEAD (advanced by the pull) not the deployed commit #12202). That would have made self-update skip the real sync on every host. A normal Update All also runs a playbook that restarts the SLM part-way through, so Python never sees a success to act on.
• The owner's ruling (deploy: the builtin updater never removes files deleted from source, so deleted modules pile up on every host #16310 comments): deletion runs inside the Ansible roles, right after each component's file sync succeeds, as one mechanism for co-located and remote nodes. deploy: ansible synchronize never deletes files removed from source on remote fleet nodes #16322 folds in. Where a component has no record of what was last deployed, a one-time cleanup runs.
• Eleven agent review rounds hardened it. Each finding is fixed and has a test:
  • its own deletion marker, never .deployed_commit
  • host-state and gitignored files kept
  • a bounded number of git calls, with build folders pruned
  • one node's failure warns and moves on, and never aborts the fleet
  • the marker survives every delete-style sync
  • target-side realpath containment
  • rename-only paths caught
  • non-ASCII names matched
  • injection resistance proven in CI
• The reachability finding. The GUI's Update All applies the backend role only through tasks_from: env_only/unit_only, never main.yml. That's the CRITICAL(deploy): Update-All uses inline tasks and applies only backend/env_only — every fix landing in an Ansible role is inert on hosts (#12777 faulthandler, #12907 consolidation, #12886 TTS all merged but never delivered) #12959 trap. So deletion is wired after every sync task update-all-nodes.yml actually runs, and the tests prove reachability, not just ordering.
• Round 12 came from a second-session review (Helper-02), which blocked the PR on B1. The delete step was a shell task using set -euo pipefail with no executable, so it ran under /bin/sh, which is dash on Ubuntu. dash rejects pipefail ("Illegal option"), so every run fell into rescue and nothing was ever deleted. The tests ran the step with bash -c, so CI couldn't see it. Round 12 fixes B1 and all six non-blocking findings.

What Changed

• Planner (runs on the controller, never on the target):

  • autobot-slm-backend/services/sync_deletions.py: pure planning.
    • diff mode (-M --diff-filter=DR)
    • bootstrap mode (ls-tree plus log -M --diff-filter=AR, 2 git calls in total)
    • new in round 12: a marker holding a commit the controller's clone doesn't have (_commit_known) returns bootstrap_required instead of an error, so it no longer rescues forever in silence
  • services/host_state_filter.py: HOST_STATE_EXCLUDES plus git check-ignore.
  • services/git_subprocess.py: one run_git chokepoint with -c core.quotePath=false.
  • CLI: scripts/sync_deletion_planner.py, which also reports bootstrap_required.

• Ansible:

  • ansible/roles/_shared/tasks/sync_deletions.yml, one block/rescue/always:
    • read the marker, falling back to reading .deployed_commit only
    • plan with delegate_to: localhost, and switch to bootstrap mode when the planner asks
    • deliver the candidate list to the target as a file written by ansible.builtin.copy, read with IFS= read -r. There's no heredoc any more, so a path named like the terminator can't end the list.
    • resolve each candidate with realpath -m on the target, and keep anything outside the root
    • quoted rm -f --, run under executable: /bin/bash
    • write the marker last
  • rescue warns and never writes the marker.
  • always removes both temp files, the one on the controller and the one on the target.
  • The symlink-leaf limitation is commented at the containment step. The repo tracks 0 symlinks.

• Wiring on the update path (playbooks/update-all-nodes.yml): the pass runs right after the sync task of every component the updater touches. Each one is pinned by its exact target directory.

  Component	Sync task	Deletion runs at
  slm-backend (PLAY 1)	:319	:375
  slm-frontend (PLAY 1)	:326	:384
  autobot_shared on the SLM (PLAY 1)	:333	:393
  libs, workspace packages (PLAY 1, new in round 12)	:342	:408
  autobot-plugins, workspace packages (PLAY 1, new in round 12)	:349	:417
  backend	:774	:804
  backend's autobot_shared	:782	:838
  frontend	:1233	:1255
  npu-worker	:1306	:1329
  browser-worker	:1454	:1475
  ai-stack (non-standard source, strip-4)	:1614	:1632
  shared, on non-backend nodes	:1666	:1688
  slm-agent	its own copy tasks	in roles/slm_agent/tasks/main.yml, which both plays import in full

  The provisioning paths (provision-fleet-roles.yml, ansible/site.yml, deploy-slm-manager.yml) keep the main.yml wiring. That's two call sites of one shared task file, not two mechanisms.

• The marker .autobot_sync_deletions_commit is excluded from all 6 delete-style syncs, and it's in HOST_STATE_EXCLUDES.

• AC3: services/slm_frontend_build.py _remove_legacy_previous, called from the publish step (_swap → _publish_build → build_slm_frontend).

• AC4: roles/backend/tasks/npu_workers_cleanup.yml, included on the update path in update-all-nodes.yml and from main.yml. It removes the nested npu_workers.yaml only when both copies exist and their sha256 checksums match, and otherwise reports. DELIVERED["backend"] in tests/test_update_all_applies_roles_12959.py includes npu_workers_cleanup.

• Drift covering every file:

  • services/full_tree_drift.py and GET /code-sync/drift/full, with verdicts modified, removed_from_source (plus its commit), build_bundle and host_state:<category>
  • the SLM GUI's new FullTreeDriftPanel.vue on the Code Sync page, filterable by verdict, with its own error state
  • the regenerated openapi.json/api.ts come from the repo's types bot
  • the existing drift card is unchanged

• Consolidation: the Python apply calls are gone from api/code_sync.py, now 6084 lines instead of 6094, with both ceiling files lowered.

Verification

• Tests:
  • tests/services/sync_deletions_test.py:
    • diff, rename, never-tracked, host-state, gitignored, traversal, bootstrap, rename-only and non-ASCII cases
    • a bounded git-call count for a venv full of files
    • an unknown previous commit gives bootstrap_required, while known and same commits never ask for it
  • scripts/sync_deletion_planner_test.py
  • tests/services/full_tree_drift_test.py
  • repo_tests/sync_deletions_ansible_wiring_16310_test.py:
    • the rescue path never writes the marker, and the marker is written last
    • .deployed_commit is never touched
    • containment runs before removal
    • every delete-style sync excludes the marker
    • the prune list matches deploy_artifacts
    • the npu_workers gating, and its delivery on the update path
    • the provisioning playbooks run the roles in full
    • the hostile-filename canary: it runs the real delete step under the interpreter the task declares (Ansible's /bin/sh default if none is declared), from a list file, including a candidate named like the old heredoc terminator
  • repo_tests/sync_deletions_target_pinning_and_shell_safety_16310_test.py, new in round 12:
    • each sync site's deletion include is pinned by its exact sync_deletions_target_dir, so removing any one fails
    • every shell task in this PR's task files that uses bash-only syntax must declare executable: /bin/bash, with a guard-the-guard test
    • both temp files are removed in always:, never only in rescue:
  • test_site_yml_frontend_play_applies_only_existing_roles is xfail(strict=True) for site.yml's Frontend play references a nonexistent role frontend_app #16342, a pre-existing broken frontend_app reference.
  • FullTreeDriftPanel.test.ts covers the three verdict kinds, the filter, and the panel's own error state.
• Independent safety review of the fleet scope:
  • The path mapping was traced for every component with real deleted files, including ai-stack's --strip-components=4 and slm-agent's per-file copies.
  • A file tracked at the new commit can't become a candidate in either mode, by construction.
  • Model caches, chromadb, logs and data live outside every deployed tree (/var/lib/..., /var/log/...).
• The pre-push hook runs the changed test modules locally. Earlier rounds caught bugs in the tests themselves, all now fixed: a cross-package import, a leaked services stub (fixed by moving two tests under tests/services/), a hostile filename containing /, and an empty commit missing --allow-empty. CI runs everything else. ansible-lint and --syntax-check weren't run locally, and no repo code was executed by hand, per the repo rule.
• Open, and deliberately not claimed:
  • AC5 (0 "removed from source" on all four components after one GUI update) needs host evidence. It should also confirm that nothing outside the repo lives under the browser-worker's install directory.
  • A filename with a literal newline is left in place, never deleted. That's documented as an accepted limitation.
  • A symlink leaf that points outside the tree is refused by containment. 0 symlinks are tracked.
  • Folding slm_manager's old synchronize … delete: true into this mechanism is deploy: consolidate slm_manager's delete:true synchronize into the git-aware deletion pass #16338.

Model Used

Claude Opus 5 as coordinator. A senior-backend-engineer subagent did the implementation over twelve rounds. Each round was reviewed by independent code-reviewer subagents, and round 12 answers a second-session review by another Claude session (Helper-02).

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: fa4a0777-0e84-4f67-bd50-60b91885dce7

---

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.}

[mrveiss]

Two corrections to the description, posted as a comment so editing the description doesn't cancel in-flight CI:

• The number of review rounds: it's eleven, not eight.
• "Not run locally" is no longer accurate. The pre-push hook ran the changed test modules locally and caught three rounds of bugs in the tests themselves, all now fixed:
  • a cross-package import in repo_tests
  • a leaked services stub. The fix moves full_tree_drift_test.py and sync_deletions_test.py under autobot-slm-backend/tests/services/, whose conftest registers services as a hollow real-path package, following manifest_loader_cache_test.py's precedent.
  • a hostile filename containing /, and an empty commit missing --allow-empty

CI runs everything else. ansible-lint wasn't run locally, and no repo code was executed by hand, per the repo rule. The two moved tests are cited in the description at their new tests/services/ paths.

[mrveiss]

Second-session review of 32d73ea: changes requested (1 blocking)

Read from the PR head and base a12b4bf79. I executed nothing from the codebase; the only command run was dash on the test install's host.

Blocking

B1. The delete step can't run under /bin/sh on Debian/Ubuntu, so no host ever deletes anything and no marker ever advances.

• roles/_shared/tasks/sync_deletions.yml:164-186 is an ansible.builtin.shell task. It starts with set -euo pipefail and sets no executable:.
• The shell module runs cmd with /bin/sh, and nothing at this head overrides that. git grep ansible_shell_executable finds 0 hits across the repo, including ansible.cfg, the inventories and services/inventory_builder.py. The same grep does find ansible_python_interpreter there, so the inventory is being read.
• On the test install's host, /bin/sh is dash 0.5.11: dash -c 'set -euo pipefail' gives set: Illegal option -o pipefail, rc 2. That host is the SLM, so it's the co-located target of PLAY 1.
• Result: every run fails into the rescue, which logs a warning and writes no marker. The next run retries from the same baseline, forever. It fails safe, but deploy: the builtin updater never removes files deleted from source, so deleted modules pile up on every host #16310 AC1 and deploy: ansible synchronize never deletes files removed from source on remote fleet nodes #16322 AC1 are never met in practice.
• The repo's own convention already covers this. Every one of the other 18 ansible tasks that use pipefail sets executable: /bin/bash. The only other matches are 7 .sh scripts and templates, which carry their own shebangs. This task is the one exception.
• CI can't see it, because test_the_delete_step_shell_resists_injection_and_path_escapes runs the script with bash -c (wiring test line 369).
• Fix. Either add executable: /bin/bash, or drop -o pipefail, since the script has no pipeline and set -eu is enough. Then pin it with a test: assert the task sets executable, or run the extracted script under sh/dash. Ubuntu runners have dash.

Non-blocking

1. The ordering test doesn't pin each component's own include.
   • test_deletion_task_runs_immediately_after_its_sync_task accepts any later sync_deletions.yml include.
   • Example: for [PLAY 2] Backend | Deploy autobot_shared (line 758), the first include after it is the backend one (780, target autobot-backend). So removing the autobot_shared include (814) would still pass.
   • Suggest matching the include's sync_deletions_target_dir to the deployed component, with no other deploy task in between.
2. Two PLAY 1 includes aren't pinned. SLM | Deploy autobot-slm-backend and SLM | Deploy autobot_shared are missing from _SYNC_THEN_DELETE_SITES. Their includes exist (376, 394); only the slm-frontend site is pinned.
3. Two PLAY 1 deploys have no deletion include. libs and autobot-plugins (unarchive into /opt/autobot/) get none, so files deleted from those workspace packages still pile up on the SLM. They're outside AC5's four components; fold them in, or file them.
4. Heredoc terminator.
   • A candidate whose text is exactly AUTOBOT_SYNC_DELETIONS_LIST ends the list early, and the remaining lines then run as shell, as root.
   • The input is the deploy repo, which is already trusted to ship code, so this isn't an escalation.
   • The cheap fix is to have the planner refuse that name, or to ship the list as a file instead of inline.
5. Temp file left behind. The bootstrap ansible.builtin.tempfile on the controller is never removed: one file per component per node, on the first run.
6. A previous commit the controller's clone lacks never recovers. After a re-clone or a shallow fetch, git diff fails and the node rescues on every run, warning forever with nothing escalated. It fails safe. Consider falling back to bootstrap, or surfacing it in the drift check.
7. Symlink at the leaf. realpath -m on the full path follows a symlink leaf. One pointing outside the root would be refused and would pin the marker, although rm -f on a link removes only the link. This is theoretical today: the repo tracks 0 symlinks.

Verified

• Host-state protection.
  • Both plans send every candidate through HOST_STATE_EXCLUDES, with re-includes taking precedence: .env, .env.*, data, logs, /config/, both markers and /ansible/enroll.yml. They also run git check-ignore at the checked-out commit.
  • security: untrack two committed key files and make secret scanning see the whole tree (#16275) #16300's two untracked key files sit outside every synced component (repo root, autobot-infrastructure/), and its new .gitignore line covers the service-keys directory in any case.
• One-time cleanup gating.
  • The bootstrap plan runs only when both markers are absent.
  • The marker is written last in the block, after the deletion; the rescue never writes it.
  • The marker is excluded from every delete-style rsync and listed in HOST_STATE_EXCLUDES.
  • The npu_workers cleanup stats both files with sha256, removes only on a checksum match, and reports otherwise. It's reachable on the update path via include_role ... tasks_from (PLAY 2, line 795) and on provisioning via main.yml.
• Containment. realpath -m runs on the target, rm -f -- is quoted, and the heredoc delimiter is quoted, so nothing expands. The planner is lexically contained too: no absolute paths, no ...
• block/rescue. A rescued failure doesn't mark the host failed, so it doesn't trip PLAY 2's any_errors_fatal; this is the same pattern as PLAY 1's agent redeploy. The rescue only runs debug.
• Reachability on the update path. An include follows every deploy PLAY 2 runs: backend, autobot_shared twice, frontend, NPU, browser and AI stack. PLAY 1 has includes for slm-backend, slm-frontend and autobot_shared. slm_agent is covered through its main.yml, which both plays import_role in full.
• The test moves. The three module-scope real-loads capture and restore sys.modules in try/finally, including the synthetic services.git_tracker stub. conftest.py stubs services.full_tree_drift, so api.code_sync collects.
• CI. 0 failures and 37 pending at this head, so I'm not citing a green run.

[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.

[mrveiss]

Coordinator's independent review of round 12, 16764c297..3aadb1424: approve. No blocking or medium findings. Helper-02's second-session delta review is separate and still to come.

Checked by running the real step, not by reading assertions:

• B1. The extracted delete cmd: was rerun in a scratch directory. Under /bin/sh (dash) it fails with set: Illegal option -o pipefail, rc 2, and deletes nothing, which is the bug. Under /bin/bash it deletes correctly and refuses the ../outside.txt escape (rc 1). args.executable == "/bin/bash" is set, and the canary reads the interpreter from the YAML (defaulting to /bin/sh), so reverting the fix fails it.
• N4. The rendered script was run against filenames named AUTOBOT_SYNC_DELETIONS_LIST, filenames shaped like $(...), and ones with a leading dash: no injection. IFS= read -r plus the quoted "$target_dir/$rel" never re-parses content. The list file comes from ansible.builtin.tempfile (mkstemp, 0600, an unpredictable path), and copy keeps that mode. The empty-list case never reaches the task.

Confirmed by reading:

• N3. libs and autobot-plugins are archived with git archive -- libs/ and -- autobot-plugins/ (update-all-nodes.yml:128-146), then unarchived to the base directory with no strip (:343-354). The deletion pass's source and target directories match that.
• N6. git cat-file -e <sha>^{commit} runs as an argv list with no shell. There's a warning in both the logger and an Ansible debug task, and compute_bootstrap_plan is untouched.
• N1/N2. The pins key on each include's unique sync_deletions_target_dir, so removing any one fails. The cross-module import from repo_tests is structurally sound (__init__.py, pythonpath = ., importlib mode). The import itself wasn't executed; CI does that.
• Ratchets. The original test file is exactly 674 lines, its frozen ceiling. The new files are well under 600.

Low, not pursued in this PR:

• _commit_known doesn't hex-validate the marker content. It's not exploitable: there's no shell, and a malformed value makes git exit non-zero, which is the safe bootstrap fallback.
• One pre-existing 51-line test function, which round 12 shortened from 56.

[mrveiss]

Second-session delta review, 16764c297..3aadb1424: B1 and N1-N7 resolved; 3 non-blocking notes

Read from git objects at 3aadb14. Nothing executed. CI at this head: 0 failures and 30 pending, so no green is cited.

Resolved

• B1. The delete step declares args: executable: /bin/bash (sync_deletions.yml:252-258).
  • test_bash_only_shell_tasks_declare_the_bash_executable scans every bash-only shell: task in the new task files. test_the_guard_can_see_a_bash_only_shell_task is its known positive.
  • The hostile-filename canary now runs [<declared interpreter>, "-c", script], defaulting to /bin/sh. Removing the executable: would therefore run it under dash on an Ubuntu runner and fail. That's the mutation the old bash -c could never catch.
• N1/N2. _SYNC_THEN_DELETE_SITES pins each site to its exact sync_deletions_target_dir: all five PLAY 1 unarchives and all seven PLAY 2 deploys, plus the four role sites. See note 1 below for the one site this doesn't fully pin.
• N3. libs (update-all-nodes.yml:408) and autobot-plugins (:417) get includes after the autobot_shared one, with source code_source/{libs,autobot-plugins} and target {{ autobot.base_dir }}/…. Both sites are in the table.
• N4. There's no heredoc any more. The list is a target-side tempfile plus copy (:205-218), read with IFS= read -r (:232-247). AUTOBOT_SYNC_DELETIONS_LIST is now an ordinary canary candidate.
• N5. Both temp files are removed in always: (:288-301), guarded by .path is defined. That correctly covers a skipped register, which has skipped: true and no path. Two tests pin it: one that cleanup happens in always:, and one that it never lives only in rescue:.
• N6.
  • compute_deletion_plan checks cat-file -e <commit>^{commit} and returns bootstrap_required=True with no error. The CLI then exits 0 with the field set.
  • sync_deletions.yml:124-137 folds that into _sd_need_bootstrap and warns.
  • Four new tests cover it: the unknown commit (service and CLI), a known commit, and the same-commit no-op.
• N7. Documented at the containment case (:235-238).

Non-blocking

1. One site's pin can still be satisfied by the wrong include.
   • [PLAY 2] Backend | Deploy autobot_shared (backend nodes) and [PLAY 2] Shared | Deploy autobot_shared (every other node) both target the literal /opt/autobot/autobot_shared.
   • test_deletion_task_matching_target_dir_follows_its_sync_task searches _flatten(...) from the sync task onward across the whole playbook. So if the backend-node include (:814 area) were deleted, the backend site would find the later non-backend include (:1669 area) and still pass. Backend nodes would then lose their autobot_shared deletion pass: N1's shape, for the one duplicated target.
   • Fix: stop the search at the next deploy/sync task, or match the include's when: as well as its target dir.
2. The always: cleanup is the one path the rescue doesn't cover.
   • A task failing in always: isn't rescued, so under PLAY 2's any_errors_fatal: true it would abort the fleet. That breaks the invariant this file states for everything else.
   • file: state=absent rarely fails, but a leaked temp file is the lesser harm.
   • Fix: add failed_when: false (or ignore_errors: true) to both cleanup tasks, with a comment saying why.
3. Make the fallback's boolean explicit.
   • _sd_plan (:191) selects with a Jinja if _sd_need_bootstrap, on a fact built by set_fact from a folded expression.
   • If that fact ever came through as the string "False", the if would be truthy. The skipped bootstrap result would then be selected, fail from_json, and send every normal diff-mode run to rescue.
   • I believe ansible-core yields a real bool for a single-expression value, but no test exercises this path. (_sd_need_bootstrap | bool) there, and when: _sd_need_bootstrap | bool, removes the question.

None of these block. B1 was the blocker, and it's fixed and pinned.

[mrveiss]

Round 13, 3aadb1424..ae04398c2: Helper-02's three notes, plus two guards for single-node installs.

The three notes from Helper-02's round-12 delta review:

1. A pin could be satisfied by the wrong include. The two PLAY 2 autobot_shared deploys target the same directory, so removing the backend's deletion include still passed via the later Shared one. A positional bound "up to the next sync task" doesn't work, because PLAY 1 runs all five syncs first and then all five deletions. So each site is now pinned by both its sync_deletions_target_dir and its unique sync_deletions_label. The planted regression test_planted_regression_a_removed_include_is_not_masked_by_a_later_shared_target reproduces the old masking and catches it.
2. always: cleanup could abort the fleet. Both cleanup tasks are now failed_when: false, and the target-side one is also ignore_unreachable: true. That's an ansible-core task keyword, and the installed 2.17 accepts it. Tests: test_always_cleanup_tasks_are_never_fatal, plus a planted regression. On a single-node install this matters more, because an abort leaves the whole system half-updated.
3. The bootstrap switch now uses | bool in its set_fact and all 6 later uses. Tests: test_bootstrap_fact_uses_are_bool_filtered, plus a planted regression.

Two guards for single-node installs (raised by the owner: on an all-in-one host every component's directory is on one filesystem). New file repo_tests/sync_deletions_cross_component_isolation_16310_test.py:

• The deletion target equals where the sync actually lands the files, for all 12 sites. The landing directory is derived from the playbook itself. Play 0's [PRE-FLIGHT] Create component archives builds each archive with git archive -- <path>, and the test asserts there's no --prefix. So unarchive into /opt/autobot/ lands files in /opt/autobot/<path>, and the AI Stack keeps its --strip-components=4 case. All 12 match, and none is left undeterminable. An unknown archive name, or a --prefix rewrite, fails loudly, and a planted mismatch is caught.
• No two deletion passes with different sources share or nest a target directory. The comparison is by path segment, after normalising {{ autobot.base_dir }} and /opt/autobot. Today the 12 targets are siblings, and the only repeat is autobot_shared, three times with the same source; the shared marker makes the later passes no-ops. Planted cases: a nested pair fails, an equal target with a different source fails, and an equal target with the same source passes.

Line numbers in update-all-nodes.yml: unchanged since round 12. The body's wiring table still holds.

[mrveiss]

Second-session delta review, 3aadb1424..ae04398c2: my three notes resolved; answers to the two questions; 3 low notes

Read from git objects at ae04398. Nothing executed. CI at this head: 0 failures and 30 pending, so no green is cited.

Resolved

1. The wrong-include pin.
   • Each site is now pinned by sync_deletions_target_dir AND a per-site unique sync_deletions_label. That's the right call, because PLAY 1 runs all five unarchives before its five deletion passes, so a positional bound would break there.
   • test_planted_regression_a_removed_include_is_not_masked_by_a_later_shared_target proves both directions: target_dir alone still reaches the Shared include, and target_dir plus label doesn't.
2. Cleanup can't abort. Both always: cleanups are failed_when: false, and the target-side one is also ignore_unreachable: true. test_always_cleanup_tasks_are_never_fatal pins it.
3. | bool is on the fact itself (:131-133) and on every use: :144, :165, :173, :182, :194, and the _sd_plan selection at :198.

Question 1: can ignore_unreachable on the cleanup hide anything that matters? No.

• It can't hide an outage. It keeps a host that went unreachable between the delete step and the cleanup in the play for one more task. Its next task outside this block then meets the same unreachable host and gets PLAY 2's normal handling. So it defers detection by one task and hides nothing about the node.
• It isn't what makes an unreachable node survive. If the host was already unreachable during the block, the unreachable task isn't rescued, and the host leaves the play before always:. The "one node never aborts the fleet" contract holds for this module's own failures; unreachability stays PLAY 2's existing semantics. It'd be worth saying so in that comment, so the contract isn't read as covering unreachable nodes.
• What it does hide: a leaked /tmp/autobot-sync-deletions-list-* file. It's root-owned and 0600, and holds relative paths planned for deletion, nothing secret.
• One low note, affecting both cleanups. failed_when: false makes a cleanup failure (a permission error, say) completely silent. A register: plus a debug warning when the result is failed or is unreachable would keep "nothing swallowed" true.

Question 2: would the landing-directory derivation catch a future --prefix or archive rename? Mostly, with two gaps.

• Caught:
  • a renamed loop name whose src isn't updated, since the name must be in Play 0's loop or the test asserts;
  • a changed loop path, since the landing directory then mismatches the target;
  • an unrecognised sync module, dest or src shape;
  • a literal --prefix.
• Gap A: the --prefix check reads only the task's shell key (_assert_no_archive_prefix_rewrite: str(task.get("shell", ""))).
  • Converting that task to ansible.builtin.shell: or command: would make the check read "" and pass vacuously.
  • Fix: read the command from whichever of those keys is present, and assert it was found and contains git and archive (a known positive).
• Gap B: --prefix isn't the only way to flatten an archive.
  • Git's ref:path tree-ish form (git archive {{ deploy_ref }}:autobot-backend) stores entries WITHOUT the path, and uses no --prefix. The derivation would then report dest/autobot-backend while files land in dest/.
  • Fix: pin the shape the derivation relies on. Assert the loop command contains -- {{ item.path }} (the pathspec form), rather than only the absence of --prefix.
• Low: --strip-components=N is treated as "dest IS the landing dir" for any N (_actual_sync_dest).
  • That's only true when N equals the depth of the archived path. It's 4 for AI Stack's autobot-infrastructure/shared/docker/ai-stack/, which is correct today.
  • Parsing AI Stack's -- <path> and computing dest plus the path minus its first N segments would catch a mismatched N, or a path whose depth changes.

Stated boundary

• The new cross-component tests (the landing directory, and no shared or nested targets) cover the 12 update-all-nodes.yml sites.
• The four role sites (backend, frontend, slm_manager, slm_agent) are outside both, because their targets are role variables ({{ backend_code_dir }} and so on).
• That's fine, but say so in the docstring (RATCHET_BASELINES rule 1), so "all targets are isolated" isn't read as covering the provisioning path too.

None of this blocks. My three notes are fixed and pinned.

[mrveiss]

CI red at ae04398c2, root cause: a readonly/mutable type mismatch in the new drift panel. SLM Frontend Check fails with vue-tsc TS2345, nine times, in autobot-slm-frontend/src/components/FullTreeDriftPanel.vue at 158:63, 161:49, 165:74, 168:49, 175:69, 176:84, 179:67, 180:59 and 185:39.

The template passes each component as a deeply readonly object ({ readonly component: string; readonly compared: number; readonly drifted: readonly {…}[]; readonly exclusions: { readonly [x: string]: number }; … }). The helpers it calls take the mutable interface FullTreeComponentDrift (src/composables/useCodeSync.ts:178): filesByVerdict (:59), buildBundleCount (:63), hostStateEntries (:70) and isComponentClean (:76). Both files are in this PR's diff.

Fix: the helpers only read, so type their parameter as DeepReadonly<FullTreeComponentDrift> (from vue), or declare the interface's arrays and records readonly at the source. Don't cast.

[mrveiss]

CI red at 97b1d9d18: one new, unaudited detect-secrets finding, in this PR's own test file. Secret Detection (whole tree): rescan reads 1332 findings against the committed baseline's 1331 (floor 1300). The new one: autobot-slm-frontend/src/components/FullTreeDriftPanel.test.ts:54, Hex High Entropy String — a fixture commit-hash literal (detail: '<12-hex-chars>') for a removed_from_source file entry, which is exactly what a real commit SHA in this domain looks like to the entropy plugin. It's a false positive, not a real secret.

Fix: detect-secrets audit .secrets.baseline and mark that finding "not a secret", then commit the updated baseline. Follows the same pattern as #16300/#16353's audited entries.

[mrveiss]

Delta af5ec1cf3..b6fd5ac5c: still approved. This head is main merged forward three times, with no change to the PR itself.

• It is a fast-forward from af5ec1cf3, adding 3 Merge remote-tracking branch 'origin/main' commits. The 11 non-merge commits they bring in are all main's own, ending at fcd03d0ab. The PR has no new commits of its own, and there are 0 trailers.
• All 38 of the PR's files have net +/− lines identical to the approved head, measured against the new merge base, fcd03d0ab.
• The shard-4 red is fixed by main, not by an edit here. repo_tests/frontend_api_contract_ratchet_test.py is now byte-identical to main (inline_generics: 564, from chore(ratchet): lower the frontend style counts to batch 2b's combined tree — batch 2b vehicle and final member (#15455) #16596's fcd03d0ab).
• The branch is 0 behind main.

The approval covers the code only. CI was still queued at this head, so merging still needs it green.

[mrveiss]

Delta review, head moved twice since the last verdict (b6fd5ac → 49e3dea → now cb0a3b5): approve.

Two new commits, both checked against their own evidence:

• 331c10f (config: drop @computed_field on external_url, keep @cached_property): correct fix for the delta finding on 3426b6e. @computed_field would make the property participate in settings.model_dump()/.json(), re-running the socket probe the original 54 modules have import-time side effects: 51 SLM api modules open a socket when imported #16262 fix exists to defer — same bug class, different trigger. Verified the commit's claim directly: no model_dump/.json()/jsonable_encoder call on the Settings object anywhere in autobot-slm-backend/ — plain @cached_property is behavior-identical for all current (plain attribute) call sites.

• cb0a3b5 (guard: shrink the remaining 47 cleared import-hermeticity offenders, closes 54 modules have import-time side effects: 51 SLM api modules open a socket when imported #16262): verified against the cited full-population sweep run (34760508167) directly — pulled its failure assertion, which lists exactly the baseline entries that "no longer fail that way." Diffed that list against the 47 lines actually removed from import_hermeticity_known_offenders.py: identical sets, no more, no less. Combined with the earlier 4-entry shrink, this correctly closes out all 51 of the SLM's api.* entries; the two remaining unrelated entries are untouched, as claimed.

CI at cb0a3b5: 0 fail, 0 pending. mergeable still shows CONFLICTING — expected, same ratchet/baseline collision pattern as the rest of batch 2d (import_hermeticity_known_offenders.py / CLAUDE_RULES.md); rebase belongs with 2d vehicle assembly, not this review.