Skip to content

Kill mutation survivors in the post-turn quality stop hook (#36, #37, #38, #40) - #50

Open
leynos wants to merge 3 commits into
mainfrom
kill-mutation-survivors
Open

Kill mutation survivors in the post-turn quality stop hook (#36, #37, #38, #40)#50
leynos wants to merge 3 commits into
mainfrom
kill-mutation-survivors

Conversation

@leynos

@leynos leynos commented Jul 13, 2026

Copy link
Copy Markdown
Owner

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 in hooks/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)

Status Before After
Killed 151 461
Survived 235 129
No tests 639 429
Total generated 1025 1019

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 + 10 run_stop_checks#39; 18 repo_root + 18 get_make_targets#36) and 12 newly identified equivalents (7 format_reason, 3 parse_make_targets, 2 truncate — justified below).

Review walkthrough

  • hooks/test_post_turn_quality_stop_hook.py — new test classes for truncate (boundary and exact head/tail split), parse_make_targets and is_missing_makefile (representative make -qp fixture), detect_categories and default_categories (full-dict equality across every extension group), format_reason (exact multi-line rendering, the 60-file elision boundary, missing-key command entries), the full parse_env tuple and parse_max_output fallback, and run() (real subprocess capturing text output, the NotADirectoryError branch, the filename-less FileNotFoundError guard). Existing plumbing tests gain exact argv (call_args_list) assertions, exact error-message equality in place of substring checks, wiring assertions in compush_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 mutate only at statement level (its pragma visitor ignores trailing comments inside call argument lists), so the worklist's four equivalents were handled structurally:

  • parse_env flag defaults ("""XXXX" on POST_TURN_ALWAYS_FETCH/POST_TURN_COMPUSH): hoisted into a single flag_default = "" # pragma: no mutate statement with a justification comment — any mutated default is still non-truthy to parse_bool_env.
  • run() check=Falsecheck=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 via CompletedProcess.returncode.
  • Newly identified during the re-run: the POST_TURN_MAX_OUTPUT_CHARS default ("12000" → non-numeric) falls back to parse_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 the if x comprehension guard.
  • parse_make_targets (3): corrupting one element of the startswith(("#", "\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, and parse_env. Representative transcript:

=== RED: format_reason literal (x_format_reason__mutmut_2)
    ("Post-turn checks failed." -> "XXPost-turn checks failed.XX")
E   AssertionError: unexpected minimal reason: 'XXPost-turn checks failed.XX\n\nError: boom…'
2 failed, 2 passed, 68 deselected in 0.15s
=== GREEN: format_reason literal (reverted)
4 passed, 68 deselected in 0.12s

(Two same-length mutations initially produced misleading transcripts because Python's mtime-plus-size .pyc invalidation served stale bytecode; the cycles were re-run with PYTHONDONTWRITEBYTECODE=1 and 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:

Validation

  • make ci (check-fmt, lint, typecheck, full pytest suite — 149 tests) green.
  • Full mutmut run re-runs after each commit; scoped mutmut 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:

  • Add comprehensive unit tests for truncate, make target parsing, missing-makefile detection, category detection/defaults, environment parsing, and max-output parsing in the post-turn quality stop hook.
  • Add tests covering real subprocess execution, error fallbacks, and boundary conditions in run(), including NotADirectoryError and FileNotFoundError cases.

Enhancements:

  • Tighten existing tests to assert exact git command invocations, precise error messages, and compush_check wiring and messages.
  • Refine run() and parse_env() implementation to remove redundant arguments and centralize environment defaults with pragma annotations for known equivalent mutants.

leynos added 2 commits July 13, 2026 20:51
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.
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Summary

  • Added targeted unit tests covering mutation survivors across hook parsing, truncation, category detection, formatting, environment handling, git wiring, and subprocess execution.
  • Expanded run() resilience coverage for non-zero commands, missing directories, file-based working directories, and filename-less FileNotFoundError.
  • Removed the redundant check=False argument and refactored environment defaults without changing behaviour.
  • Improved assertions for exact git commands, error messages, fallback fields, and formatted reasons.
  • Mutation results improved from 151 to 461 killed mutants, with survivors reduced from 235 to 129.
  • 149 tests pass and make ci is green.

Walkthrough

The subprocess wrapper was simplified, while tests now verify exact Git commands, error messages, fallback process fields, output handling, formatting, category detection, and environment parsing.

Changes

Quality hook coverage

Layer / File(s) Summary
Subprocess fallback contract
hooks/post-turn-quality-stop-hook.py, hooks/test_post_turn_quality_stop_hook.py
Remove the explicit check=False argument and expand coverage for captured output, missing or invalid working directories, fallback fields, and filename-less FileNotFoundError.
Git and compush decision contracts
hooks/test_post_turn_quality_stop_hook.py
Assert exact Git command wiring, upstream and unpushed-commit boundaries, error strings, helper call arguments, and JSON reasons.
Helper parsing and rendering coverage
hooks/post-turn-quality-stop-hook.py, hooks/test_post_turn_quality_stop_hook.py
Cover truncation, Makefile and category parsing, reason formatting, environment defaults and overrides, and maximum-output parsing.

Possibly related issues

  • #36 — Strengthens tests for the same Git plumbing helpers, command arguments, boundary cases, and exact reasons.

Suggested labels: Issue

Poem

Tighten each assertion, let exact truths shine,
Capture stdout on every command line.
Guard every fallback, parse each flag,
Keep Git’s reasons neatly on track.
Test, verify, and let quality fly.

🚥 Pre-merge checks | ✅ 19 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR adds broad tests for parse_make_targets, format_reason, category detection and parse_env that are outside #37. Split the non-run() coverage into separate PRs or link the relevant issues, and keep this PR focused on the run() subprocess wrapper survivors.
✅ Passed checks (19 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the mutation-survivor work and includes the linked issue references.
Description check ✅ Passed The description stays on-topic and describes the same mutation-survivor changes.
Linked Issues check ✅ Passed The PR covers #37 by testing real subprocess execution and both fallback branches in run().
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Testing (Overall) ✅ Passed The added tests hit real subprocess execution, both OSError fallbacks, full parse/format outputs, and exact wiring, so plausible regressions would fail.
User-Facing Documentation ✅ Passed No user-facing behaviour changed; only internal hook code and tests changed, and docs/users-guide.md was untouched.
Developer Documentation ✅ Passed The PR only refactors hook internals and adds tests; it introduces no new API, architecture, tooling, or build contract needing docs, ADR, or execplan updates.
Module-Level Documentation ✅ Passed Both touched Python modules begin with descriptive docstrings stating purpose, utility, and the test module’s relationship to the hook.
Testing (Unit And Behavioural) ✅ Passed PASS: the PR adds focused unit tests for edge cases plus real-subprocess and OS-error behavioural coverage at the hook boundary.
Testing (Property / Proof) ✅ Passed Do not request property or proof tooling: the PR only tightens unit tests and cleans equivalent-mutant handling; it introduces no new broad invariant or lemma.
Testing (Compile-Time / Ui) ✅ Passed Keep the assertion-based tests; no Rust/TypeScript compile-time path exists, and the hook output checks are focused enough without snapshots.
Unit Architecture ✅ Passed The PR keeps subprocess and env access isolated at explicit boundaries; the new tests and tiny refactors make fallibility clearer, not blurrier.
Domain Architecture ✅ Passed Keep this as infrastructure glue: the PR only tweaks the stop-hook and its tests; it does not pull transport or filesystem concerns into any domain model.
Observability ✅ Passed No new operational behaviour appears; the PR is test-only plus equivalent-mutant refactors, and no observability gaps were introduced.
Security And Privacy ✅ Passed No secrets, auth changes, unsafe sinks, or broadened permissions were added; the hook still uses list argv with no shell, and tests use only fake data and env names.
Performance And Resource Use ✅ Passed No new unbounded loops, repeated I/O, or hot-path allocation regressions appear; runtime scans stay linear and truncation remains fixed at 60 items.
Concurrency And State ✅ Passed PASS: preserve single-threaded, locally owned HookState flow; no async, locks, background tasks, or shared mutable globals were added.
Architectural Complexity And Maintainability ✅ Passed Only direct tests changed; the hook code stays explicit and local, with no new layers, registries, or hidden lifecycle hooks.
Rust Compiler Lint Integrity ✅ Passed This PR only changes hooks/test_post_turn_quality_stop_hook.py; no Rust files, lint suppressions, or clone-heavy ownership changes are present.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch kill-mutation-survivors

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Reviewer's Guide

This 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 handling

flowchart 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)")]
Loading

File-Level Changes

Change Details Files
Strengthen existing git-plumbing and compush-check tests with exact command, error-message, and wiring assertions to better constrain behaviour under mutation.
  • Add unittest.mock.call imports and assert exact call_args_list for git diff/ls-files/rev-parse/rev-list invocations.
  • Replace substring-based error-message checks with exact string equality for git error paths.
  • Add tests for edge conditions such as rev-list count == 1 and empty outputs, and assert compush_check calls its helpers with precise arguments.
hooks/test_post_turn_quality_stop_hook.py
Add focused unit tests for run(), truncate(), parse_make_targets(), is_missing_makefile(), detect_categories(), default_categories(), format_reason(), parse_env(), and parse_max_output to pin down their behaviour and kill surviving mutants.
  • Add round-trip tests for run() covering normal subprocess output capture, NotADirectoryError cwd handling, and filename-less FileNotFoundError fallbacks.
  • Define TRUNCATE_MARKER and parametrized tests that cover truncate() boundaries and exact head/tail splits for truncated text.
  • Add representative make -qp fixture output and verify parse_make_targets() target extraction and is_missing_makefile() case-insensitive detection.
  • Verify default_categories() and detect_categories() mappings across Python/TS, Rust, and Markdown extension groups.
  • Add multi-scenario format_reason() tests for minimal error-only state, full state with commands and targets, file-list truncation at 60/61 files, and treatment of commands missing exit_code.
  • Add parse_env() tests for default tuple and environment-variable overrides, and parse_max_output() tests for valid integers and non-numeric/empty fallbacks to 12000.
hooks/test_post_turn_quality_stop_hook.py
Refactor parse_env() and run() implementation to structurally suppress known equivalent mutants using pragma comments, without altering runtime behaviour.
  • Remove the redundant check=False argument from subprocess.run in run(), documenting that non-zero exit codes are intentionally surfaced via CompletedProcess.returncode.
  • Hoist POST_TURN_ALWAYS_FETCH and POST_TURN_COMPUSH flag defaults into a single flag_default variable annotated with # pragma: no mutate to prevent equivalent mutants of empty-string defaults.
  • Hoist POST_TURN_MAX_OUTPUT_CHARS default into max_output_default annotated with # pragma: no mutate to prevent equivalent non-numeric default mutants, while still letting parse_max_output enforce the 12000 fallback.
hooks/post-turn-quality-stop-hook.py

Assessment against linked issues

Issue Objective Addressed Explanation
#37 Add tests for run() that execute a real subprocess command and assert decoded stdout/stderr and the actual exit code, ensuring text=True, capture_output=True usage and that check=True would raise instead of returning.
#37 Add tests that exercise both error-path fallbacks of run(): the missing-cwd (FileNotFoundError with a non-matching filename) and NotADirectoryError branches, asserting the full fallback CompletedProcess fields (args, returncode, stdout, stderr).
#37 Add a test for the filename-less FileNotFoundError guard in run(), confirming that the cwd fallback is returned rather than re-raised and that args mirrors the command.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@leynos leynos changed the title Kill mutation survivors in the post-turn quality stop hook Kill mutation survivors in the post-turn quality stop hook (#36, #37, #38, #40) Jul 17, 2026
Add issue references to the docstrings of the new tests and tightened
assertions in the post-turn quality stop hook suite, so each kill can
be traced back to #36, #37, #38, and #40.
@leynos
leynos marked this pull request as ready for review July 25, 2026 14:53

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added the Issue label Jul 29, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8cd59d2 and 97441e9.

📒 Files selected for processing (2)
  • hooks/post-turn-quality-stop-hook.py
  • hooks/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)

Comment on lines 151 to 157
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
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
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

Comment on lines +887 to +896
# 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)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
# 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.

Comment on lines +533 to +587
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}"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Comment on lines +589 to +593

# ---------------------------------------------------------------------------
# truncate
# ---------------------------------------------------------------------------

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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

Comment on lines +771 to +772
"Fix the failures above. The checks will re-run at the end of"
" the next turn.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mutation testing: run() subprocess wrapper survivors

2 participants