Skip to content

docs(AGENTS): py/mixed-returns does not model a pytest outcome's NoReturn, and the boundary is graded - #3556

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/mixed-returns-terminal-outcome-is-no-return
Sep 12, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/mixed-returns-terminal-outcome-is-no-return

Conversation

@cagataycali

Copy link
Copy Markdown
Member

What

An AGENTS.md entry under CI Security Baseline, beside the py/unused-import / cast("X", ...) one, recording how a py/mixed-returns alert on a test helper is adjudicated - and a grader, tests/test_mixed_return_helpers_end_in_a_pytest_outcome.py, that pins the boundary the adjudication rests on.

Why

A helper that returns a value on one path and ends in pytest.fail(...) on the other has no implicit return: pytest.fail, pytest.skip, pytest.exit and pytest.xfail are declared -> NoReturn in _pytest/outcomes.py, as the __call__ of an outcome class, which is what the CodeQL Python analysis does not follow. The query has reported that shape three times and none was adjudicated until the third held a merge:

alert site terminal call cost
823 tests/simulation/test_recording_rate_matches_control_frequency.py pytest.fail open on main for 45 days
1140 tests/drivers/ur/test_ur_sim_joint_order_matches_the_wire.py pytest.skip, closing a try handler open on main for 12 days
1206 tests/mesh/test_mesh_guide_opening_block_starts_the_mesh.py pytest.fail review thread gated #3551 under required_review_thread_resolution; merged 8 s after the resolve

The entry records the counterfactual rather than the query: replace the outcome with a call that can return and mypy reports Missing return statement [return] on an annotated helper, which is already inside call-test-lint. Measured, that grader has two holes in the test trees: -> Any (alert 1140's shape) is not graded for a missing return, and [tool.mypy] relaxes disallow_untyped_defs for tests.* / tests_integ.*, so an unannotated helper's body is not read. A helper ending in print(...) or a cleanup call in either shape is exactly the defect the query names, with nothing else to report it.

The grader

Derives from tests/ and tests_integ/ every function that returns a value and can fall off its end through a bare call - the terminal statement, or the last statement of an if / try / with branch it ends in - and refuses one whose call is not a declared NoReturn (the pytest four, sys.exit, os._exit). Whatever the annotation. Controls pin the classifier on synthetic sources: a print tail is an offender under a concrete, Any and absent annotation alike; every spelling of an outcome (pytest.fail, bare skip, aliased fail as bail, sys.exit) is accepted; the try-handler shape is read; a bare return or a nested def's return is not a value return. Non-vacuity asserts the sweep finds the idiom, and one cell pins that hatch run lint still runs mypy over both test trees, which is the first row of the counterfactual table.

Population on this tree: 5 sites, all five ending in a pytest outcome, 0 offenders - the three alerts above plus tests/simulation/mujoco/test_concurrency.py::_read_first_frame_or_skip and tests_integ/groot/test_groot_integration.py::_find_server_script. scripts/check_whole_tree_graders.py derives the new grader into its roster (139) without an edit.

Verification

  • new grader: 8 passed
  • ruff check / ruff format --check / mypy (pyproject config) on the new file: clean
  • graders that read AGENTS.md or the test tree: test_codeql_query_filters, test_cast_string_imports_are_the_names_only_binding, test_changelog_fragments, test_test_case_names_describe_behaviour, test_no_host_paths, test_on_disk_text_io_states_utf8, test_docstring_xref_roles_resolve, test_except_tuples_state_their_real_scope, test_sys_modules_removal_leaves_no_orphan: 172 passed
  • intake: check_merge_base_overlap.py --paths reports no behaviour-bearing overlap; AGENTS.md shared as prose with fix(simulation/isaac): Isaac Sim backend #3343 only

Alerts 823 and 1140 are dismissed as false positive with a pointer at this entry; 1206 was dismissed the same way on #3551.

No behaviour change. Docs and a test only.

…turn, and the boundary is graded

A test helper that returns a value on one path and ends in pytest.fail(...)
on the other has no implicit return: the outcome is declared -> NoReturn in
_pytest/outcomes.py, as the __call__ of an outcome class, which is what the
CodeQL Python analysis does not follow. Three alerts, one class, none
adjudicated until the third held a merge under
required_review_thread_resolution.

Records the counterfactual (mypy reports Missing return statement on an
annotated helper once the outcome is replaced by a call that can return, and
reports nothing for -> Any or an unannotated test helper) and adds a grader
that derives every such helper from the test trees and refuses one whose
terminal call is not a declared NoReturn - the one shape where the exemption
would suppress a true finding and the one mypy cannot report in a test.
@cagataycali
cagataycali marked this pull request as ready for review September 12, 2026 16:19
@cagataycali
cagataycali enabled auto-merge (squash) September 12, 2026 16:19

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Docs + test-only change. Adds an AGENTS.md entry under CI Security Baseline adjudicating py/mixed-returns alerts on test helpers that end in a pytest outcome (fail/skip/exit/xfail are declared -> NoReturn, which the CodeQL Python analysis does not model), records the mypy counterfactual that separates a false positive from a real fall-through, and ships tests/test_mixed_return_helpers_end_in_a_pytest_outcome.py to grade the boundary the exemption rests on — that every mixed-return helper's terminal call is a declared NoReturn, whatever the annotation (covering the -> Any and unannotated holes mypy leaves in the test trees). Verified against the head tree: the grader passes (8 passed), and its derived population is exactly the 5 sites the description claims (the three adjudicated alerts plus _read_first_frame_or_skip and _find_server_script), all ending in pytest outcomes. New files are ASCII-clean, host-path-free, and the changelog fragment is named for the PR number. No behaviour change, no public API, no security surface.

What's good

  • The grader derives its population from the tree (_TREES loop variable, per the whole-tree-check convention in PR Workflow step 2), so a fourth instance is graded on arrival.
  • Controls pin the classifier on synthetic sources in both directions (a print tail is an offender under every annotation shape; every outcome spelling including aliased imports is accepted), plus non-vacuity and a pin that hatch run lint still runs mypy over both test trees — so the counterfactual the adjudication cites cannot silently stop being graded.
  • The _call_tails docstring explicitly scopes what the exemption does NOT cover (silent fall-throughs are left to the query), so the grader cannot be misread as blanket-exempting py/mixed-returns in tests.
  • Does not touch the CodeQL query filter, consistent with the two-id pin in tests/test_codeql_query_filters.py.

@cagataycali
cagataycali merged commit f0d859b into strands-labs:main Sep 12, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants