Kill mutation survivors in the post-turn quality stop hook (#36, #37, #38, #40) - #50
Kill mutation survivors in the post-turn quality stop hook (#36, #37, #38, #40)#50leynos wants to merge 3 commits into
Conversation
Add targeted unit tests for the highest-value mutation-testing survivors reported against the stop hook: exact rendering tests for format_reason, boundary and split tests for truncate, representative parsing tests for parse_make_targets and is_missing_makefile, extension-mapping tests for detect_categories, full-tuple parse_env and parse_max_output tests, equality tests for default_categories, real-subprocess and error-branch tests for run(), and argv, wiring, and exact-message assertions for the git plumbing helpers. Annotate the four equivalent mutants (subprocess check=False and the falsy env-var defaults) with pragma no mutate comments, splitting the affected arguments onto their own lines so the suppression does not shield killable mutants on the same line.
mutmut 3.6 only honours pragma comments at statement level, so the argument-level annotations added earlier were ignored. Hoist the falsy env-var defaults into their own pragma-suppressed statements, drop the redundant check=False argument from run() (False is the subprocess default, so both of its mutants were equivalent by construction), and suppress the max-output default whose mutation falls back to the same 12000 value. Also convert the remaining substring assertions on git error messages to exact equality, cover truncate at max_chars=1, extend the make -qp fixture with a mid-stream blank line and a mixed pattern/plain rule, and exercise format_reason with a command entry lacking cmd/stdout/stderr keys and with a falsy base ref.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughThe subprocess wrapper was simplified, while tests now verify exact Git commands, error messages, fallback process fields, output handling, formatting, category detection, and environment parsing. ChangesQuality hook coverage
Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideThis PR adds high-precision tests around the post-turn quality stop hook to kill/suppress surviving mutation-testing mutants, and performs small refactorings in the hook implementation to structurally suppress known equivalent mutants without changing behaviour. Flow diagram for updated parse_env default handlingflowchart TD
parse_env[parse_env]
flag_default["flag_default = '' (no mutate)"]
max_output_default["max_output_default = '12000' (no mutate)"]
parse_env --> flag_default
parse_env --> max_output_default
base_ref_env["os.environ.get POST_TURN_BASE_REF"]
always_fetch_env["os.environ.get POST_TURN_ALWAYS_FETCH"]
max_output_env["os.environ.get POST_TURN_MAX_OUTPUT_CHARS"]
compush_env["os.environ.get POST_TURN_COMPUSH"]
parse_env --> base_ref_env
parse_env --> always_fetch_env
parse_env --> max_output_env
parse_env --> compush_env
always_fetch_env --> parse_bool_env
compush_env --> parse_bool_env
max_output_env --> parse_max_output
flag_default --> always_fetch_env
flag_default --> compush_env
max_output_default --> max_output_env
parse_bool_env[parse_bool_env]
parse_max_output[parse_max_output]
parse_bool_env --> always_fetch
parse_bool_env --> compush
parse_max_output --> max_out
base_ref_env --> base_ref
base_ref[base_ref]
always_fetch[always_fetch]
max_out[max_out]
compush[compush]
base_ref --> result_tuple
always_fetch --> result_tuple
max_out --> result_tuple
compush --> result_tuple
result_tuple[("(base_ref, always_fetch, max_out, compush)")]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hooks/post-turn-quality-stop-hook.py`:
- Around line 151-157: Update the subprocess.run call in the hook’s command
execution helper to restore explicit check=False, and add the existing # pragma:
no mutate annotation used by parse_env() to suppress the mutation warning.
Preserve the current controlled-command noqa and non-zero return-code behavior.
- Around line 887-896: Update the max_output_default value used by
parse_max_output in the surrounding configuration setup to an unparsable
sentinel instead of duplicating "12000". Preserve parse_max_output's existing
fallback so it remains the single source of truth for the default maximum
output.
In `@hooks/test_post_turn_quality_stop_hook.py`:
- Around line 533-587: Move test_real_command_captures_text_output,
test_file_as_cwd_returns_error, and
test_filename_less_file_not_found_uses_fallback from TestGetMakeTargets into
TestRunOSError, placing them near test_nonexistent_cwd_returns_error. Preserve
each test’s implementation and assertions unchanged.
- Around line 771-772: Parenthesize the implicit two-part string concatenations
in the list literals at both occurrences, including the entries near the
relevant test messages, so Ruff ISC004 is satisfied and the expected output
remains unchanged.
- Around line 589-593: Split hooks/test_post_turn_quality_stop_hook.py into
focused modules using the existing test_run.py, test_helpers.py,
test_formatting.py, and test_env.py banner sections as boundaries, keeping each
module under 400 logical lines. Move the shared _completed, REPO, and ENV_VARS
setup into conftest.py or a fixtures module, then update imports and references
so all tests retain their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63f908cd-e37a-4313-80ea-de6878b440fe
📒 Files selected for processing (2)
hooks/post-turn-quality-stop-hook.pyhooks/test_post_turn_quality_stop_hook.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cuprum(auto-detected)
| try: | ||
| # Deliberately without check=True: non-zero return codes are | ||
| # surfaced through CompletedProcess.returncode, not exceptions. | ||
| # (Passing check=False explicitly only bred equivalent mutants.) | ||
| return subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings). | ||
| cmd, cwd=str(cwd), text=True, capture_output=True, check=False | ||
| cmd, cwd=str(cwd), text=True, capture_output=True | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the explicit check=False and mute the mutant with a pragma.
Ruff (PLW1510) now flags Line 155. Use the same # pragma: no mutate device already adopted in parse_env() instead of dropping the keyword, so both the linter and the mutation run stay quiet.
🔧 Proposed fix
- # Deliberately without check=True: non-zero return codes are
- # surfaced through CompletedProcess.returncode, not exceptions.
- # (Passing check=False explicitly only bred equivalent mutants.)
+ # Deliberately check=False: non-zero return codes are surfaced
+ # through CompletedProcess.returncode, not exceptions.
return subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings).
- cmd, cwd=str(cwd), text=True, capture_output=True
+ cmd,
+ cwd=str(cwd),
+ text=True,
+ capture_output=True,
+ check=False, # pragma: no mutate
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| # Deliberately without check=True: non-zero return codes are | |
| # surfaced through CompletedProcess.returncode, not exceptions. | |
| # (Passing check=False explicitly only bred equivalent mutants.) | |
| return subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings). | |
| cmd, cwd=str(cwd), text=True, capture_output=True, check=False | |
| cmd, cwd=str(cwd), text=True, capture_output=True | |
| ) | |
| try: | |
| # Deliberately check=False: non-zero return codes are surfaced | |
| # through CompletedProcess.returncode, not exceptions. | |
| return subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings). | |
| cmd, | |
| cwd=str(cwd), | |
| text=True, | |
| capture_output=True, | |
| check=False, # pragma: no mutate | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 154-156: Use of unsanitized data to create processes
Context: subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings).
cmd, cwd=str(cwd), text=True, capture_output=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 154-156: Command coming from incoming request
Context: subprocess.run( # noqa: S603 # valid: command and args are controlled (no shell, no user-supplied command strings).
cmd, cwd=str(cwd), text=True, capture_output=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[warning] 155-155: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/post-turn-quality-stop-hook.py` around lines 151 - 157, Update the
subprocess.run call in the hook’s command execution helper to restore explicit
check=False, and add the existing # pragma: no mutate annotation used by
parse_env() to suppress the mutation warning. Preserve the current
controlled-command noqa and non-zero return-code behavior.
Source: Linters/SAST tools
| # Equivalent mutant: a mutated (non-numeric) default falls back to | ||
| # parse_max_output's own 12000 default, so it cannot change behaviour. | ||
| max_output_default = "12000" # pragma: no mutate | ||
| base_ref = os.environ.get("POST_TURN_BASE_REF", "origin/main") | ||
| always_fetch = parse_bool_env(os.environ.get("POST_TURN_ALWAYS_FETCH", "")) | ||
| max_out = parse_max_output(os.environ.get("POST_TURN_MAX_OUTPUT_CHARS", "12000")) | ||
| compush = parse_bool_env(os.environ.get("POST_TURN_COMPUSH", "")) | ||
| always_fetch = parse_bool_env( | ||
| os.environ.get("POST_TURN_ALWAYS_FETCH", flag_default) | ||
| ) | ||
| max_out = parse_max_output( | ||
| os.environ.get("POST_TURN_MAX_OUTPUT_CHARS", max_output_default) | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Stop duplicating the 12000 default.
parse_max_output() already owns 12000; the string copy here can drift from it. Feed an unparsable sentinel and let the single source of truth apply.
♻️ Proposed refactor
- # Equivalent mutant: a mutated (non-numeric) default falls back to
- # parse_max_output's own 12000 default, so it cannot change behaviour.
- max_output_default = "12000" # pragma: no mutate
+ # Any non-numeric default delegates to parse_max_output's own default.
+ max_output_default = "" # pragma: no mutate📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Equivalent mutant: a mutated (non-numeric) default falls back to | |
| # parse_max_output's own 12000 default, so it cannot change behaviour. | |
| max_output_default = "12000" # pragma: no mutate | |
| base_ref = os.environ.get("POST_TURN_BASE_REF", "origin/main") | |
| always_fetch = parse_bool_env(os.environ.get("POST_TURN_ALWAYS_FETCH", "")) | |
| max_out = parse_max_output(os.environ.get("POST_TURN_MAX_OUTPUT_CHARS", "12000")) | |
| compush = parse_bool_env(os.environ.get("POST_TURN_COMPUSH", "")) | |
| always_fetch = parse_bool_env( | |
| os.environ.get("POST_TURN_ALWAYS_FETCH", flag_default) | |
| ) | |
| max_out = parse_max_output( | |
| os.environ.get("POST_TURN_MAX_OUTPUT_CHARS", max_output_default) | |
| ) | |
| # Any non-numeric default delegates to parse_max_output's own default. | |
| max_output_default = "" # pragma: no mutate | |
| base_ref = os.environ.get("POST_TURN_BASE_REF", "origin/main") | |
| always_fetch = parse_bool_env( | |
| os.environ.get("POST_TURN_ALWAYS_FETCH", flag_default) | |
| ) | |
| max_out = parse_max_output( | |
| os.environ.get("POST_TURN_MAX_OUTPUT_CHARS", max_output_default) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/post-turn-quality-stop-hook.py` around lines 887 - 896, Update the
max_output_default value used by parse_max_output in the surrounding
configuration setup to an unparsable sentinel instead of duplicating "12000".
Preserve parse_max_output's existing fallback so it remains the single source of
truth for the default maximum output.
| def test_real_command_captures_text_output(self, tmp_path: Path) -> None: | ||
| """run() captures decoded stdout/stderr and the real exit code. | ||
|
|
||
| Kills the ``run`` text/capture-output keyword survivors tracked | ||
| in #37. | ||
| """ | ||
| result = hook.run(["sh", "-c", "echo out; echo err >&2; exit 3"], tmp_path) | ||
| assert result.returncode == 3, ( | ||
| f"expected returncode 3 but got {result.returncode!r}" | ||
| ) | ||
| assert result.stdout == "out\n", ( | ||
| f"expected decoded stdout 'out\\n' but got {result.stdout!r}" | ||
| ) | ||
| assert result.stderr == "err\n", ( | ||
| f"expected decoded stderr 'err\\n' but got {result.stderr!r}" | ||
| ) | ||
|
|
||
| def test_file_as_cwd_returns_error(self, tmp_path: Path) -> None: | ||
| """run() with a file as cwd returns the NotADirectoryError fallback. | ||
|
|
||
| Kills the ``run`` ``NotADirectoryError``-branch field survivors | ||
| tracked in #37. | ||
| """ | ||
| file_cwd = tmp_path / "not-a-dir" | ||
| file_cwd.write_text("plain file\n") | ||
| cmd = ["git", "status"] | ||
| result = hook.run(cmd, file_cwd) | ||
| assert result.returncode == 1, ( | ||
| f"expected returncode 1 for file cwd but got {result.returncode!r}" | ||
| ) | ||
| assert result.args == cmd, ( | ||
| f"expected fallback args to echo the command but got {result.args!r}" | ||
| ) | ||
| assert result.stdout == "", ( | ||
| f"expected empty fallback stdout but got {result.stdout!r}" | ||
| ) | ||
| assert "director" in result.stderr.lower(), ( | ||
| f"expected stderr to describe the directory error but got {result.stderr!r}" | ||
| ) | ||
|
|
||
| def test_filename_less_file_not_found_uses_fallback(self) -> None: | ||
| """A FileNotFoundError without a filename maps to the cwd fallback. | ||
|
|
||
| Kills the ``run`` filename-less ``FileNotFoundError`` survivor | ||
| tracked in #37. | ||
| """ | ||
| exc = FileNotFoundError(2, "No such file or directory") | ||
| with patch.object(hook.subprocess, "run", side_effect=exc): | ||
| result = hook.run(["git", "status"], Path(".")) | ||
| assert result.returncode == 1, ( | ||
| f"expected fallback returncode 1 but got {result.returncode!r}" | ||
| ) | ||
| assert result.args == ["git", "status"], ( | ||
| f"expected fallback args to echo the command but got {result.args!r}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move these three run() tests into TestRunOSError.
They sit inside TestGetMakeTargets ("Tests for make target enumeration") and exercise hook.run instead, so the subprocess coverage is filed under the wrong contract. Relocate them above Line 513 alongside test_nonexistent_cwd_returns_error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/test_post_turn_quality_stop_hook.py` around lines 533 - 587, Move
test_real_command_captures_text_output, test_file_as_cwd_returns_error, and
test_filename_less_file_not_found_uses_fallback from TestGetMakeTargets into
TestRunOSError, placing them near test_nonexistent_cwd_returns_error. Preserve
each test’s implementation and assertions unchanged.
|
|
||
| # --------------------------------------------------------------------------- | ||
| # truncate | ||
| # --------------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this module: it is now well past the 400 logical line cap.
The file runs to at least Line 921. Break it along the banner comments already present (test_run.py, test_helpers.py, test_formatting.py, test_env.py) and move _completed, REPO and ENV_VARS into conftest.py or a fixtures/ module.
As per path instructions, "Files must not exceed 400 logical lines: Decompose large modules into subpackages" and "Use pytest fixtures for shared setup (conftest.py or fixtures/)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/test_post_turn_quality_stop_hook.py` around lines 589 - 593, Split
hooks/test_post_turn_quality_stop_hook.py into focused modules using the
existing test_run.py, test_helpers.py, test_formatting.py, and test_env.py
banner sections as boundaries, keeping each module under 400 logical lines. Move
the shared _completed, REPO, and ENV_VARS setup into conftest.py or a fixtures
module, then update imports and references so all tests retain their current
behavior.
Source: Path instructions
| "Fix the failures above. The checks will re-run at the end of" | ||
| " the next turn.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Parenthesise the implicit concatenation inside these list literals.
Ruff ISC004 fires on both sites: an unparenthesised two-part string in a list reads like a forgotten comma, and a genuine missing comma here would silently weaken the expected output. Wrap each pair.
🔧 Proposed fix (apply at both sites)
- "Fix the failures above. The checks will re-run at the end of"
- " the next turn.",
+ (
+ "Fix the failures above. The checks will re-run at the"
+ " end of the next turn."
+ ),Also applies to: 833-834
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 771-772: Unparenthesized implicit string concatenation in collection
Did you forget a comma?
(ISC004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/test_post_turn_quality_stop_hook.py` around lines 771 - 772,
Parenthesize the implicit two-part string concatenations in the list literals at
both occurrences, including the entries near the relevant test messages, so Ruff
ISC004 is satisfied and the expected output remains unchanged.
Source: Linters/SAST tools
Closes #37 and #38. Addresses #36 and #40. Issues #39 and #41 are deferred in full.
Survivor-triage PR for the mutation-testing run recorded against
main(b850e2f). It adds targeted unit tests for the highest-value surviving mutants inhooks/post-turn-quality-stop-hook.py, and eliminates or suppresses the equivalent mutants identified at triage.Before/after (full
mutmut run, mutmut 3.6.0, CPython 3.14)Six equivalent mutants are no longer generated (pragma suppression and the removal of a redundant argument — see below). The 129 remaining survivors break down as 117 deferred real gaps (71
prepare_run_stop_checks+ 10run_stop_checks→ #39; 18repo_root+ 18get_make_targets→ #36) and 12 newly identified equivalents (7format_reason, 3parse_make_targets, 2truncate— justified below).Review walkthrough
hooks/test_post_turn_quality_stop_hook.py— new test classes fortruncate(boundary and exact head/tail split),parse_make_targetsandis_missing_makefile(representativemake -qpfixture),detect_categoriesanddefault_categories(full-dict equality across every extension group),format_reason(exact multi-line rendering, the 60-file elision boundary, missing-key command entries), the fullparse_envtuple andparse_max_outputfallback, andrun()(real subprocess capturing text output, theNotADirectoryErrorbranch, the filename-lessFileNotFoundErrorguard). Existing plumbing tests gain exact argv (call_args_list) assertions, exact error-message equality in place of substring checks, wiring assertions incompush_check, and the ahead-by-exactly-one boundary case.hooks/post-turn-quality-stop-hook.py— equivalence handling only; no behavioural change (the full suite and a fresh mutation run confirm).Equivalent mutants (category 2)
mutmut 3.6 honours
# pragma: no mutateonly at statement level (its pragma visitor ignores trailing comments inside call argument lists), so the worklist's four equivalents were handled structurally:parse_envflag defaults (""→"XXXX"onPOST_TURN_ALWAYS_FETCH/POST_TURN_COMPUSH): hoisted into a singleflag_default = "" # pragma: no mutatestatement with a justification comment — any mutated default is still non-truthy toparse_bool_env.run()check=False→check=None/dropped: the argument was the subprocess default, so both mutants were equivalent by construction; the redundant argument is removed and a comment records that non-zero return codes are deliberately surfaced viaCompletedProcess.returncode.POST_TURN_MAX_OUTPUT_CHARSdefault ("12000"→ non-numeric) falls back toparse_max_output's own 12000 default — hoisted and suppressed the same way.Twelve further equivalents remain in the survivor list unsuppressed, because each sits on a line that also carries killable mutants (line-level suppression would shield them):
format_reason(7):c.get("exit_code", …)defaults are unreachable — the failures filter guarantees the key is present;c.get("stdout"/"stderr", None/dropped)defaults are filtered identically to""by theif xcomprehension guard.parse_make_targets(3): corrupting one element of thestartswith(("#", "\t", " "))guard is unobservable — the rule regex rejects those lines anyway.truncate(2):<=→<at both boundaries converges on the same output through the fall-through slice paths.Red-green evidence
Each new test was verified against a hand-applied mutant diff (fail) and the reverted source (pass); cycles were run for
format_reason,run,default_categories,has_unpushed_commits, andparse_env. Representative transcript:(Two same-length mutations initially produced misleading transcripts because Python's mtime-plus-size
.pycinvalidation served stale bytecode; the cycles were re-run withPYTHONDONTWRITEBYTECODE=1and clean caches, and both then behaved correctly.)Deferred work
The 400-line test-budget tolerance for this PR is spent (392 net new test lines). Deferred explicitly:
repo_root(18) andget_make_targets(18) argv/message survivors.prepare_run_stop_checks,run_stop_checks).targets_for_categories,dedup_preserve_order,parse_hook_input,resolve_start_cwd,block_and_print, andfail_stateremain in the no-tests bucket.Validation
make ci(check-fmt, lint, typecheck, full pytest suite — 149 tests) green.mutmut runre-runs after each commit; scopedmutmut run 'hooks.post-turn-quality-stop-hook.x_format_reason*'confirmed the final residual kill.Summary by Sourcery
Strengthen post-turn quality hook behavior around git status, upstream detection, commit/push checks, environment parsing, and subprocess execution, while annotating equivalent mutation points.
New Features:
Enhancements: