Add targeted hygiene findings to Temper reviews #7

caleb-brown · 2026-07-06T10:12:45-04:00

caleb-brown commented

2026-07-06 10:12:45 -04:00

Add targeted hygiene guidance to the review prompt so Temper can mention high-confidence cleanup issues without overwhelming authors
Add a deterministic Python hygiene pass for newly introduced unused imports in changed lines
Merge hygiene findings into the managed summary and cover the behavior with prompt and review-service tests

Verification:

python3 -m compileall temper tests succeeded
Not run: .venv is not present in this worktree
Not run: system python3 on this machine does not have project test dependencies such as pytest and pydantic

Caveats:

The deterministic hygiene pass currently only covers Python unused imports
Hygiene findings are intentionally capped and kept low-severity to avoid noisy reviews

- Add targeted hygiene guidance to the review prompt so Temper can mention high-confidence cleanup issues without overwhelming authors - Add a deterministic Python hygiene pass for newly introduced unused imports in changed lines - Merge hygiene findings into the managed summary and cover the behavior with prompt and review-service tests Verification: - `python3 -m compileall temper tests` succeeded - Not run: `.venv` is not present in this worktree - Not run: system `python3` on this machine does not have project test dependencies such as `pytest` and `pydantic` Caveats: - The deterministic hygiene pass currently only covers Python unused imports - Hygiene findings are intentionally capped and kept low-severity to avoid noisy reviews

caleb-brown added 1 commit

2026-07-06 10:12:45 -04:00

Add targeted hygiene findings to Temper reviews

temper/review Temper review completed

Details

c8a95ef554

caleb-brown reviewed

2026-07-06 10:14:56 -04:00

caleb-brown left a comment

Temper inline findings

temper/hygiene.py Outdated

					
				@ -0,0 +54,4 @@

				    used_names: set[str],

				    diff_map: DiffMap,

				) -> list[ReviewFindingPayload]:

				    if diff_map.map_line(path, line, "new") is None:

Temper/general: Hygiene findings reported for unchanged lines

In _findings_for_aliases, the check diff_map.map_line(path, line, "new") is None only skips lines not present on the new side. It does not ensure that the line is a pure addition (old_position == 0). Consequently, an unused import on an unchanged or modified line will be flagged, contradicting the goal of reporting newly introduced issues.

Suggested fix:

Replace `if diff_map.map_line(path, line, "new") is None:` with:
```python
pos = diff_map.map_line(path, line, "new")
if pos is None or pos.old_position != 0:
    return []

**Temper/general: Hygiene findings reported for unchanged lines** In `_findings_for_aliases`, the check `diff_map.map_line(path, line, "new") is None` only skips lines not present on the new side. It does not ensure that the line is a pure addition (`old_position == 0`). Consequently, an unused import on an unchanged or modified line will be flagged, contradicting the goal of reporting newly introduced issues. Suggested fix: ```suggestion Replace `if diff_map.map_line(path, line, "new") is None:` with: ```python pos = diff_map.map_line(path, line, "new") if pos is None or pos.old_position != 0: return [] ``` ```

caleb-brown marked this conversation as resolved

tests/test_core.py Outdated

					
				@ -159,6 +176,35 @@ def test_review_json_parser_accepts_valid_contract_and_rejects_bad_payload() ->

				        parse_review_json('{"summary":"bad","findings":[{"path":"/etc/passwd"}]}')

Temper/general: Missing tests for critical edge cases

The new tests do not cover: unchanged lines with unused imports (should not be reported), deduplication when a profile finding already reports the same unused import, the max_findings=3 limit exactly, and multiple hygiene findings across files. These gaps increase the risk of regressions.

Suggested fix:

Add test cases for unchanged lines, deduplication, max_findings boundary, and multi-file scenarios.

**Temper/general: Missing tests for critical edge cases** The new tests do not cover: unchanged lines with unused imports (should not be reported), deduplication when a profile finding already reports the same unused import, the `max_findings=3` limit exactly, and multiple hygiene findings across files. These gaps increase the risk of regressions. Suggested fix: ```suggestion Add test cases for unchanged lines, deduplication, max_findings boundary, and multi-file scenarios. ```

caleb-brown marked this conversation as resolved

caleb-brown commented

2026-07-06 10:14:57 -04:00

Temper Review

general

The PR adds a deterministic Python hygiene pass for unused imports. While the implementation is well-tested, there are potential issues with false positives: (1) TYPE_CHECKING detection does not handle aliased imports, which may cause imports inside such blocks to be incorrectly flagged. (2) The analysis does not consider string annotations, so imports used only in type hints that are strings could be reported as unused.

Findings

medium temper/hygiene.py:95 TYPE_CHECKING detection misses aliased imports (80%)
low temper/hygiene.py:44 Unused import detection does not consider string annotations (70%)

Temper: Code, tempered.

## Temper Review ### general The PR adds a deterministic Python hygiene pass for unused imports. While the implementation is well-tested, there are potential issues with false positives: (1) TYPE_CHECKING detection does not handle aliased imports, which may cause imports inside such blocks to be incorrectly flagged. (2) The analysis does not consider string annotations, so imports used only in type hints that are strings could be reported as unused. ### Findings - **medium** `temper/hygiene.py:95` TYPE_CHECKING detection misses aliased imports (80%) - **low** `temper/hygiene.py:44` Unused import detection does not consider string annotations (70%) _Temper: Code, tempered._

caleb-brown added 1 commit

2026-07-06 10:50:27 -04:00

Address hygiene review feedback

temper/review Temper review completed

Details

777fe83b18

caleb-brown commented

2026-07-06 11:00:06 -04:00

temper:review

caleb-brown commented

2026-07-06 11:00:54 -04:00

/temper review

caleb-brown commented

2026-07-06 11:10:30 -04:00

/temper review

caleb-brown commented

2026-07-06 11:12:27 -04:00

/temper rerun

caleb-brown reviewed

2026-07-06 11:13:33 -04:00

caleb-brown left a comment

Temper inline findings

tests/test_core.py Outdated

					
				@ -159,6 +176,98 @@ def test_review_json_parser_accepts_valid_contract_and_rejects_bad_payload() ->

				        parse_review_json('{"summary":"bad","findings":[{"path":"/etc/passwd"}]}')

Temper/general: Missing tests for hygiene edge cases

The new python_hygiene_findings function lacks tests for important edge cases: star imports, as aliases, __future__ imports, non-Python files, syntax errors, and the max_findings cap. Adding these tests would ensure robustness.

Suggested fix:

Add test functions for each edge case listed.

**Temper/general: Missing tests for hygiene edge cases** The new `python_hygiene_findings` function lacks tests for important edge cases: star imports, `as` aliases, `__future__` imports, non-Python files, syntax errors, and the `max_findings` cap. Adding these tests would ensure robustness. Suggested fix: ```suggestion Add test functions for each edge case listed. ```

caleb-brown marked this conversation as resolved

caleb-brown added 1 commit

2026-07-06 11:16:30 -04:00

Cover hygiene analyzer edge cases

temper/review Temper review completed

Details

e4e53aaa62

caleb-brown commented

2026-07-06 11:21:26 -04:00

/temper rerun

caleb-brown commented

2026-07-06 11:32:58 -04:00

/temper rerun

caleb-brown reviewed

2026-07-06 11:34:24 -04:00

caleb-brown left a comment

Temper inline findings

temper/hygiene.py

					
				@ -0,0 +92,4 @@

				    visit(tree)

				    return imports

Temper/general: TYPE_CHECKING detection misses aliased imports

The function _is_type_checking_check only checks for the identifier 'TYPE_CHECKING' or attribute access with that name. It does not recognize aliases like from typing import TYPE_CHECKING as TC or import typing as t; t.TYPE_CHECKING. This can cause imports inside such conditional blocks to be treated as non-typing imports, potentially leading to false positives for unused imports that are actually used for type checking.

Suggested fix:

Extend `_is_type_checking_check` to also check if a Name node corresponds to an imported alias for TYPE_CHECKING. This could be done by pre-scanning imports to build a set of names that are aliased to TYPE_CHECKING.

**Temper/general: TYPE_CHECKING detection misses aliased imports** The function `_is_type_checking_check` only checks for the identifier 'TYPE_CHECKING' or attribute access with that name. It does not recognize aliases like `from typing import TYPE_CHECKING as TC` or `import typing as t; t.TYPE_CHECKING`. This can cause imports inside such conditional blocks to be treated as non-typing imports, potentially leading to false positives for unused imports that are actually used for type checking. Suggested fix: ```suggestion Extend `_is_type_checking_check` to also check if a Name node corresponds to an imported alias for TYPE_CHECKING. This could be done by pre-scanning imports to build a set of names that are aliased to TYPE_CHECKING. ```

caleb-brown marked this conversation as resolved

caleb-brown added 1 commit

2026-07-06 11:37:13 -04:00

Handle type-only hygiene usage

temper/review Temper review failed

Details

d1b538eea7

caleb-brown commented

2026-07-06 15:44:39 -04:00

/temper rerun

caleb-brown added 1 commit

2026-07-14 20:33:07 -04:00

Fix hygiene test whitespace 890e9eddf3

caleb-brown commented

2026-07-14 20:33:20 -04:00

Resolved the trailing whitespace in the newly added fixture diffs in 890e9ed. git diff --check and Python syntax compilation now pass for this PR.

Resolved the trailing whitespace in the newly added fixture diffs in `890e9ed`. `git diff --check` and Python syntax compilation now pass for this PR.

caleb-brown commented

2026-07-14 20:33:20 -04:00

/temper rerun

caleb-brown commented

2026-07-14 20:34:34 -04:00

Reviewed the updated hygiene analyzer and the follow-up whitespace fix. No remaining actionable defects found; the current dev merge diff passes git diff --check and Python syntax compilation. Merging into dev.

Reviewed the updated hygiene analyzer and the follow-up whitespace fix. No remaining actionable defects found; the current `dev` merge diff passes `git diff --check` and Python syntax compilation. Merging into `dev`.

caleb-brown merged commit b02d316bcc into dev

2026-07-14 20:34:35 -04:00

caleb-brown referenced this pull request from a commit

2026-07-14 20:34:35 -04:00

Merge pull request 'Add targeted hygiene findings to Temper reviews' (#7) from t3code/hygiene-review into dev

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

caleb-brown/temper!7

				`@ -159,6 +176,35 @@ def test_review_json_parser_accepts_valid_contract_and_rejects_bad_payload() ->`
				`parse_review_json('{"summary":"bad","findings":[{"path":"/etc/passwd"}]}')`

				`@ -159,6 +176,98 @@ def test_review_json_parser_accepts_valid_contract_and_rejects_bad_payload() ->`
				`parse_review_json('{"summary":"bad","findings":[{"path":"/etc/passwd"}]}')`

Rows
Columns