Skip to content

fix(config): finish the CONFIG_DIR consumer pass and guard the dead-path class (#14892) - #15157

Merged
mrveiss merged 4 commits into
Dev_new_guifrom
issue-14892-configdir
Aug 28, 2026
Merged

mrveiss merged 4 commits into
Dev_new_guifrom
issue-14892-configdir

Conversation

@mrveiss

@mrveiss mrveiss commented Aug 28, 2026 •

Copy link
Copy Markdown
Owner

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.py resolved CONFIG_DIR / "services.json", assigned it to self.config_file, and never read it again.

Before converting that consumer I established which of three things it was:

  1. a read that was meant to exist and never landed → wire it in;
  2. a consumer that genuinely does not need the file → the assignment is debris, and the honest fix is removing it and whatever implies it;
  3. something outside the repo supplying the file at runtime → neither, and the criterion needs restating.

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.
  • ServiceDiscovery is instantiated exactly once (service_discovery.py:638) with no argument; no caller has ever passed config_file.
  • _init_default_services() already sources every endpoint from unified_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_file parameter 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:

  1. 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.
  2. The endpoints that file would have supplied already have a live source: _init_default_services() reads every one of them from unified_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_DIR under 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.yaml and security/sso_config.yaml are not, and git log --all shows neither has ever existed. SecurityPolicyManager and SSOIntegrationFramework have 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 no agent_behavior section — and config.yaml has had no such section for its whole life, so debug_publish() could never be enabled and nothing said so. It also returned yaml.safe_load's None verbatim for an empty file, so the next _is_debug_mode() call would raise AttributeError.

Making that distinguishable needed the loader the five security managers already share. That loader lived in security/enterprise/config_loading.py, and security.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 to autobot_shared/config_file_loading.py as load_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

# Consumer Verdict What was done
1 autobot-backend/event_manager.py:45 (config.yaml) wired in Routed through load_config_file; config_source/config_path now carry the fact. An absent agent_behavior section is reported at WARNING instead of silently answering False. An empty file no longer becomes a None config. agent_behavior.debug_mode added to config.yaml so the knob the code reads actually exists — behaviour-neutral (false is the current effective value).
2 autobot-backend/utils/service_discovery.py:124 (services.json) removed config_file parameter, the attribute, the resolved default and the misleading Issue #380 comment all deleted; now-unused PATH import dropped. Proof it was debris: file never existed in any commit, argument never passed, attribute never read, and endpoints already come from the SSOT.
3 autobot-backend/config/service_config.py:355 left Reports str(PATH.CONFIG_DIR) as a fallback path for get_path("config"). It loads no config, so there is no miss to distinguish. Nothing to change.
4 security/enterprise/sso_integration.py:95 (security/sso_config.yaml) left, with a filed gap Already distinguishable (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.
5 security/enterprise/domain_reputation.py:33 (security/domain_security.yaml) left Already distinguishable; file present. Call site retargeted to the canonical loader.
6 security/enterprise/security_policy_manager.py:112 (security/security_policies.yaml) left, with a filed gap Already distinguishable. File has never existed — pinned and filed as #15154. Call site retargeted.
7 security/enterprise/compliance_manager.py:134 (security/compliance.yaml) left Already distinguishable; file present. Call site retargeted.
8 security/enterprise/threat_detection/engine.py:62 (security/threat_detection.yaml) left Already distinguishable; file present. Call site retargeted.
9 autobot-backend/utils/error_catalog.py:481 (error_messages.yaml) left Already distinguishable via 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 needs importorskip("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) over autobot-backend, autobot_shared and autobot-infrastructure/shared/scripts for PATH.CONFIG_DIR / "x" and PATH.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 via inspect.signature, so the field cannot return under another spelling.
  • TestEventManagerDistinguishesAMiss — the agent_behavior key is present in the checked-in file, a miss reports builtin-defaults, an empty file does not become a None config.

Python file-size ratchet: error_catalog.py and service_discovery.py both 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.py is 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.py at 1743, autobot-backend/security/session_ownership.py at 858).

Verification

Criterion Kind Evidence
AC1 — checkout resolution static / unit PATH.CONFIG_DIR == project_root()/"autobot-infrastructure"/"shared"/"config", asserted in TestConfigDirIsReal (4 tests, on base).
AC1 — deployed-host resolution unmet Not provable from this branch. See below.
AC2 — constant fixed static Landed on base at autobot_shared/ssot_constants.py:395.
AC3 — nine consumers distinguishable static / unit Per-consumer table above. repo_tests/config_dir_resolves_14892_test.py — 20 passed.
AC4 — resolved directory exists in a checkout unit TestConfigDirIsReal + the five parametrised tracked files, with a non-empty floor.
No regression in the moved loader unit 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.
Size ratchet unit 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") to ServiceDiscovery.__init__. Observed:

AssertionError: config files named through PATH.CONFIG_DIR that do not exist:
  autobot-backend/utils/service_discovery.py:127 names 'services.json'
assert not ["autobot-backend/utils/service_discovery.py:127 names 'services.json'"]

AssertionError: ServiceDiscovery.config_file is back. It named CONFIG_DIR/services.json,
a file no commit has ever added, and nothing read it.
assert not True
 +  where True = hasattr(<utils.service_discovery.ServiceDiscovery object ...>, 'config_file')

2 failed, 18 passed

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:

AssertionError: the CONFIG_DIR consumer sweep found 0 call sites; it has always found more
assert 0 >= 5
 +  where 0 = len([])

AssertionError: empty sweep — see test_the_sweep_finds_consumers_at_all
assert []

AssertionError: security/security_policies.yaml is no longer named by any consumer — drop it from the exemption
assert 'security/security_policies.yaml' in set()

3 failed, 17 deselected

Both mutations were reverted and the head re-verified before commit.

What is still unmet

  • AC1, the deployed-host half. The criterion asks what CONFIG_DIR should 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.py keeps both the autobot-infrastructure/ and the bare infrastructure/ candidates and reports every path it searched. That is mitigation, not proof.
  • The two absent security policy files are pinned and filed (security: SecurityPolicyManager and SSOIntegrationFramework read policy files that have never existed #15154), not resolved. Whether security_policies.yaml and sso_config.yaml should exist — and what a reviewed policy should say, given the SSO defaults carry placeholder SAML/LDAP values — is a security decision.
  • No route-level or host verification of any kind is claimed. Nothing here was exercised against a running system.

Model Used

Claude Opus 5 (1M context).

@github-actions

Copy link
Copy Markdown
Contributor

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

github-actions Bot and others added 2 commits August 28, 2026 05:29
Applied Black, isort, and autoflake to match code-quality checks.
Triggered by workflow auto-fix-formatting.yml.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant