[ChrisRackauckas-Claude]

What and why

Three related ArrayPartition indexing paths assumed every partition (and
every copyto! destination) is 1-based, and silently compute the wrong
linear offset for a partition with non-1-based indices (e.g. an
OffsetArray):

1. getindex/setindex!(::ArrayPartition, i::Int) (src/array_partition.jl:308,337)
   compute the within-partition position as length(x) + i, which is only
   correct when firstindex(x) == 1. Fixed to
   firstindex(x) + length(x) + i - 1.
2. copyto!(dest::AbstractArray, A::ArrayPartition) (:238) always starts
   writing dest at linear index 1. Fixed to start at firstindex(dest).
3. The same method's inner per-partition assignment,
   dest[cur:(cur+length(x)-1)] .= vec(x), additionally breaks for a
   non-1-based partition x: vec on an OffsetArray preserves its offset
   axes, so the dot-assignment's axis check fails against the (always
   1-based) destination range. Switched to the 3-arg
   copyto!(dest, cur, x), which copies by position regardless of x's own
   axes.

@inbounds is kept in both getindex/setindex!; the fix stays inside the
existing bounds-checking structure (@boundscheck checkbounds(A, i) still
runs first).

Correctness: failing-before / passing-after

Reused the audit's probes (scratch/probes/p_ap_offset_getindex.jl,
p_ap_offset_setindex.jl, p_ap_copyto_offsetdest.jl).

getindex, ArrayPartition([1.0, 2.0], OffsetArray([10.0,20.0,30.0], 0:2)),
reading A[3], A[4], A[5] (expected 10.0, 20.0, 30.0):

• Before: (20.0, 30.0, 6.9412553390231e-310) (garbage from reading one
  element past the OffsetArray's parent, i.e. silently wrong, plus
  undefined memory on the last read).
• After: (10.0, 20.0, 30.0).

setindex!, A[5] = 99.0 into
ArrayPartition([1.0, 2.0], OffsetArray(zeros(3), 0:2)):

• Before: parent(o) = [0.0, 0.0, 0.0] under default --check-bounds=auto
  (write landed one slot past the end of o's backing Vector, so it's
  invisible via parent, but is an out-of-bounds write — confirmed it
  throws BoundsError under --check-bounds=yes).
• After: parent(o) = [0.0, 0.0, 99.0], and --check-bounds=yes passes.

copyto!, copyto!(OffsetArray(zeros(4), 0:3), ArrayPartition([1.0,2.0],[3.0,4.0])):

• Before: parent(dest) = [0.0, 1.0, 2.0, 3.0] (everything shifted one slot;
  4.0 dropped). Throws BoundsError under --check-bounds=yes.
• After: parent(dest) = [1.0, 2.0, 3.0, 4.0], and --check-bounds=yes passes.

All three re-confirmed on Julia 1.12.4 and 1.10.12, with and without
--check-bounds=yes.

Also checked collect(A) for an ArrayPartition with an OffsetArray
partition (exercises the copyto!(dest::AbstractArray, A::ArrayPartition)
path via Base.collect/_collect_indices), which the first version of this
fix (changing only the starting index, not the inner assignment) still broke
with DimensionMismatch: arrays could not be broadcast to a common size
under the dest[range] .= vec(x) form; collect(A) == [1.0, 2.0, 10.0, 20.0, 30.0]
now.

Normal (non-offset, same-size, mismatched-size, scalar-partition) indexing
and copyto! behavior is unchanged — re-ran scratch/probes/p_ap_mismatch.jl
and got byte-identical output before/after.

Performance

Candidate 13 in the audit noted a possible +2.5%, within noise cost for the
getindex/setindex! change. Measured directly (Julia 1.12.4 and 1.10.12,
@benchmark 5000 samples, ArrayPartition(rand(1000), rand(1000)),
reading/writing index 1500):

	before	after
1.12.4 getindex	5.64 ns	5.64 ns
1.12.4 setindex!	4.43 ns	4.43 ns
1.10.12 getindex	7.63 ns	7.64 ns
1.10.12 setindex!	4.83 ns	4.83 ns

No measurable difference (firstindex on a Vector is a compile-time
constant 1, so the extra term constant-folds away).

Test results

Added test/Core/partitions_test.jl::"OffsetArray partitions index correctly",
using OffsetArrays.jl (newly added to [extras]/[targets]/[compat] in
Project.toml, as this package had no existing OffsetArrays test
dependency).

GROUP=Core: all 12 Core testsets pass (Partitions Tests: 135 Pass, 0 Fail).
GROUP=QA: QA: 20 Pass, 0 Fail.

Both re-run on Julia 1.12.4 and 1.10.12.

What was not verified

• Downstream, GPU, AD, and NoPre (JET) test groups were not run.
• OffsetArray support elsewhere in the package (vector_of_array.jl) is a
  separate, already-known issue (candidate 16/18 in the audit, ragged
  VectorOfArray + OffsetArray inner arrays) and is out of scope here.
• Did not check PermutedDimsArray destinations specifically (the same
  for type in [AbstractArray, PermutedDimsArray] loop defines both methods,
  so the fix applies to both, but the regression test only exercises
  OffsetArray/Vector).

What a reviewer should push back on

Whether copyto!(dest, cur, x) (3-arg) is the right general replacement for
dest[range] .= vec(x) for every x type this loop can see (plain Array,
OffsetArray, and whatever other AbstractArray partitions appear in
practice) — it relies on copyto!'s own linear-index contract for src
rather than broadcasting, which should be strictly more permissive, but is
worth double-checking against any partition type with unusual iteration
order.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

• Risk: low
• Blast radius: src/array_partition.jl, three methods
  (getindex/setindex!(::ArrayPartition, ::Int),
  copyto!(dest::AbstractArray/PermutedDimsArray, ::ArrayPartition)); no
  signature or public API change. Only changes behavior for non-1-based
  partitions/destinations, which previously silently corrupted data or threw
  under --check-bounds=yes; 1-based (the common case) is unaffected.
• Evidence: failing-before/passing-after on three probes plus a collect
  check, both with and without --check-bounds=yes, on Julia 1.12.4 and
  1.10.12; no performance change; Core and QA test groups pass.
• Independent review: pending
• Merge: auto-merge candidate, pending independent review

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #672 (comment)

[ChrisRackauckas]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

Reviewed head ef22751f7fb4cde51f5d1043422b50f7a080eb53 (matches gh pr view 672 --json headRefOid). The PR links no issue. The closest are #28 (OffsetArrays support, from 2018) and #675 (VectorOfArray with OffsetArray inner arrays, which the PR leaves out of scope). There is no root-level AGENTS.md or CLAUDE.md in the repo, so I applied the user-level CLAUDE.md rules.

Blocking findings

None.

All three changes are correct for every case I tested:

• getindex and setindex! now compute firstindex(x) + length(x) + i - 1.
• copyto! now starts at cur = firstindex(dest).
• The inner per-partition copy is now copyto!(dest, cur, x).

firstindex is the right choice even for N-d OffsetArray partitions, whose linear indices are 1-based. I confirmed this by running the probe: the 2-d offset partition gives the same result before and after the change. The new test's expected values are literals worked out by hand (10.0, 20.0, 30.0, [1,2,3,4]), not copied from the code's output. Without the fix the test fails, and with it the test passes; details below.

Non-blocking findings

1. The test is narrower than the fix (confirmed by running). test/Core/partitions_test.jl:420-441 does not cover:

   • an offset destination together with an offset partition;
   • a Number partition with an offset destination (on master this misplaces data, and throws BoundsError under --check-bounds=yes);
   • an empty partition followed by an offset partition (on master [1.0, 0.0] instead of [1.0, 2.0]);
   • an N-d OffsetArray partition;
   • a PermutedDimsArray destination.

   My probe shows the PR handles all of these correctly. Adding them in a loop would lock that in.

2. The comment in the test is written about the old behaviour (read, rule violation). test/Core/partitions_test.jl:421-423 says "...assumed every partition ... were 1-based, silently reading/writing the wrong element". That describes the bug the PR replaces, which the user-level CLAUDE.md "describe what exists" rule forbids. The PR adds 6 comment lines out of 38 added lines (about 16%, against a ~10% guideline). The 3-line comment in src/array_partition.jl:246-248 is acceptable because it explains why 3-arg copyto! is used.

3. Merge conflict with Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 (confirmed with gh pr diff 670). Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 also appends a testset at test/Core/partitions_test.jl hunk @@ -416,3 +416,67 @@. Whichever lands second will hit a textual conflict. It is trivial to resolve.

4. Inconsistent with sibling PRs on the new test dependency. This PR adds OffsetArrays (MIT, test-only, [extras]/[targets]/[compat] = "1.17"). Siblings Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667 and Add O(1) fast path to AbstractVectorOfArray getindex #668 deliberately hand-roll a minimal OffsetVec "avoiding an OffsetArrays dependency". Either approach is acceptable, but the repo should settle on one. The PR body does not state the license of the new dependency, which CLAUDE.md asks for.

