Skip to content

Fix comm_cfg anchor-holder key leaking into config dicts - #773

Merged
thusser merged 2 commits into
developfrom
feature/comm-cfg-anchor-leak
Aug 17, 2026
Merged

Fix comm_cfg anchor-holder key leaking into config dicts#773
thusser merged 2 commits into
developfrom
feature/comm-cfg-anchor-leak

Conversation

@thusser

@thusser thusser commented Aug 17, 2026

Copy link
Copy Markdown
Member

Summary

  • pre_process_yaml's whole-file {include file} spliced the entire included file in, including keys whose sole purpose is holding a YAML anchor for <<: *anchor use elsewhere -- e.g. comm.shared.yaml's comm_cfg: &comm, used across every monti/monet/iagvt/polaris config. That leaked comm_cfg reached Object.__init__'s **kwargs and was silently dropped, indistinguishable from a real config typo.
  • Whole-file includes now drop any key reload_anchors() identifies as an anchor holder for that file before splicing; keyed includes of the same key ({include file key}) are untouched.
  • Handles the resulting edge case where the entire include is anchor-only keys (comm.shared.yaml's only top-level key), by dropping the splice placeholder instead of emitting an empty {} mapping, which broke YAML parsing when followed by block-style content.
  • Verified against a real pyobs-monet config (config/central/imagedb.yaml): comm_cfg no longer leaks, comm: <<: *comm still resolves correctly.

Implements the comm_cfg portion of specs/plans/2026-08-09-object-kwarg-validation.md. The environment/database wrapper-key question and Object.__init__'s warn/raise enforcement are intentionally left open -- see that plan's Decision section for why.

Test plan

  • tests/utils/test_config.py: 3 new tests -- alias resolution (previously uncovered), keyed-include-of-anchor-holder is kept, whole-file-include no longer leaks the anchor holder
  • Full suite: pytest -m "not integration and not xmpp" -- 1488 passed, 25 skipped, 0 failed
  • ruff check / black --check / pyrefly check clean on changed files
  • Manual check against pyobs-monet/config/central/imagedb.yaml

pre_process_yaml's whole-file {include file} splices the entire included
file, including any key whose sole purpose is holding a YAML anchor for
<<: *anchor use elsewhere (e.g. comm.shared.yaml's comm_cfg: &comm). That
leaked key then reached Object.__init__'s **kwargs and was silently
dropped there -- masking real config typos, since the include leak looked
identical to a typo.

Drop anchor-holder keys from whole-file splices using the (keyword, anchor)
pairs reload_anchors() already extracts; keyed includes of the same key are
left untouched. Handle the resulting all-keys-stripped case (comm.shared.yaml's
only top-level key is the anchor holder) by dropping the splice placeholder
entirely instead of emitting an empty "{}" mapping, which broke YAML parsing
when followed by block-style content.

Verified against a real pyobs-monet config: comm_cfg no longer leaks, and the
comm: <<: *comm alias still resolves correctly.

See specs/plans/2026-08-09-object-kwarg-validation.md for the investigation
and remaining open items (environment/database wrapper keys, Object.__init__
enforcement level).
@thusser

thusser commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Approach is sound and it fixes the comm.shared.yaml case, but two edge cases need attention before merge.

1. Empty-dict splice isn't scoped to whole-file includes (pyobs/utils/config.py:42)

The isinstance(include_dict, dict) and not include_dict branch fires for keyed includes too, not just the anchor-trimmed whole-file case. A keyed include that legitimately selects an empty mapping now degrades:

# inc.yaml
somekey: {}
# main.yaml
outer:
  {include inc.yaml somekey}

Before: outer: {}. After: outer: null (the {} placeholder is dropped). The not key.strip() guard on line 38 only protects the anchor-dropping block, not this one. Gate line 42 with the same whole-file condition (or track whether the dict was trimmed) so keyed empty-dict includes keep emitting {}.

2. Anchor-holder detection isn't top-level-scoped (pyobs/utils/config.py:39)

reload_anchors's regex (\S*): &(\S*) also captures nested anchors, and keyword is just the last non-space token before : &, not a top-level path. So a nested key that shares a name with a top-level key causes the top-level key to be silently dropped from a whole-file include:

# inc.yaml
type: Foo
camera:
  type: &cam
    model: X

After a whole-file include, top-level type: Foo is gone (only camera survives). Restrict the drop to keys that are actual top-level anchor holders, not name-matched against every anchor anywhere in the file.

Both are silent-data-loss shapes, i.e. exactly the class of bug this PR is meant to kill. They're unlikely in today's fleet configs (only comm.shared.yaml is whole-file included, and its lone anchor is top-level), but worth closing with a regression test each.

Otherwise good: tests/utils/test_config.py passes (19 passed), the xfail removal is correct, the new replace_aliases coverage fills a real gap, and the plan doc update accurately leaves environment/database and the Object.__init__ enforcement open.

Two silent-data-loss bugs found in review of the comm_cfg anchor-leak fix:

1. The "drop an empty splice placeholder" special case wasn't scoped to
   whole-file includes, so a keyed include that legitimately selects an
   empty mapping (`{include file key}` where key's value is `{}`) silently
   became `null` instead of `{}`.

2. Anchor-holder detection reused reload_anchors(), which matches
   `keyword: &anchor` at any nesting depth via a plain (not line-anchored)
   regex. A top-level key could be incorrectly dropped from a whole-file
   include just because some unrelated *nested* key elsewhere in the file
   happened to share its name and carry an anchor.

Fixed by gating the empty-splice case on the same whole-file condition as
the anchor-drop itself, and by adding top_level_anchor_keywords() -- a
line-anchored regex restricted to unindented keys -- used only for the
drop decision. reload_anchors() itself is unchanged, since replace_aliases()
still needs to resolve anchors at any nesting depth.

Added regression tests for both. Full suite: 1490 passed, 25 skipped.
Re-verified against all 803 yaml files in pyobs-monet, pyobs-iagvt, and
pyobs-iag50: same result as before (2 pre-existing, unrelated errors; no
comm_cfg leaks in any consuming config).
@thusser

thusser commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Both fixed in 8124467.

1. Empty-splice drop now scoped to whole-file includes only. Gated that branch on the same not key.strip() condition as the anchor-drop itself, so a keyed include of a legitimately-empty mapping ({include file key} where key's value is {}) still emits {} instead of being silently dropped to null.

2. Anchor-holder detection now top-level-scoped. Added top_level_anchor_keywords(), a line-anchored (^, no leading whitespace) regex restricted to unindented keys, used only for the whole-file drop decision. reload_anchors() itself is untouched -- replace_aliases() still needs it to resolve anchors at any nesting depth, so changing its general behavior wasn't an option.

Reproduced both bugs from your examples before fixing, to confirm they were real. Added a regression test for each (test_pre_process_yaml_keyed_empty_dict_include_is_not_dropped, test_pre_process_yaml_whole_file_include_keeps_top_level_key_matching_nested_anchor_name). Full suite: 1490 passed, 25 skipped. Re-ran the 803-file scan across pyobs-monet/pyobs-iagvt/pyobs-iag50 -- same result as before (2 pre-existing, unrelated errors; no comm_cfg leaks in any consuming config).

@thusser
thusser merged commit a5646fb into develop Aug 17, 2026
3 checks passed
@thusser
thusser deleted the feature/comm-cfg-anchor-leak branch August 17, 2026 18:22
thusser added a commit that referenced this pull request Aug 18, 2026
comm_cfg fix (#773) and this pass together close out every confirmed
dead/misplaced/typo'd kwarg found by re-running the investigation as a
static check across pyobs-monet, pyobs-iagvt, pyobs-iag50, and
pyobs-polaris (815 real config files). environment/database, the last
open blocker on the Object.__init__ warn/raise decision, is confirmed
gone -- nothing found is blocking that decision anymore.
thusser added a commit that referenced this pull request Sep 2, 2026
* Fix comm_cfg anchor-holder key leaking into config dicts

pre_process_yaml's whole-file {include file} splices the entire included
file, including any key whose sole purpose is holding a YAML anchor for
<<: *anchor use elsewhere (e.g. comm.shared.yaml's comm_cfg: &comm). That
leaked key then reached Object.__init__'s **kwargs and was silently
dropped there -- masking real config typos, since the include leak looked
identical to a typo.

Drop anchor-holder keys from whole-file splices using the (keyword, anchor)
pairs reload_anchors() already extracts; keyed includes of the same key are
left untouched. Handle the resulting all-keys-stripped case (comm.shared.yaml's
only top-level key is the anchor holder) by dropping the splice placeholder
entirely instead of emitting an empty "{}" mapping, which broke YAML parsing
when followed by block-style content.

Verified against a real pyobs-monet config: comm_cfg no longer leaks, and the
comm: <<: *comm alias still resolves correctly.

See specs/plans/2026-08-09-object-kwarg-validation.md for the investigation
and remaining open items (environment/database wrapper keys, Object.__init__
enforcement level).

* Address review: scope empty-splice drop and anchor detection correctly

Two silent-data-loss bugs found in review of the comm_cfg anchor-leak fix:

1. The "drop an empty splice placeholder" special case wasn't scoped to
   whole-file includes, so a keyed include that legitimately selects an
   empty mapping (`{include file key}` where key's value is `{}`) silently
   became `null` instead of `{}`.

2. Anchor-holder detection reused reload_anchors(), which matches
   `keyword: &anchor` at any nesting depth via a plain (not line-anchored)
   regex. A top-level key could be incorrectly dropped from a whole-file
   include just because some unrelated *nested* key elsewhere in the file
   happened to share its name and carry an anchor.

Fixed by gating the empty-splice case on the same whole-file condition as
the anchor-drop itself, and by adding top_level_anchor_keywords() -- a
line-anchored regex restricted to unindented keys -- used only for the
drop decision. reload_anchors() itself is unchanged, since replace_aliases()
still needs to resolve anchors at any nesting depth.

Added regression tests for both. Full suite: 1490 passed, 25 skipped.
Re-verified against all 803 yaml files in pyobs-monet, pyobs-iagvt, and
pyobs-iag50: same result as before (2 pre-existing, unrelated errors; no
comm_cfg leaks in any consuming config).
thusser added a commit that referenced this pull request Sep 2, 2026
comm_cfg fix (#773) and this pass together close out every confirmed
dead/misplaced/typo'd kwarg found by re-running the investigation as a
static check across pyobs-monet, pyobs-iagvt, pyobs-iag50, and
pyobs-polaris (815 real config files). environment/database, the last
open blocker on the Object.__init__ warn/raise decision, is confirmed
gone -- nothing found is blocking that decision anymore.
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