Repository navigation
feat(type-safety): add python-type-safety skill - #3
Merged
Merged
Conversation
Move blackbox, parameterized, and type-safety skeleton from .agents/skills to src/; tests, CI, README, CONTRIBUTING, and AGENTS.md point at src/ again. .agents/ is gitignored machine-local copies only. Type-safety references/cases.yaml land in follow-ups; structure and quality tests fail on exactly those missing files.
Reviewer's GuideThis PR adds a checker-gated python-type-safety skill with advisory static scanning, plugin and data-safety controls, evaluation fixtures, and evidence-report requirements, while making src/ the canonical skill tree and updating CI, documentation, catalog metadata, and repository quality tests to discover and validate all three skills. Sequence diagram for checker-gated type-safety analysissequenceDiagram
participant User
participant Skill as PythonTypeSafetySkill
participant Scanner as scan_annotations.py
participant Primary as ProjectNativeChecker
participant Secondary as CrossCheckChecker
participant Report as EvidenceReport
User->>Skill: Request typing diagnosis or annotation work
Skill->>Primary: Run baseline checker
Primary-->>Skill: Baseline errors and status
Skill->>Scanner: Scan supplied source text
Scanner-->>Skill: Advisory findings
alt Unknown mypy plugin
Skill-->>User: Ask approval or refuse checker execution
else Checker is safe to run
Skill->>Primary: Check boundary-first annotations
Primary-->>Skill: Updated errors and status
Skill->>Secondary: Run report-only cross-check
Secondary-->>Skill: Divergences and status
end
Skill->>Report: Record commands, statuses, findings, gaps, and limitations
Report-->>User: Evidence report
Flow diagram for the Python type-safety skill workflowflowchart TD
A["Classify typing target and scope"] --> B["Read checker config and Python floor"]
B --> C["Run project-native checker baseline"]
C --> D["Run scan_annotations.py advisory scan"]
D --> E["Annotate public boundary first"]
E --> F["Fix checker errors and rerun"]
F --> G{"Unknown mypy plugin?"}
G -->|Yes| H["Refuse or ask before execution"]
G -->|No| I["Run secondary checker report-only"]
H --> I
I --> J["Write evidence report"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 14 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="150-151" />
<code_context>
+ emit(
+ "bare-ignore",
+ "suppression hygiene",
+ f"{path}:{lineno}",
+ line.strip()[:200],
+ )
+
</code_context>
<issue_to_address>
**Sensitive data reaches scanner output**
When a matching source line contains a secret or the supplied path is private, `_scan_bare_ignores` copies source text into `evidence`, and `scan_source_files` includes the supplied path in `location`; `main` prints both in its JSON output, exposing sensitive data to callers and saved reports.
Redact sensitive source text and private paths before including them in findings.
Also at `src/python-type-safety/scripts/scan_annotations.py:101`, `src/python-type-safety/scripts/scan_annotations.py:226`.
</issue_to_address>
### Comment 2
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="205" />
<code_context>
+ """Read stdin, emit stable JSON, and return a process status."""
+ _parser().parse_args()
+ try:
+ payload = json.load(sys.stdin, parse_constant=_reject_non_finite_json)
+ result = scan_source_files(payload)
+ output = json.dumps(
</code_context>
<issue_to_address>
**Large input exhausts scanner memory**
When stdin contains a JSON payload larger than the per-file or file-count limits, `main` uses `json.load` to materialize all stdin before `_validate_payload` checks its limits, so the scanner can exhaust memory and terminate before returning an input error.
Read stdin with a byte limit and enforce an aggregate payload limit before scanning.
Also at `src/python-type-safety/scripts/scan_annotations.py:13`.
</issue_to_address>
### Comment 3
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="95" />
<code_context>
+ if node.args.kwarg is not None:
+ parameters.append((node.args.kwarg, f"**{node.args.kwarg.arg}"))
+ for arg, display in parameters:
+ if arg.arg in ("self", "cls"):
+ continue
+ if arg.annotation is None:
</code_context>
<issue_to_address>
**Ordinary parameters escape annotation findings**
When a free function or static method has an unannotated parameter named `self` or `cls`, `_scan_functions` skips parameters named `self` or `cls` without checking whether they are method receivers, so the scanner omits their missing-annotation findings and underreports coverage.
Skip `self` and `cls` only when they are receiver parameters of instance or class methods.
</issue_to_address>
### Comment 4
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="181" />
<code_context>
+ path = entry["path"]
+ content = entry["content"]
+ try:
+ tree = ast.parse(content)
+ except (SyntaxError, ValueError) as error:
+ emit("unparseable-file", "executability", f"{path}:1", str(error)[:200])
</code_context>
<issue_to_address>
**Valid type comments appear missing**
When a target function uses a valid mypy-style signature type comment, `ast.parse` does not retain type comments by default, so `_scan_functions` sees no annotations and reports false missing-annotation findings for those functions.
Parse source with type comments enabled.
</issue_to_address>
### Comment 5
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="68-73" />
<code_context>
+
+
+def _contains_any(annotation: ast.expr) -> bool:
+ for node in ast.walk(annotation):
+ if isinstance(node, ast.Name) and node.id == "Any":
+ return True
+ if isinstance(node, ast.Attribute) and node.attr == "Any":
+ return True
+ return False
+
+
</code_context>
<issue_to_address>
**Quoted `Any` escapes precision findings**
When a function annotation uses a quoted forward reference such as `"Any"`, `_contains_any` does not inspect string forward references, so the scanner omits the `Any` precision finding for annotations a checker can resolve as `Any`.
Recognize `Any` inside string forward-reference annotations.
</issue_to_address>
### Comment 6
<location path="src/python-type-safety/references/typing-patterns.md" line_range="32" />
<code_context>
+
+- **Narrow with `assert` or `isinstance`:** never silence a narrowing error with
+ `cast` when an assertion expresses the invariant.
+- **`Never` and `NoReturn`:** mark exhaustive branches and never-returning
+ helpers so the checker verifies exhaustiveness.
+- **No untyped defs:** every function needs full annotations including the
</code_context>
<issue_to_address>
**Exhaustiveness annotations break Python 3.10**
When a project supports Python 3.10 and follows the exhaustive-branch recommendation using `typing.Never`, `typing.Never` is unavailable before Python 3.11, so following the recommendation with a `typing` import raises `ImportError` and breaks the project at runtime.
Version-gate `Never` and recommend `typing_extensions.Never` for floors below 3.11, or use `NoReturn` where appropriate.
</issue_to_address>
### Comment 7
<location path="src/python-type-safety/scripts/scan_annotations.py" line_range="84" />
<code_context>
+ unannotated top-level function.
+ """
+ for node in ast.walk(tree):
+ if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+ continue
+ parameters: list[tuple[ast.arg, str]] = [
</code_context>
<issue_to_address>
**Variable Any annotations go undetected**
When a scoped module or class uses `Any` on an annotated variable or attribute, `_scan_functions` skips non-function AST nodes, and the scanner never inspects annotated assignments, so `Any` on module variables or class attributes produces no precision finding. Those annotations escape the scanner's review despite the skill requiring every `Any` to be justified.
Inspect `ast.AnnAssign` annotations as well as function signatures, and add tests for module-level and class-level `Any` annotations.
</issue_to_address>
### Comment 8
<location path="src/python-type-safety/SKILL.md" line_range="19" />
<code_context>
+# Python type safety
+
+Make the project's annotations prove what the code claims. Measure coverage statically,
+annotate the public boundary first, drive the project-native checker to zero, and
+confirm nothing with pattern-matching alone: only a checker run confirms a finding.
+
</code_context>
<issue_to_address>
**Coverage deltas are unsupported**
When a typing target includes declarations beyond function signatures or the report requires a before/after coverage delta, when the workflow needs an annotation-coverage delta, `scan_source_files` returns only advisory findings and a truncation flag; it provides neither a coverage denominator nor counts, and it scans function signatures rather than module or class attributes. The report therefore cannot produce the promised coverage delta for the full typing target without an unspecified, separate measurement.
Define the coverage population and have the workflow or scanner report comparable annotated/eligible counts for the scoped target, including supported public attributes.
Also at `src/python-type-safety/scripts/scan_annotations.py:83-117`, `src/python-type-safety/scripts/scan_annotations.py:188`.
</issue_to_address>
### Comment 9
<location path="src/python-type-safety/references/checker-gates.md" line_range="20" />
<code_context>
+ `disallow_any_generics`, `warn_return_any`, `warn_unused_ignores`,
+ `no_implicit_optional`, `strict_equality`.
+- **pyright baseline:** `typeCheckingMode = "standard"`, raising to `"strict"`
+ when the error budget allows. Record the mode and every overridden diagnostic
+ rule in the report.
+
</code_context>
<issue_to_address>
**Pyright gate is not strict**
When a project uses pyright and remains at the documented standard baseline, a pyright run in `standard` mode can pass without the stricter unknown-type diagnostics enabled, while the README presents the gate as strict. Users can therefore report a zero-error type-safety gate without enforcing the advertised strictness.
Make strict mode the required pyright gate, or describe standard mode explicitly as a non-strict baseline and do not present its zero-error result as a strict gate.
Also at `README.md:34`.
</issue_to_address>
### Comment 10
<location path="src/python-type-safety/SKILL.md" line_range="27" />
<code_context>
+- Name the typing target, public boundary, consumer, checker, and mode before touching
+ code. State the mode explicitly: full loop by default, read-only only when the user
+ explicitly requests no fix.
+- Prefer the project's configured checker (mypy or pyright) and its existing config
+ files. Do not silently install a checker; propose the install and ask first.
+ The second checker is a report-only cross-check; never chase divergences with
</code_context>
<issue_to_address>
**Configured checkers lack primary selection**
When a project configures both mypy and pyright without declaring which is its primary gate, the instructions treat each configured checker as the project checker but also make the second checker report-only, leaving no rule for choosing the gate when both mypy and pyright are configured. The agent can gate one arbitrarily and leave errors in the other unaddressed.
Specify how to identify the primary checker from project automation, and ask the user to choose if that does not resolve the ambiguity.
Also at `src/python-type-safety/references/checker-gates.md:8-9`.
</issue_to_address>
### Comment 11
<location path="src/python-type-safety/references/evidence-report.md" line_range="35" />
<code_context>
+- Python floor (requires-python):
+- Working directory (project-relative or redacted):
+- Environment fingerprint (runtime, OS, locale, timezone, and non-sensitive settings):
+- Baseline error count:
+- Typing budget: files / errors / checker runs / time
+- Approval status for permitted non-sensitive live, destructive, or cost-incurring work:
</code_context>
<issue_to_address>
**Final error count lacks a field**
When a user completes a typing run using the prescribed evidence-report template, the report template explicitly asks for a baseline error count but has no corresponding final-count field, while the output contract requires both. An agent following the template can omit the final count and leave the required before/after evidence incomplete.
Add an explicit final error count to the template and require it in the report contract.
Also at `src/python-type-safety/SKILL.md:109`.
</issue_to_address>
### Comment 12
<location path="src/python-type-safety/evals/cases.yaml" line_range="63" />
<code_context>
+ silenced files are wanted.
+ kind: near-miss
+ expected:
+ activates: false
+ framework_native: true
+ checker_gate_required: true
</code_context>
<issue_to_address>
**Typing request marked inactive**
When the semantic eval uses this fixture to assess the skill's activation behavior, the fixture expects the skill not to activate for a request to add annotations and gate them with checker errors. That contradicts the skill's stated trigger and rewards an agent for declining the core task rather than activating and redirecting the unsafe `Any`-silencing and report-skipping parts.
Expect activation and test that the agent redirects the silencing request while retaining the required evidence report.
</issue_to_address>
### Comment 13
<location path="src/python-type-safety/references/typing-patterns.md" line_range="21-22" />
<code_context>
+
+## Generics and overloads
+
+- **TypeVars:** bind with `bound=` where the contract names one; avoid
+ unbounded `TypeVar` on public boundaries.
+- **Overloads:** use `@overload` only when one signature cannot express the
+ boundary; keep the implementation signature compatible with every overload.
</code_context>
<issue_to_address>
**Generic APIs lose type relationships**
When a public generic API needs an unbounded `TypeVar` to preserve a type relationship, `TypeVar` without a bound is often the correct way to preserve relationships between arbitrary input and output types, as in an identity function. Following this advice can replace that relationship with `object`, `Any`, or narrower overloads, degrading inference or rejecting valid callers.
Recommend an unbounded `TypeVar` when it expresses a real input/output relationship, and use `bound=` only when the contract imposes that restriction.
</issue_to_address>
### Comment 14
<location path="CONTRIBUTING.md" line_range="34" />
<code_context>
+`SKILL.md` at its root. Keep `src/` as the only canonical skill tree; do not mirror content
+under `skills/` or commit a copy under `.agents/skills/` (a `.agents/skills/` copy is
+machine-local only, for local agent discovery and testing — refresh it with
+`cp -r src/<skill> .agents/skills/` and never commit it).
### Portable frontmatter
</code_context>
<issue_to_address>
**Local skill copy command fails**
When a contributor follows the copy instruction on a fresh checkout without an existing `.agents/` directory, `cp` cannot create the `.agents/skills/` destination when the parent `.agents/` directory is absent, so the documented refresh command fails on a clean checkout and does not create the local discovery copy.
Create the destination first, for example with `mkdir -p .agents/skills` before copying the skill.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 14 findings to address first, and this adds a new agent skill and changes the repository’s canonical skill layout and CI discovery checks. Reverting restores the repository and CI behavior, but copies of the skill may already be installed and agents could have acted on incorrect guidance, so those downstream effects would require bounded cleanup rather than being fully undone by the revert.
Blocking findings: src/python-type-safety/scripts/scan_annotations.py:151, src/python-type-safety/scripts/scan_annotations.py:205, src/python-type-safety/scripts/scan_annotations.py:95, src/python-type-safety/scripts/scan_annotations.py:181, src/python-type-safety/scripts/scan_annotations.py:73, and 9 more
Comment on lines
+150
to
+151
| f"{path}:{lineno}", | ||
| line.strip()[:200], |
There was a problem hiding this comment.
🔴 Critical · Sensitive data reaches scanner output
When a matching source line contains a secret or the supplied path is private, _scan_bare_ignores copies source text into evidence, and scan_source_files includes the supplied path in location; main prints both in its JSON output, exposing sensitive data to callers and saved reports.
Redact sensitive source text and private paths before including them in findings.
Also at src/python-type-safety/scripts/scan_annotations.py:101, src/python-type-safety/scripts/scan_annotations.py:226.
Prompt for AI agents
In `src/python-type-safety/scripts/scan_annotations.py` at lines 150-151:
**Sensitive data reaches scanner output**
When a matching source line contains a secret or the supplied path is private, `_scan_bare_ignores` copies source text into `evidence`, and `scan_source_files` includes the supplied path in `location`; `main` prints both in its JSON output, exposing sensitive data to callers and saved reports.
Redact sensitive source text and private paths before including them in findings.
Also at `src/python-type-safety/scripts/scan_annotations.py:101`, `src/python-type-safety/scripts/scan_annotations.py:226`.| """Read stdin, emit stable JSON, and return a process status.""" | ||
| _parser().parse_args() | ||
| try: | ||
| payload = json.load(sys.stdin, parse_constant=_reject_non_finite_json) |
There was a problem hiding this comment.
🟡 Medium · Large input exhausts scanner memory
When stdin contains a JSON payload larger than the per-file or file-count limits, main uses json.load to materialize all stdin before _validate_payload checks its limits, so the scanner can exhaust memory and terminate before returning an input error.
Read stdin with a byte limit and enforce an aggregate payload limit before scanning.
Also at src/python-type-safety/scripts/scan_annotations.py:13.
Prompt for AI agents
In `src/python-type-safety/scripts/scan_annotations.py` at line 205:
**Large input exhausts scanner memory**
When stdin contains a JSON payload larger than the per-file or file-count limits, `main` uses `json.load` to materialize all stdin before `_validate_payload` checks its limits, so the scanner can exhaust memory and terminate before returning an input error.
Read stdin with a byte limit and enforce an aggregate payload limit before scanning.
Also at `src/python-type-safety/scripts/scan_annotations.py:13`.
Comment on lines
+68
to
+73
| for node in ast.walk(annotation): | ||
| if isinstance(node, ast.Name) and node.id == "Any": | ||
| return True | ||
| if isinstance(node, ast.Attribute) and node.attr == "Any": | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🟡 Medium · Quoted Any escapes precision findings
When a function annotation uses a quoted forward reference such as "Any", _contains_any does not inspect string forward references, so the scanner omits the Any precision finding for annotations a checker can resolve as Any.
Recognize Any inside string forward-reference annotations.
Prompt for AI agents
In `src/python-type-safety/scripts/scan_annotations.py` at lines 68-73:
**Quoted `Any` escapes precision findings**
When a function annotation uses a quoted forward reference such as `"Any"`, `_contains_any` does not inspect string forward references, so the scanner omits the `Any` precision finding for annotations a checker can resolve as `Any`.
Recognize `Any` inside string forward-reference annotations.| - Name the typing target, public boundary, consumer, checker, and mode before touching | ||
| code. State the mode explicitly: full loop by default, read-only only when the user | ||
| explicitly requests no fix. | ||
| - Prefer the project's configured checker (mypy or pyright) and its existing config |
There was a problem hiding this comment.
🟡 Medium · Configured checkers lack primary selection
When a project configures both mypy and pyright without declaring which is its primary gate, the instructions treat each configured checker as the project checker but also make the second checker report-only, leaving no rule for choosing the gate when both mypy and pyright are configured. The agent can gate one arbitrarily and leave errors in the other unaddressed.
Specify how to identify the primary checker from project automation, and ask the user to choose if that does not resolve the ambiguity.
Also at src/python-type-safety/references/checker-gates.md:8-9.
Prompt for AI agents
In `src/python-type-safety/SKILL.md` at line 27:
**Configured checkers lack primary selection**
When a project configures both mypy and pyright without declaring which is its primary gate, the instructions treat each configured checker as the project checker but also make the second checker report-only, leaving no rule for choosing the gate when both mypy and pyright are configured. The agent can gate one arbitrarily and leave errors in the other unaddressed.
Specify how to identify the primary checker from project automation, and ask the user to choose if that does not resolve the ambiguity.
Also at `src/python-type-safety/references/checker-gates.md:8-9`.| - Python floor (requires-python): | ||
| - Working directory (project-relative or redacted): | ||
| - Environment fingerprint (runtime, OS, locale, timezone, and non-sensitive settings): | ||
| - Baseline error count: |
There was a problem hiding this comment.
🟡 Medium · Final error count lacks a field
When a user completes a typing run using the prescribed evidence-report template, the report template explicitly asks for a baseline error count but has no corresponding final-count field, while the output contract requires both. An agent following the template can omit the final count and leave the required before/after evidence incomplete.
Add an explicit final error count to the template and require it in the report contract.
Also at src/python-type-safety/SKILL.md:109.
Prompt for AI agents
In `src/python-type-safety/references/evidence-report.md` at line 35:
**Final error count lacks a field**
When a user completes a typing run using the prescribed evidence-report template, the report template explicitly asks for a baseline error count but has no corresponding final-count field, while the output contract requires both. An agent following the template can omit the final count and leave the required before/after evidence incomplete.
Add an explicit final error count to the template and require it in the report contract.
Also at `src/python-type-safety/SKILL.md:109`.| silenced files are wanted. | ||
| kind: near-miss | ||
| expected: | ||
| activates: false |
There was a problem hiding this comment.
🟡 Medium · Typing request marked inactive
When the semantic eval uses this fixture to assess the skill's activation behavior, the fixture expects the skill not to activate for a request to add annotations and gate them with checker errors. That contradicts the skill's stated trigger and rewards an agent for declining the core task rather than activating and redirecting the unsafe Any-silencing and report-skipping parts.
Expect activation and test that the agent redirects the silencing request while retaining the required evidence report.
Prompt for AI agents
In `src/python-type-safety/evals/cases.yaml` at line 63:
**Typing request marked inactive**
When the semantic eval uses this fixture to assess the skill's activation behavior, the fixture expects the skill not to activate for a request to add annotations and gate them with checker errors. That contradicts the skill's stated trigger and rewards an agent for declining the core task rather than activating and redirecting the unsafe `Any`-silencing and report-skipping parts.
Expect activation and test that the agent redirects the silencing request while retaining the required evidence report.# Conflicts: # .github/workflows/ci.yml # README.md # skills.sh.json # tests/test_quality_contracts.py # tests/test_skill_structure.py
Scanner: receiver-aware self/cls skip, signature type comments, quoted-Any forward references, AnnAssign Any detection, coverage summary counts. Docs: Never version gate, TypeVar guidance, pyright strict gate, primary-checker selection, final error count field, mkdir -p discovery copy. Declined with rationale: scanner-output redaction, stdin byte cap, near-miss activation (see PR comment).
Contributor
Author
Sourcery review triage (all 14 addressed or answered)Fixed in 5fb37cc (with regression tests in
Declined with rationale:
Also in this push: merged |
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
1 participant
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.
Summary
python-type-safetyskill: checker-gated diagnose-annotate-gate loop (mypy/pyright, report-only cross-check, boundary-first annotation, plugin refuse-or-ask gate).scan_annotations.pyscanner plus eval fixtures (incl. plugin-execution safety case) and quality-contract registration.src/restored as the canonical skill tree; tests, CI validators, discovery smoke, README, skills.sh.json, and CONTRIBUTING point at it.Test Plan
uv run --locked --group dev pytest— 444 passedruff check+ruff format --checkcleanagentskills validate src/python-type-safety— Valid skill--liston a pristine tree finds all 3 skills;--skillinstall verifiedmainpre-audit, so whoever merges second (this or the audit branch) must reconcile CI/README/skills.sh.json to all 4 skillsSummary by Sourcery
Add a Python type-safety skill and make
src/the canonical source for all repository skills.New Features:
python-type-safetyskill for boundary-first annotation, checker-gated typing workflows, cross-checker reporting, and evidence capture.Enhancements:
src/as the canonical skill tree and update repository tests, discovery checks, contributor guidance, and documentation to use it.CI:
src/paths.Documentation:
Tests:
Chores: