JSON Patch write-location and semantic qualification

[yellowman]

Scope is the seams this campaign measured, not full RFC 6902 conformance.

One invariant throughout: an operation applies to a location that operation permits, or fails without changing the document. Every defect here is a violation reported as success — the audit trail asserting a change that did not happen, or a change to somewhere other than the place named.

Found by driving ConfigOps against SPEC §10 on a running instance. A published workflow was patched, the API returned 200, the patch was marked applied, an artifact_version was written — and the configuration the chat path consumes did not change.

What was wrong

Write locations. add and replace shared a creating traversal, so replace /a/b on a document with no a invented one and reported success. remove treated a missing target as nothing to do, which makes a removal that addressed the wrong path indistinguishable from one that did its job. Array bounds silently appended past the end.

move atomicity. The destination was validated against the pre-removal document. RFC 6902 §4.4 defines a move as a remove followed by an add, so the add must be legal in what the remove leaves behind:

{"a": {"x": 1}}          move /a    -> /a/child   left {}
{"xs": ["a","b","c"]}    move /xs/0 -> /xs/3      left {"xs": ["b","c"]}

Both raised and destroyed the value. move now rehearses the whole operation on a copy first.

A constant array ceiling. MAX_LIST_EXTENSION = 1024 sat in the write path only, so position 1024 was addressable by remove, test, copy and move and refused to replace and add. Its stated reason — that /xs/999999999 would allocate a billion placeholders — was obsolete; nothing pads a list. Measured with the length check removed, that pointer produces one append at index 2, so what the check defends is the address, not the heap.

RFC 6901 tokenization. Parsing was strip("/"), split("/") and "drop the empty ones", with no escape decoding — four separate rewrites of the caller's address:

/a~1b   names key `a/b`      wrote key `a~1b`
/a//b   is `a`, ``, `b`      wrote a.b
/a/     is a's `` member     replaced `a` itself
a/b     is not a pointer     accepted as /a/b

The escape case is the worst: both spellings can be real keys in one document, so it succeeds against a valid location nobody named. A root-path claim was also inverted in the code and in a test that agreed with it — "" is the whole document, / is the member keyed "".

Operand shape. {"op":"replace","path":"/k"} wrote {"k": None}, destroying a value on behalf of an operand nobody supplied. Half-formed ops were skipped in silence, and the artifact route carried on into update_private_artifact and wrote a version. An empty operation list did the same on both callers.

Index grammar. str.isdigit() admitted 01, 007, ١ and ０ as ordinary indices, so several spellings named one position — and ², which isdigit() accepts and int() rejects, left as an uncaught ValueError.

Pointer operand types. Presence was required, type was not, so a non-string path or from reached .startswith() and escaped as AttributeError.

test equality. Python makes True == 1 and carries it recursively through containers. test guards the operations behind it, so this let a mutation run on a precondition never met — measured at the route as 200, the guarded replace applied, and a version written.

Every artifact writer held a lock over a write it had already computed. SPEC §10.1 says apply loads the current schema. ConfigOps, the private PATCH route, and all five training call sites read the row, computed the whole new document, and handed it to a store method that took FOR UPDATE and wrote what it was given — so the lock serialized the write without covering the read behind it:

writer A reads schema N, computes N + D
writer B locks, reads N, writes N + C, commits
writer A takes the lock, writes its precomputed N + D
-> C is gone

Reachable in both directions. Interleaved one way an ordinary edit vanishes; the other way an applied ConfigOps patch does, while its audit row still says applied. Training's promotion was the worst case: dict(adapter.schema) came from a snapshot taken before the training run, so that window is minutes.

Apply and delete then took the two rows in opposite orders. Locking the config_patch row to make one patch apply exactly once created an ABBA cycle: delete_private_artifact takes the artifact and deletes it, and config_patch.artifact_id is ON DELETE CASCADE, so the delete reaches the patch rows through the artifact. Measured: {'apply': 'DeadlockDetected', 'delete': 'ok'}.

What changed

• _walk_parent creates nothing; _require_target enforces RFC §4.2/§4.3 for remove and replace.
• move rehearses on a deep copy before touching the real document; copy needs none, since reading mutates nothing.
• The array bound is the list's own length. MAX_LIST_EXTENSION and _ensure_list_capacity are gone.
• _segments_or_raise is a real RFC 6901 tokenizer: leading / required, empty tokens preserved, ~1 then ~0 decoded in that order, malformed escapes refused. Error details render through _pointer, which re-escapes.
• validate_op / validate_ops are one operand rule; ArtifactPatchRequest and ConfigOps call it rather than keeping copies.
• _read_index matches the RFC production, strictly on both the positive and negative forms.
• json_equal implements RFC §4.6 equality.
• apply_config_patch, update_artifact and update_private_artifact take a build_schema callable rather than a finished document, applied to the schema read under the row lock. description is no longer replayed from a stale read.
• Apply takes the artifact lock first, the order every other writer here uses, then locks the patch row and re-checks its identity and status. One approved patch still applies exactly once: two applies contend on the artifact, the winner marks the patch, and the loser then sees applied.
• All five training call sites build from the locked row, each changing only the fields it owns.
• meta_ops gives the internal producers one leaf op and never a parent create, since a parent decision made at proposal time can be stale by apply time.
• Docs: ArtifactPatchRequest and the route docstring said /schema/foo, which named a key inside the schema. Both callers hand the engine artifact.schema itself.
• _walk_existing removed — no caller left once every verb walks without creating.

Verification

Every defect was reproduced before being fixed, most through the real caller — private-artifact PATCH, ConfigOps propose/decide/apply, or recommend_adapter_pruning — asserting the HTTP status, the stored schema, and the artifact_version count, because the version write is the dishonest part.

The concurrency witnesses are deterministic by construction rather than by timing: each holds one operation at a seam that already exists in the code, and releases it only once the server reports a backend genuinely waiting on a lock.

31 mutations, 29 killed. Complements rather than repetition, so witnesses are shown to measure distinct mechanism:

pair	discrimination
pre-removal destination check vs. no check	the first kills only the two new move witnesses; the older one survives it
ceiling restored vs. length check deleted	each kills what the other leaves alive
six pointer rewrites, one at a time	each kills only its own witnesses
Python == vs. recursion removed	the second kills exactly the container three
five concurrency regressions	each kills exactly one witness — lost update, exactly-once, deadlock, route overwrite, promotion

Two mutations survive by design and a third exists to say why: the operand rule has one definition and two enforcement points, so removing either alone changes nothing observable, while removing both exposes the route witness and only that.

Full lane at the current head: 2998 passed, 27 skipped. Both lint selections clean.

Not in scope

SPEC §10.2 sandbox simulation and artifact-version rollback remain unimplemented and are recorded in docs/ISSUES.md as nonconformances, each with its own tranche to follow. Reference validation — graph integrity first, then adapter/cluster/tool references scoped by each consumer's real resolver — is a separate tranche pair.

ConfigPatchRequest still validates only op and path at proposal time; a malformed proposal can be stored pending but cannot be applied, since _apply_patch_to_schema calls validate_ops before iterating and before persistence.

docs/ISSUES.md also records a PoolClosed teardown race seen once in the xdist lane and not reproduced, unrelated to this change by reachability.

🤖 Generated with Claude Code

---

Generated by Claude Code

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2486f65. Configure here.}