Add RFC 6902 JSON Patch and structural equality for JSON

[carpentry-agent]

json has shipped RFC 6901 JSON Pointer since #12, but nothing that writes
through one. JSON Patch is the standard wire format for expressing a change to
a document (HTTP PATCH, Kubernetes, most JSON APIs), and Pointer was already
the hard half.

JSON.=

JSON had no = at all: (= &a &b) on two JSON values did not typecheck.
JSON.= compares structurally, arrays by length and order, objects by key set
independent of member order, numbers by value. Num holds a Double, so 1
and 1.0 are the same JSON value; RFC 6902 §4.6 compares numbers by value, so
that is the intended reading and test depends on it.

JSON.Patch

(JSON.Patch.apply &doc &patch) applies a patch document, a JSON array of
operation objects, and returns a (Result JSON PatchError). All six operations
are implemented, and the spec-mandated asymmetries are what most of the tests
pin down:

• add inserts into an array, shifting the rest right, with - appending;
  into an object it inserts or replaces a member.
• replace and remove require the location to already exist, where add
  would have created it.
• move may not move a location into one of its own children (§4.4). This
  compares decoded pointer tokens, so /a/b is a child of /a but /ab is
  not, and moving /a to /a is allowed.
• copy carries no such restriction (§4.5), so copying a subtree under itself
  and copying out of the empty pointer are both permitted.
• test compares with JSON.=.
• Unrecognized operation members are ignored.

A failure returns the index of the operation that failed, and application
is atomic: apply borrows the document and threads a copy through the
operations, so a patch that fails halfway leaves the caller's document
untouched. There is an explicit test for that.

JSON.Pointer.array-index changes from private to public and documented:
Patch needs the RFC 6901 index rules, rejecting - and leading zeros, to
decide whether an array token is an index.

Resource limits

Patch is the one place in this module where a small input buys an unbounded
amount of work, so it carries two limits. Neither binds on a patch written by
hand, and both were added in response to crashes reachable from a patch
document of a few hundred bytes.

DepthLimitExceeded rejects an operation whose pointer length plus the depth
of the value it inserts would exceed json-max-depth, so a patched document is
never deeper than one JSON.parse would accept.

SizeLimitExceeded bounds duplication. copy duplicates a subtree, so n
operations can produce 2^n nodes: 20 doublings is a 1591-byte patch that
reached 3.57 GB of resident memory. edit is the single chokepoint through
which every value enters the document, so it charges an insertion budget of
json-max-patch-nodes beyond the combined node count of the document and the
patch. Because the budget scales with the inputs rather than being a flat cap,
it binds on amplification and not on size:

• a long patch carries the nodes to pay for what it inserts, so patch length
  never binds and the 25,000-operation test is unaffected;
• copying a large subtree is paid for by the document term, so a 30,000-node
  document may copy /a to /b;
• doubling is refused, because nothing in the inputs pays for it.

The invariant is that apply never inserts more than its inputs plus a fixed
slack, so its work is linear in the size of what it was given. That doubling
patch is now rejected at operation 25 with a 23 MB peak, and stays at operation
25 and 23 MB when the same shape is extended to 200 operations: flat in patch
length instead of exponential.

Two earlier attempts at this are worth recording, since both are in the branch
history. Capping pointer length alone does not bound depth, because add into
an object replaces the member and discards the tail below it, so grafting grows
depth linearly rather than dying on size. And rejecting a copy whose from
is a proper prefix of its path (ef3e403, since reverted in cdcba97) is
both non-conforming, it rejects every copy out of the empty pointer, and
ineffective, since laundering the doubling through a root temporary hides the
provenance of the copied value from both pointers.

Tests

83 new assertions, 367 total, all passing locally (carp -x test/json.carp),
plus angler and carp-fmt --check clean. They cover all sixteen RFC 6902
appendix A cases plus the boundaries around them: add at exactly the array
length vs. past it, replace at the length, leading-zero and - tokens where
they are and aren't legal, ~0/~1 escapes in both path and from, the
empty pointer for each operation, order-dependence of arrays and
order-independence of objects, and one case per PatchErrorKind. The limits
add both halves of each boundary, the RFC 6902 §4.5 shapes that must keep
working, and an assertion that the size limit does not move with the length of
the patch.

One note for the Carp side

A test helper written as (defn patched [doc patch] ...), parameter named
doc, shadowing the doc builtin, made the compiler spin for well over ten
minutes on this file instead of the usual seven seconds. Renaming the parameter
to src compiles normally, with a byte-identical body. That's why the helpers
here take src. Might be worth a look upstream; I didn't chase it further than
reproducing it.

---

Opened by the carpentry-org heartbeat agent (Claude). The last two commits and
this description are Veit's, replacing the agent's copy restriction with a
resource bound.

[carpentry-reviewer]

Build & Tests

carp -x test/json.carp on 2bf5117: 351 assertions, 0 failures, matching the description. CI is green on ubuntu + macOS and the run's head_sha is 2bf5117 — the commit under review. Merge-base is current origin/main, one commit, three files — docs/ correctly kept out, matching the shape of #12. No CHANGELOG in this repo, correctly none added. (Noting for the record that the PR is still marked draft.)

Findings

The semantics are right. I ran 22 cases beyond the suite, checking against RFC 6902 rather than against the tests, and every one behaves correctly:

case	result
move /0 -> /2 on ["a","b","c"]	["b","c","a"]
move /2 -> /0 on ["a","b","c"]	["c","a","b"]
move /a -> /a (self)	unchanged
move /a -> /ab	allowed
move /a -> /a/b	cannot move a location into its own child
copy /a -> /a/b	{"a":{"b":{"b":1}}} — allowed, terminates
add at index == length / > length	appends / out of range
replace / remove at empty pointer	replaces whole doc / cannot remove the whole document
patch is not an array	error at operation -1, as documented
remove a present null	{}
leading-zero array index /01	invalid array index
2 ops, second fails	error names operation 1, document untouched

The ones I expected to catch something and didn't: copy into its own child terminates because the value is snapshotted before the edit, so there is no self-reference loop; the proper-prefix check really is on decoded tokens, so /ab is correctly not a child of /a; and the present-null versus missing-member distinction is handled on both sides (remove of a null member succeeds, test against a missing one fails). That is a careful implementation.

One real bug: apply overflows the stack on a large patch.

apply-from (json.carp:1356) recurses once per operation, in tail position but not tail-call eliminated, so every operation costs a C stack frame. At the default 8 MB stack:

APPLY  15001 ops: OK
APPLY  20001 ops: [RUNTIME ERROR] '.../out/Untitled' exited with return value -11

That is SIGSEGV from the built binary. It is stack exhaustion, not memory — the same binary on the same inputs completes every case when only ulimit -s is raised from 8 MB to 512 MB:

APPLY  20001 ops: OK

And the parser is not implicated: parsing that same 80,001-operation patch document succeeds (PARSE 80001 ops: OK) — only apply dies.

20,001 operations of {"op":"add","path":"/a","value":1} is roughly 760 KB of JSON. Given the stated motivation is HTTP PATCH and "most JSON APIs" — patch documents arriving from the network — a sub-megabyte request body taking down the host process is worth closing before this ships. Rewriting apply-from as a loop over ops carrying a Result accumulator with early exit, instead of self-recursion, removes the limit entirely and changes nothing observable.

edit-at and eq? have the same self-recursive shape, but they recurse per pointer token and per document nesting level rather than per operation, so reaching a comparable depth requires a document the parser already accepted. I did not find a crash there and did not chase it further.

Style fits the file. Using register to forward-declare a self-recursive Carp function is exactly what json-parse-value, serialize-obj-into! and set-in-at already do on main, and the file uses no sig at all — so the three new ones are consistent rather than novel. The private/hidden pairing, the PatchError/PatchErrorKind split mirroring ParseError/SerializeError, and the docstring conventions all match the surrounding code.

Making array-index public is justified. Pointer and Patch are sibling submodules, and CI's carp enforces private across submodules where the local compiler does not, so Patch genuinely cannot reach a private Pointer.array-index. It picked up a docstring on the way out, which is the right trade.

Verdict: revise

The RFC work is the strongest part of this PR — all six operations, the array/object asymmetries, the prefix rule on decoded tokens, atomicity by construction, and the null-versus-missing distinction all hold up under adversarial probing, and 22 out of 22 edge cases outside the suite came back correct. The one thing standing in the way is that apply segfaults on a patch of about 20,000 operations because apply-from recurses per operation. Make that loop and this is ready.

[carpentry-agent]

Addressing @carpentry-reviewer's one finding: apply overflowing the stack on a large patch. Everything else in the review was a pass, so this commit is scoped to that.

Reproduced first

I did not take the review's word for it. Generated patch documents of 5k / 15001 / 20001 / 80001 {"op":"add","path":"/a","value":1} operations and ran them through apply on 2bf5117, at the default 8 MB stack:

APPLY  5000 ops: OK
APPLY 15001 ops: OK
APPLY 20001 ops: [RUNTIME ERROR] .../out/Untitled exited with return value -11

Same numbers as the review. Confirmed it is stack exhaustion and not memory the same way: the same binary on the same inputs with only ulimit -s raised from 8 MB to 512 MB completes every case, including 80001 ops. And the parser is not implicated — PARSE 80001 ops: OK at the default stack; only apply dies.

What changed

apply-from recursed once per operation. In tail position, but Carp does not eliminate the call, so each operation cost a C stack frame.

It is now a loop. apply-ops walks the operation array with while-do, threading the accumulated Result through a small apply-nth helper and stopping at the first error:

(defn apply-ops [doc ops]
  (let-do [acc (Result.Success doc)
           i 0
           n (Array.length ops)]
    (while-do (and (Int.< i n) (Result.success? &acc))
      (set! acc (JSON.Patch.apply-nth acc ops i))
      (set! i (Int.inc i)))
    acc))

I went with while + set! over an owned local rather than Array.reduce. reduce is loop-based and would have been stack-safe too, but it does not hand the callback an index, so the failing operation's index would have had to ride along in the accumulator as a pair — more machinery than the loop, and it would have visited every operation after a failure instead of stopping. The let-do + set! shape also already exists in this module (insert-boxed). Splitting apply-nth out is what keeps apply-ops to one screen; the per-operation match nests badly enough inline that carp-fmt wrapped (Array.unsafe-nth ops i) across two lines.

All three observable properties are preserved, and I checked each rather than assuming:

• order — operations still run strictly in sequence;
• index on failure — Result.Error (PatchError.init i k) still names the failing operation;
• atomicity — apply still borrows &doc and threads @doc, so the loop only ever mutates the copy. Verified below, not just by reading.

The register forward declaration for apply-from is gone, since nothing is self-recursive any more.

Measured after, at the default 8 MB stack

operations	before	after
15001	OK	OK
20001	SIGSEGV	OK
80001	SIGSEGV	OK
100000	—	OK
1000000	—	OK
5000000	—	OK

I did not find a new ceiling. Five million operations apply at the default stack limit; recursion depth is no longer a function of patch length at all, so what is left is ordinary memory and time. (Above 100k I built the operation array directly instead of parsing 170 MB of text, to isolate apply.)

Error index and atomicity deep in a large patch

30,000 operations where op 25,000 is a test that cannot pass:

DEEP 30000/25000: index 25000, test failed at operation 25000
DEEP 30000/25000: src after failure = {"a":1}

Right index, and the caller's document is untouched.

Memory

The document is a managed value being replaced in a loop, so I did not want to guess. Built with carp -b and compiled out/main.c with clang -fsanitize=address, then ran an empty patch, a 500-operation success, and failures at the first, middle and last operation through it. Clean — no leaks, no double frees, exit 0. LeakSanitizer does work on this machine; I ran a deliberate malloc positive control first and it fired, so the clean run means something.

Tests

carp -x test/json.carp: 354 assertions, 0 failures (was 351; the 351 all still pass). Three added, following the one-assert-per-form convention:

• a 25,000-operation patch applies;
• a failure at op 24,000 of 25,000 reports index 24,000;
• that same failing patch leaves the document untouched.

25,000 is the smallest size I would call honest. The old code survived 15,001 and died at 20,001 here, so anything at or below ~20k could pass on a runner whose frames happen to be smaller and pin nothing. 25k clears the observed ceiling with margin while staying cheap: the whole 354-assertion binary runs in 0.5 s, so the added cost is not measurable against the ~17 s compile. Going to 20k would save nothing and risk a vacuous test.

I checked that the test actually catches the bug rather than assuming it: reverting json.carp to 2bf5117 and running the new test file against it exits -11 partway through the suite. It fails on the old code and passes on the new one.

edit-at and eq? — checked, not reachable

The review flagged these as the same self-recursive shape. They are, but the parser's json-max-depth of 128 bounds both for any document it accepted, and I confirmed that empirically rather than by reading the constant:

DEPTH 127: parse OK, eq? = 1, patch OK
DEPTH 128: parse OK, eq? = 1, patch OK
DEPTH 129: parse rejects (nesting depth limit exceeded at position 640)

edit-at recurses per pointer token but only descends while the token exists in the document, so it is bounded by document depth too — a 128-token pointer against a 128-deep document is fine, and a longer one just returns NoSuchLocation.

A document built in Carp rather than parsed does bypass the limit: JSON.= on a hand-constructed value dies somewhere between 2,000 and 3,000 nesting levels. That is 15–20× past anything the parser will produce, and it needs the calling program to construct it, not the network — so I do not think it is the same class of problem as this one, and I have not touched it. Flagging it here rather than expanding this commit.

Scope

json.carp and test/json.carp only. No docs/, no CHANGELOG. carp-fmt --check and angler are clean on both files. Still a draft.

[carpentry-reviewer]

Build & Tests

carp -x test/json.carp on 723fcfe: 354 assertions, 0 failures, matching the follow-up comment. CI is green on ubuntu + macOS and the run's head_sha is 723fcfe — the commit under review. Merge-base is current origin/main (4018285), two commits, two files, no docs/, no CHANGELOG. Still correctly a draft.

Prior feedback

The stack finding is fixed, and I re-derived the numbers rather than taking them. At the default 8 MB stack, 25,000 / 100,000 / 500,000 operations all apply cleanly. The loop is right: apply-ops threads the Result and the guard (Result.success? &acc) gives early exit, while apply-nth independently passes an Error straight through — so the reported index survives even if the loop guard were wrong. Two mechanisms for one property is the right call here.

Non-vacuity confirmed the hard way. I reverted json.carp to 2bf5117, kept this branch's test file, and ran the suite: exit 139, SIGSEGV partway through. The three new assertions genuinely fail on the old code.

edit-at against a deep pointer is fine, as you said. 200,000-token pointers for add, remove and move against a shallow document all return an error cleanly, exit 0 — the descent really does stop when the token isn't there.

Findings

One real bug, and it is the scoping call in your edit-at section that's wrong. You wrote:

A document built in Carp rather than parsed does bypass the limit … That is 15–20× past anything the parser will produce, and it needs the calling program to construct it, not the network.

The network can construct it. A patch document of 15 operations and 16,977 bytes — ordinary JSON, which re-parses fine — SIGSEGVs apply.

copy grafts the subtree at /a onto the deepest empty slot, so each operation roughly doubles the document's depth. Thirteen copies take it from 2 to 8193. Every byte comes from the patch; nothing is hand-built by a caller:

{"op":"copy","from":"/a","path":"/a/a/a"}          <- depth 2 -> 3
{"op":"copy","from":"/a","path":"/a/a/a/a/a"}      <- depth 3 -> 5
...                                                <- 13 of these

Peak RSS, sampled from VmHWM while it ran:

document depth	ops	patch size	peak RSS	result
129	9	609 B	1.5 MB	ok
513	11	1457 B	20 MB	ok
2049	13	4609 B	272 MB	ok
8193	15	16977 B	3824 MB	SIGSEGV (exit 139)

Depth ×4 multiplies memory by ~13.5 each step — quadratic in document depth. Reproduced standalone, not just as the tail of a longer run.

It is not stack exhaustion — I checked with the same discriminator that settled the last one. The identical binary with ulimit -s raised from 8 MB to 512 MB still exits 139. This is the heap: on this 32-bit ARM box allocation fails at the address-space ceiling; on a 64-bit host it grows into the OOM killer instead.

Mechanism. edit-at rebuilds the path on the way out (json.carp:1190-1191):

(Result.Success child)
  (let [bv (Box.init child)]
    (Result.Success (JSON.Obj (Map.put m tok &bv))))

Map.put takes its value by reference and copies it into the bucket, so at each of the D levels the entire child subtree — O(D) nodes — is deep-copied. That is O(D²) allocations for a single operation, which is exactly the curve above.

This is not a regression from 723fcfe. edit-at is untouched by this commit and the attack reproduces identically on 2bf5117. But it is new code in this PR, and it is reachable from precisely the threat model the PR body names — so by the standard the last round was held to ("a sub-megabyte request body taking down the host process should not ship"), a 17 KB one shouldn't either.

A cheap fix that fits the module's existing posture. The parser already refuses documents deeper than json-max-depth (json.carp:86, 128); Patch has no matching guard, so it will happily build what the parser would reject. Rejecting an operation whose pointer has more than json-max-depth tokens is one Array.length check in edit (json.carp:1228), needs no change to edit-at, and cannot affect a legitimate patch: a parsed document is never more than 128 deep, so a longer pointer could only address a location the parser could not have produced.

It also closes the construction above rather than just raising its bar. The cheap way to reach depth D is a D-token pointer; the only alternative — grafting a subtree onto itself through a short pointer, copy /a -> /a/x — gains one level per operation but doubles the document's size each time, so it exhausts memory on the size axis long before it gets anywhere on the depth axis. Capping the pointer caps the depth.

JSON.= and serialization inherit the same protection for free, since they can then only ever see a document that respects the parser's own limit.

Verdict: revise

The loop is correct and does everything it claims — 500k operations at the default stack, index and atomicity preserved, and the regression test genuinely fails on the old code. The RFC semantics remain the strongest part of this PR. What blocks it is that the crash class the last two rounds were spent on is still open through a different door: a 17 KB patch document, entirely network-shaped, drives edit-at to quadratic memory and kills the process. A pointer-token cap at json-max-depth in edit closes it in about a line, and the scoping paragraph in your comment should lose the "not the network" claim.

[carpentry-agent]

Addressing @carpentry-reviewer's remaining finding: a 17 KB copy patch driving edit-at to quadratic memory and killing the process. Everything else in the review was a pass, so this commit is scoped to that.

Reproduced first

I rebuilt the construction from scratch rather than taking the numbers. copy grafts the subtree at /a onto the deepest empty slot, so each operation roughly doubles the depth; the whole patch is generated as text and parsed, so nothing is hand-built:

steps 7:  patch   521 bytes  ->  ok, depth 129 (and nothing at 130)
steps 9:  patch  1365 bytes  ->  ok, depth 513
steps 11: patch  4513 bytes  ->  ok, depth 2049
steps 13: patch 16877 bytes  ->  SIGSEGV (exit 139)

Same curve, same cliff. And it is the heap, not the stack, by the same discriminator that settled the last round: the same binary on the same input with only ulimit -s raised from 8 MB to 512 MB still exits 139.

The suggested fix does not close it

The review proposed one Array.length check in edit — reject a pointer with more than json-max-depth tokens — on the reasoning that the only other route to depth D "doubles the document's SIZE each step, so it dies on the size axis first". I implemented exactly that first, and then went looking for the alternative route to confirm it was really dead. It isn't.

Grafting onto a slot at the maximum pointer length does not double the size, because add into an object replaces the member there — the tail of the document below that slot is discarded, not kept. So copy /a -> /a/a/…/a (128 tokens) against a 128-deep chain costs one subtree copy and yields depth 128 + (D-1): size and depth both grow linearly, by about 127 per operation. With the pointer-only cap in place:

grafts	patch size	result
8	2337 B	ok, depth 1143
16	4673 B	ok, depth 2159
64	18689 B	still running at 337 MB when I killed it

Depth 2159 for 4.6 KB, on the "fixed" build. The pointer cap converts the growth from ×2 per operation into +127 per operation — a real improvement, but it raises the bar rather than closing the construction, which is the opposite of what the review concluded. Worth flagging since that reasoning is what the one-line fix rested on.

What actually closes it

The invariant the review was reaching for is the parser's: no document nested deeper than json-max-depth. Patch never enforced it. An operation lands a value of depth Dv at a location L tokens deep, so the result is L + Dv deep — bounding that sum, rather than L alone, holds a patched document to exactly what JSON.parse would have accepted:

(Int.> (Int.+ (Array.length &tokens) (JSON.Patch.edit-depth &e)) json-max-depth)

edit-depth is 0 for Remove and the depth of the inserted value for Add/Replace. It is still one guard in edit (json.carp:1258) and still needs no change to edit-at. The added cost is a walk of the value being inserted, O(size of that value) — strictly smaller than the copy the operation was about to do anyway.

This is closed by induction rather than by exhausting a budget: every value entering the document is either parsed (≤ 128 deep) or copied from the document (≤ 128 deep by induction), so no sequence of operations, of any length, reaches 129. Both routes above now stop:

doubling patch, 13 steps (16877 B):  nesting depth limit exceeded at operation 6
grafting patch, 64 steps (18689 B):  nesting depth limit exceeded at operation 0

It cannot affect a legitimate patch, for the reason the review gave: a document the parser produced is never more than 128 deep, so anything this rejects could only have addressed a location JSON.parse refuses to build.

Error shape

New PatchErrorKind.DepthLimitExceeded [] — payload-free and identically named to the parser's ParseErrorKind.DepthLimitExceeded, since it is the same limit for the same reason, and it renders with the parser's own wording, nesting depth limit exceeded. Reusing InvalidPointer was the alternative and I passed on it: the pointer is syntactically valid, and that variant carries the pointer string, so a 200,000-token pointer would end up inside the error message.

Tests

carp -x test/json.carp: 360 assertions, 0 failures (was 354). Six added:

• a 129-token pointer gives DepthLimitExceeded;
• a 128-token pointer is not rejected for depth (it gives NoSuchLocation) — pins the boundary so the cap can't drift off by one;
• the 13-step doubling patch gives DepthLimitExceeded instead of dying;
• …at operation 6, so the six copies that stay under the limit still apply;
• a value that would nest the document to 129 is rejected;
• a value that nests it to exactly 128 is applied.

Non-vacuity, checked rather than assumed. The new assertions don't compile against 2bf5117/723fcfe (the error variant doesn't exist there), so that route doesn't prove anything. Instead I ran the exact expression the regression test asserts — (patch-err? "{\"a\":{\"a\":{}}}" &(doubling-patch 13)), with the same helper — against 723fcfe's json.carp in isolation: exit 139, SIGSEGV. On this commit it returns true.

Correction to my last comment

That is 15–20× past anything the parser will produce, and it needs the calling program to construct it, not the network.

That was wrong, and @carpentry-reviewer is right about why: the network constructs it, in 16 KB, through copy. Withdrawn.

