🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

#!/bin/bash
set -euo pipefail

printf 'Files matching plugin_manager.py:\n'
fd -a 'plugin_manager.py' . || true

printf '\nRelevant file outline:\n'
ast-grep outline unstract/core/src/unstract/core/plugins/plugin_manager.py --view compact || true

printf '\nRelevant lines 180-250:\n'
sed -n '180,250p' unstract/core/src/unstract/core/plugins/plugin_manager.py | nl -ba -v180

printf '\nSearch plugin_manager load_plugins context and metadata source:\n'
rg -n "def load_plugins|metadata\\(|metadata_items|\\.items\\(\\)|except KeyError|key\\.endswith|_cls|_class" unstract/core/src/unstract/core/plugins/plugin_manager.py

#!/bin/bash
set -u

FILE="unstract/core/src/unstract/core/plugins/plugin_manager.py"
TESTFILE="unstract/core/tests/test_plugin_manager.py"

printf 'Line count for plugin_manager.py:\n'
wc -l "$FILE"

printf '\nRelevant lines 180-250 from %s:\n' "$FILE"
sed -n '180,250p' "$FILE" | awk '{printf "%5d  %s\n", NR+179, $0}'

printf '\nSearch loader/relevant symbols:\n'
rg -n "def load_plugins|\\bmetadata\\b|metadata_items|\\.items\\(\\)|except KeyError|key\\.endswith|_cls|_class|isinstance\\(.*key.*str" "$FILE"

printf '\nRelevant plugin_manager tests, if any:\n'
if [ -f "$TESTFILE" ]; then
  rg -n "metadata|endswith|non-string|AttributeError|load_plugins|load_plugin" "$TESTFILE" || true
else
  echo "test file not found"
fi

printf '\nStandalone Python behavior for non-string dict key with endswith in try/except KeyError:\n'
python3 - <<'PY'
def probe(metadata, suffixes):
    plugin_data = {}
    try:
        for key, value in metadata.items():
            if key.endswith(suffixes) and key not in plugin_data:
                plugin_data[key] = value
        return "ok", plugin_data
    except KeyError:
        return "keyerror", {}

print(probe({123: "value"}, ("_cls", "_class")))
PY

#!/bin/bash
set -u

TESTFILE="unstract/core/tests/test_plugin_manager.py"

printf 'Tests around non-class metadata fields:\n'
sed -n '38,95p' "$TESTFILE" | awk '{printf "%5d  %s\n", NR+37, $0}'

printf '\nRelevant plugin manager implementation before load_plugins:\n'
sed -n '60,175p' unstract/core/src/unstract/core/plugins/plugin_manager.py | awk '{printf "%5d  %s\n", NR+59, $0}'

printf '\nPython behavior for metadata keys that satisfy get() but fail only at endswith:\n'
python3 - <<'PY'
def full_probe(metadata):
    try:
        if metadata.get("disable", False):
            return "skipped"
        if not metadata.get("is_active", True):
            return "inactive"
        plugin_name = metadata["name"]
        plugin_data = {
            "version": metadata.get("version", "unknown"),
            "name": "ignored",  # model the existing plugin_data with static name? No.
        }
        for key, value in metadata.items():
            if key.endswith(("_cls", "_class")) and key not in plugin_data:
                plugin_data[key] = value
        return "loaded", plugin_name, plugin_data
    except KeyError as e:
        return "keyerror", str(e)
    except Exception as e:
        return type(e).__name__, str(e)

print(full_probe({"name": "ok", "_cls": "cls", "nonstring": None}))
print(full_probe({"name": "ok", 123: "value"}))
PY

Guard metadata keys before calling endswith.

Metadata keys are read through metadata.get(...) before being passed to key.endswith(...), so a non-string key can reach this call and raise AttributeError. Handle that exception too, or validate/skip invalid keys before suffix matching so broken metadata doesn’t break the entire plugin load pass.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@unstract/core/src/unstract/core/plugins/plugin_manager.py` around lines 218 -
224, Update the metadata iteration in the plugin loading logic to validate that
each key is a string before applying the "_cls"/"_class" suffix check. Skip
non-string keys while preserving the existing condition that only adds missing
plugin_data fields.

[Silent failure — Medium] This loop propagates class handles by name only — it copies whatever keys end in _cls/_class, with no notion of a required field. That fixes the headers_cache_class drop, but the mechanism that made it invisible is untouched: if a plugin declares a misnamed field (header_cache_class, headers_cache_clss), the correct key is simply absent and verticals_usage_plugin["headers_cache_class"]() in backend/plugins/workflow_manager/workflow_v2/api_hub_usage_utils.py:38,99 raises KeyError again — swallowed by the same broad except Exception (lines 51/74/101) this PR was written to defeat.

Consider having consumers read with .get() and log an explicit failure, or validating a plugin's required class fields at load time, so a typo errors loudly at startup instead of failing silently per-request. (Narrowing those except Exception blocks to distinguish a permanent misconfiguration from a transient runtime error is the more complete fix, but is outside this PR's changed lines.)

[Maintainability — Low] The _cls/_class suffix is now load-bearing behavior but is documented only in this comment. Consider promoting it to a module-level constant (e.g. _CLASS_HANDLE_SUFFIXES = ("_cls", "_class")) so the contract is greppable and testable. Also consider dropping the headers_cache_class example from the comment — it names a single enterprise plugin's field, so it is the part most likely to rot; the commit message already records the motivating case.

[Nit — Low] key not in plugin_data can never be false here: at this point plugin_data holds only version/module/metadata (none end in _cls/_class) and metadata keys are already unique, so the guard is effectively dead. Fine to keep as future-proofing against a reserved key that someday ends in _class, but if so, add a one-line note saying it's purely defensive — otherwise it reads as load-bearing when it isn't. Flagged independently by the code-review, simplifier, and test passes.

[Test gap — High value] The real behavioral widening in this change is that the loop propagates a *_cls/*_class key regardless of its value, whereas the old code copied 3 known class fields. No test pins that contract. A future "only propagate real classes" isinstance/callable check would pass all 4 tests here while silently reverting the fix for a field whose value is None/lazy at load time.

Suggest adding a test with metadata {"name": "sample_plugin", "headers_cache_class": None} asserting plugin["headers_cache_class"] is None — this locks "propagate by name, not by value."

[Test gap — Medium] The skip condition at plugin_manager.py:202 is metadata.get("disable", False) or not metadata.get("is_active", True). This test exercises only the disable=True operand; the is_active=False branch is separate and could regress independently (e.g. someone flips the default or drops the operand). Suggest mirroring this test with metadata {"name": "sample_plugin", "is_active": False} and asserting not manager.has_plugin("sample_plugin").