Stop committing in read-only repository methods - #94
Merged
Conversation
find(), paginated_find() and cursor_paginated_find() opened their session without passing commit=False, so every read took the commit path and issued a COMMIT. The mechanism to avoid this already existed and was used correctly by get() and get_many(): _get_session(commit=False) resolves to SessionHandler.get_session(read_only=True), which skips the commit. The three find variants simply never passed the flag. Adds a regression test asserting that no read operation reaches SessionHandler.commit. Verified to fail before this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V
CI resolved dependencies fresh on every run while the local .venv used uv.lock, so the two drifted apart and CI broke without a code change. Two independent failures resulted: - ruff 0.16 formats Python inside Markdown fenced blocks, so README.md became a format target. docs/ was already excluded; add *.md so the exclusion is consistent rather than accidental. - ruff 0.16 promoted RUF036, flagging 22 uses of Union[None, X]. Reordered to Union[X, None]; purely an annotation change. Switches tox to uv-venv-lock-runner so every environment resolves through `uv sync --locked` instead of re-resolving, and commits uv.lock so CI has something to lock against. `--locked` fails if the lock is stale, so dependency changes must go through the lock file deliberately. Also silences two mypy operator errors in result_presenters.py. The isinstance guard above each comparison already guarantees matching operand types, but mypy narrows them to independent unions and considers mismatched combinations reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V
CursorReference.value was Union[StrictStr, StrictInt, UUID], so mypy narrowed a cursor value and its reference into two independent unions and reported the mismatched combinations (str >= UUID and so on) as errors, even though the isinstance guard above each comparison rules them out. Makes CursorReference generic in its value type, so the two operands are provably the same type and the comparison needs no suppression. The TypeVar is constrained rather than bound. A bound TypeVar was tried first and is not sufficient: it binds happily to the union of its constraints, so `compare(str, int)` type-checks. That would look like a fix while enforcing nothing. Backwards compatible. The parameter is inferred when omitted, existing annotations naming the class bare still check, and pydantic validation is unchanged - verified that strict rejection of float and bool, and non-coercion of str/int, behave identically before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V
febus982
added a commit
that referenced
this pull request
Aug 3, 2026
find(), paginated_find() and cursor_paginated_find() no longer commit as of #94, which landed ahead of this redesign. The document described the bug in the present tense and framed the fix as pending. Kept in the motivation section rather than deleted: it is the clearest small illustration of the underlying problem, which is the repository deciding when to commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V
febus982
added a commit
that referenced
this pull request
Aug 3, 2026
* Ignore superpowers working documents Specs and plans produced during design sessions are scratch working files, not published documentation. Published design documents live in docs/designs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V * Add V1 transaction lifecycle redesign document Describes the planned unification of the three current transaction modes behind a TransactionScope that owns the boundary, with repositories resolving their session per call from a contextvar. Covers the motivating defects, the new component model, transaction semantics, framework integration, the subclass extension contract, and the deprecation path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V * Mark the read-commit defect as fixed find(), paginated_find() and cursor_paginated_find() no longer commit as of #94, which landed ahead of this redesign. The document described the bug in the present tense and framed the fix as pending. Kept in the motivation section rather than deleted: it is the clearest small illustration of the underlying problem, which is the repository deciding when to commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
find(),paginated_find()andcursor_paginated_find()issue aCOMMITon every call, even though they only runSELECTs.Observed session events per operation before this change:
Cause
The mechanism to avoid this already exists and is used correctly by
get()andget_many():_get_session(commit=False)(sync.py:83, 99)SessionHandler.get_session(read_only=True)(sync.py:284)_session_handler.py:63)The three
findvariants never passed the flag, so they took thecommit=Truedefault.Fix
Pass
commit=Falseat the three read call sites in each of the sync and async repositories. Write methods (save,save_many,delete,delete_many) are unchanged and still commit.Impact
For a pure
SELECTthe observable result is unchanged — the transaction ends either way. What changes is that a read no longer flushes and commits unrelated pending state, and no longer spends a round trip doing it. This matters most when a repository shares a session with other work.Verification
test_read_operations_do_not_commitasserts no read operation reachesSessionHandler.commit, parametrised over sync and async.[sync]and[async]) and passes with it.ruff format --checkandruff check: clean.mypy: unchanged — the 12 pre-existing errors inresult_presenters.pyare present onmaintoo and are untouched by this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01BJb27fB7d7HbQqfXc7U66V