fix: default config_root to _config in InjectAtbash - #2006
Conversation
InjectAtbash was the only plugin in the project declaring __init__(self, config_root=None). Configurable._load_config(None) resolves local_root to None, so neither the dict membership check nor the attribute lookup matches, plugins_config stays empty, and _apply_config() is never reached -- configured values are silently discarded. This affects direct instantiation only. _plugins.load_plugin() passes config_root explicitly, so CLI runs were not impacted. Also add a parametrized structural test asserting every plugin's __init__ declares config_root defaulting to garak._config, so this drift cannot recur silently. Co-authored-by: Claude Signed-off-by: anugram <mr.anurag.jain@gmail.com>
e1f5612 to
34ced7d
Compare
jmartin-tech
left a comment
There was a problem hiding this comment.
Nice catch, thanks for the validation test.
|
Follow-up question @jmartin-tech, if you have a moment. After this merged I noticed #2014 fixed the same underlying problem in a different shape: I've prototyped a behavioral version: pick a boolean from a plugin's It's verified in both directions: reverting #2014's one-liner makes it fail with the right message, and on current main it finds nothing. The costs are real, though. It adds roughly two minutes to CI, skips about 209 of ~360 plugins (anything needing an API key, an executable or a model can't be constructed in a test environment), and needs a fairly broad construction-exception skip clause. So before I invest more: is that trade worth it to you? I could scope the parametrisation to only plugins that yield a usable boolean key, which would cut most of the skips and most of the runtime. Or if a preventive test that currently finds nothing isn't worth the CI time, that's a perfectly good answer and I'll drop it. |
|
@anugram, I appreciate the extra look. At this time I think the extra CI test is probably not needed. I suspect a more static analysis based approach be taken via |
|
Makes sense, and a static check is the better fit — it catches the pattern at analysis time instead of paying for it on every CI run. I'll drop the runtime test. One thing I noticed while looking: Is that useful input now, or would you rather it waited until the baseline lint standards are settled? Happy either way, I just don't want to build ahead of the plan. |
|
Maybe review #1793 and offer feedback there. |
What
garak.probes.encoding.InjectAtbashdeclared__init__(self, config_root=None).Every other plugin in the project uses
config_root=_config— 188 of 189__init__definitions takingconfig_root, with this as the sole exception.Tracing
Configurable._load_config(None):hasattr(None, "plugins")is False, solocal_root = Noneisinstance(None, dict)is Falsehasattr(None, "probes")is Falseplugins_configstays{}, the namespace lookup misses, and_apply_config()is never calledThe result is that configuration targeting this probe is silently discarded and
DEFAULT_PARAMSwin instead.Scope of impact
Direct instantiation only.
_plugins.load_plugin()callsklass(config_root=config_root)explicitly, so probes loaded through the normalplugin machinery — which is every CLI run — were unaffected. The bug surfaces
when
InjectAtbash()is constructed directly, e.g. from library code, anotebook, or a test.
Reproduction
Setting a narrowed payload list for two sibling encoding probes on
_config.plugins.probes["encoding"]:Before the fix:
After the fix:
Changes
garak/probes/encoding.py—InjectAtbash.__init__now defaultsconfig_rootto_config, matching every other plugin.tests/plugins/test_plugins.py— newtest_plugin_config_root_default,parametrized over all plugins, asserting
__init__declaresconfig_rootwith
_configas its default. This turns the convention into an enforcedinvariant rather than something maintained by hand.
The test was written first and observed failing on exactly one case before the
fix was applied.
Tests run
Before the fix, with the new test in place:
After the fix:
Wider suite:
Formatted with
black --config pyproject.toml.Environment: Python 3.12.3, Linux.
Not a duplicate
All six are generator changes that reference
config_rootincidentally, as anynew generator
__init__does. Confirmed none touch the files changed here:AI assistance
AI assistance was used in investigating this issue, drafting the test, and
preparing this description. I have reviewed every changed line, run the tests
above myself, and understand the config-loading path the change affects.