5. The PR body understates the master bug (confirmed by running). The body says the out-of-bounds setindex! write is "invisible via parent". In fact, running the PR's own new testset against master with the default bounds checking crashed Julia 1.12 with signal 11 (Segmentation fault) during GC (scratch/base_newtest.txt), which points to heap corruption. This makes the fix more important than the body suggests.

6. PR body problems (read).

   • It cites "the audit's probes (scratch/probes/*.jl)" and "candidate 13/16/18", which are not in the repo and not linked, so readers cannot check them.
   • It does not open with the UNREVIEWED banner CLAUDE.md requires as its first line (there is only a "Please ignore until reviewed" line partway down), and it ends with a "Generated with Claude Code" line plus a session link instead of the footer format.
   • The commit trailers repeat Co-Authored-By: Claude and Claude Sonnet 5.

   Process only; none of this affects the code.

7. Possible downstream risk (suspected from reading, not observed). The inner copy changed from a broadcast (dest[r] .= vec(x)) to 3-arg copyto!, which goes through Base's generic linear-index 5-arg copyto! for anything that is not an Array or a GPUArray. A partition type that supports broadcasting but has slow or unsupported scalar getindex would regress. I found no such case:

   • JLArray (with scalar indexing disallowed), views, reshapes, sparse, SArray/SMatrix, Adjoint, ranges and nested ArrayPartition all give the same results.
   • One case improved: copyto!(::Vector, ArrayPartition(JLArray...)) errored with "Scalar indexing is disallowed" on master and works now.
   • CUDA was not tested.

What I ran

Setup: TMPDIR was set to the review tmp/, and all scratch work went in scratch/. The checkout was never modified; git status is clean. Before-fix runs used a copy, scratch/base, with src/ taken from origin/master. Test groups ran in a copy, scratch/pr.

• Probe script scratch/probe.jl (47 cases). I ran it on master and on the PR, each under the default bounds checking and under --check-bounds=yes, on Julia 1.12; output is in scratch/probe_out.txt and part{1..4}.txt.

  • Before → after: offset1d getindex goes from [1,2,20,30,7.1e-322] to [1,2,10,20,30].
  • offset1d collect goes from DimensionMismatch to correct.
  • setindex! goes from [0,7,0] (the write to index 5 went out of bounds) to [7,0,9].
  • Offset-destination copyto! goes from [0,1,2,3] to [1,2,3,4].
  • Offset destination with an offset partition, a Number partition with an offset destination, and an empty partition followed by an offset partition are now all correct.
  • Every other case is byte-identical before and after: 1-based baseline, N-d offset, eltype conversion, InexactError still raised, AssertionError on length mismatch, BoundsError at index 0 and length+1, and PermutedDimsArray, view and strided-view destinations.
  • PR output is identical with and without --check-bounds=yes.

• Same probe on Julia 1.10.12 under --check-bounds=yes against the PR checkout: identical to 1.12.

• The PR's new testset, extracted (scratch/newtest.jl):

  • master, default bounds checking: segfault;
  • master, --check-bounds=yes: Fail 2, Error 3;
  • PR on 1.12: Pass 7;
  • PR on 1.10: Pass 7.

• GROUP=Core Pkg.test() on Julia 1.12 (PR copy): all 12 testsets pass, including Partitions Tests 135/135. VecOfArr Indexing shows 2 Broken; those @test_broken markers are in a file the PR does not touch.

• GROUP=QA on Julia 1.12: QA 20/20.

• Runic --check on the two changed .jl files: clean. typos on the diff: clean.

• Micro-benchmark (scratch/bench.jl, best of 200, two rounds each):

  • copyto! 2x1000: 294 → 291 ns
  • copyto! 4x3: 61 → 54 ns
  • mixed eltype: 286 → 278 ns
  • getindex: unchanged

  No regression.

• Sibling-PR overlap: gh pr diff for Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667–Fix method ambiguity in AbstractVectorOfArray similar() breaking QA on Julia 1.10 #678.

What I did not verify

• CUDA, AD and Downstream test groups: no GPU here, and I limited runs to the changed code. GPUArrays behaviour was checked only through JLArrays.
• GROUP=Core on Julia 1.10 (only the probes and the new testset were run on 1.10), and the Downgrade lane (OffsetArrays 1.17.0 was what resolved locally, so the compat floor was exercised).
• The PR body's @benchmark numbers. My own timing shows no regression, but it was not a reproduction of their setup.

---

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

[ChrisRackauckas]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Merged the default branch into this PR to resolve conflicts (head c4d4523). Re-review the resolution before merging.

Conflict resolution report

STATUS: RESOLVED

PR: #672 (branch fix/ap-offsetarray-indexing)

Conflicting files

• test/Core/partitions_test.jl: both sides appended a new testset at the end of the file. The PR added
  "OffsetArray partitions index correctly". Master added "Mixed-eltype in-place broadcast does not allocate"
  (from Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666, commit 1826933). Kept both testsets, in that
  order, each with its own end. The PR's using ..., OffsetArrays import merged cleanly. Runic made no changes.

All other files auto-merged. I checked src/array_partition.jl: the PR's fixes are present
(cur = firstindex(dest), copyto!(dest, cur, A.x[i]), and firstindex(x) + length(x) + i - 1 in
getindex/setindex!), and so is master's @inline unpack_args(i, args::Tuple{Any}).
The PR's OffsetArrays additions to Project.toml ([compat]/[extras]/[targets]) are intact.

Tests (Julia 1.12.7, default juliaup channel)

julia --project -e 'using Pkg; Pkg.instantiate()'
# scratch env = repo (dev) + lib/RecursiveArrayToolsShorthandConstructors (dev) + Test, Statistics, ArrayInterface, Adapt, OffsetArrays
julia --project=scratch/testenv -e 'using Test; @testset "Partitions Tests" include("test/Core/partitions_test.jl")'

Result: Partitions Tests | 136 Pass, 136 Total (0 fail, 0 error). That count covers the PR's OffsetArray
testset and master's allocation testset. I did not run the full Core, QA, or other groups.

---

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (merge), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

[ChrisRackauckas-Claude]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Codex CLI 0.166.0 / gpt-6.1-sol), session 01a19787-e19a-7fd3-8f3f-eaf27fe65d10, reviewed head 8716769

• Verdict: MERGE-WITH-ISSUES
• Risk: medium-high (MH1: getindex/setindex! and nonscalar copy dispatch change on the ArrayPartition main lane; offset inputs gain correct behavior.)
• Round / slot: 1 / 1
• Blocking priorities: none

Review written by the reviewer named above in a session other than the PR author's, and copied here by the posting agent without change to its findings. The posting agent did not re-run the reviewer's commands.

Summary: Corrects offset-index handling in ArrayPartition scalar indexing, assignment, and copying. The focused regression file fails on the merge-base and passes on head; no blocking defect was found, with one comment-hygiene suggestion.

Blast radius: ArrayPartition linear reads/writes and copyto! into AbstractArray or PermutedDimsArray destinations; ordinary arrays, offset arrays, and partition-backed downstream solver state.

Evidence: Blind assessment recorded and hashed before author context. Julia 1.13.2 focused partition tests: head 136/136, exit 0; identical head test file against merge-base 130 pass, 2 fail, 2 error, exit 1. Core, AD, GPU, and package Downstream CI pass. The failing SciMLSensitivity Core4 job has the identical Enzyme assertion in a completed master run. The additional reviewer probe has an invalid destination-comparison assertion, detailed below.

Blocking findings

None.

Non-blocking findings

1. [P3] Comment hygiene at test/Core/partitions_test.jl:421 and src/array_partition.jl:246 (suspected from reading): the test comment narrates historical assumptions, and six comment lines among 38 added lines exceed the rubric's approximate 10% guideline. Shorten the comments and state the present contract, such as “Check positional indexing and copying with offset axes.” This does not affect correctness.

What I ran

All three Julia processes ran sequentially with Julia 1.13.2, four Julia threads, one OpenBLAS thread, an isolated temporary directory, and the supplied writable depot followed by the default depots. No package suite or Pkg.test() was run. Environment paths below are represented by variables to avoid machine-specific paths.

gh pr view 672 --repo SciML/RecursiveArrayTools.jl --json number,url,state,isDraft,headRefOid,baseRefName,baseRefOid
git -C src rev-parse HEAD
git -C src fetch origin '+refs/pull/672/head:refs/remotes/pr/672' master
git -C src diff origin/master...HEAD
git -C src merge-base origin/master HEAD
git -C src diff 76bec79f7a09ce9546709376a0d7eb95deaf90f9...HEAD --numstat
sha256sum INITIAL.md > INITIAL.sha256
gh pr view 672 --repo SciML/RecursiveArrayTools.jl --json title,body,statusCheckRollup
gh api --paginate repos/SciML/RecursiveArrayTools.jl/pulls/672/commits --jq '.[] | {sha: .sha, parents: [.parents[].sha], message: .commit.message}'
gh pr checks 672 --repo SciML/RecursiveArrayTools.jl
gh run list --repo SciML/RecursiveArrayTools.jl --branch master --event push --limit 5 --json databaseId,workflowName,conclusion,headSha,url
gh run list --repo SciML/RecursiveArrayTools.jl --branch master --event push --workflow IntegrationTest --limit 5 --json databaseId,conclusion,headSha,url
XDG_CACHE_HOME="$REVIEW_CACHE" gh run view 38872096841 --repo SciML/RecursiveArrayTools.jl --log-failed
XDG_CACHE_HOME="$REVIEW_CACHE" gh run view 41050637371 --repo SciML/RecursiveArrayTools.jl --log-failed

