MAINT Enable ruff PGH rule for pygrep-hooks linting#1416
MAINT Enable ruff PGH rule for pygrep-hooks linting#1416romanlutz wants to merge 1 commit intoAzure:mainfrom
Conversation
romanlutz
commented
Feb 27, 2026
romanlutz
commented
- Add PGH to ruff select list
- Fix PGH003: replace bare # type: ignore\ with specific mypy error codes
- Fix PGH004: use colon syntax for # noqa:\ directives
- Add docstring to MessagePiece.to_message (exposed by fixing blanket noqa)
- Remove obsolete flake8 directive in test_unicode_confusable_converter.py
There was a problem hiding this comment.
Pull request overview
This PR enables Ruff’s PGH (pygrep-hooks) lint rules and updates existing # type: ignore / # noqa directives across the codebase to satisfy the new lint requirements, plus a small docstring/cleanup touch-up.
Changes:
- Enable
PGHin Ruff’s selected rule set. - Replace bare
# type: ignorewith code-specific ignores and normalize# noqa:syntax. - Minor hygiene updates (docstring addition, remove obsolete
flake8: noqa).
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Enable Ruff PGH rules |
| pyrit/auth/copilot_authenticator.py | Make type: ignore codes explicit |
| pyrit/cli/frontend_core.py | Narrow type: ignore codes |
| pyrit/common/display_response.py | Normalize noqa / ignore syntax |
| pyrit/common/notebook_utils.py | Normalize type: ignore code |
| pyrit/common/yaml_loadable.py | Use specific type: ignore |
| pyrit/datasets/seed_datasets/remote/red_team_social_bias_dataset.py | Type-ignore **kwargs expansion |
| pyrit/memory/azure_sql_memory.py | Narrow ignore on __tablename__ |
| pyrit/memory/memory_interface.py | Narrow ignore in query conditions |
| pyrit/memory/memory_models.py | Narrow ignores on entry field access |
| pyrit/memory/sqlite_memory.py | Narrow ignores on ORM attrs/export |
| pyrit/models/message_piece.py | Add docstring; normalize ignores/noqa |
| pyrit/models/seeds/seed.py | Narrow ignore on Jinja env init |
| pyrit/prompt_converter/selective_text_converter.py | Narrow ignore on strategy call |
| pyrit/prompt_target/azure_ml_chat_target.py | Narrow ignore in exception path |
| pyrit/prompt_target/openai/openai_chat_target.py | Narrow ignore for multimodal dict parts |
| pyrit/prompt_target/openai/openai_target.py | Narrow ignore for token provider wrapper |
| pyrit/prompt_target/text_target.py | Narrow ignores in CSV import |
| pyrit/scenario/core/scenario.py | Narrow ignore for REQUIRED_VALUE default |
| pyrit/scenario/scenarios/airt/psychosocial_scenario.py | Narrow ignore on scorer wrapping |
| pyrit/score/conversation_scorer.py | Narrow ignore on dynamic class bases |
| tests/integration/ai_recruiter/test_ai_recruiter.py | Narrow ignore on async call |
| tests/unit/common/test_helper_functions.py | Narrow ignore for invalid arg test |
| tests/unit/converter/test_add_image_text_converter.py | Narrow ignore for invalid arg test |
| tests/unit/converter/test_add_text_image_converter.py | Narrow ignore for invalid arg test |
| tests/unit/converter/test_azure_speech_converter.py | Narrow ignore for invalid arg test |
| tests/unit/converter/test_bin_ascii_converter.py | Narrow ignore for invalid attr/arg tests |
| tests/unit/converter/test_image_compression_converter.py | Narrow ignore for invalid arg tests |
| tests/unit/converter/test_math_prompt_converter.py | Narrow ignore for invalid arg test |
| tests/unit/converter/test_prompt_converter.py | Normalize noqa: + narrow ignore |
| tests/unit/converter/test_unicode_confusable_converter.py | Remove obsolete flake8: noqa |
| tests/unit/executor/workflow/test_xpia.py | Narrow ignore for invalid arg tests |
| tests/unit/memory/memory_interface/test_interface_attack_results.py | Narrow ignore for collection comparisons |
| tests/unit/memory/test_azure_sql_memory.py | Narrow ignores around sessions |
| tests/unit/models/test_message_piece.py | Narrow ignore for invalid arg tests |
| tests/unit/prompt_normalizer/test_prompt_normalizer.py | Narrow ignore for invalid arg/attr tests |
| tests/unit/scenarios/test_cyber.py | Narrow ignore for scorer target access |
| tests/unit/scenarios/test_jailbreak.py | Narrow ignore for scorer target access |
| tests/unit/scenarios/test_leakage_scenario.py | Narrow ignore for scorer internals |
| tests/unit/scenarios/test_scam.py | Narrow ignore for scorer target access |
| tests/unit/score/test_conversation_history_scorer.py | Narrow ignore for dynamic attrs |
| tests/unit/score/test_true_false_composite_scorer.py | Narrow ignore for invalid scorer test |
| tests/unit/setup/test_configuration_loader.py | Narrow ignore for invalid arg test |
| tests/unit/setup/test_initialization.py | Narrow ignore for invalid arg test |
| tests/unit/setup/test_pyrit_initializer.py | Narrow ignores for abstract/None usage |
| tests/unit/target/test_huggingface_chat_target.py | Narrow ignore for arg type |
| tests/unit/target/test_openai_chat_target.py | Narrow ignores in multimodal assertions/mocks |
| tests/unit/target/test_openai_response_target.py | Narrow ignores in multimodal assertions/mocks |
pyrit/scenario/core/scenario.py
Outdated
| self, | ||
| *, | ||
| objective_target: PromptTarget = REQUIRED_VALUE, # type: ignore | ||
| objective_target: PromptTarget = REQUIRED_VALUE, # type: ignore[attr-defined] |
There was a problem hiding this comment.
objective_target is typed as PromptTarget but the default value is the REQUIRED_VALUE sentinel. The ignore code attr-defined won’t match the mypy error here (typically assignment for an incompatible default), which will break the mypy --strict pre-commit hook. Use the correct ignore code (or adjust the type to include the sentinel) so mypy is actually suppressed.
| objective_target: PromptTarget = REQUIRED_VALUE, # type: ignore[attr-defined] | |
| objective_target: PromptTarget = REQUIRED_VALUE, # type: ignore[assignment] |
| try: | ||
| return str(response.json()["output"]) | ||
| except Exception as e: | ||
| if response.json() == {}: | ||
| raise EmptyResponseException(message="The chat returned an empty response.") from e | ||
| raise e( | ||
| f"Exception obtaining response from the target. Returned response: {response.json()}. " | ||
| + f"Exception: {str(e)}" # type: ignore | ||
| + f"Exception: {str(e)}" # type: ignore[union-attr] | ||
| ) from e |
There was a problem hiding this comment.
In the exception handler, e is an exception instance, so raise e(...) will raise TypeError and mask the real parsing error. Raise a concrete exception type instead (e.g., raise RuntimeError(...) from e or raise type(e)(...) from e). The current type: ignore[...] on the string concatenation also looks unrelated to the actual issue.
| minutes_left = (expiry_time - current_time).total_seconds() / 60 | ||
| logger.info(f"Cached token is valid for another {minutes_left:.2f} minutes") | ||
|
|
||
| return token_data # type: ignore | ||
| return token_data # type: ignore[union-attr] | ||
|
|
There was a problem hiding this comment.
This # type: ignore[union-attr] on the return statement is unlikely to match the actual mypy error under --strict (commonly no-any-return here, since json.loads(...) yields Any). If the ignore code doesn’t match, mypy will both report the real error and treat this ignore as unused. Prefer typing/casting the parsed JSON to dict[str, Any] (or update the ignore to the correct mypy error code).
| self._save_token_to_cache(token=bearer_token, expires_in=token_expires_in) | ||
| else: | ||
| logger.error(f"Failed to retrieve bearer token within {self._token_capture_timeout} seconds.") | ||
|
|
||
| return bearer_token # type: ignore | ||
| return bearer_token # type: ignore[union-attr] | ||
| except Exception as e: |
There was a problem hiding this comment.
Same issue here: bearer_token is derived from json.loads, so it is easy for mypy to infer Any and raise no-any-return under --strict. # type: ignore[union-attr] won’t suppress that and may be flagged as an unused ignore. Consider casting data["access_token"] to str (and expires_in to int | None) or use the correct ignore code.
| # Wrap with conversation scorer to evaluate full conversation history | ||
| conversation_scorer: FloatScaleScorer = create_conversation_scorer(scorer=psych_scorer) # type: ignore | ||
| conversation_scorer: FloatScaleScorer = create_conversation_scorer(scorer=psych_scorer) # type: ignore[union-attr] | ||
|
|
There was a problem hiding this comment.
create_conversation_scorer is annotated to return Scorer, so assigning it to a FloatScaleScorer-typed variable typically triggers a mypy assignment error (not union-attr). With mypy --strict, the current ignore code may not suppress the real error and can be reported as an unused ignore. Prefer cast(FloatScaleScorer, ...) here (or improve create_conversation_scorer typing via overloads) instead of type: ignore[union-attr].
pyrit/memory/memory_interface.py
Outdated
| entries: Sequence[SeedEntry] = self._query_entries( | ||
| SeedEntry, | ||
| conditions=and_(SeedEntry.dataset_name is not None, SeedEntry.dataset_name != ""), # type: ignore | ||
| conditions=and_(SeedEntry.dataset_name is not None, SeedEntry.dataset_name != ""), # type: ignore[union-attr] |
There was a problem hiding this comment.
SeedEntry.dataset_name is not None is a Python identity check and evaluates to a plain bool, not a SQLAlchemy expression, so it doesn’t do what it appears to and also tends to cause a mypy arg-type error inside and_(...). Consider removing that clause or using the SQLAlchemy form (e.g., SeedEntry.dataset_name.isnot(None)), and update/remove the type: ignore[...] accordingly.
| conditions=and_(SeedEntry.dataset_name is not None, SeedEntry.dataset_name != ""), # type: ignore[union-attr] | |
| conditions=and_(SeedEntry.dataset_name.isnot(None), SeedEntry.dataset_name != ""), |
| message_piece = MessagePiece( | ||
| role=row["role"], # type: ignore | ||
| role=row["role"], # type: ignore[union-attr] | ||
| original_value=row["value"], | ||
| original_value_data_type=row.get["data_type", None], # type: ignore | ||
| original_value_data_type=row.get["data_type", None], # type: ignore[union-attr] | ||
| conversation_id=row.get("conversation_id", None), | ||
| sequence=int(sequence_str) if sequence_str else None, | ||
| labels=labels, | ||
| response_error=row.get("response_error", None), # type: ignore | ||
| response_error=row.get("response_error", None), # type: ignore[union-attr] | ||
| prompt_target_identifier=self.get_identifier(), |
There was a problem hiding this comment.
row.get["data_type", None] is not valid for a dict (it subscripts the get method) and will raise TypeError at runtime. Use row.get("data_type")/row.get("data_type", None) instead, and consider validating/casting CSV strings (e.g., role/data_type) rather than suppressing with type: ignore here.
| words = prompt.split(self._word_separator) | ||
|
|
||
| # Get selected word indices | ||
| selected_indices = self._selection_strategy.select_words(words=words) # type: ignore | ||
| selected_indices = self._selection_strategy.select_words(words=words) # type: ignore[return-value] | ||
|
|
||
| # If no words selected, return original prompt |
There was a problem hiding this comment.
The # type: ignore[return-value] on select_words is likely the wrong error code (mypy typically reports this as attr-defined since TextSelectionStrategy has no select_words). With mypy --strict, this can fail CI due to an unsuppressed error and/or an unused-ignore. Prefer casting self._selection_strategy to WordSelectionStrategy inside the word-level branch (or change the ignore to the correct code).
romanlutz
force-pushed
the
romanlutz/ruff_pgh
branch
from
119aa4f to
82aa59f
Compare
- Add PGH to ruff select list - Fix PGH003: replace bare \# type: ignore\ with specific mypy error codes - Fix PGH004: use colon syntax for \# noqa:\ directives - Add docstring to MessagePiece.to_message (exposed by fixing blanket noqa) - Remove obsolete flake8 directive in test_unicode_confusable_converter.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
romanlutz
force-pushed
the
romanlutz/ruff_pgh
branch
from
82aa59f to
7ddad6a
Compare
| @@ -371,7 +371,7 @@ def test_factory_preserves_wrapped_scorer(): | |||
| assert hasattr(conv_scorer, "_wrapped_scorer") | |||
| wrapped = conv_scorer._wrapped_scorer | |||
| assert wrapped is original_scorer | |||
| assert wrapped.custom_attr == "test_value" # type: ignore | |||
| assert wrapped.custom_attr == "test_value" # type: ignore[abstract] | |||
There was a problem hiding this comment.
# type: ignore[abstract] on custom_attr is the wrong mypy error category here (this is dynamically adding/accessing an attribute that isn’t declared on MockFloatScaleScorer). Use the appropriate ignore code (typically attr-defined) or avoid the ignore by using setattr/a typed protocol if you want the test to be type-checker-friendly.
| assert messages[0]["role"] == "user" | ||
| assert messages[0]["content"][0]["type"] == "text" # type: ignore | ||
| assert messages[0]["content"][1]["type"] == "image_url" # type: ignore | ||
| assert messages[0]["content"][0]["type"] == "text" # type: ignore[method-assign] | ||
| assert messages[0]["content"][1]["type"] == "image_url" # type: ignore[method-assign] |
There was a problem hiding this comment.
These # type: ignore[method-assign] comments don’t match what the lines are doing (they’re reads/indexing into messages[...], not assigning to a method). If you want these ignores to be meaningful, update them to the correct mypy error code for the underlying typing issue (e.g., an index/TypedDict/union-content error) or refactor with cast to avoid the ignore.
| def test_validate_request_unsupported_data_types(target: OpenAIChatTarget): | ||
| image_piece = get_image_message_piece() | ||
| image_piece.converted_value_data_type = "new_unknown_type" # type: ignore | ||
| image_piece.converted_value_data_type = "new_unknown_type" # type: ignore[method-assign] |
There was a problem hiding this comment.
image_piece.converted_value_data_type = ... is an attribute assignment; # type: ignore[method-assign] is intended for assignments to methods and likely won’t suppress the intended type-checking error. Use the correct ignore code for an incompatible attribute assignment (commonly assignment) or adjust the test to avoid mutating the typed field directly.
| def test_validate_request_unsupported_data_types(target: OpenAIResponseTarget): | ||
| image_piece = get_image_message_piece() | ||
| image_piece.converted_value_data_type = "new_unknown_type" # type: ignore | ||
| image_piece.converted_value_data_type = "new_unknown_type" # type: ignore[method-assign] |
There was a problem hiding this comment.
image_piece.converted_value_data_type = ... is an attribute assignment; # type: ignore[method-assign] is intended for assignments to methods and likely won’t suppress the intended type-checking error. Use the correct ignore code for an incompatible attribute assignment (commonly assignment) or adjust the test to avoid mutating the typed field directly.
| message_piece = MessagePiece( | ||
| role=row["role"], # type: ignore | ||
| role=row["role"], # type: ignore[arg-type] | ||
| original_value=row["value"], | ||
| original_value_data_type=row.get["data_type", None], # type: ignore | ||
| original_value_data_type=row.get("data_type", None), # type: ignore[arg-type] | ||
| conversation_id=row.get("conversation_id", None), | ||
| sequence=int(sequence_str) if sequence_str else None, | ||
| labels=labels, | ||
| response_error=row.get("response_error", None), # type: ignore | ||
| response_error=row.get("response_error", None), # type: ignore[arg-type] | ||
| prompt_target_identifier=self.get_identifier(), |
There was a problem hiding this comment.
import_scores_from_csv currently passes None for fields that are not Optional in MessagePiece (original_value_data_type, response_error, and sequence). This will raise at runtime for the documented minimal CSV format (e.g., only role,value). Use sensible defaults when keys are missing (e.g., default data type to "text", response_error to "none", and sequence to -1 or omit the argument) and consider validating/normalizing the CSV values before constructing MessagePiece.
| assert messages[0]["role"] == "user" | ||
| assert messages[0]["content"][0]["type"] == "input_text" # type: ignore | ||
| assert messages[0]["content"][1]["type"] == "input_image" # type: ignore | ||
| assert messages[0]["content"][0]["type"] == "input_text" # type: ignore[method-assign] | ||
| assert messages[0]["content"][1]["type"] == "input_image" # type: ignore[method-assign] | ||
| assert messages[1]["role"] == "assistant" | ||
| assert messages[1]["content"][0]["type"] == "output_text" # type: ignore | ||
| assert messages[1]["content"][0]["type"] == "output_text" # type: ignore[method-assign] | ||
| assert messages[2]["role"] == "user" | ||
| assert messages[2]["content"][0]["type"] == "input_text" # type: ignore | ||
| assert messages[2]["content"][1]["type"] == "input_image" # type: ignore | ||
| assert messages[2]["content"][0]["type"] == "input_text" # type: ignore[method-assign] | ||
| assert messages[2]["content"][1]["type"] == "input_image" # type: ignore[method-assign] |
There was a problem hiding this comment.
These # type: ignore[method-assign] comments don’t match what the lines are doing (they’re reads/indexing into messages[...], not assigning to a method). If you want these ignores to be meaningful, update them to the correct mypy error code for the underlying typing issue (e.g., an index/TypedDict/union-content error) or refactor with cast to avoid the ignore.
|
|
||
| Returns: | ||
| Message: A Message containing this piece. | ||
| """ |
There was a problem hiding this comment.
MessagePiece.to_message uses a local import. If this is to avoid a circular import (likely, given pyrit.models.message imports MessagePiece), please add a brief comment explaining that rationale so future refactors don’t “fix” it back to a top-level import.
| """ | |
| """ | |
| # Local import is required to avoid a circular dependency because | |
| # pyrit.models.message imports MessagePiece. |
