fix(loader): preserve non-env {{...}} placeholders in ConfigLoader (#81)

[owen-pengtao]

Summary

Fixes #81 — ConfigLoader.load_config corrupts non-env {{...}} placeholders in the plugin config.

load_config renders the entire config.yaml through one Jinja pass:

jinja_env = SandboxedEnvironment(loader=jinja2.BaseLoader(), autoescape=True)
rendered_template = jinja_env.from_string(template).render(env=os.environ)

Because the render only defines env, any {{...}} that is not an env reference is silently rendered to "" at load time — including placeholders a plugin legitimately stores in its config to render itself later, at runtime. The reported symptom: WebhookNotification's default_template

{ "event": "{{event}}", "timestamp": "{{timestamp}}", "violation": {{violation}}, "metadata": {{metadata}} }

is blanked to { "event": "", "timestamp": "", "violation": , ... } at load, so the plugin posts an all-empty, invalid-JSON body.

Fix

Stop treating the configuration as a template at all. A narrow regular expression resolves only the documented {{ env.NAME }} syntax and leaves every other byte of the file untouched:

_ENV_REFERENCE = re.compile(r"{{\s*env\.([A-Za-z_][A-Za-z0-9_]*)\s*}}")

def _replace(match: "re.Match[str]") -> str:
    # A missing variable keeps its original ``{{ env.NAME }}`` text.
    return os.environ.get(match.group(1), match.group(0))

Thanks to @araujof for catching that the original DebugUndefined approach was not sufficient. It does not make the document stop being a Jinja template, so it left five holes — all of them verified against jinja2 3.1.6:

input	DebugUndefined result
{{ env.URL }} where the value contains &	...?b=1&c=2 — HTML-escaped by autoescape=True
{{ env.MISSING }} (unset)	{{ no such element: os._Environ object['MISSING'] }}
{{event}}	{{ event }} — whitespace normalised
{{ event.name }}	raises UndefinedError
{{ event | upper }}	{{ EVENT }} — the filter rewrote the placeholder
{% if flag %}kept{% endif %}	`` — the block was evaluated away

The whitespace row is the decisive one: WebhookNotification._render_template substitutes the exact literal {{event}} with str.replace, so the canonicalised {{ event }} never matches. DebugUndefined would only have traded "blanked to empty" for "literal {{ event }} left in the JSON body" — issue #81 would not actually have been fixed. Preservation has to be byte-for-byte.

Behaviour notes

• Unset environment variable — {{ env.MISSING }} is now preserved verbatim rather than collapsing to "", so a missing variable stays visible instead of silently emptying a value.
• No more HTML escaping — the config is YAML, not HTML. autoescape=True was turning a valid query-string & into & inside loaded values. That is fixed as a side effect and covered by a test.
• Narrower accepted syntax — {{ env['NAME'] }} and {{ env.NAME | default('x') }} are no longer interpolated. No configuration in this repository uses either form, but it is a behaviour change worth a release note; the default filter can be added to the restricted syntax later if it turns out to be needed.
• use_jinja parameter name kept — cpex/framework/external/mcp/server/server.py passes it, so renaming would break the public signature. Its docstring now describes what it actually does.
• YAML quoting boundary — an env value containing a double quote now breaks the surrounding quoted scalar and raises, where autoescape previously hid it by loading a mangled " value. Failing loudly is the better of the two; a characterisation test pins the behaviour so any future escaping/quoting change is deliberate.

Tests

tests/unit/cpex/framework/loader/test_config_loader.py — 9 tests covering exactly the cases requested in review:

• {{ env.X }} is substituted;
• a non-env placeholder survives byte-for-byte (the [BUG]: ConfigLoader.load_config Jinja pass blanks non-env {{...}} placeholders in plugin config #81 regression);
• nested expressions ({{ event.name }}) are preserved, not raised on;
• filters ({{ event | upper }}) are preserved, not applied;
• {% ... %} blocks are preserved, not evaluated;
• an unset environment variable is preserved verbatim;
• substituted values are not HTML-escaped;
• an env value that breaks YAML quoting raises rather than loading silently wrong;
• use_jinja=False leaves everything literal.

Verification:

• 9/9 new tests and 3/3 module doctests pass.
• The new suite was run against both previous implementations (0.1.x baseline and the DebugUndefined commit) — 7 of 8 applicable tests fail on each, so these are genuine regression tests rather than tests written to fit the new code.
• Full unit suite: 1702 passed. The 10 failures and the tests/unit/cpex/tools/test_cli.py collection error are pre-existing and unrelated — the identical set fails on the unmodified 0.1.x baseline (Windows path / venv / catalog environment issues).

Signed-off-by: Owen Peng tao.peng@tibco.com

🤖 Generated with Claude Code

[owen-pengtao]

can anyone review this pr?

[araujof]

can anyone review this pr?

@owen-pengtao I was busy with something else, but marking myself to review it this week.

[owen-pengtao]

@araujof do you able to review this pr week?

[araujof]

@owen-pengtao Thanks for this PR. DebugUndefined does not preserve arbitrary non-env Jinja.** The entire YAML file is still evaluated as a Jinja template. For example, {{ event.name }} raises UndefinedError, filters can transform placeholders, and conditional blocks can disappear. An unset {{ env.MISSING }} also becomes Jinja diagnostic text rather than remaining {{ env.MISSING }} as described in the PR. Use genuinely env-only interpolation and add regression coverage for nested expressions, filters, conditionals, and missing environment variables.

Below is a suggested alternative that addresses this problem. Would you like to try it and update your PR?

Suggested implementation

Replace only the explicitly supported {{ env.NAME }} syntax instead of rendering the entire configuration as Jinja. For example:

import re

_ENV_REFERENCE = re.compile(r"{{\s*env\.([A-Za-z_][A-Za-z0-9_]*)\s*}}")


def _interpolate_env(template: str) -> str:
    def replace(match: re.Match[str]) -> str:
        name = match.group(1)
        # Preserve an unset reference. This policy could instead raise a clear
        # configuration error, but it should be explicit and tested.
        return os.environ.get(name, match.group(0))

    return _ENV_REFERENCE.sub(replace, template)

Then use _interpolate_env(template) in the use_jinja branch. Given:

endpoint: "{{ env.WEBHOOK_URL }}"
default_template: "{{ event.name | upper }}"

only WEBHOOK_URL would be substituted; the runtime template would remain byte-for-byte unchanged. If | default(...) must remain supported, add it as an explicitly parsed part of this restricted syntax rather than evaluating the whole YAML document with Jinja.

Add tests confirming that nested expressions, filters, and {% ... %} blocks remain unchanged, plus a test defining the behavior of an unset environment variable.

[owen-pengtao]

@araujof Thank you — you were right on every point, and the PR is updated to your suggested approach (pushed as 8698ace).

I verified each of your claims against jinja2 3.1.6 before changing anything, with the exact configuration the PR used (SandboxedEnvironment(autoescape=True, undefined=DebugUndefined)):

input	result
{{ env.URL }} where the value contains &	...?b=1&c=2 — HTML-escaped by autoescape=True
{{ env.MISSING }} (unset)	{{ no such element: os._Environ object['MISSING'] }}
{{event}}	{{ event }} — whitespace normalised
{{ event.name }}	raises UndefinedError
{{ event | upper }}	{{ EVENT }}
{% if flag %}kept{% endif %}	`` — evaluated away

All confirmed. The unset-variable row is also a regression the PR itself introduced, and it contradicted my own PR description — thank you for catching that.

One more piece of evidence for your "byte-for-byte" requirement, which I think strengthens the case beyond style. I went and read the motivating consumer, plugins/webhook_notification/webhook_notification.py in IBM/mcp-context-forge:

placeholder = f"{{{{{key}}}}}"          # -> "{{event}}", no inner spaces
result = result.replace(placeholder, ...)

It is an exact str.replace on the literal {{event}}. Since DebugUndefined canonicalises that to {{ event }}, the replace would never match — so the DebugUndefined version did not actually fix #81. It only swapped the failure mode from "blanked to empty" to "literal {{ event }} left in the JSON body". The payload was broken either way. Byte-for-byte preservation is the requirement, which is exactly what your regex approach delivers.

What the new commit does

Replaces the Jinja pass with your narrow _ENV_REFERENCE regex, and removes the now-unused jinja2 imports. An unset variable is preserved verbatim, as in your snippet; I kept use_jinja as the parameter name because cpex/framework/external/mcp/server/server.py passes it and renaming would break the public signature, but its docstring now describes the real behaviour.

Tests — the regression coverage you asked for, 9 cases: env substitution; byte-for-byte placeholder preservation; nested expressions; filters; {% ... %} blocks; unset environment variable; plus two the investigation turned up — that substituted values are no longer HTML-escaped, and use_jinja=False.

I also ran the new suite against both previous implementations (the 0.1.x baseline and the DebugUndefined commit): 7 of the 8 applicable tests fail on each, so they are genuine regression tests rather than tests fitted to the new code.

Two things worth flagging for your call:

1. Narrower accepted syntax. {{ env['NAME'] }} and {{ env.NAME | default('x') }} are no longer interpolated. Nothing in this repository uses either form, but it is a behaviour change for downstream configs — should it get a release note, and do you want | default(...) added to the restricted syntax now, or left until someone needs it?
2. A YAML quoting boundary that dropping autoescape exposes. An env value containing a double quote terminates the quoted scalar it lands in, so it now raises a YAMLError. Previously autoescape hid this by rewriting the quote to " and loading a silently wrong value. I think failing loudly is clearly better, so I pinned it with a characterisation test rather than adding escaping — but if you would rather the loader quote/escape substituted values properly, that is a slightly larger change and I am happy to do it here or in a follow-up.

Full unit suite: 1702 passed. The 10 failures plus the test_cli.py collection error are pre-existing and unrelated — the identical set fails on an unmodified 0.1.x checkout on my machine (Windows path / venv / catalog issues).