Bugfix/general

[graepaul]

Summary

I did a scan of potential bugs in the repo. The below fixes are to cleanup various bugs that were found. All new issues generated a new unit tests and then were improved so that test would pass.

1. Literal fields produced unconstrained CLI arguments

Files: typeutils.py, cli/dynamicparserbuilder.py
Tests: test_dynamic_parser_builder.py, test_type_utils.py

• process_type() reduced any parameterized type to a single inner_type with next(arg for arg in get_args(...)). For Literal["fast","slow"] that kept "fast" and dropped the rest, so the allowed-value set was lost before it ever reached the parser. - get_literal_choices() returned None on both branches — it computed a value and then discarded it. add_argument() looked for choices by scanning get_args(annotation) for a nested Literal, which only matches Optional[Literal[...]]; for a bare -Literal[...] the args are the values themselves, so no match. Net effect: --mode accepted any string and the constraint was enforced later by pydantic, if at all.

-build_type_class() now keeps the full value list for Literal, get_literal_choices() reads it, and add_argument() handles a bare Literal annotation. metavar formatting also assumed str values and would raise TypeError on an int-valued Literal.

2. Annotated was not stripped inside a Union

Files: typeutils.py
Tests: test_type_utils.py

• process_type() unwrapped Annotated at the top level only. Inside the Union branch, Optional[Annotated[int, "meta"]] yielded TypeClass(type_class=Annotated, inner_type=int), so every consumer keyed on int/str/list missed it and fell through to the generic str default.

3. File artifacts could be written outside the run directory

Files: connection/inband/inband.py
Tests: test_file_artifact.py

Both artifact classes built their target with os.path.join(log_path, self.filename). os.path.join discards the left operand when the right is absolute, so an artifact carrying /tmp/x.txt wrote to /tmp/x.txt and escaped the run directory silently. A filename containing a subdirectory failed differently — the parent was never created, so open() raised FileNotFoundError.

New BaseFileArtifact.resolve_write_path() is shared by both subclasses: it drops the path anchor and any ../. segments, joins the remainder under log_path, and creates parents. Nested names are preserved; nothing can resolve outside the log directory.

4. Shared hook list leaked log paths between plugins

Files: interfaces/plugin.py, base/inbandcollectortask.py
Tests: test_plugin_interface.py, test_datacollector.py

PluginInterface.__init__ assigned the caller's task_result_hooks list by reference and then appended to it. When the executor passes one hook list to several plugins, the first plugin's FileSystemLogHook ends up in the shared list; the second plugin's guard tested isinstance(hook, FileSystemLogHook) with no path comparison, matched the first plugin's hook, and skipped adding its own — so it wrote its results into the first plugin's directory. Now the list is copied per plugin and the guard compares log_base_path.

Separately, InBandDataCollector.__init__ did not accept or forward log_path, so in-band collectors dropped it entirely.

5. Connection-manager cache handed out its internal dict

Files: pluginregistry.py
Tests: test_connection_manager_entrypoints.py

load_connection_managers_from_entry_points() returned the module-level cache by reference; a caller mutating the returned dict corrupted the cache for the rest of the process. Two of the three return paths now copy.

Incomplete: the double-checked return inside the lock (pluginregistry.py:233) still returns the cache uncopied, which is the path a second thread takes. The copy added at line 228 is redundant — _load_connection_managers_uncached() already returns managers.copy() at line 207.

6. JSON/config load failures were swallowed or crashed the run

Files: configregistry.py, cli/inputargtypes.py, cli/compare_runs.py
Tests: test_config_registry.py, test_input_arg_types.py, test_compare_runs.py

• ConfigRegistry caught ValidationError/JSONDecodeError and passed, so a malformed config silently vanished and the run proceeded with an incomplete plugin set. It now raises RuntimeError naming the file.
• ModelArgHandler.process_file_arg passed whatever json.load returned into self.model(**data). A JSON array or string produced a bare TypeError out of argparse instead of a usable message. arg_check() rejects non-objects with ArgumentTypeError.
• _load_plugin_data_from_run caught JSONDecodeError/TypeError/OSError but not pydantic's ValidationError, so one malformed result.json aborted the whole comparison. It is now skipped with a warning like the other failures.

Call this out in the PR description: the ConfigRegistry change is a behavior change, not just a crash fix. Any unrelated .json sitting in a config directory that used to be ignored will now fail the run. (This is why the test fixtures moved into fixtures/valid_configs/.) The new raise RuntimeError(...) statements also drop the cause — they should use from e.

7. Built-in plugin configs were mutated in place

Files: cli/helper.py
Tests: test_cli_helper.py

get_plugin_configs() appended built_in_configs[config] by reference, and the caller then merges global_args into those objects. The registry's copy of the built-in config carried that mutation forward, so a second config selection in the same process started from dirty state. Now deep-copied on append.

Note: base_config is constructed fresh on line 115, so the [deepcopy(c) for c in [base_config]] on line 120 protects nothing and reads oddly — worth collapsing to a plain [base_config] or [deepcopy(base_config)].

8. CLI logging defects

Files: cli/helper.py
Tests: test_cli_helper.py

• generate_reference_config() called logger.warning with a two-%s format string and one argument. logging traps the TypeError internally and emits --- Logging error --- instead of the warning, so the skip reason was never recorded. Arity is now correct, though the second value is always None at that point — the message could be reworded.
• dump_to_csv() logged "Data written to csv file" after the try/except, so it reported success even when the write raised and was swallowed. The success log moved inside the try, with FileNotFoundError and ValueError reported distinctly.

9. Subclass validation ran against abstract bases

Files: interfaces/task.py, interfaces/dataanalyzertask.py
Tests: test_task.py, test_dataanalyzer.py

Task.__init_subclass__ raised TypeError whenever TASK_TYPE was None, including for abstract intermediate classes that legitimately leave it unset, and it assumed the attribute exists. Both checks are now gated on inspect.isabstract(cls). DataAnalyzer got the same treatment for DATA_MODEL, plus a check that analyze_data is defined and callable.

The DataAnalyzer condition is redundant as written — (not isabstract and not getattr(cls, "DATA_MODEL", None)) or (not isabstract and cls.DATA_MODEL is None); the first clause already covers the second.

10. Collector returning no data was reported as success

Files: interfaces/datacollectortask.py
Tests: test_datacollector.py

The guard was if data is None and not result.status. ExecutionStatus is a plain enum.Enum, so every member — including UNSET — is truthy, and not result.status is always False. The branch never executed: a collector that returned no data and set no status was finalized as-is rather than being marked EXECUTION_FAILURE. The check is now explicit against {OK, UNSET}, with a default message when the collector supplied none.

11. Data-model discovery picked the wrong file

Files: interfaces/dataplugin.py, models/datamodel.py
Tests: test_dataplugin.py, test_datamodel.py

• _find_datamodel_path() tested endswith("datamodel.json"), == want_json, and endswith(".log") inside a single pass over os.listdir(). Whichever file the OS listed first won, so a .log in the same directory could shadow the real data-model JSON — nondeterministic across machines. Now two ordered passes: model JSON first, .log only as fallback. The suffix match was also generic; it is now keyed to the model class name.
• Collector log directories were built with pascal_to_snake() while they are written using resolve_log_dir_name(), so any plugin with a registered log-dir-name override was looked up under the wrong path and its data never found.
• DataPlugin.data setter reported the expected type as self.DATA_MODEL.__class__.__name__, which is the metaclass — every error read expected ModelMetaclass.
• DataModel.import_model() called tarfile.is_tarfile() before the os.path.isdir() check. On a directory that raises IsADirectoryError, so folder-based import never worked. Order is now isdir → tarfile → JSON file. self.model_fields also moved to self.__class__.model_fields (instance access is deprecated in pydantic 2.11+).

Dead code introduced here: the trailing return cls() in import_model is unreachable after the if/elif/else.

12. Zero-width regex match hung the analyzer

Files: base/regexanalyzer.py
Tests: test_regexanalyzer.py

check_all_regexes() drives while search_from <= len(content) and advances with search_from = match_obj.end(). For a pattern that can match empty (^, \b, a*), end() == start() == search_from, so the cursor never moves — infinite loop, and in the ungrouped path it appends an event every iteration, so memory grows until the process dies. The cursor is now forced forward one character on a zero-width match.

13. Negative MCE bank numbers parsed as ranges

Files: base/match_ignore.py
Tests: test_match_ignore.py

parse_mce_bank_spec() treated any token containing - as a range, so "-5" split into ("", "5") and int("") raised a bare ValueError with no context. Empty endpoints and negative end values are now rejected with an explicit Invalid MCE bank range message.

14. Sudo password sent without a trailing newline

Files: connection/inband/inbandremote.py
Tests: test_shellcommand.py

Operator precedence bug. The expression was:

self.ssh_params.password.get_secret_value() if self.ssh_params.password else "" + "\n"

A conditional expression binds looser than +, so this parses as password if password else ("" + "\n"). When a password was actually configured, it was written to stdin with no terminating newline — the remote sudo prompt never saw a completed line and the command blocked until timeout. The newline only appeared in the branch where there was no password. Now parenthesized so "\n" is appended in both cases.

15. HTTP-date Retry-After crashed OEM diagnostic collection

Files: connection/redfish/redfish_oem_diag.py
Tests: test_redfish_oem_diag.py

RFC 7231 allows Retry-After as either delta-seconds or an HTTP-date. int(resp.headers.get("Retry-After", 1) or 1) raises ValueError on the date form, aborting collection against BMCs that use it. Now falls back to the 1-second default.

16. Utility fixes

Files: utils.py
Tests: test_utils.py

• bytes_to_human_readable() topped out at TB, so petabyte-scale values rendered as 1000.0TB. Added a PB tier.
• find_annotation_in_container() called issubclass(item, target_type) on every get_args element. Literal args are values, not types, so issubclass("fast", str) raised TypeError: issubclass() arg 1 must be a class. Non-types are now converted with type(item) before the check.

17. CollectorArgs config key had no effect

Files: models/collectorargs.py
Tests: test_collectorargs.py

model_config was a raw dict containing "exclude_none": True. That is not a pydantic model-config key — it is a model_dump() argument — so it was inert while looking like it did something. Replaced with a typed ConfigDict(extra="forbid"), which also makes the mistake a type error next time.

Test plan

• pytest test/unit
• pytest test/functional (if applicable)
• pre-commit run --all-files

Checklist

• Added/updated tests (or explained why not)
• Updated docs/README if behavior changed
• No secrets or credentials committed