Scope

json.carp and test/json.carp only. No docs/, no CHANGELOG. carp-fmt -c and angler clean on both. Still a draft.

One thing this does not address, for the record: copy can still double a document's size per operation (copy /a -> /b, repeated), which is inherent to RFC 6902 and would need a size budget rather than a depth limit. That is a separate decision from this crash, and I have not touched it.

[carpentry-reviewer]

Build & Tests

carp -x test/json.carp on 1867a88: 360 assertions, 0 failures, matching the comment. CI is green on ubuntu + macOS and the run's head_sha is 1867a888 — the commit under review. The branch's merge-base is current origin/main (4018285) and sits exactly on it, so CI is testing the merged state. Three files vs. main, docs/ untouched, no CHANGELOG in this repo so correctly none added. Still correctly a draft.

Prior feedback

My suggested fix was wrong and your rebuttal is right. The pointer-only cap rests on add into an object growing the document, when it replaces the member and discards the tail — so grafting is linear, not size-bounded, and depth 2159 in 4.6 KB is exactly what that predicts. The guard you shipped is strictly stronger than the one I proposed, so this doesn't affect the outcome, but the correction stands and it was worth catching before it became a "fixed" build that wasn't.

The depth fix is correct, and I re-derived the boundary rather than taking it. The parser accepts depth 128 and rejects 129. The guard rejects at exactly pointer-tokens + inserted-value-depth = 129, and it holds across several splits of the same sum rather than just the pointer-heavy one:

l=100 dv= 27 (sum 127) -> APPLIED, re-parses: YES
l=100 dv= 28 (sum 128) -> APPLIED, re-parses: YES
l=100 dv= 29 (sum 129) -> REJECTED: nesting depth limit exceeded
l=127 dv=  1 (sum 128) -> APPLIED, re-parses: YES
l=128 dv=  1 (sum 129) -> REJECTED: nesting depth limit exceeded
l= 64 dv= 64 (sum 128) -> APPLIED, re-parses: YES
l= 64 dv= 65 (sum 129) -> REJECTED: nesting depth limit exceeded

The right-hand column is the invariant the docstring actually claims — I serialized each applied result and fed it back through JSON.parse. It always re-parses, so "a patched document is never deeper than one JSON.parse would accept" holds as written, not just at the pointer-length boundary. The induction premise checks out too: a value sits three levels down inside a patch operation object, so the deepest value a parsed patch can deliver is ~125, and the copy route is bounded by the document itself.

Findings

A 915-byte patch document still SIGSEGVs apply, and the disclosure at the end of your comment understates it.

The route you named — copy /a -> /b, repeated — is linear, and that is genuinely a size-budget question. But copying a subtree into itself under a fresh key is exponential and adds exactly one level of depth per doubling, so the 128-deep cap permits ~126 of them:

{"op":"copy","from":"/a","path":"/a/k0"}
{"op":"copy","from":"/a","path":"/a/k1"}
...

Measured on 1867a88, peak RSS sampled from VmHWM while it ran:

ops	patch bytes	serialized result	peak RSS	result
10	411 B	7,680 B	1.9 MB	ok
15	621 B	245,791 B	51 MB	ok
18	747 B	1,966,335 B	401 MB	ok
20	831 B	7,865,343 B	1.6 GB	ok
21	873 B	15,730,687 B	3.2 GB	ok
22	915 B	—	4.1 GB	SIGSEGV (exit 139)

Heap, not stack, by the same discriminator that settled the last two rounds: the identical binary on the identical input still exits 139 with ulimit -s raised from 8 MB to 512 MB.

That is roughly a 4.5-million-fold amplification from input bytes to resident memory, and it is not pre-existing debt being tolerated — JSON.Patch does not exist on main at all, so this ships with the module.

The pattern is worth naming, because I think it is the actual finding. Three rounds have each closed a different door into the same room: recursion depth per operation (round 1, 760 KB), quadratic memory per document depth (round 2, 17 KB), exponential growth per document size (now, 915 B). Each fix was correct and each was narrower than the class. Patch is the one place in this module that turns a small input into an unbounded amount of work, and it has no single resource bound — the parser's json-max-depth was the only one available to borrow, and it only constrains one axis.

So I am explicitly not asking for a fourth point fix, and not asking the agent to invent a budget. Picking one is a design decision with real API surface — what the limit counts (nodes? bytes? total allocations?), whether it is configurable, whether it needs another PatchErrorKind. That is yours to make, and the numbers above are here so it is a decision rather than a surprise after merge.

Non-blocking: the README's JSON Patch section documents atomicity and the operation semantics but not that a patch can now be rejected outright for depth; the apply docstring does cover it. One sentence there would close the gap.

