timholy/claude_config/review-integration

Review the Julia package in the current working directory for issues that the structured /review-design and /review-api skills are designed not to surface. Those skills work from distilled artifacts (a conceptual map, an API inventory) and explicitly assume the code is correct and idiomatic. This skill is the deliberate inverse: read source, tests, and documentation directly, and look for things that only become visible inside the implementation and at the package's seams with the outside world.

Three focus areas:

1. Dependency-interface compliance — for each declared dependency, is the package using that dependency's public API correctly and idiomatically? Does the package implement the interfaces it claims to support (Tables, AbstractTrees, AbstractArrays, Iterator protocol, etc.) completely and consistently?
2. Latent bugs and suspicious code — paths that look wrong, error handling that masks failures, off-by-ones, type instabilities that change semantics (not just performance), tests that pass for the wrong reason.
3. User friction — the kind of paper cuts that don't violate any convention but make the package annoying to use: bad error messages, missing convenience constructors, common tasks that take N steps when they should take one, undocumented preconditions, examples that don't run.

This skill produces a tiered report and an INTEGRATION_REVIEW_PLAN.md that decomposes the agreed-upon findings into chunks consumed by the companion /review-implement skill. Do not implement any changes in this skill — that is /review-implement's job.

The package module name is determined from Project.toml.

Phase 0 — Size preflight

This skill reads source/tests/docs into the main context (unlike /review-design and /review-api, which delegate the bulk read to a distillation subagent). Cost therefore scales with package size, and depth of analysis degrades when the model is asked to read too much in one pass.

Measure the total character count across src/, ext/, test/, docs/src/, README.md, and any examples/ directory:

find src ext test docs/src examples -type f \( -name '*.jl' -o -name '*.md' \) 2>/dev/null | xargs wc -c 2>/dev/null | tail -1
# plus README.md

Apply the attention ceiling (window-independent — reflects how much source the model can read carefully in one pass, not how much fits):

• Under ~250K chars total: proceed to Phase 1 as written.
• ~250K–500K chars: surface the size to the user and offer a strategy menu before proceeding:
• (a) Restrict scope to one or two of 2a–2f (which areas matter most to you?).
• (b) Restrict scope to a subdirectory of src/ (e.g., one major submodule).
• (c) Use an Explore-style subagent for targeted queries per check (loses the "one careful reading pass" benefit but preserves coverage).
• (d) Proceed anyway with the understanding that depth will suffer — acceptable for a first-pass scan, not for a thorough review.
• Above ~500K chars: refuse the default mode. Require the user to pick (a), (b), or (c). Do not pretend to do the full review.

These thresholds are about attention, not context capacity: a large context window lets more source fit, but it does not make one careful reading pass of 500K characters any more reliable. If the user wants to push past them, that is their judgment call, and the report should say so.

Record the measurement and the chosen strategy in one line at the top of the eventual report (e.g., "Phase 0: 312K chars total; user chose strategy (a), focusing on 2a and 2c").

Phase 1 — Read directly

Do not invoke pkg-conceptual-mapper or julia-api-inventory — they would defeat the purpose. Instead, read directly:

1. Project.toml — note every [deps] entry and its declared [compat] bound. These are the dependencies whose interfaces the package consumes.
2. All files in src/. Read implementations, not just signatures. Note using/import statements per file — which dependency-provided symbols are actually used where.
3. test/ — the test suite is documentation of intended behavior and a good place to spot tests that pass for the wrong reason.
4. README.md, docs/src/, and any examples/ — the user-facing surface. Examples that no longer run, or that demonstrate awkward idioms, are friction.

For large packages, prioritize: files most central to the package's purpose first, then peripheral utilities. If a file is clearly machine-generated or vendored, note it and move on.

Phase 2 — Checks by focus area

2a. Dependency-interface compliance

Out of scope — delegated to deterministic tooling, do not duplicate:

• "Does the package import non-public symbols from a dependency?" → ExplicitImports.jl (/freshen-explicit-imports) catches this reliably across using/import and qualified-access forms.
• "Are there declared dependencies no longer used in source?" → Aqua.jl's test_stale_deps (/freshen-aqua) catches this.

If the user has not yet run those skills on this package, mention it in the report's preamble and recommend running them — but do not attempt the same analysis by hand in this skill. Doing so produces noisier, less reliable findings than the tools, and wastes the reading-pass budget on a problem that is already solved.

In scope — qualitative dep-usage questions tooling cannot answer. For each [deps] entry, identify what kind of dependency it is and apply the relevant checks:

• Interface packages (Tables, AbstractTrees, ArrayInterface, SciMLBase, MLJModelInterface, CommonSolve, StatsAPI, etc.): does the package implement the full interface required for its types to function with the broader ecosystem? E.g., a "Tables-compatible" type that defines Tables.istable and Tables.rows but not Tables.schema will silently fail in some downstream consumers. Look up the actual interface contract — do not guess.
• Generic programming via interfaces and traits, vs representation-dependent code. This is the most valuable check in 2a and the one most worth thinking carefully about. Look for places where the package writes hand-rolled methods that depend on a concrete representation when a generic interface would do. Symptoms:
• for i in 1:length(v); v[i]; ... instead of for x in v or for i in eachindex(v). The explicit-index form silently breaks for OffsetArrays, sparse iteration, generators, etc.
• if A isa Matrix ... else ... end branches that could be handled by dispatching on AbstractMatrix or a trait (IndexStyle, ArrayInterface.parent_type).
• Direct field access (x.foo) on a type owned by another package, when an accessor function exists. Field layouts are not API.
• Methods specialized on Vector{T} / Matrix{T} / Array when the operation works for any AbstractArray and would benefit from being callable with views, GPU arrays, sparse arrays, dual numbers, etc. (See the user's CLAUDE.md guidance on argument annotations — overly-restrictive types silently exclude valid inputs.)
• Hand-coded property dispatch (Symbol-keyed if/elseif) where multiple dispatch on a wrapper type would be more idiomatic.
• Numerical/algorithmic packages (LinearAlgebra, SparseArrays, Statistics, FFTW): is the package using the right entry point? Hand-rolled loops where a Base/stdlib function would do are often less stable as well as slower. Conversely: is the package using a high-level entry point when a lower-level one would compose better with the caller's chosen factorization or storage format?
• Container/iterator packages (DataStructures, OrderedCollections, IterTools): is the package using public constructors and accessors? (Internal-field access is caught by ExplicitImports for qualified accesses, but unqualified field-access on a value of a dep type — e.g., x.internal_field after x = SomeDep.Foo() — is not always caught and is worth a manual check.)
• Logging, Printf, Dates, Random: standard pitfalls — println in library code (instead of @info/@debug or writing to a passed IO), rand() without a passed RNG (breaks reproducibility for callers), now() instead of Dates.now(UTC) for stored timestamps, etc.

Also check [compat] bounds: is the lower bound of a dependency so old that the package is forced to avoid newer, better APIs? (The inverse — too tight upper bounds — is a separate maintenance question, not an integration concern.)

2b. Implementations of standard Julia interfaces

Independent of declared dependencies, check the package's own types against the implicit interfaces of Base:

• Iterator protocol: if iterate is defined, are length/size defined when knowable, and IteratorSize / IteratorEltype traits set when the defaults are wrong?
• Indexing: if getindex is defined, are firstindex/lastindex defined? Does axes return something sensible for eachindex to consume?
• AbstractArray subtypes: size, getindex, setindex! (if mutable), IndexStyle — and do operations like similar, copy, broadcasting actually work, or do they fall through to a wrong default?
• == defined without matching hash (Base's == / hash contract — review-design checks this from the map; here, look for it inside src/).
• show methods: does show(io, ::MIME"text/plain", x) exist for types a user will actually see in the REPL? Does show(io, x) produce something parseable or at least round-trippable?
• convert and constructor methods that return the wrong type: as long as T is concrete, the contract should almost always be convert(::Type{T}, x)::T (i.e., it should return a T). Example: convert(Vector{Real}, list) should never return a Vector{Int} even if list contains nothing but Ints. However, convert(Real, x) can return an Int since all instances must have concrete type and Real is not concrete.

2c. Latent bugs and suspicious code

Read with a skeptical eye. Things to flag:

• Bare catch clauses that swallow exceptions, or catch e blocks that log-and-continue when the operation cannot meaningfully proceed.
• try blocks with no finally around a resource (file handle, lock) that needs deterministic cleanup.
• Floating-point comparisons with == where ≈ / isapprox is intended.
• Off-by-one risks at boundaries (1:length(x) where eachindex(x) would be safer; for i in 1:n-1 patterns).
• Type-unstable code where the instability changes semantics, not just performance — e.g., a function that sometimes returns Int and sometimes Vector{Int} depending on input.
• Mutation of a shared default argument value (function f(x, buf=Float64[]) — every caller shares the same buffer).
• Tests that assert nothing meaningful: @test foo(x) !== nothing, @test_nowarn around a function that never warns anyway, tests that would pass even if the implementation were return input.
• @assert used for input validation (gets compiled out under --check-bounds=no in some settings — throw(ArgumentError(...)) is the right tool).
• eval / @eval at runtime in performance-sensitive paths.
• World-age hazards: @eval followed by an immediate call within the same function.

For each, name the file and line, quote the relevant snippet, and explain the failure mode. Distinguish "definitely a bug" from "smells like a bug, worth a closer look."

2d. User friction

Read the README and examples as if you have never seen this package before. For each common task the package supports, count the steps. Flag: