test(slm): guard the canonical/Ansible-mirror file pairs against drift (#14584)

[mrveiss]

Closes #14584

Thinking Path

Started from the precedent the issue names, check_role_facts_synced.py, but before extending it looked for whether a more directly-relevant mechanism already existed for this exact pair — and found one: pipeline-scripts/detect-agent-code-drift.sh, wired into the required code-quality check since #1629, already doing a recursive byte-diff of autobot-slm-backend/slm/agent/ against its Ansible mirror.

Reading it explained the incident directly: it excluded --exclude='*_test.py' from the comparison — a blanket wildcard, not a named exception — which is exactly the class of file that silently drifted. So the root cause wasn't "no guard exists," it was "the guard's own exclude list was blind to precisely the file class that broke." Fixing that one line, in the existing required check, closes the reported gap more directly than adding a second, parallel mechanism would.

Establishing scope next (the issue's explicit ask): ansible/roles/*/files/ resolves to exactly two role directories in this repo — slm_agent and backend. Tracing each role's tasks/main.yml copy: src: entries back to a canonical source elsewhere in the tree found:

• slm_agent: 8 files under slm/agent/ (6 deployed modules + version_test.py + health_collector_state_change_test.py, both test-support mirrors kept in sync so CI can run the exact bytes the role ships, The cross-tree module loader in the slm health test needs a manual stub for every new backend import #14538/test(slm): fix redis health test pollution and cross-tree loader fragility #14576). A 9th canonical file, health_collector_probe_test.py, is deliberately NOT mirrored — its own docstring says it is canonical-tree-only.
• backend: 2 files (permission_rules.yaml, error_messages.yaml), both already byte-identical to their canonical autobot-backend/ sources but covered by no existing check at all.

pairs_found = 10.

Deeper question the issue asks to judge: is byte-identical duplication the right arrangement, or should the ansible role reference the canonical file instead of holding a copy? I could not find a reason it has to be a copy — Ansible's copy: module can source from outside the role's own files/ (a relative src: path, or ansible.builtin.copy reading from anywhere the control node can see), so referencing autobot-slm-backend/slm/agent/*.py directly and dropping the mirror entirely looks structurally possible. I'm not making that change here — it's #13916's scope, and untangling why the mirror was introduced in the first place (possibly role-portability, possibly just how #1629 was originally written) needs more digging than this PR's budget. Recording it as unjustified-as-far-as-I-can-tell rather than silent.

What Changed

• pipeline-scripts/detect-agent-code-drift.sh: replaced the blanket --exclude='*_test.py' with a named exception for the one deliberately-canonical-only file (health_collector_probe_test.py), so both version_test.py and health_collector_state_change_test.py are now compared like any other file. Extended the same script to also cover the backend role's permission_rules.yaml/error_messages.yaml pair. Refactored into compare_dir_pair/compare_file_pair helpers that each fail loudly on content drift (naming both paths + a diff), a missing directory, or a missing individual file, and added a PAIRS_COMPARED counter that fails the run if it ever reaches zero — a future edit that guts every pair-registration call must not read as clean.
• .github/workflows/code-quality.yml: fixed a stranded path-filter entry, autobot-infrastructure/ansible/roles/slm_agent/files/**, which named a directory that has never existed in this repo (confirmed via git log -S on the line) and so matched nothing since it was added. Corrected to the real path and widened both the push-trigger paths: list and the changes job's paths-filter to the second pair, so editing either side of either pair actually triggers the check that guards it.
• autobot-slm-backend/ansible/tests/detect_agent_code_drift_test.py (new): regression coverage that runs the real script against synthetic trees built under tmp_path, never against this checkout's own files. Covers a synced tree passing, the deliberate probe-test exception not false-positiving, content drift naming both paths, a missing mirror file, a missing mirror directory, a missing canonical file, and a mutated copy of the script with every registration call stripped still refusing to report a zero-pairs result as clean.

Verification

This check runs inside code-quality, which is a required status check on Dev_new_gui.

Scratch-checkout verification (under /tmp, never against this repo's own tracked files), against a tree seeded from the real 10 pairs:

baseline:                                    OK -- 10 canonical/mirror file pair(s) byte-identical (exit 0)
one-char diff in the target file's mirror:   DRIFT DETECTED (slm_agent) -- both paths named, diff shows the exact line (exit 1)
restore:                                     OK -- 10 pairs (exit 0)
delete the mirror copy:                      DRIFT DETECTED (slm_agent) -- "Only in <canonical>: <file>" (exit 1)
restore:                                     OK -- 10 pairs (exit 0)
delete the whole mirror directory:           ERROR: mirror directory not found (exit 1, distinct message)
registration calls stripped (simulated):     ERROR: resolved zero canonical/mirror file pairs (exit 1)

Before/after counts: before this PR, the script silently compared 0 test-file pairs (blanket-excluded) out of what should have been 2, and 0 of the 2 backend-role pairs (not covered at all) — 6 of 10 real pairs actually enforced. After, all 10 are enforced, and the run reports the count so a future drop is visible (OK -- 10 ... pairs vs some smaller number).

black/isort/flake8 ran clean against the new test file locally; bash -n and python3 -c "yaml.safe_load(...)" confirm the shell script and the edited workflow YAML both parse. Did not run the Python suite locally in a way that stands as merge evidence — see the python-suite shard N/12 runs on this PR directly; that job is not a required check.

Model Used

Claude Sonnet 5

[mrveiss]

Review — APPROVE, and the reframing is worth more than the guard

I asked you to build a drift guard. You found one already existed, had been required since #1629, ran on every PR — and was blind to precisely the file class that drifted. That is a much better answer than a new guard, and I verified both halves:

detect-agent-code-drift.sh:35   --exclude='*_test.py'          # blanket wildcard
code-quality.yml:390            bash pipeline-scripts/detect-agent-code-drift.sh

So the root cause was never "no guard existed". It was an exclusion broad enough to cover the thing it was guarding. The mirror could drift indefinitely with a required check passing on every commit — which is exactly why the incident got misattributed to shard flakiness for a full CI cycle.

And the second finding compounds it

code-quality.yml lines 20 and 64 both name autobot-infrastructure/ansible/roles/slm_agent/files/**. That directory does not exist — the real pairs live under autobot-slm-backend/ansible/roles/{slm_agent,backend}/files. Confirmed with git log -S that it never existed here.

So the path filter pointed at nothing while the guard excluded the drifting files. Two independent, silent failures stacked on the same mechanism: the job might not fire on a mirror change, and if it did fire it would not look at the changed files. Either alone is a hole; together they explain the incident completely.

That is the third path-filter defect this session — after code-quality's filter omitting the guards' own inputs (#14578) and the filter not covering the .j2 templates it guarded (#14575). Worth noticing as a class rather than three coincidences: a path filter is a guard on a guard, and nothing guards it.

The rest

pairs_found = 10, and the two backend-role YAML pairs were covered by nothing at all before this. Before: 6 of 10 enforced. After: 10 of 10, with the count reported so a silent drop is visible.

The named exception for health_collector_probe_test.py — canonical-only per its own docstring — is the right shape: a specific documented carve-out rather than a wildcard that happens to include it. That is the exact distinction that failed here.

The mutation coverage is thorough in the way that matters: content drift naming both paths and the differing line, a missing file, a missing directory with a distinct message, and — the one I most wanted — stripping the registration calls to simulate the guard being gutted, which reports resolved zero canonical/mirror file pairs and exits 1. A guard that cannot detect its own evisceration is not a guard.

Being straight about enforcement location is right too: the new pytest lives under autobot-slm-backend, so it runs in python-suite, which is not required — and the actual enforcement is the shell script in code-quality, which is. Saying that plainly rather than implying the test is the gate is the honest framing.

On the duplication question: "could not find a documented reason, flagged as unjustified as far as found, restructuring left to #13916" is exactly the right answer to give. You were asked to judge it, not to fix it, and "I looked and found no justification" is a finding — it tells #13916 that the copy is probably removable rather than load-bearing.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!