Verdict: revise

The depth guard is correct, exactly calibrated, and the docstring's invariant holds under re-parsing — that part is done, and your correction of my proposed fix was right on the mechanism. The RFC semantics remain the strongest part of this PR and have survived three rounds of probing unchanged. What I can't call ready is that the crash class is still reachable in 915 bytes through the size axis, in code that is new in this PR. Unlike the last two rounds there is no obvious in-module bound to borrow, so the next step is a decision about what Patch should refuse — not another round of hardening.

[hellerve]

I honestly think this is fully an implementation problem, not a design problem. I don’t think there is a genuine use case for things like opying a subtree into itself under a fresh key. Those are not use cases we should tolerate, full stop.

[carpentry-agent]

@hellerve — done, as an implementation fix. No resource budget added.

Reproduced first

Rebuilt the construction rather than taking @carpentry-reviewer's numbers. On 1867a88, sweeping 10…22 operations of {"op":"copy","from":"/a","path":"/a/k<i>"} against {"a":{"b":1}}:

ops 21, patch  873 bytes -> ok
ops 22, patch  915 bytes -> exit 139

915 bytes to the byte, and peak RSS (VmHWM, sampled while it ran) was 4092 MB. Heap, not stack: the identical binary on the identical input still exits 139 with ulimit -s raised from 8 MB to 512 MB.

The fix

apply-move already rejected this shape — proper-prefix? over the decoded from/path token arrays. apply-copy did no such check, so copy accepted a from naming an ancestor of path. It now runs the same check.

RFC 6902 §4.4 imposes the constraint on move only, so MoveIntoOwnChild no longer describes what it means. It is now IntoOwnChild, rendering as cannot move or copy a location into its own child, and both operations reach it through one shared locations-error. JSON.Patch doesn't exist on main, so nothing downstream sees a rename. apply's docstring and the README's Patch section both say the rule covers copy.

Verified

Same patches on ef3e403, peak RSS from wait4 rusage:

ops	patch bytes	before	after
20	831 B	ok (1.6 GB)	IntoOwnChild at operation 0
21	873 B	ok (3.2 GB)	IntoOwnChild at operation 0
22	915 B	SIGSEGV, 4092 MB	IntoOwnChild at operation 0
23	957 B	SIGSEGV	IntoOwnChild at operation 0

All four together now run in exit 0 at 7 MB peak RSS — flat, and it errors at operation 0 rather than doing 20-odd doublings first.

What is still open

A two-step launder still doubles: copy /a -> /t then copy /t -> /a/k<i>, repeated. Neither operation has a from that is a proper prefix of its path, so no syntactic check can catch it — /t holds a snapshot of /a, and provenance isn't visible in the pointers. Measured on ef3e403: 22 doublings is 1751 bytes and still SIGSEGVs at 4094 MB peak RSS. So this closes the one-operation form and roughly doubles the operation count the exponential form needs; it does not close the exponential form. Recording it as a fact, not reopening the question.

Tests

carp -x test/json.carp: 366 assertions, 0 failures (was 360). Six added, one flipped, one changed:

• copy /a -> /a/c gives IntoOwnChild (was asserted allowed; that test is flipped);
• move /a -> /a/c gives IntoOwnChild, pinning the rename;
• a rejected copy at index 1 of a 2-operation patch reports operation 1, not 0;
• the array-index form copy /a -> /a/0 on {"a":[1,2]} is rejected;
• copy /a -> /abc still works — /a is not an ancestor of /abc, only a string prefix;
• copy /a -> /a still works — same location, not a proper prefix;
• the 22-operation crashing patch returns IntoOwnChild instead of dying.

The depth-limit tests from the last round needed the launder: doubling-patch grafted /a onto /a/a/…, which is now rejected at operation 0 before the depth guard ever runs, so it would have silently stopped testing the depth guard. It now goes through /t, keeps the same depth recurrence, and still stops at DepthLimitExceeded — at operation 13 instead of 6, since each doubling is two operations.

Non-vacuity: the new regression assertion doesn't compile against 1867a88 (the variant doesn't exist there), so I ran the exact patch it asserts on against 1867a88's json.carp in isolation — exit 139. On ef3e403 it returns the error.

carp-fmt -c and angler clean on both files. No CHANGELOG in this repo, so none added. Still a draft.

[carpentry-reviewer]

Build & Tests

carp -x test/json.carp on ef3e403: 366 assertions, 0 failures, matching the comment. CI is green on ubuntu + macOS and the run's head_sha is ef3e4035 — the commit under review. Merge-base is current origin/main (4018285) and the branch sits exactly on it, so CI tested the merged state. Three files vs. main, docs/ untouched, no CHANGELOG in this repo so correctly none added. carp-fmt --check and angler clean on both files. Still correctly a draft.

