Repository navigation
Conversation
Dmao233
marked this pull request as ready for review
August 31, 2026 01:24
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3abf79995d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1096
to
+1098
| if ini_value is None: | ||
| return None | ||
| return int(ini_value) |
There was a problem hiding this comment.
When an existing INI file contains max_warnings = or native TOML contains max_warnings = "", this new None-only check passes the empty string to int(). Previously, the truthiness check treated these values as unset; now a successful test run ends with an uncaught ValueError traceback and exit code 1. Preserve the empty-string case as unset while still distinguishing the integer 0 from the new None default.
Useful? React with 👍 / 👎.
pytest-dev#14955) Bumps [pytest-rerunfailures](https://github.com/pytest-dev/pytest-rerunfailures) from 16.5 to 16.6. - [Changelog](https://github.com/pytest-dev/pytest-rerunfailures/blob/master/CHANGES.rst) - [Commits](pytest-dev/pytest-rerunfailures@16.5...16.6) --- updated-dependencies: - dependency-name: pytest-rerunfailures dependency-version: '16.6' dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* [pre-commit.ci] pre-commit autoupdate updates: - [github.com/astral-sh/ruff-pre-commit: v0.16.4 → v0.16.5](astral-sh/ruff-pre-commit@v0.16.4...v0.16.5) - [github.com/woodruffw/zizmor-pre-commit: v1.29.0 → v1.30.0](zizmorcore/zizmor-pre-commit@v1.29.0...v1.30.0) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Fixes pytest-dev#14445 - assertion rewriting evaluated NamedExpr (:=) expressions multiple times, causing side effects to fire repeatedly. The root cause was the `variables_overwrite` mechanism which stored and re-evaluated NamedExpr AST nodes in subsequent assertions, in `_call_reprcompare`'s results tuple, and in explanation formatting. The fix: - visit_NamedExpr: reference the target variable in explanations instead of re-evaluating the full expression - visit_Compare: assign left-side NamedExpr to a temp before right-side hoisting; freeze left_res when a comparator walrus targets the same name; replace NamedExpr entries in `results` with target variables - visit_BoolOp: capture short-circuit condition in a stable temp for the explanation path; remove walrus target rename logic - visit_Call: remove variables_overwrite substitution (walrus now properly assigns to user variables in its natural evaluation position) - Remove variables_overwrite, scope tracking, Sentinel class Co-authored-by: Claude Opus 5 <ai@anthropic.com> Co-authored-by: Claude Code <ai@anthropic.com>
Co-authored-by: Claude Sonnet 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
Add tests for two remaining walrus double-evaluation scenarios: - Bare NamedExpr as BoolOp operand evaluated twice via condition check - Same walrus target in chained comparison evaluated multiple times Co-authored-by: Claude Sonnet 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
Use the already-assigned res_var to build the short-circuit condition instead of the raw visitor result, preventing bare NamedExpr operands from being evaluated a second time when checking truthiness. Co-authored-by: Claude Sonnet 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
In a chained comparison like `(x := f()) < (x := g()) < (x := h())`, each NamedExpr comparator is now assigned to a temp variable so it evaluates exactly once. Previously the raw NamedExpr node would be reused as left_res in the next iteration, causing double evaluation. Co-authored-by: Claude Sonnet 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
When multiple walrus operators target the same variable in a BoolOp (e.g., `assert (x := side_effect()) and (x := False)`), the assertion explanation previously showed the final value of `x` for all operands because the format context evaluated lazily after all operands ran. Fix by tracking Name/NamedExpr operand values in stable @py_assert variables (via self.assign) immediately after evaluation, then pointing the explanation format context at the tracked copy. This uses the same value-tracking mechanism already used by visit_Call, visit_Attribute, etc. Fixes the case reported by @bluetech in PR review. Co-authored-by: Claude Sonnet 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
Replace the blanket snapshot-all-operands approach with a targeted one: pre-scan the BoolOp to find walrus targets, then only snapshot operands whose value a later walrus would corrupt. Snapshot rules: - NamedExpr (non-last): always, to avoid re-evaluating side effects - Name with later walrus conflict: to freeze the pre-overwrite value - Everything else: use res directly (stable @py_assert or plain name) Non-walrus BoolOps now generate identical code to 8.3.5 (no snapshots). Co-authored-by: Claude Opus 4 <ai@anthropic.com> Co-authored-by: Cursor <ai@cursor.com>
The rewriter hoists each operand into its own statement, but a plain
name is left as a bare load evaluated when the enclosing expression is
assembled -- after the statements of the operands that follow it. A
walrus operator in a later operand rebinds the name in between, so both
the value used and the value reported were the post-walrus one, while
Python evaluates the earlier operand first:
assert value != identity(value := value.lower())
visit_BoolOp already guarded against this; extract its pre-scan as
_walrus_targets() and add visit_operand() to apply the same freeze in
visit_Compare, visit_Call and visit_BinOp. visit_Compare previously
matched only a comparator that *was* a NamedExpr, missing walrus
operators nested inside it; visit_Call did not guard at all, so an
earlier argument saw a later argument's assignment.
These cases predate the walrus rework -- they fail on main too.
Closes the single-eval-walrus, order-compare-left, order-call-argument
and order-binop-left groups in the coverage matrix. order-call-argument
keeps one entry: a bare walrus argument is still substituted into a
later one, which visit_operand does not yet see because the operand is a
NamedExpr rather than a Name.
Reported-by: Denis Scapin
Co-authored-by: Claude Opus 5 <ai@anthropic.com>
Co-authored-by: Claude Code <ai@anthropic.com>
TestAssertionRewriteWalrusOperator predates the coverage matrix and asks its questions through runpytest(): twelve tests that mostly check ret == 0. Nine of them the matrix already answers in-process and more precisely -- what the failure message says, what the operands saw, how often each ran. Two are worth more than a deletion, so they move rather than vanish: assert not (a and ((a := False) is False)) reads back as an introspection case, and the composite chain assert a and True and ((a := False) is False) and (a is False) and ... as an evaluation-order one. That second is why the deletion is not just tidying: it passes on main, and it passes there because of the bug. Main rewrites the walrus target to an internal temp and substitutes the stored NamedExpr into every later read of the name -- including the next statement, so the `assert a is None` that is supposed to verify the outcome re-runs the walrus and creates it. Asked through returned values instead of ret == 0, main fails the case. TestIssue14445 loses the four tests that restate matrix single-evaluation entries this PR already adds, and keeps the two reproducers from the issue. One test survives in place: nothing the rewriter stores may outlive a statement, which needs two tests in one module to observe and so cannot be said in-process. Co-authored-by: Claude Opus 5 <ai@anthropic.com> Co-authored-by: Claude Code <ai@anthropic.com>
…14445-assert-walrus fix(rewrite): prevent walrus operator double evaluation in assertions
bluetech
force-pushed
the
cursor/max-warnings-toml-int-6b43
branch
2 times, most recently
from
September 5, 2026 18:51
0b7c71f to
de7bf7f
Compare
The max_warnings option was registered without a type (defaulting to 'string'), so integer values in native TOML config raised a TypeError. It is now registered with type=int | str, accepting both int and string values in TOML while keeping the string form working for backward compatibility. An explicit integer 0 is distinguished from the unset default. Co-authored-by: Cursor Grok 4.6 <cursoragent@cursor.com>
bluetech
force-pushed
the
cursor/max-warnings-toml-int-6b43
branch
from
September 5, 2026 19:07
de7bf7f to
ee08c00
Compare
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
3 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.
The
max_warningsoption was registered without a type, so it defaulted to a string with an empty default. Native TOML integers such asmax_warnings = 0were then rejected (expects a string, got int), even though the configuration reference and the warnings how-to document an unquoted integer and declare the option asint.This follows the
truncation_limit_*convention from pytest-dev#14692 / pytest-dev#14675: register the option asint | strso native TOML integers and existing string values (quoted TOML, INI,-o) both remain supported._get_max_warnings()now treatsNoneas unset, so an explicit integer0is distinct from the empty default.Fixes pytest-dev#14953
Verification
Manual cases (one warning,
--disable-warnings): unquoted and quoted0inpytest.tomland[tool.pytest],pytest.ini,-o max_warnings=0, and--max-warningsoverriding ini all exit 6 (MAX_WARNINGS_ERROR). Unset still exits 0.Checklist
changelog/14953.bugfix.rstCo-authored-bycommit trailer.