Git/API/log retrieval commands exited 0; gh pr checks exited 1 for the failing job. Initial log retrieval could not write the default read-only cache; a writable isolated cache allowed retrieval. The merge-base equals the API's base SHA above; the local origin/master tracking ref is newer, at 6dfb88ba7b0efc56deb7da26d75388d95955c2ff. The head equals the live target SHA.

The focused driver activates a separate temporary environment for each checkout, develops RecursiveArrayTools and its local ShorthandConstructors sublibrary, adds OffsetArrays/Statistics/ArrayInterface/Adapt, and includes the head's unchanged test/Core/partitions_test.jl inside a testset for both runs.

JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 JULIA_PKG_PRECOMPILE_AUTO=0 TMPDIR="$REVIEW_TMP" JULIA_DEPOT_PATH="$SHARED_DEPOT:" timeout 3600 julia +1.13.2 --startup-file=no --check-bounds=yes validation/focused.jl head
JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 JULIA_PKG_PRECOMPILE_AUTO=0 TMPDIR="$REVIEW_TMP" JULIA_DEPOT_PATH="$SHARED_DEPOT:" timeout 3600 julia +1.13.2 --startup-file=no --check-bounds=yes validation/focused.jl base
JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 JULIA_PKG_PRECOMPILE_AUTO=0 TMPDIR="$REVIEW_TMP" JULIA_DEPOT_PATH="$SHARED_DEPOT:" timeout 3600 julia +1.13.2 --startup-file=no --check-bounds=yes --project=env-head validation/probe.jl

Head, exit 0: Focused partitions (head) | 136 136. Base, exit 1: Focused partitions (base) | 130 2 2 134. The added offset testset passes all seven assertions on head. On base, A[3] returns 20 instead of 10, A[4] returns 30 instead of 20, A[5] throws BoundsError, and the subsequent B[5] = 99 throws BoundsError. The base run therefore does not reach the offset-destination assertion. Existing assertions outside the new testset pass in both runs. The new expected values are literal positional values independent of the implementation.

Final probe, exit 1: Independent offset/shape/copy probes | 226 73 299, zero errors. It covers negative/zero/positive offsets, empty and zero-dimensional partitions, scalars, nested partitions, matrices, permuted matrices, views, offset destinations, writes, and bounds. All 73 failures occur in the reviewer's assertion [v for v in dest] == expected: that comprehension preserves destination axes/shape, whereas expected is a plain vector. Thus these failures do not establish a package defect. The other 226 assertions pass, including reads, writes, bounds, and copy return identity. I did not rerun or claim a clean passing probe.

Inspected PR CI, PR IntegrationTest, and master IntegrationTest. In both integration logs, SciMLSensitivity Core4's EnsembleThreads test at test/Core4/ensembles.jl:19 raises AssertionError: mi2 !== nothing in Enzyme's src/rules/parallelrules.jl:233. This exact failure is independently confirmed on master, rather than inferred from a red workflow.

Author trailers come from ef22751f7fb4cde51f5d1043422b50f7a080eb53, the last single-parent commit in the PR commits API response: Agent-Harness: Claude Code 2.1.294 and Agent-Model: claude-sonnet-5.

What I did not verify

I did not independently run GPU, AD, downstream solver suites, benchmarks, QA, other Julia versions, or other platforms; CI exercises GPU and AD. No AD rules or GPU kernels are changed. The base regression testset aborts at setindex!, so offset-destination failing-before was not separately established. The third run's shape/axes mistake prevents an all-pass independent copy-content result for its multidimensional and offset destinations; I did not repair and rerun it because of the three-run limit. I did not reproduce the downstream Enzyme failure locally. No prior review file was read and no upstream write was made.

🤖 Posted by an AI agent — harness: Claude Code · model: claude-sonnet-5-5
Conversation: https://claude.ai/code/session_01379heiiB35TtywhR7uSU74