refactor: split allowlist module to prevent circular imports by majamassarini · Pull Request #3180 · packit/packit-service

majamassarini · 2026-08-31T10:39:54Z

Summary

Split the allowlist module into two separate modules to break the circular import chain that caused ImportError on Python 3.14. This is a cleaner alternative to the local import workaround in PR #3178.

Root Cause

The circular import chain was:

allowlist.py → helpers/testing_farm.py → checker/testing_farm.py
→ handlers/__init__.py → handlers/forges.py → allowlist.py

The issue occurred because allowlist.py imported job helpers (CoprBuildJobHelper, TestingFarmJobHelper) which eventually led back to importing allowlist.py through the handlers chain.

Changes

Core module (`allowlist.py`)

Removed job helper imports (breaks the circular dependency)
Kept all core allowlist management methods:
- Static methods for checking/managing namespaces
- FAS authentication methods
- Namespace approval/denial methods

New event checker module (`allowlist_checker.py`)

Created AllowlistChecker class extending Allowlist
Moved all event checking methods that depend on job helpers:
- check_and_report() and all _check_*() event methods
- _check_pr_report_status() which uses job helpers
Imports job helpers only in this module

Updated imports

jobs.py: now uses AllowlistChecker for check_and_report()
Test files: updated to use AllowlistChecker where needed
handlers/forges.py: continues to use Allowlist (core methods only)

Benefits Over Local Imports (PR #3178)

Cleaner separation of concerns: Core allowlist logic is separated from event checking/reporting
No runtime overhead: Local imports are checked on every method call
Better maintainability: Clear dependency boundaries make the code easier to understand
Future-proof: Works with all Python versions, including 3.14+

Testing

The test from PR #3178 (test_no_circular_import_in_allowlist) now passes without needing local imports. The allowlist.py module can be imported independently:

from packit_service.worker.allowlist import Allowlist  # ✓ No circular import!

Review

Findings

Low

[authorization-bypass-via-wrong-class] packit_service/worker/allowlist.py — After the split, code that instantiates the base Allowlist class directly would not have access to check_and_report() or any _check_*() methods. If a future contributor adds a new authorization-gated code path and mistakenly uses Allowlist instead of AllowlistChecker, the call would raise AttributeError (fail-closed, not fail-open). This is a defense-in-depth observation — the current codebase has only one production caller (jobs.py), which is correctly updated.
[naming-convention] packit_service/worker/allowlist_checker.py:36 — AllowlistChecker inherits all allowlist mutation methods (approve_namespace, deny_namespace, remove_namespace) even though its purpose is event checking and reporting. The inheritance approach minimizes disruption to callers but slightly blurs the responsibility boundary. Minor naming/abstraction concern.
[formatting convention] tests/integration/test_commit_comment.py:3 — The PR removes the blank line between the license header (# SPDX-License-Identifier: MIT) and the first import statement in 5 test files (test_commit_comment.py, test_issue_comment.py, test_new_hotness_update.py, test_pr_comment.py, test_pr_comment_monorepo.py). The established codebase convention is to have a blank line after the SPDX header before imports.
Remediation: Restore the blank line after the license header in the five affected test files to match the prevailing style.

Previous run (2)

Review

Findings

Low

[naming-coherence] packit_service/worker/allowlist_checker.py:36 — The project has an existing worker/checker/ package with a Checker abstract base class hierarchy (ActorChecker, etc.). The new AllowlistChecker is unrelated to that hierarchy — it extends Allowlist, not Checker. The separate package namespacing provides disambiguation, but the naming overlap may cause confusion for contributors unfamiliar with the codebase.
[scope-alignment] Issue Prevent future circular import errors #3088 tracks the project's strategic approach for circular imports (TYPE_CHECKING guards / from __future__ import annotations). This PR solves Circular import in allowlist script #3177 with module splitting instead, establishing a second pattern for resolving circular imports in this codebase.
[architectural-coherence] packit_service/worker/allowlist_checker.py:36 — The subclass adds no state and overrides no methods — it is purely a container for methods moved out of the parent. The parent's docstring references the subclass (see AllowlistChecker), creating an inverted knowledge dependency. See also: [naming-coherence] finding at this location.
[naming-convention] tests/integration/test_commit_comment.py:23 — The import AllowlistChecker as Allowlist aliases the class to a different name, masking the actual type at call sites. Production code (jobs.py) uses the real name AllowlistChecker. This pattern is repeated across 8 test files. test_allowlist.py already uses the real names correctly.
[type-annotation-consistency] packit_service/worker/allowlist_checker.py:162 — The inline type annotation job_helper_kls: type[Union[TestingFarmJobHelper, CoprBuildJobHelper]] present in the original _check_pr_report_status method was dropped during the move.

betulependule

LGTM 🙏

fullsend-ai-review · 2026-08-31T12:04:45Z

🤖 Review · ❌ Terminated · Started 12:04 PM UTC · Ended 12:20 PM UTC

Commit: c644943 · View workflow run →

centosinfra-prod-github-app · 2026-08-31T12:15:44Z

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/69447f6945354bfdaeb054369c75cf87

✔️ pre-commit SUCCESS in 1m 54s
❌ packit-service-tests FAILURE in 10m 54s

fullsend-ai-review · 2026-08-31T12:20:34Z

🤖 Finished Review · ✅ Success · Started 12:04 PM UTC · Completed 12:20 PM UTC

Commit: c644943 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.44

Split the allowlist module into two separate modules to break the circular import chain that caused ImportError on Python 3.14: allowlist.py → helpers/testing_farm.py → checker/testing_farm.py → handlers/__init__.py → handlers/forges.py → allowlist.py Changes: Core module (allowlist.py): - Removed job helper imports (CoprBuildJobHelper, TestingFarmJobHelper) - Kept all core allowlist management methods: - Static methods for checking/managing namespaces - FAS authentication methods - Namespace approval/denial methods New event checker module (allowlist_checker.py): - Created AllowlistChecker class extending Allowlist - Moved all event checking methods that depend on job helpers: - check_and_report() and all _check_*() event methods - _check_pr_report_status() which uses job helpers - Imports job helpers only in this module Updated imports: - jobs.py: now uses AllowlistChecker for check_and_report() - Test files: updated to use AllowlistChecker where needed - handlers/forges.py: continues to use Allowlist (core methods only) Benefits over local imports (PR packit#3178): 1. Cleaner separation of concerns: Core allowlist logic vs event checking 2. No runtime overhead: Local imports are checked on every call 3. Better maintainability: Clear dependency boundaries 4. Future-proof: Works with all Python versions, including 3.14+ The allowlist.py module can now be imported independently without triggering the circular import chain. Assisted-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>

fullsend-ai-review · 2026-08-31T12:36:31Z

🤖 Review · ❌ Terminated · Started 12:36 PM UTC · Ended 12:50 PM UTC

Commit: 682fed6 · View workflow run →

centosinfra-prod-github-app · 2026-08-31T12:40:12Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/5f722b506d3d4141b9c317c108e8d37b

✔️ pre-commit SUCCESS in 2m 18s
✔️ packit-service-tests SUCCESS in 3m 36s

fullsend-ai-review · 2026-08-31T12:50:21Z

🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:50 PM UTC

Commit: 682fed6 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.51

majamassarini requested review from a team, mfocko and nforro as code owners August 31, 2026 10:39

usercont-release-bot added this to Packit pull requests Aug 31, 2026

github-project-automation Bot moved this to New in Packit pull requests Aug 31, 2026

majamassarini force-pushed the refactor-allowlist-split-modules branch from a9c0732 to b9450f7 Compare August 31, 2026 10:44

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 31, 2026

betulependule approved these changes Aug 31, 2026

View reviewed changes

majamassarini force-pushed the refactor-allowlist-split-modules branch from b9450f7 to c644943 Compare August 31, 2026 12:03

This comment was marked as outdated.

Sign in to view

majamassarini force-pushed the refactor-allowlist-split-modules branch from c644943 to 682fed6 Compare August 31, 2026 12:35

fullsend-ai-review Bot approved these changes Aug 31, 2026

View reviewed changes

majamassarini merged commit 337bca6 into packit:main Aug 31, 2026
24 checks passed

github-project-automation Bot moved this from New to Done in Packit pull requests Aug 31, 2026

majamassarini mentioned this pull request Aug 31, 2026

fix(#3177): break circular import in allowlist module #3178

Closed

3 tasks

Uh oh!

Conversation

majamassarini commented Aug 31, 2026

Summary

Root Cause

Changes

Core module (allowlist.py)

New event checker module (allowlist_checker.py)

Updated imports

Benefits Over Local Imports (PR #3178)

Testing

Related

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

centosinfra-prod-github-app Bot commented Aug 31, 2026

Uh oh!

majamassarini commented Aug 31, 2026

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

centosinfra-prod-github-app Bot commented Aug 31, 2026

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Low

Review

Findings

Low

Uh oh!

This comment was marked as outdated.

Uh oh!

betulependule left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

centosinfra-prod-github-app Bot commented Aug 31, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

centosinfra-prod-github-app Bot commented Aug 31, 2026

Uh oh!

fullsend-ai-review Bot commented Aug 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Core module (`allowlist.py`)

New event checker module (`allowlist_checker.py`)

fullsend-ai-review Bot commented Aug 31, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 31, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 31, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 31, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 31, 2026 •

edited

Loading