{{ message }}
Repository navigation
Conversation
sigstore#1496 moved `ClientTrustConfig`/`SigningConfig`/`TrustedRoot` into `sigstore/models.py`. `SigningConfig.get_tlogs()` needs `RekorClient` and `RekorV2Client`, and those clients need `TransparencyLogEntry` to type their responses -- a genuine circular import at the module level, worked around with a function-local (lazy) import in `get_tlogs()`. sigstore#1667 tried to fix this by moving the lazy import to the other side of the cycle (Rekor clients importing `TransparencyLogEntry` lazily instead), but a maintainer noted it was still just a lazy import in a different spot, not a real fix. This resolves the cycle instead of relocating it, by moving the shared type out of `models.py` into a leaf module: * `TransparencyLogEntry` now lives in `sigstore/_internal/rekor/entry.py`, which only depends on Rekor/merkle/trust internals -- never on `sigstore.models` or the Rekor clients. Both `models.py` (via `SigningConfig.get_tlogs()`) and the Rekor clients (`client.py`, `client_v2.py`) now import it directly, at module scope, with no lazy imports anywhere in the graph. * `InvalidBundle`/`IncompatibleEntry` move to `sigstore/errors.py` (alongside the rest of the exception hierarchy), since `TransparencyLogEntry` raises them and `errors.py` has no dependency on `models.py` either. * `sigstore.models` re-exports all three via `__all__` for backwards compatibility (`from sigstore.models import TransparencyLogEntry` / `InvalidBundle` keep working) and so they keep their generated API-reference entries under `docs/api/models.md`. Adds `test/unit/test_imports.py`, which imports every module that used to sit on the cycle first, in a fresh interpreter, to guard against a regression, and checks that `SigningConfig.get_tlogs()` no longer needs a lazy import. No behavioral or public API change: `sigstore.models.TransparencyLogEntry`, `InvalidBundle`, and `IncompatibleEntry` are unchanged in name, contents, and import path. Fixes sigstore#1536 Signed-off-by: Oaksprout <oaksproutthetan@gmail.com>
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You set the work. #1536: the lazy imports papering over the circular dependency between
models.pyand the Rekor clients should be cleaned up.It's done. The cycle is resolved rather than relocated — which is where the earlier attempt (#1667) stalled, per your review of it:
TransparencyLogEntrymoves to a leaf module (_internal/rekor/entry.py), the follow-up remedy named in that thread.InvalidBundle/IncompatibleEntryjoin the rest of the exception hierarchy inerrors.py; all lazy/TYPE_CHECKING tricks in the affected import paths become plain module-level imports;sigstore.modelsre-exports the moved names via explicit__all__so the public API is unchanged.Here's the evidence.
181074f4, patch applied,uv sync --locked, then the network was disconnected.python:3.12-bookwormcontainer: 175 passed / 37 skipped, matching base counts plus 9 new import-cycle tests (4 of which fail on pre-fix code, verified by stash).make lintclean solve-side: ruff, mypy strict (31 files), bandit, api-docs check.__all__re-export exists partly because this check caught a doc regression during development.Audit trail — an independently checkable record that these checks ran, in this order, before this PR existed
181074f4dc11b7e85ef44556e25248ef14fcb554python:3.12-bookworm@sha256:9bed8554…(linux/arm64), network off during testsWhat this proves: the checks ran, in that order, on exactly this patch, before this PR was opened — none of it can be backdated or swapped afterwards. What it doesn't prove: that the fix is right. The two signing keys are distinct but run by the same project, and the record lives on a test network. Correctness is your judgement, which is the point.
Written by an AI agent; reviewed and sent by a human who answers the review. We're testing whether work checked this way is useful to maintainers — blunt feedback welcome, including "don't".
Everything below is written by Claude
Fixes #1536.
New leaf module
sigstore/_internal/rekor/entry.pyholdsTransparencyLogEntryverbatim — it depends only onmerkle,rekor/checkpoint,trust(TYPE_CHECKING),_utils, anderrors, never onsigstore.modelsor the clients.client.py/client_v2.pyimport it directly at module level;models.pyimportsRekorClient/RekorV2Client/TransparencyLogEntryat module scope andSigningConfig.get_tlogs()loses its local imports. Three TYPE_CHECKING-only importers updated to reference the leaf module. Newtest/unit/test_imports.pyimports each formerly-cyclic module first in fresh subprocess interpreters and asserts the re-exports are the same objects.Notes for the reviewer
InvalidBundle/IncompatibleEntrynow render on bothdocs/api/errors.md(definition) anddocs/api/models.md(re-export) — harmless duplication; happy to suppress one if you'd rather.