Skip to content

diagnose: _POLICY_* interception matches bare key names at any nesting depth #652

Description

@pbean

What

diagnostics._scrub_policy (src/bmad_loop/diagnostics.py:427-434) tests each key against _POLICY_COUNT_KEYS, _POLICY_BOOL_KEYS and _POLICY_KEYSET_KEYS by bare key name, with no record of which section it sits in:

key = str(raw_key)
if key in _POLICY_COUNT_KEYS:
    out[f"{key}_count"] = len(value) if isinstance(value, (list, tuple, dict)) else 0
elif key in _POLICY_BOOL_KEYS:
    out[f"{key}_set"] = bool(value)
elif key in _POLICY_KEYSET_KEYS and isinstance(value, dict):
    ...

The sets are named for specific fields — extra_args/env/worktree_seed (count), commit_message_template (bool), settings (plugin-id keyset, hardened in #186) — but the match is depth-blind. Any future policy field that happens to reuse one of those names, anywhere in the tree, is silently rewritten by a rule that was not written for it.

Why it matters

This is an interception over-reach, not a leak — the failure direction is loss of diagnostic fidelity, not disclosure:

  • a new foo.settings that is a list rather than a plugin table would be reduced to a sorted identifier-gated keyset;
  • a new foo.env string would be emitted as env_count: 0 (the else 0 arm), which reads as "empty" rather than "not a collection";
  • the reduced key is renamed (_count/_set), so a dump consumer sees a field that does not exist in Policy and cannot tell the substitution happened.

Nothing collides today — the name reuse is hypothetical, which is why this is a hardening item rather than a bug report.

Suggested fix

Make the interception section-aware: match on the dotted path (adapter.extra_args, plugins.settings, scm.commit_message_template, and so on) rather than the bare leaf name, threading the path through the recursion in _scrub_policy. That also makes the sets self-documenting about which section each rule was written for.

Context

Found while writing the key-passthrough invariant tests for #202, and deliberately left out of scope there (#202 changes no runtime behavior — it is a comment plus tests). Related: #186, which hardened the plugins.settings interception this rides on.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Robustness, enhancement, tests, or docs worth schedulingarea:configBMAD config resolution and the setup skillarea:engineOrchestrator engine and run lifecycleenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions