fix(lint): tune pylintrc and fix violations across core modules

[ppradyoth]

Summary

This is the draft standard requested in #1792. It fixes genuine pylint violations, documents intentional exceptions with inline suppressions, and removes the unrecognised suggestion-mode option — so the Garak linting workflow can be enabled for CI runs.

Every suppression has a comment explaining why it is intentional. Happy to iterate on any of these in review.

Changes

pylintrc

• Remove suggestion-mode=yes — removed in pylint 3.x, caused E0015 on every run
• Add too-many-positional-arguments to disable list (consistent with existing too-many-arguments exemption)
• Add Director to ignored-classes — payload_list is dynamic and cannot be inferred at static-analysis time

garak/exception.py — add module docstring

garak/command.py

• Add docstrings to all public functions
• Remove unused plugin_info as get_plugin_info import alias inside print_plugins
• Suppress broad-exception-caught in end_run — report failures must not crash the CLI
• Suppress no-member on cli_args — set dynamically by argparse

garak/configurable.py

• Add module and class docstrings; class docstring explains the ENV_VAR dynamic attribute pattern
• Fix logging f-strings to %s lazy formatting
• Suppress no-member on self.ENV_VAR — defined by subclasses (as confirmed in Linting CI fails on main due to pre-existing pylint violations #1792)
• Suppress unsupported-membership-test on _supported_params — guarded by isinstance above
• Suppress access-member-before-definition on api_key — intentional lazy-set pattern

garak/interactive.py

• Add module docstring, class docstring, and missing function docstrings
• Replace list comprehension with set comprehension
• Fix logging f-strings
• Make return statements consistent in do_probe
• Remove f-string without interpolation
• Rename unused cmd2 interface params to _command/_line/_args
• Suppress no-member on self.settings — set dynamically by cmd2.Cmd

garak/payloads.py

• Fix import order: stdlib before third-party
• Add docstrings to module-level search() and load()
• Suppress not-an-iterable/unsubscriptable-object in Director — payload_list is None at class level but always a dict after _refresh_payloads()

garak/_config.py

• Add missing docstrings
• Fix logging f-strings
• Rename dummy → _dummy in _garak_user_agent (required by requests UA callback signature)
• Remove unnecessary elif after raise
• Use generator in any() instead of list
• Add # pylint: disable=global-statement with explanations — _config is a module-level singleton by design

garak/report.py

• Fix import order: stdlib (datetime) before third-party
• Replace range(len(evals)) with direct iteration
• Suppress comparison-with-itself on all_tags == all_tags — standard pandas NaN sentinel

Test plan

• pylint --rcfile=pylintrc garak/ — no new errors on changed files
• pytest tests/ passes — no behaviour changes, docstrings/style only
• Linting CI workflow passes on this branch

Open questions for maintainers

1. Are there other intentional dynamic-attribute patterns we should add to generated-members or ignored-classes?
2. Preferred CI gate: minimum pylint score threshold or just exit code 0?
3. Should enabling the linting workflow for PRs be part of this PR or a follow-up?

Closes #1792

[ppradyoth]

Remaining violations after this PR

Running pylint --rcfile=pylintrc garak on this branch shows 1,068 remaining violations across 106 files (excluding import-error and cyclic-import which are environment/architecture issues unrelated to lint style).

Here's the breakdown by type to help prioritise the next passes:

Count	Message	Notes
640	no-member	Dominant issue — almost entirely the plugin system's dynamic attribute loading (generators, probes, detectors, buffs). The generated-members or ignored-classes config is the right fix rather than 640 inline suppresses.
86	logging-fstring-interpolation	Mechanical fix — f"..." → %s lazy format throughout probes/generators.
63	missing-function-docstring	Spread across probe, generator, detector, and harness modules.
35	no-else-return / 12 no-else-raise	Style fixes, safe to automate.
35	broad-exception-caught	Needs case-by-case review — some may be intentional (network calls, plugin loading).
32	use-list-literal	Mechanical: list() → [].
25	wrong-import-order	Stdlib before third-party — mechanical.
18	access-member-before-definition	Likely the same lazy-set pattern as configurable.py.
15	undefined-variable	Worth reviewing individually — could be real bugs or inference failures.
14	missing-class-docstring	Mechanical.
13	missing-module-docstring	Mechanical.
12	unused-argument	Mix of interface callbacks (suppress with _param) and real dead args.
5	inconsistent-return-statements	Small, safe fixes.

The 640 no-member violations are the blocker. Most come from garak's plugin system where attributes are set dynamically (e.g. self.generations, self.name, self.tags on probe/generator instances). The cleanest fix is probably extending generated-members in pylintrc with the known dynamic attribute names, or adding the base plugin classes to ignored-classes. Happy to take a pass at this if you can confirm which attribute names are set dynamically by the framework.

The remaining ~400 violations are largely mechanical and can be addressed file-by-file. I can batch them up as follow-on PRs once this draft standard is agreed.

[ppradyoth]

Suggested next steps after this draft

I think the cleanest path is to keep this PR focused on establishing the linting baseline and fixing the concrete violations in the touched core files, then handle the larger remaining reduction as follow-up work once maintainers agree on policy.

A local before/after count with pylint --rcfile=pylintrc --disable=import-error,cyclic-import garak shows:

Scope	Before this PR	After this PR	Delta
Whole repo	1,144	1,068	-76
Files touched by this PR	69	0	-69

The biggest remaining bucket is no-member at 640 violations. Most of those look like false positives from garak's dynamic plugin/config loading, so I would avoid scattering hundreds of inline suppressions. Better follow-up options are:

1. Extend generated-members with known dynamic attribute names set by the framework.
2. Add narrowly scoped ignored-classes where a class is intentionally dynamic and static inference is not useful.
3. Declare common dynamic attributes on shared base classes where doing so improves readability and type clarity.
4. Reserve inline # pylint: disable=... comments for exceptional cases that are local and intentional.

For the CI gate, my recommendation is to start with an agreed baseline/exit-code gate rather than a broad score threshold. A score threshold can still allow new important errors through if the aggregate score remains high enough.

I would also suggest enabling the linting workflow in a follow-up PR after the dynamic-attribute policy is agreed, so this PR can stay reviewable and serve as the draft standard requested in #1792.

[anugram]

@jmartin-tech pointed me here from a thread on #2006. Some context for @ppradyoth: I recently landed a small config_root fix in configurable.py, and the follow-up discussion was about whether a pylint rule could catch that class of bug in future — which led to "the lint standards need to settle first", which is this. So I have some interest in seeing it land.

I pulled the branch and ran the suite plus a few pylint experiments rather than just reading the diff. I'm new to the repo and not a maintainer, so treat the opinions below as input rather than verdicts — the numbers are reproducible if you want to check them.

Suite passes on the branch: 4323 passed, 97 skipped in ~10m23s, Python 3.12.3 / Linux, pylint 4.0.6. No new dependencies. I spot-checked the conversions that could have hidden a behaviour change — set([...]) → {...}, any([...]) → any(...), return → return None, and the dummy → _dummy rename in _garak_user_agent (safe: requests calls default_user_agent() with no arguments, from default_headers(), its only call site). All clean. The disable/enable pairing around each block-scoped suppression is nice — the boundary is explicit rather than running to the end of the enclosing scope.

One substantive finding.

pylint garak is not deterministic, and it's entirely cyclic-import.

Five runs on unmodified main, same tree, cache cleared first:

--jobs=1 :  1178, 1173, 1178
--jobs=10:  1184, 1185          (jobs=10 being the pylintrc setting)

Diffing two runs' sorted output gives 19 differing lines, and all 19 are (cyclic-import), all reported against garak/analyze/__init__.py:1:0:

< garak/analyze/__init__.py:1:0: R0401: Cyclic import (garak.command -> garak.probes.base)
> garak/analyze/__init__.py:1:0: R0401: Cyclic import (garak.command -> garak.harnesses.probewise)
> garak/analyze/__init__.py:1:0: R0401: Cyclic import (garak.command -> garak.harnesses.probewise -> garak.harnesses.base -> garak.probes.base)

Nothing else varies — no no-member drift — and it isn't a parallelism artefact, since --jobs=1 moves too. pylint reports one representative path per import cycle and which path it picks depends on module traversal order, so the same underlying cycles surface with different text on each run. The cycles are real; only the rendering is unstable.

cyclic-import is explicitly in the enable= list (pylintrc:638), so it's in scope for the baseline. Two consequences:

• The before/after counts can't be compared at single-digit precision. The noise floor is roughly ±6, which is a meaningful fraction of the ~76 improvement claimed here.
• More importantly, if the plan is eventually to gate PRs on lint results, R0401 would make that gate flaky however it's thresholded.

Options as I see them: keep cyclic-import enabled but exclude it from any count-based comparison; disable it in the baseline and track import cycles separately, since it reads more like a design signal than a per-PR check; or fix the cycles (garak.command ↔ garak.probes.base, garak._config → garak.command → garak.analyze.report_digest → garak.evaluators.base), which is plainly its own piece of work and not this PR's job.

Two smaller notes:

ignored-classes=...,Director is unqualified, so it matches any class named Director in any module. The comment directly above that option notes qualified names are supported, so garak.payloads.Director would scope it to the intended
one.

AGENTS.md asks contributors to "catch specific exception types; avoid except Exception". The broad-exception-caught suppression in end_run is probably right for report building that mustn't crash a completed run, and it's tightly scoped — but this PR is where that becomes de facto policy, so it might warrant an explicit call rather than arriving via a suppression.

Also: the branch is 442 commits behind upstream/main, so the 1,144 → 1,068 figures describe May's tree. Happy to re-measure after a rebase.

Deferring the dynamic no-member violations to a follow-up sounds right to me. The docstring coverage and lazy-%s logging conversions are straightforward wins and I'd like to see this land.

(I used AI assistance analysing the diff; the commands and numbers above are from my own machine.)

[ppradyoth]

@anugram this is a useful review. You found a real problem. I reproduced all of it and acted on all three points.

Rebased onto current main and force-pushed. The PR went from CONFLICTING to MERGEABLE.

Two files conflicted.

• _config.py: loaded was renamed to is_loaded upstream. Kept the rename, moved my docstring and global-statement disable onto it.
• _config.py: parse_plugin_spec was rewritten upstream to go through _selection._resolve_plugin_paths. My any([...]) conversion there is dead, so I dropped it and took main's version whole.
• command.py: print_buffs gained a selected_buffs arg. Kept the arg and the docstring.

Everything else applied clean.

cyclic-import: confirmed.

pylint 4.0.5, astroid 4.0.4, Python 3.13.5, macOS. Not your exact setup, but the behaviour matches.

12 runs of pylint garak --jobs=1 on unmodified main gave counts from 1266 to 1270.

Then I diffed full runs against each other. Every varying line was R0401. Filter R0401 out and the runs are byte-identical at 1252 messages, same md5 across two separate sessions.

So R0401 is the only unstable check in the whole run. Your read was right.

I took your second option and disabled it, with the measurements written into pylintrc rather than left implicit. Reproducibility has to come before any gate. Happy to flip to "keep it enabled, exclude it from counts" if a maintainer prefers. One line either way.

There is a second source of drift and it is not R0401.

With R0401 disabled, main is stable at a fixed job count but not across job counts.

--jobs=1  : 1244, 1244
--jobs=10 : 1251, 1251

All 7 differing lines are no-member on command.py:52, Instance of 'TransientConfig' has no 'cli_args' member. They appear under --jobs=10 and vanish under --jobs=1.

You said no no-member drift, and that holds within a fixed job count. It only shows up when the job count changes. Low impact today because pylintrc pins jobs=10. But it means the deferred dynamic-no-member work has a determinism angle too, not just a noise-reduction one.

This branch happens to remove it. The no-member disable on that line kills all 7.

Numbers.

tree	jobs=1	jobs=10
main, main's pylintrc	1266-1270	unstable
main, minus R0401	1252
main, this branch's pylintrc	1244	1251
this branch	1179	1179

1252 to 1179, a drop of 73. 8 of that is the pylintrc change and 65 is code fixes.

The old 1144 to 1068 figures were May's tree. Ignore them.

One caveat on the branch row. The count is 1179 both ways and byte-identical within a job count. Across job counts, one R1705 on detectors/base.py:172 renders as "elif" in one and "else" in the other. Same finding, different wording, count unaffected.

Director: fixed, now garak.payloads.Director. Only one Director in the tree today, but the bare name would silently widen the moment someone adds another.

broad-exception-caught: agreed that a suppression should not be the thing that sets policy. I expanded the comment to state the reasoning and name it as a deliberate exception to the AGENTS.md rule. The JSONL log is flushed and closed before that call, so a digest failure costs the HTML summary and nothing else.

@jmartin-tech your call on this one. Keep the carve-out, or make write_report_digest catch specific types?

Test suite: my run was my machine running out of disk, not the branch.

My full run on the rebased branch gave 4 failed, 4532 passed, 59 skipped, 2211 errors. I chased it down and it is local.

The errored tests are the HF-model-backed ones, detectors.unsafe_content.* and friends, which pull weights on first use. Free space on my system went from 4.6 GB to 2 GB during a single suite run, about 0.5 GB a minute, with the HF cache sitting at 10 GB. Once it ran out, everything after that errored in setup and teardown. tests/plugins/test_plugins.py passes on its own, 720 passed in 3.4s, and the individual erroring tests pass in isolation.

A control run on unmodified main was clean through 18 percent before I stopped it to avoid filling my disk.

So your 4323 passed, 97 skipped stands. Nothing here points at the diff, or this repo. I will not have a full local green run until I clear space, so trust CI and your numbers over mine.

Dynamic no-member still deferred to a follow-up.

[anugram]

Ran the rebased branch on Linux. Three things.

Test suite is green. Since you're blocked on disk locally:

5678 passed, 100 skipped in 563.99s (9m23s)

pr1793 at 08b62ce, 0 commits behind upstream/main. Python 3.12.3, pylint 4.0.6, astroid 4.0.4, Linux. No failures, no errors. Free space unchanged across the run, so nothing resembling what you hit. The rebase is clean as far as the suite is concerned.

Your cross-job no-member finding reproduces exactly. On main (afae291b), cyclic-import filtered:

--jobs=1  : 1159
--jobs=10 : 1166
7 differing lines, all (no-member), all garak/command.py:52
  (cols 47, 78, 234, 280, 327)
jobs=10 repeated: byte-identical

Same count, same file, same check, same direction as your macOS run. And you're right that my "no no-member drift" was too broad — it holds within a fixed job count, not across. Your framing is the correct one.

On the branch: --jobs=1 and --jobs=10 both give 1091, identical. So the no-member disable does remove the cross-job drift, as you said.

The absolute counts are environment-dependent, and that may be the more useful result. Side by side:

measurement	mine (Linux, py3.12.3, pylint 4.0.6)	yours (macOS, py3.13.5, pylint 4.0.5)
main, jobs=1, R0401 on	1173	1266–1270
main, jobs=1, R0401 off	1159	1244
main, jobs=10, R0401 off	1166	1251
branch, jobs=1	1091	1179
branch, jobs=10	1091	1179

Every absolute number differs by a consistent 85–88. Every internal delta is identical: +7 across job counts on main in both, zero across job counts on the branch in both.

The branch rows are the interesting ones, since we're almost certainly on the same commit there — you force-pushed, I fetched after. Same code, 88 messages apart, on pylint 4.0.6 vs 4.0.5.

Could you confirm which main SHA your 1266–1270 was measured on? If it's afae291b like mine, then the gap on the main rows is environmental too, and the conclusion is that deltas reproduce across environments while absolute counts don't. Which would say that if a gate ever happens, it has to be on the change in count rather than the count itself — a contributor on a different pylint patch release would otherwise show an ~88-message "regression" having changed nothing.

Nothing here argues against the pylintrc change you've made. If anything it supports writing the measurements in rather than leaving them implicit — though it might be worth noting the environment alongside the numbers, since they don't travel.

The write_report_digest reasoning about the JSONL being flushed and closed first is a good argument and belongs in that comment regardless of which way @jmartin-tech calls it. Leaving that one to him.

[ppradyoth]

@anugram confirmed, afae291b. Same SHA as yours.

Both my measurement sessions had upstream/main at afae291b postrel version bump, and the branch was rebased onto exactly that. So the main rows are the same tree and the 85-88 gap is environmental.

Your conclusion is the right one and it is a better result than the thing I set out to measure. Gate on the delta, never the count. A contributor on a different pylint patch release would otherwise show an ~88 message regression having changed nothing at all.

I took your suggestion and wrote the environment into pylintrc next to the numbers, plus the delta point so nobody has to rediscover it. Pushed as 35ced988.

Two notes on that push.

It is comment-only. Zero functional change, count still 1179 here. So your green suite run at 08b62ce still applies, the tree is identical apart from those lines.

I also corrected a number of my own while I was in there. The committed comment said 1266-1270 was 1267-1270. I had observed 1266 twice in my second measurement session and quoted the narrower range from the first. The range across all 12 runs is 1266-1270 and that is what is in there now.

Thanks for running the suite. 5678 passed, 100 skipped on Linux settles it. My 2211 errors were free space on my system going from 4.6 GB to 2 GB mid-run with a 10 GB HF cache, so every model-backed detector test downstream of that errored. Nothing to do with the diff, or this repo.

The branch is 5 commits behind main again as of now. Happy to rebase, but it is still MERGEABLE so I would rather not churn the SHA under your verification unless something actually conflicts.

@jmartin-tech the broad-exception-caught call in end_run is the one open item. Reasoning is in the code comment either way.

[anugram]

Verified 35ced988: diff against 08b62ce is pylintrc comments only, no functional change, and pylint still reads 1091 here. So the 5678 passed, 100 skipped run carries forward.

Agree on not rebasing. While it's MERGEABLE there's nothing to gain from churning the SHA, and it resets the verification for no benefit. If it goes CONFLICTING I'll re-run against the new head.

One thing to fix in the new comment before this lands, and it's my error not yours — I mis-stated that range. "Scores 85-88 lower" is sitting right after the 1266-1270 figures, but those are unfiltered counts and 85-88 was the filtered gap. Against 1266-1270 my number is 1173, so that comparison is 93-97 lower.

basis	yours	mine	gap
R0401 enabled, jobs=1	1266-1270	1173	93-97
R0401 filtered, jobs=1	1244	1159	85
R0401 filtered, jobs=10	1251	1166	85
branch, either job count	1179	1091	88

The reason the two differ is itself worth a line: back out the R0401 counts and it's 14 cyclic-import messages on my setup against 22-26 on yours. So R0401's message count is environment-dependent, separately from which cycle paths each
run renders — two independent sources of variance in the one check. It also means the environment gap isn't a single number; it depends on whether R0401 is being counted.

Simplest fix is probably to say 93-97 next to the unfiltered figures, since that's what they are. The deltas-reproduce/absolutes-don't conclusion is unaffected either way.