Repository navigation
fix(config): finish the CONFIG_DIR consumer pass and guard the dead-path class (#14892) - #15157
Merged
Merged
Conversation
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.
4 tasks
This was referenced Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #14892 — AC1's deployed-host half is unmet and cannot be met from this branch. See "What is still unmet".
Thinking Path
The prior pass on #14892 landed AC1 (checkout half), AC2 and AC4. AC3 — "for each of the nine consumers, confirm whether a missing config is distinguishable from a loaded one; where it is not, make it so" — was reported as done, but one consumer had changed by zero lines:
utils/service_discovery.pyresolvedCONFIG_DIR / "services.json", assigned it toself.config_file, and never read it again.Before converting that consumer I established which of three things it was:
The evidence settles it as (2), and the deciding fact is not "nothing reads it" — it is that a working config source already exists:
git log --all -- "**/services.json"is empty. No commit on any branch has ever added that file. So the read never landed and there is nothing to wire to.ServiceDiscoveryis instantiated exactly once (service_discovery.py:638) with no argument; no caller has ever passedconfig_file._init_default_services()already sources every endpoint fromunified_config_manager.get_distributed_services_config()— the SSOT. Wiring in a JSON file would fork the SSOT, which is the opposite of the fix.So the field went, along with the
config_fileparameter and the comment that implied a config file was being read.Why deletion is the right verb here, given that debris is normally unfinished work. The standing rule is that anything debris-shaped gets wired in, and deletion is correct only once the work has demonstrably landed. That rule assumes there is work to finish. Here there is not, and two facts prove it rather than assume it:
git log --all --oneline -- "**/services.json"returns nothing. Not "removed at some point" — the file has never existed in any commit on any branch in this repository's history. There is no landed work to point the read at, and no unlanded work either._init_default_services()reads every one of them fromunified_config_manager.get_distributed_services_config(), which is the SSOT. So the capability is not missing — it is present and served from the canonical place.Wiring the field in would therefore not complete anything. It would create a second, file-based configuration source competing with the SSOT for the same values, which is the fork the canonical-source rule exists to prevent. Removal is the only option that leaves exactly one source of truth.
That left a more interesting question: what class of defect is "a path to a file that has never existed, stored where it reads as live configuration"? It is invisible to the constant fix — correcting
CONFIG_DIRunder it changes literally nothing. So this PR adds a guard for the class rather than the instance: a sweep over every consumer that names a config file through the constant, asserting the file is actually there.The guard found two more on its first run, and they contradict the issue text. #14892 states that the
security/subtree holds all five manager configs. Three are present.security/security_policies.yamlandsecurity/sso_config.yamlare not, andgit log --allshows neither has ever existed.SecurityPolicyManagerandSSOIntegrationFrameworkhave therefore only ever run their built-in defaults — which is the "more serious half" the issue itself named. #14892 has made that distinguishable (config_source+ a WARNING naming the path, and no decoy write); deciding what the reviewed policy should say is a security decision, not a refactor, so it is filed as #15154 and pinned in the guard as a two-entry ratchet rather than silently exempted.Consumer 1,
event_manager.py, also turned out not to satisfy AC3. It returned the same{"agent_behavior": {"debug_mode": False}}on a miss, on a parse error, and on a file with noagent_behaviorsection — andconfig.yamlhas had no such section for its whole life, sodebug_publish()could never be enabled and nothing said so. It also returnedyaml.safe_load'sNoneverbatim for an empty file, so the next_is_debug_mode()call would raiseAttributeError.Making that distinguishable needed the loader the five security managers already share. That loader lived in
security/enterprise/config_loading.py, andsecurity.enterprise.__init__eagerly imports all five managers, one of which needs an optional dependency — a core module cannot take that import to read a YAML file. So the loader moved toautobot_shared/config_file_loading.pyasload_config_file, which is where core rule 2 says it belongs. It is a move, not a fork: the old module is deleted and all six call sites point at the one copy.What Changed
autobot-backend/event_manager.py:45(config.yaml)load_config_file;config_source/config_pathnow carry the fact. An absentagent_behaviorsection is reported at WARNING instead of silently answering False. An empty file no longer becomes aNoneconfig.agent_behavior.debug_modeadded toconfig.yamlso the knob the code reads actually exists — behaviour-neutral (falseis the current effective value).autobot-backend/utils/service_discovery.py:124(services.json)config_fileparameter, the attribute, the resolved default and the misleadingIssue #380comment all deleted; now-unusedPATHimport dropped. Proof it was debris: file never existed in any commit, argument never passed, attribute never read, and endpoints already come from the SSOT.autobot-backend/config/service_config.py:355str(PATH.CONFIG_DIR)as a fallback path forget_path("config"). It loads no config, so there is no miss to distinguish. Nothing to change.security/enterprise/sso_integration.py:95(security/sso_config.yaml)config_source, WARNING, no decoy write). The file has never existed — pinned in_KNOWN_ABSENT_CONFIG_FILES, filed as #15154. Call site retargeted to the canonical loader.security/enterprise/domain_reputation.py:33(security/domain_security.yaml)security/enterprise/security_policy_manager.py:112(security/security_policies.yaml)security/enterprise/compliance_manager.py:134(security/compliance.yaml)security/enterprise/threat_detection/engine.py:62(security/threat_detection.yaml)autobot-backend/utils/error_catalog.py:481(error_messages.yaml)source/searched_paths. Its docstring still said the constant was broken and that fixing it was #14892's job; that is no longer true, so the docstring was corrected. Both spellings stay in the candidate list deliberately — the loader must not depend on the constant.Loader consolidation:
security/enterprise/config_loading.py→autobot_shared/config_file_loading.py,load_security_config→load_config_file. Six call sites updated. The AC3 test no longer needsimportorskip("cachetools"), so it runs everywhere instead of skipping.New guards in
repo_tests/config_dir_resolves_14892_test.py:TestEveryNamedConfigFileExists— AST sweep (not a regex: this module and several consumers discuss these call shapes in prose) overautobot-backend,autobot_sharedandautobot-infrastructure/shared/scriptsforPATH.CONFIG_DIR / "x"andPATH.get_config_path("a", "b"), asserting each named file is present.test_the_sweep_finds_consumers_at_all— the empty-enumeration floor.test_the_known_absent_files_are_still_absent_and_still_named— a two-entry ratchet, not a hole: a third absent filename fails, and an entry that stops applying also fails.TestServiceDiscoveryKeepsNoDeadConfigPath— asserted on the class viainspect.signature, so the field cannot return under another spelling.TestEventManagerDistinguishesAMiss— theagent_behaviorkey is present in the checked-in file, a miss reportsbuiltin-defaults, an empty file does not become aNoneconfig.Python file-size ratchet:
error_catalog.pyandservice_discovery.pyboth land exactly on their existing baselines (775 and 677). Neither baseline was raised. Docstrings were tightened to fit rather than the ceiling moved.That tightening is itself worth recording, and is filed against #14762:
service_discovery.pyis pinned at exactly 677 and the ratchet fails below the number as well as above, so a change that was a net deletion still needed three rounds of shortening an explanatory docstring to land back on 677. A zero-slack ceiling does not only block growth — it taxes the comment budget of every change that touches the file, including the removals it should be encouraging. Two further instances are recorded on that issue (autobot_shared/env_registry.pyat 1743,autobot-backend/security/session_ownership.pyat 858).Verification
PATH.CONFIG_DIR == project_root()/"autobot-infrastructure"/"shared"/"config", asserted inTestConfigDirIsReal(4 tests, on base).autobot_shared/ssot_constants.py:395.repo_tests/config_dir_resolves_14892_test.py— 20 passed.TestConfigDirIsReal+ the five parametrised tracked files, with a non-empty floor.pytest autobot-backend/security autobot-backend/utils/error_catalog_resolution_test.py autobot-backend/event_manager_multiclient_14814_test.py autobot_shared→ 2418 passed, 15 skipped, 3 deselected, 2 xfailed.repo_tests/python_file_size_ratchet_test.py+ratchet_base_guard_test.py+ this file → 64 passed. No baseline raised.Contrast mutation 1 — reintroduce the dead path (run against the final head)
Re-added
self.config_file = str(PATH.CONFIG_DIR / "services.json")toServiceDiscovery.__init__. Observed:Contrast mutation 2 — empty the enumeration (run against the final head)
Set
_SWEPT_TREES = (), proving a sweep that finds nothing goes red rather than passing by asserting over an empty list:Both mutations were reverted and the head re-verified before commit.
What is still unmet
CONFIG_DIRshould resolve to on a deployed host, and warns the spelling may differ from a checkout. Nothing on this branch is deployed, so there is no host evidence and this is not provable here. What the branch does instead is refuse to depend on the answer:error_catalog.pykeeps both theautobot-infrastructure/and the bareinfrastructure/candidates and reports every path it searched. That is mitigation, not proof.security_policies.yamlandsso_config.yamlshould exist — and what a reviewed policy should say, given the SSO defaults carry placeholder SAML/LDAP values — is a security decision.Model Used
Claude Opus 5 (1M context).