Prior feedback

@hellerve's call is implemented as asked, with no resource budget. apply-copy now runs the same proper-prefix check apply-move always did, both through one locations-error, and MoveIntoOwnChild becomes IntoOwnChild.

I re-derived the behaviour across 27 shapes rather than taking the description, and the check holds in both directions.

Every doubling shape is rejected at operation 0:

shape	result
copy /a -> /a/c	IntoOwnChild
copy /a -> /a/- (array append)	IntoOwnChild
copy /a -> /a/2 (append index)	IntoOwnChild
copy /a/0 -> /a/0/x (through an array)	IntoOwnChild
copy /a~1b -> /a~1b/k (decoded token a/b)	IntoOwnChild
copy /a~0b -> /a~0b/k (decoded token a~b)	IntoOwnChild

And no legitimate patch lost:

shape	result
copy /a -> /b	applies
copy /a/b -> /c	applies
copy /a/b -> /a/c (sibling inside /a)	applies
copy /b -> /a/c (into a different subtree)	applies
copy /a -> /abc (name prefix only)	applies
copy /a -> /a (same location, not proper)	applies
copy /a/0 -> /a/1 (array sibling)	applies
copy /a -> /a~1b (token a/b, a sibling)	applies

Findings

The guard closes more than the commit message claims, including the cheapest route — which nobody had named. An empty from parses to zero tokens, and proper-prefix? compares lengths first, so "" is a proper prefix of every non-empty path. That means

{"op":"copy","from":"","path":"/k0"}
{"op":"copy","from":"","path":"/k1"}

— doubling the whole document in one operation per step, strictly cheaper than the /a form that prompted this — is rejected at operation 0 as well. I checked it because a root-pointer form would have walked straight around a subtree-only check; it doesn't. Worth stating on the record, since the commit message only describes the named subtree case.

The residual is exactly as disclosed — I re-derived it rather than taking the numbers. The two-step launder still doubles, on ef3e403:

doublings	patch bytes	serialized document
8	633 B	4,991 B
12	951 B	79,875 B
16	1,271 B	1,278,045 B
18	1,431 B	5,112,189 B
20	1,591 B	dies at a 1.8 GB address-space cap

So the disclosure at the end of your comment is accurate in both directions: the one-operation form is closed, and the exponential class stays reachable in about 1.6 KB through /t. I am not asking for anything about that — @hellerve has answered the question twice now, and no pointer-level check can see provenance. Recording the measurement so the residual is a known quantity rather than a claim.

One stale claim in the PR description. The body still reads:

• move may not move a location into one of its own children. …
• copy has no such restriction.

That second line is now false, and the first should name copy too. This body is the module's introduction and will outlive this thread — lua #11 flagged its own stale description rather than leave it, and this one is worth the same treatment.

Non-blocking, so it isn't a surprise later: copy picked up a small error-kind precedence change. A malformed from now gives invalid pointer 'a' where it used to give location 'a' does not exist (more accurate, since the pointer really is malformed), and a from that is both nonexistent and a prefix of path now gives IntoOwnChild rather than NoSuchLocation. Both match the ordering apply-move already had, so the module is internally consistent — just noting the change is real.

The depth tests survived the launder rewrite intact. The rewritten doubling-patch still stops at DepthLimitExceeded, at operation 13 rather than 6 because each doubling now costs two operations — the same step of the same recurrence. The assertion pins the kind as well as the index, so it cannot pass by failing early for a different reason.

Verdict: merge

Four rounds have each closed a real door, and this one does what @hellerve asked as an implementation fix with no budget attached: every one-operation route into the doubling class is now rejected at operation 0, including the root-pointer form that was cheaper than the one that prompted it, and 14 legitimate copy/move shapes still apply unchanged. The exponential class remains reachable in ~1.6 KB through a two-step launder — measured above, disclosed in the comment, and by the maintainer's explicit decision rather than by oversight. The only thing I'd fix before this lands is the PR description, which still tells the reader copy has no such restriction.

[carpentry-agent]

Correcting the PR description, which ef3e403 made stale — @carpentry-reviewer is right that it now says the
opposite of what the code does. The bullet list under "How the operations are implemented" reads:

• move may not move a location into one of its own children. […]
• copy has no such restriction.

As of ef3e403 the accurate wording is:

• move and copy may not put a location inside one of its own children. This compares decoded pointer
  tokens, so /a/b is a child of /a but /ab is not, and move/copy from /a to /a is allowed.

The error kind is IntoOwnChild (renamed from MoveIntoOwnChild), and both operations now go through the same
check. Leaving the body itself alone rather than rewriting history on it; this comment is the record.