|
| 1 | +--- |
| 2 | +name: scrutinise |
| 3 | +description: Scrutinise newly added or changed code on the current branch against main. Checks new types, methods, changed signatures, overloads, and helpers for necessity, correctness, clarity, consistency, robustness, and minimality. Reviews new tests for gap coverage, overlap with existing tests, minimality, and use of established testing patterns. Invoke with /scrutinise. |
| 4 | +tools: Bash, Glob, Grep, Read, Edit, Write |
| 5 | +--- |
| 6 | + |
| 7 | +# Scrutinise |
| 8 | + |
| 9 | +Review all changes on the current branch relative to `main`. Cover source and tests separately, then simplify. |
| 10 | + |
| 11 | +## Step 1: Gather the diff |
| 12 | + |
| 13 | +```bash |
| 14 | +git diff main...HEAD --name-only |
| 15 | +git diff main...HEAD -- src/ ext/ test/ |
| 16 | +``` |
| 17 | + |
| 18 | +Read changed files in full before commenting. |
| 19 | + |
| 20 | +## Step 2: Source |
| 21 | + |
| 22 | +For every new type, method, changed signature, or overload: |
| 23 | + |
| 24 | + - **Necessary?** Does existing infrastructure (`@zero_derivative`, `@from_rrule`, broader signatures) already cover this? Could an overload be eliminated by broadening an existing one? |
| 25 | + - **Correct?** Tangent/cotangent types consistent with `tangent_type`? `@is_primitive` declared? For `rrule!!`: pullback restores mutations, aliasing handled. For `frule!!`: dual propagation correct, removable singularities handled. |
| 26 | + - **Clear and consistent?** Names and structure match the surrounding file and `src/rules/`. `NoTangent`/`ZeroTangent` used correctly. |
| 27 | + - **Robust?** Edge cases (empty arrays, zero-size structs, complex types) handled or explicitly excluded. Fails loudly on unsupported inputs. |
| 28 | + - **Minimal?** No dead branches, unused arguments, or speculative generalisations. |
| 29 | + |
| 30 | +For every new helper: does it genuinely aid readability or reduce duplication, or can it be inlined? Does it belong in `src/utils.jl` or is it rule-local? |
| 31 | + |
| 32 | +For every new or changed **comment**: |
| 33 | + |
| 34 | + - **WHY not WHAT?** Delete comments that restate what the code already says (variable names, types, control flow). Keep only non-obvious constraints, invariants, and design rationale. |
| 35 | + - **Accurate?** Does the comment still match the code? Stale or contradictory comments are worse than none. |
| 36 | + - **Brief?** Trim verbose multi-line blocks to the minimum that preserves the WHY. Cross-references (`see X for WHY`) are fine but the local comment should still give enough context to understand the constraint without chasing the reference. |
| 37 | + |
| 38 | +For every new or changed **docstring**: |
| 39 | + |
| 40 | + - **Correct?** Does it accurately describe current behaviour, including any overloads (e.g. `Ptr` special cases)? |
| 41 | + - **No leaking internals?** Docstrings are public-facing; do not refer users to internal comments or implementation details they cannot rely on. |
| 42 | + - **Concise?** One sentence for simple functions; a short paragraph for complex ones. Avoid restating the signature. |
| 43 | + |
| 44 | +## Step 3: Tests |
| 45 | + |
| 46 | +For every new or changed test: |
| 47 | + |
| 48 | + - **Real gap?** Would removing it leave a regression undetected, or is it duplicating interpreter-level coverage via `TestResources.generate_test_functions()`? |
| 49 | + - **No overlap?** Check the corresponding test file and `test/front_matter.jl` for existing tests on the same rule/type. |
| 50 | + - **Minimal?** Smallest example that exercises the gap; no redundant argument combinations. |
| 51 | + - **Right pattern?** Rules → `test_rule`. Tangents → `test_tangent` / `test_tangent_type_and_tglob_type_agree`. Duals → `test_dual` / `test_fdata` / `test_rdata`. Allocations → `count_allocs`. Malformed rules → `DebugMode`. Flag any test reimplementing logic already in the test utilities. |
| 52 | + |
| 53 | +## Step 4: Output |
| 54 | + |
| 55 | +Findings grouped by file, labelled: |
| 56 | + |
| 57 | + - **Unnecessary** / **Incorrect** / **Unclear** / **Inconsistent** / **Non-minimal** / **Fragile** |
| 58 | + - **Comment: stale** / **Comment: explains WHAT** / **Comment: too verbose** / **Comment: missing WHY** |
| 59 | + - **Docstring: incorrect** / **Docstring: leaks internals** / **Docstring: too verbose** |
| 60 | + - **Test: redundant** / **Test: missing pattern** / **Test: weak gap** |
| 61 | + |
| 62 | +No issues in a section → write "No issues." Do not suggest additions beyond what the diff introduces. |
| 63 | + |
| 64 | +## Step 5: Simplify |
| 65 | + |
| 66 | +Invoke the `simplify` skill to apply code-quality and reuse fixes to the changed files. |
0 commit comments