feat(#4890): native Bolt Duration, Point, Path + semantic errors (#4997, #4998, #4999)

[robfrank]

What does this PR do?

Adds native Bolt wire serialization for three Cypher types and fixes semantic-error classification, closing three sub-issues of the #4890 tracking issue:

• Bolt: implement native Duration and Point structure mapping #4997 (TYPE-011, TYPE-012) - native Duration (struct tag 0x45) and spatial Point (0x58/0x59) encoding, plus inbound decoding so both round-trip as query parameters. Point detection is structural (Cypher point() returns a Map, there is no Point class), keyed on a recognized crs value (cartesian/cartesian-3D/WGS-84/WGS-84-3D) or a numeric srid, plus numeric x/y; the inbound decode carries a derived crs so an echoed point re-encodes as a Point.
• Bolt: emit native Path structures for query results #4998 (TYPE-003) - native Path (0x50). BoltPath already existed but had no call site; this wires TraversalPath -> BoltPath with deduplicated nodes/rels and the signed 1-based/0-based indices sequence, deriving traversal direction per hop.
• Bolt: distinguish semantic errors from syntax errors in the RUN handler #4999 (ERR-002) - a new CommandSemanticException extends CommandParsingException lets the Bolt RUN handler return Neo.ClientError.Statement.SemanticError. Classification is scoped to the undefined-variable case that ERR-002 certifies: only the UndefinedVariable validator throws become CommandSemanticException; every other CypherSemanticValidator failure keeps throwing CommandParsingException -> SyntaxError (unchanged from before this PR). The subclass is caught by every existing catch (CommandParsingException) site unchanged, so only the Bolt handler distinguishes it.

All changes are writer/validator-side, mirroring the shipped temporal work (#4907); no query-engine behavior changes beyond the additive exception subclass.

Motivation

Depth gaps surfaced by the Group B conformance suites and pinned in bolt/conformance/spec.yaml as expected-fail. Official Neo4j drivers never received typed Duration/Point/Path values, and Neo.ClientError.Statement.SemanticError was dead code.

Related issues

• Closes Bolt: implement native Duration and Point structure mapping #4997, Bolt: emit native Path structures for query results #4998, Bolt: distinguish semantic errors from syntax errors in the RUN handler #4999
• Part of Bolt: tracking issue for protocol/type-fidelity gaps surfaced by certification #4890 (Group C tracking), epic Epic: Bolt Driver Compatibility Certification #4882
• Flips conformance cells TYPE-003/011/012 + ERR-002 to passing across all five per-language e2e suites (Java, JS, Python, C#, Go).

Additional Notes

• Module-level tests (BoltTypeRoundTripTest, PackStreamTypeInputTest, Bolt4998PathMappingTest, BoltErrorClassificationTest, CypherSemanticValidatorExceptionTest) are the hard gate and pass locally (bolt module 253/253). The five e2e suites pass in CI against the branch-built image.
• Known follow-up (not blocking): extend SemanticError classification to the remaining genuinely-semantic validator failures (type conflicts, nested aggregation, invalid DELETE target, ...) once each is verified against a real Neo4j instance. They are intentionally left as SyntaxError for now so the change certifies only what ERR-002 exercises rather than trading one misclassification for another.

Checklist

• Unit tests cover both failure and success scenarios
• Conformance spec + all five e2e suites updated

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Not up to standards ⛔

🔴 Issues 4 minor

Alerts:
⚠ 4 issues (≤ 0 issues of at least minor severity)

Results:
4 new issues

Category	Results
CodeStyle	4 minor

View in Codacy

🟢 Metrics 53 complexity

Metric	Results
Complexity	53

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Review: native Bolt Duration / Point / Path + semantic errors (4997, 4998, 4999)

Reviewed the full diff. Well-scoped, cleanly mirrors the existing temporal work (4907), and has strong module-level test coverage on both outbound encoding and inbound decoding. The CommandSemanticException extends CommandParsingException design is a nice, low-risk way to add a distinction without touching any existing catch site. A few points worth considering before merge.

Potential issues

1. The semantic-validator retarget is broader than ERR-002 and may misclassify some errors vs. real Neo4j.
CypherSemanticValidator was changed wholesale: every throw now uses CommandSemanticException, so on the Bolt path all of these surface as Neo.ClientError.Statement.SemanticError. ERR-002 only asked for the undefined-variable case. Some retargeted throws are ones Neo4j classifies as SyntaxError, not SemanticError - e.g. UnknownFunction (you already flagged this as unconfirmed), NoSingleRelationshipType / RequiresDirectedRelationship / CreatingVarLength in CREATE/MERGE, and NegativeIntegerArgument / InvalidArgumentType on SKIP/LIMIT. Since the conformance goal is driver parity, it is worth confirming these specific codes against a real Neo4j instance rather than flipping all of them in one sweep. If uncertain, consider scoping the retarget to the checks you have actually verified (undefined-variable + the ERR-002 cases) and leaving the rest as CommandParsingException, so the fix does not trade one misclassification for another.

2. Structural Point detection can misencode legitimate user maps on the outbound path.
toPointStructure runs for every returned value and treats any Map containing a crs key plus numeric x/y (or longitude/latitude) as a spatial Point, encoding it as a Bolt Point structure instead of a generic map. Documented as intentional, but it means a user who returns an ordinary nested map that happens to have those keys will silently have it rewritten into a Point on the wire. Reasonable pragmatic choice given there is no Point class, but a data-fidelity footgun. Consider tightening detection (require srid OR a recognized crs value string, not just presence of a crs key) and noting the collision in the code.

3. toPath has no guard for an empty path.
toPath does vertices.get(0) unconditionally. TraversalPath can be constructed empty (default ctor gives empty vertices), which would throw IndexOutOfBoundsException. A MATCH path always has a start vertex so this is unlikely to trigger, but a one-line guard makes it robust. The dedup logic itself (1-based signed rel index, 0-based node index, direction via edge.getOut().equals(from.getIdentity())) is correct and well tested for forward and backward hops.

Test coverage
Good - covers outbound encoding, inbound decoding, malformed-payload degradation to opaque, and the plain-map negative case. Two gaps (you flagged the first):

• type011_durationNative asserts only signature + field count, not values/order. Since Bolt Duration is [months, days, seconds, nanoseconds], a wrong order round-trips fine only because both sides share the bug; an outbound value assertion closes the loop.
• No negative test that a plain map with crs + x/y is (or is not) treated as a Point - relevant to issue 2.

Security / performance
No concerns. The per-value toPointStructure check is an instanceof fast-fail for non-maps, so no meaningful hot-path cost. No sensitive data exposure.

Overall a solid PR that closes the three sub-issues cleanly. Main ask: double-check the breadth of the SemanticError retarget (point 1) against real Neo4j.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[robfrank]

Addressed in f2181f7.

1. Semantic-validator retarget breadth. Correct - the wholesale sed over-broadened beyond what ERR-002 certifies. Scoped it back: only the 8 UndefinedVariable throws now map to CommandSemanticException -> SemanticError (the scenario ERR-002 actually tests). The other 43 validator throws revert to CommandParsingException -> SyntaxError, matching pre-PR behavior, so the untested cases (UnknownFunction, NoSingleRelationshipType, NegativeIntegerArgument, CREATE/MERGE structural checks, InvalidArgumentType, etc.) get no new, unverified classification. Per-case verification against a real Neo4j instance is left as a follow-up so we don't trade one misclassification for another.

2. Structural Point detection. Correct footgun. Tightened toPointStructure: it now requires the crs value to be a recognized spatial CRS (cartesian, cartesian-3D, WGS-84, WGS-84-3D) or a numeric srid key present - not just the presence of a crs key. Every point from Cypher point() still qualifies; an ordinary user map that merely happens to carry a crs key is no longer rewritten to a Point on the wire. Added a negative test pinning this.

3. Empty-path guard. Added - toPath returns an empty BoltPath when vertices is empty instead of vertices.get(0) throwing.

Test gaps. type011_durationNative now serializes the structure and reads it back to assert the field order/values [months=0, days=1, seconds=9000, nanos=0] under signature 0x45, so a field-order swap is caught. The plain-map-with-crs negative case is covered by the test added for point 2.

Also removed the now-dead runSingleErr in the Go suite.

[robfrank]

/gemini review

[claude]

Review: native Bolt Duration/Point/Path + semantic errors (#4997, #4998, #4999)

Solid, well-structured PR. TDD throughout, the backward-compat design (CommandSemanticException extends CommandParsingException so every existing catch keeps working and only Bolt distinguishes) is clean, and the classifyParsingError single-catch/instanceof approach is nicer than the two-catch variant the plan proposed. The Path mapping (per-hop direction sign, RID dedup of nodes/rels, signed 1-based/0-based indices) is correct and the backward-hop test is a good touch. I verified the outbound Point detection lines up with the real engine: CypherPointFunction emits crs+srid+x/y(/z) and RECOGNIZED_CRS exactly matches its four possible crs values (WGS-84, WGS-84-3D, cartesian, cartesian-3D), so engine-produced points round-trip.

A few points, none of them blockers:

1. PR description no longer matches the code (please reconcile)

The description says the change is a "wholesale semantic-validator retarget" and follow-up note #2 warns that UnknownFunction gets mapped to SemanticError "outside ERR-002's scope and unconfirmed against Neo4j." The committed CypherSemanticValidator is actually selective: only the UndefinedVariable throws (plus the one "Aggregation in ORDER BY is not projected") became CommandSemanticException. UnknownFunction, InvalidArgumentType, NestedAggregation, VariableTypeConflict, NoSingleRelationshipType, etc. all still throw plain CommandParsingException -> SyntaxError.

This is arguably better than described (it sidesteps the UnknownFunction concern entirely), but the description and follow-up note #2 are stale. Worth confirming the selectivity was intentional and updating the PR text so a future reader isn't misled about what maps where.

2. Semantic-vs-syntax classification is now partial

As a consequence of (1), ArcadeDB now returns SemanticError for undefined variables but still returns SyntaxError for other genuinely-semantic failures that Neo4j classifies as SemanticError (type conflicts, nested aggregation, invalid DELETE target, ...). That's fine for ERR-002's stated scope, but the split is inconsistent. Suggest either a tracked follow-up to align the remaining semantic classes, or a short comment in the validator documenting which throws are intentionally left as syntax errors, so the boundary is deliberate rather than accidental.

3. Outbound Point has no engine-level module test (only hand-built maps + CI e2e)

Bolt4998PathMappingTest exercises the real engine (MATCH p=... RETURN p) through toPath, which is great. The outbound Point path has no equivalent: type012_* only feed a manually constructed LinkedHashMap into toPackStreamValue, and the real RETURN point({...}) shape is only covered by the CI-only e2e suites. Since detection hinges on the exact keys CypherPointFunction emits, a cheap embedded-DB test (db.query("opencypher", "RETURN point({x:12.34,y:56.78}) AS p") -> assert BoltPointStructure) would lock the outbound mapping to the engine's actual output and catch key-shape drift in the fast gate instead of only in e2e. They line up today, but nothing in the module tests pins that.

4. toPath assumes vertices.size() == edges.size() + 1

Only vertices.isEmpty() is guarded. The hop loop does vertices.get(i + 1) for i < edges.size(), so a TraversalPath with fewer vertices than edges + 1 would throw IndexOutOfBoundsException rather than degrade. For well-formed MATCH paths the invariant holds, but a defensive guard or an explicit invariant comment would harden it against malformed traversal inputs.

5. (Accepted trade-off, just flagging) user maps can be misencoded as Points

Any returned Map carrying a crs key + numeric x/y and a recognized crs (or numeric srid) is serialized as a native Point on the wire, even when the user built it as ordinary data. The RECOGNIZED_CRS narrowing already limits this well and it mirrors Cypher's own structural point representation, so this is a reasonable trade-off - just noting it as accepted rather than hidden.

Minor / cosmetic

• fromTemporalStructure now also decodes Duration and Point signatures, so the name is a slight misnomer. Non-blocking.
• Nice that the inbound decode carries a derived crs so an echoed point re-encodes as a Point rather than a bare map - and that malformed structs stay opaque via the existing arity guard (well tested in PackStreamTypeInputTest).

Test coverage is otherwise strong (module gate 253/253, inbound + outbound + error classification + validator all covered). Once the description/scope notes are reconciled and ideally an engine-level Point outbound test is added, this looks good to merge.

[gemini-code-assist]

Code Review

This pull request implements native Bolt serialization and deserialization for Duration, Point, and Path structures, and enables the distinction between semantic and syntax errors. It introduces BoltPointStructure, maps TraversalPath to BoltPath, and decodes inbound Bolt Duration and Point structures. It also introduces CommandSemanticException to classify semantic errors separately from syntax errors. Conformance tests across multiple languages have been updated to reflect these improvements. The review feedback highlights a potential NullPointerException in BoltStructureMapper.toPointStructure when a map contains a null value for the "crs" key, as Set.of(...) is null-hostile; a code suggestion is provided to address this issue.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The RECOGNIZED_CRS set is initialized using Set.of(...), which is null-hostile in Java. Calling RECOGNIZED_CRS.contains(null) will throw a NullPointerException. If a client passes a map where the "crs" key is present but its value is null (e.g., {"crs": null, "x": 1.0, "y": 2.0}), map.containsKey("crs") will return true, and then map.get("crs") will return null, causing a NullPointerException when RECOGNIZED_CRS.contains(null) is evaluated.

To prevent this, check if the "crs" value is null before querying the set.

Suggested change

	static BoltPointStructure toPointStructure(final Object value) {
	if (!(value instanceof Map<?, ?> map) || !map.containsKey("crs"))
	return null;
	if (!RECOGNIZED_CRS.contains(map.get("crs")) && !(map.get("srid") instanceof Number))
	return null;
	static BoltPointStructure toPointStructure(final Object value) {
	if (!(value instanceof Map<?, ?> map) || !map.containsKey("crs"))
	return null;
	final Object crs = map.get("crs");
	if ((crs == null || !RECOGNIZED_CRS.contains(crs)) && !(map.get("srid") instanceof Number))
	return null;

[robfrank]

Fixed in 2251567. The crs value is now null-checked before the RECOGNIZED_CRS lookup, so {crs: null, x, y} no longer NPEs - it falls through to generic map handling (no recognized crs value and no numeric srid). Added a regression test asserting that map returns a Map, not a Point.

[robfrank]

Addressed in 2251567 + PR description update.

1. Stale PR text. Correct - the "wholesale retarget" wording predated the cycle-1 scoping. Updated the description: classification is now explicitly scoped to the UndefinedVariable throws (ERR-002), and the follow-up note reflects that the remaining semantic classes intentionally stay SyntaxError pending Neo4j verification.

2. Partial semantic/syntax split. Intentional, now documented. Added a comment at the head of CypherSemanticValidator stating the invariant: only undefined-variable violations throw CommandSemanticException (-> SemanticError, certified by ERR-002); all other failures throw CommandParsingException (-> SyntaxError) pending per-case verification. Aligning the rest is the tracked follow-up in the PR notes.

3. No engine-level outbound Point test. Added type012_enginePointOutbound to Bolt4998PathMappingTest - it runs RETURN point({x:12.34,y:56.78}) against the embedded engine and asserts the real result maps to BoltPointStructure (sig 0x58, srid 7203, x/y). This pins the outbound mapping to the engine's actual point() output in the fast gate, not only in e2e. Confirmed the engine returns a plain Map the mapper re-encodes correctly.

4. toPath invariant. vertices.isEmpty() is guarded; added a comment documenting the vertices.size() == edges.size() + 1 invariant that keeps vertices.get(i + 1) in-range for i < edges.size().

Minor. Renamed fromTemporalStructure -> fromInboundStructure since it now decodes Duration and Point too.

On the two CI reds that are not from this change: the Meterian NodeJS security finding is in base/main node dependencies (this PR touches no dependency manifest - the true diff against the merge base has zero package/lock/pom changes); it clears when the branch picks up main's dependabot bumps. Codacy's remaining items are CodeStyle minors.

[robfrank]

/gemini review

[claude]

Review: native Bolt Duration/Point/Path + semantic errors

Overall this is a clean, well-structured PR. It follows the repo's TDD conventions (tests-first, AssertJ, final everywhere, imports over FQNs, Apache headers), mirrors the existing temporal (#4907) machinery instead of inventing new plumbing, and the scoping notes in the description are honest about what's certified vs. deferred. Test coverage is genuinely strong: outbound, inbound, malformed-payload degradation, forward/backward path hops, and error classification are all exercised at the unit level plus the five e2e suites. Nice work.

A few observations, mostly minor.

Correctness / behavior

• Point misdetection of ordinary user maps (documented tradeoff, worth re-confirming). toPointStructure promotes any Map carrying a crs key (with a recognized value or numeric srid) plus numeric x/y (or longitude/latitude) into a native Point. A user document that legitimately has those fields as unrelated business data will be re-encoded as a wire Point, and any extra keys on that map are silently dropped (only srid/x/y/z survive). The guard is reasonably tight and the Javadoc calls it out, so this is acceptable, but since point() returning a plain Map is an engine implementation detail, it's the one place where behavior could surprise a user. Worth a line in release notes.

• crs/dimensionality mismatch (benign edge case). If a map has crs: "cartesian" (2D) but also carries a z/height, toPointStructure emits a 3D Point (sig 0x59, srid 9157) while the source crs said 2D. Real Neo4j point() sets crs to cartesian-3D when a third coord is present, so genuine data won't hit this, but the srid derived from dimensionality can disagree with the stated crs. Not blocking.

• Path direction + dedup logic looks right. I traced the 1-based signed rel index / 0-based node index encoding against TraversalPath's vertices.size() == edges.size() + 1 invariant, including cycles (a→b→a) and a relationship revisited in opposite directions - the signed magnitude with shared dedup index is the correct Bolt encoding. The empty-path guard on vertices.isEmpty() is a good defensive touch.

• Semantic-vs-syntax split is consistently applied. Every UndefinedVariable throw in CypherSemanticValidator (including the RETURN path via checkExpressionScope, SET/DELETE, pattern predicates, and the ORDER-BY-aggregation case) now throws CommandSemanticException; all other validator failures remain CommandParsingException. Since CommandSemanticException extends CommandParsingException, every existing catch (CommandParsingException) site is unaffected - the only distinguishing point is classifyParsingError. The class Javadoc honestly flags that the other validator failures may also be genuinely semantic and are deferred.

Minor / nits

• Redundant instanceof Map check. toPackStreamValue calls toPointStructure(value) (which does its own value instanceof Map) and then immediately re-checks value instanceof Map for the generic branch. Negligible cost since primitives/collections have already returned by then, but the double test could be collapsed by folding the point check inside the Map branch.

• "UndefinedVariable: Aggregation in ORDER BY is not projected" now maps to SemanticError. That's arguably more correct than SyntaxError, but the UndefinedVariable: prefix on a non-variable condition is a slightly misleading message - not introduced by this PR, just newly surfaced under a new error code.

Performance / security

• All new code is static and stateless; RECOGNIZED_CRS is an immutable Set.of. toPath pre-sizes its indices list. The extra toPointStructure call allocates nothing on the non-point path (early null return). No security-sensitive surface (no logging of payloads, malformed structures degrade to opaque rather than throwing out of RUN parsing). No concerns.

Test coverage

Excellent. The only gap I'd note is that Bolt4998PathMappingTest covers forward and single backward hops but not a cycle / revisited-node path (a→b→a), which is the case the dedup + node-index logic is most likely to regress on. Consider adding one to lock in that the revisited node reuses index 0 and the second rel gets a fresh 1-based index.

Nothing here is blocking. LGTM pending the maintainer's call on the Point-detection tradeoff.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[robfrank]

Addressed the concrete test-coverage item in 78e56f3.

Revisited-node / cycle path test. Added to Bolt4998PathMappingTest: a a->b->a cycle path, asserting the deduplicated encoding - nodes size 2 (the revisited start node reuses index 0), relationships size 2, and indices == [1, 1, 2, 0] (rel 1 forward to node 1, rel 2 forward to node 0). Verified empirically; matches the Bolt Path convention exactly, so the dedup + node-index-reuse path is now locked by a test rather than only hand-traced.

On the remaining observations:

• Point misdetection / extra keys dropped. Accepted, documented tradeoff - the detection is deliberately narrow (recognized crs value or numeric srid + numeric x/y) and mirrors how Cypher itself represents points as maps. Flagged for a release note so the behavior is discoverable.
• crs/dimensionality edge case (crs:"cartesian" with a z). Benign - the engine's point() emits cartesian-3D whenever a third coordinate is present, so real data never reaches the mismatch; leaving dimensionality derived from z-presence rather than special-casing a shape the engine doesn't produce.
• Redundant instanceof Map. Left as-is - collapsing it into the Map branch would require reshaping toPointStructure to take the map directly, and the double check is a no-op on the non-map hot path (early return), so the churn isn't worth the negligible saving you noted.
• "UndefinedVariable: Aggregation in ORDER BY..." message. Pre-existing validator message, not introduced here; left untouched.

[robfrank]

/gemini review

[codacy-production]

Not up to standards ⛔

🔴 Issues 4 minor

Alerts:
⚠ 4 issues (≤ 0 issues of at least minor severity)

Results:
4 new issues

Category	Results
CodeStyle	4 minor

View in Codacy

🟢 Metrics 53 complexity

Metric	Results
Complexity	53

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[gemini-code-assist]

Code Review

This pull request implements native Bolt Duration, Point, and Path structures, and distinguishes semantic from syntax errors. Specifically, it introduces the BoltPointStructure class, adds outbound and inbound mapping for TraversalPath, CypherDuration, and spatial point maps in BoltStructureMapper, and creates a new CommandSemanticException to classify undefined variable errors as SEMANTIC_ERROR rather than SYNTAX_ERROR. Additionally, it updates the conformance specification and end-to-end test suites across C#, Go, JavaScript, Python, and Java to mark these scenarios as passing. There are no review comments to assess, so no feedback on comments is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[claude]

Review: native Bolt Duration/Point/Path + semantic errors (#4997/#4998/#4999)

Reviewed the final combined diff (the review commits correctly re-scoped the semantic-error change after the initial broad retarget). Overall this is a clean, well-tested PR: TDD is evident, comments explain the wire-format reasoning, the exception change is genuinely backward-compatible (CommandSemanticException extends CommandParsingException, so every existing catch (CommandParsingException) site is unaffected), and the inbound decode path is defensively guarded (hasExpectedArity plus try/catch keeps a misbehaving client from crashing the connection). Nice work.

A few observations, none blocking:

1. Point detection is looser than the PR description states (worth tightening)

BoltStructureMapper.toPointStructure keys only on map.containsKey("crs") plus numeric x/y (or longitude/latitude) - it does NOT validate the crs value. The PR description says detection is keyed on "a recognized crs value (cartesian / cartesian-3D / WGS-84 / WGS-84-3D) or a numeric srid", but the code only checks for the presence of the crs key.

Consequence: a user query returning a plain map that happens to carry a crs key and numeric x/y (e.g. RETURN {crs:'foo', x:1, y:2}) would be silently encoded as a native Bolt Point2D and surface in a Neo4j driver as a Point rather than a Map - a behavior change for that edge case. Since CypherPointFunction always emits one of the four recognized CRS names, tightening the guard to "recognized crs name OR numeric srid present" would eliminate the false-positive class without rejecting any legitimate point. Low probability of collision, but the guard is cheaper than the surprise.

2. Semantic-error classification is keyed on message prefix / call site, not error nature

Only the 8 UndefinedVariable: throws became CommandSemanticException; the other genuinely-semantic validator failures (VariableTypeConflict, NestedAggregation, InvalidArgumentType, NoSingleRelationshipType, ...) still throw CommandParsingException mapping to SyntaxError. This is a deliberate, documented follow-up (only certifying what ERR-002 exercises), which is the right conservative call.

One small note: the "UndefinedVariable: Aggregation in ORDER BY is not projected" throw is now classified as SemanticError purely because it shares the UndefinedVariable: message prefix, even though it is really an aggregation-projection issue. Correct outcome (it is semantic), but it shows the split is effectively driven by message text / call site rather than a typed distinction - fine for now, just worth keeping in mind when the follow-up widens the net.

3. Minor: toPointStructure runs ahead of the primitive branches

In toPackStreamValue, the point check sits before the Boolean/String/Number branches, so every scalar value pays one extra instanceof Map check on the record hot path. Negligible in practice, but moving the primitive checks above the point check would keep the common case one test shorter. Optional.

4. Test coverage: strong per-direction, no full echo round-trip

Encode and decode are each well covered (BoltTypeRoundTripTest, PackStreamTypeInputTest, Bolt4998PathMappingTest incl. backward hop and cycle dedup, malformed-struct degradation). The one gap is an explicit encode -> wire -> decode -> re-encode round-trip for Point, which is exactly the behavior the derived crs key in pointMap exists to support - a single test asserting an echoed $point parameter comes back as a native Point would lock that contract in. Nice-to-have.

Correctness spot-checks (all pass)

• Bolt Path index encoding: forward = edge.getOut().equals(from.getIdentity()), 1-based signed rel index, 0-based node index, start node implicit at 0 - matches the spec and the tests.
• BoltException.SYNTAX_ERROR == BoltErrorCodes.SYNTAX_ERROR, so classifyParsingError preserves the previous default exactly.
• SRID/CRS mapping is symmetric across pointSrid and crsForSrid (4326/4979/9157/7203), so points round-trip.
• Empty path and null-crs are guarded.

Static review only; I did not run the build locally (PR reports bolt 253/253 plus five e2e suites green in CI).

[robfrank]

Thanks for the close read. On the four points:

1. Point detection already validates the crs value. The guard isn't presence-only - toPointStructure (BoltStructureMapper.java:434-438) is:

if (!(value instanceof Map<?, ?> map) || !map.containsKey("crs"))
  return null;
final Object crs = map.get("crs");
if ((crs == null || !RECOGNIZED_CRS.contains(crs)) && !(map.get("srid") instanceof Number))
  return null;

So RETURN {crs:'foo', x:1, y:2} is NOT encoded as a Point - 'foo' isn't in RECOGNIZED_CRS and there's no numeric srid, so it falls through to generic map handling. plainMapWithUnrecognizedCrsIsNotPoint in BoltTypeRoundTripTest pins exactly that. The description ("recognized crs value or numeric srid") matches the code. This was tightened earlier in the review; if you were reading the initial commit, that's the discrepancy.

2. Message-prefix-driven split. Agreed it's call-site/message driven rather than a typed taxonomy, and the UndefinedVariable:-prefixed ORDER BY case rides along. Deliberate and documented; widening to a typed distinction is the tracked follow-up.

3. Point check ahead of primitives. Leaving as-is. toPackStreamValue is the single serialization entry point, and the point check sits immediately before the Map branch it guards, which keeps the structural-type handling together and readable. The extra instanceof Map on a scalar is a no-op on correctness and negligible as you note, so I'd rather not reorder a high-traffic method for it.

4. Full echo round-trip. Added in d385984: encode point({x,y}) -> BoltPointStructure -> wire -> fromPackStreamValue -> map (with derived crs) -> re-encode -> BoltPointStructure again (srid 7203, x/y preserved). Locks the derived-crs contract that makes an echoed $point come back as a native Point.

[claude]

Review: native Bolt Duration/Point/Path + semantic errors (#4997, #4998, #4999)

Overall this is a clean, well-scoped, and thoroughly tested PR. The design mirrors the existing temporal work (#4907), reuses the PackStreamStructure/BoltPath machinery, and the defensive inbound decoding (opaque fallback on wrong arity/type) is a nice touch. I verified the wiring end-to-end: PackStreamWriter.writeValue dispatches BoltPointStructure via writeTo, RUN params are decoded through BoltMessage -> fromPackStreamValue, and all UndefinedVariable: throws in CypherSemanticValidator are consistently retargeted. Nice work.

A few observations, mostly minor / for confirmation:

Correctness & design

• Point heuristic false positives (accepted tradeoff, worth restating). toPointStructure will encode any result map carrying crs in {cartesian, cartesian-3D, WGS-84, WGS-84-3D} (or a numeric srid) plus numeric x/y as a native Point on the wire. A user legitimately returning {crs:'cartesian', x:1, y:2} as ordinary document data gets it silently reshaped into a Point. The code comment acknowledges this, and it's the least-bad option given Cypher point() returns a plain Map - just flagging it explicitly since it's a behavior a downstream user could hit.
• Decoded inbound Point is a plain Map, not a spatial value. An inbound Point param decodes to a Map with x/y/z/srid/crs. Echo (RETURN \$p) round-trips correctly (re-detected and re-encoded), but a query that uses the param spatially (e.g. WHERE n.location = \$p or distance(\$p, ...)) receives a generic map, not a real point, so spatial semantics won't apply. That may be fine for the ERR/TYPE conformance scope, but it's a fidelity gap worth a one-line note in the follow-up issue if not already tracked.
• CypherSemanticValidator.java:959 - throw new CommandSemanticException("UndefinedVariable: Aggregation in ORDER BY is not projected") reuses the UndefinedVariable: message prefix for what is really an aggregation-projection error. Classifying it as a semantic error is arguably right, but the misleading prefix means it's swept in via the "only undefined-variable" rule somewhat by accident. Consider a more accurate message prefix.
• Semantic classification only applies when the exception is thrown directly. handleRun maps via classifyParsingError inside catch (CommandParsingException). If CypherSemanticValidator ever runs during execution and its exception is wrapped (caught instead by the generic catch (Exception) -> classifyExecutionError), it would fall back to a non-semantic code. Fine for the current parse-time validation path; just a note if validation ever moves.

Test coverage

• Strong: outbound/inbound round-trips, path dedup + backward-hop + cycle, malformed-struct opacity, null/unrecognized crs negatives, and the error-classification unit tests. Good edge-case discipline.
• Small gap: there's no unit test exercising the srid-only detection branch in toPointStructure (a map with a numeric srid but no recognized crs). Every point test carries a recognized crs, so that branch (and pointSrid's fallback) is only implicitly covered. A one-line case would close it.

Nits

• BoltPointStructure duplicates the signature constants (SIGNATURE_2D/3D) that also live as SIG_POINT_2D/3D in BoltStructureMapper. Not worth a shared constant necessarily, but they must stay in sync by hand.

None of these are blocking. The hard gate (bolt module tests + the five e2e suites) plus the added unit tests give good confidence. LGTM pending the maintainers' call on the Point-as-map inbound fidelity note.

[codecov]

Codecov Report

❌ Patch coverage is 27.96610% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.16%. Comparing base (0ecd0be) to head (d385984).
⚠️ Report is 48 commits behind head on main.

Files with missing lines	Patch %	Lines
...m/arcadedb/bolt/structure/BoltStructureMapper.java	39.50%	43 Missing and 6 partials ⚠️
...om/arcadedb/bolt/structure/BoltPointStructure.java	0.00%	21 Missing ⚠️
...ery/opencypher/parser/CypherSemanticValidator.java	0.00%	8 Missing ⚠️
...m/arcadedb/exception/CommandSemanticException.java	0.00%	6 Missing ⚠️
...in/java/com/arcadedb/bolt/BoltNetworkExecutor.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5006      +/-   ##
============================================
- Coverage     36.17%   36.16%   -0.02%     
+ Complexity     1164     1153      -11     
============================================
  Files          1682     1684       +2     
  Lines        134335   134443     +108     
  Branches      28738    28762      +24     
============================================
+ Hits          48602    48622      +20     
- Misses        76663    76750      +87     
- Partials       9070     9071       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review: Native Bolt Duration, Point, Path + semantic errors (#4997, #4998, #4999)

Reviewed the source (BoltStructureMapper, BoltPointStructure, BoltPath, CypherSemanticValidator, CommandSemanticException, BoltNetworkExecutor classification) and the accompanying tests. This is a well-structured, well-tested PR. Findings below, none blocking.

Strengths

• Real round-trip tests. PackStreamTypeInputTest / BoltTypeRoundTripTest write actual bytes through the production PackStreamWriter, read them back with PackStreamReader, then decode - so the tests exercise the wire format, not just the mapper in isolation. Negative cases (plain map, unrecognized crs, null crs, malformed/short structs) are covered too.
• Defensive inbound decoding. hasExpectedArity + the try/catch (RuntimeException) in fromInboundStructure keep a misbehaving client from propagating an IndexOutOfBounds/ClassCast out of RUN parsing - the parameter just stays opaque. Good call given this runs on the connection thread.
• Backward compatibility. CommandSemanticException extends CommandParsingException, so every existing catch (CommandParsingException) site is unaffected; only the Bolt handler distinguishes it via classifyParsingError. The scoping to the UndefinedVariable case is applied consistently (all 8 UndefinedVariable throw sites converted, everything else left as CommandParsingException).
• Path direction logic is correct and tested. forward = edge.getOut().equals(from.getIdentity()) matching the 1-based signed rel-index convention is verified with forward, backward, and cycle-dedup cases. Nested structures (Point/Path/Duration inside a list/map) serialize correctly because PackStreamWriter.writeValue dispatches on PackStreamStructure.
• Protocol-version caveat documented. The Javadoc on toTemporalStructure ties the legacy (pre-5.0) encoding to SUPPORTED_VERSIONS and leaves a TODO for the day Bolt 5.0 is negotiated - exactly the kind of latent-bug note that saves a future maintainer.

Minor observations (non-blocking)

1. Point vs. user-map ambiguity (by design, worth restating). toPointStructure encodes any map carrying a recognized crs string (or numeric srid) plus numeric x/y as a native Point on the wire. A user who legitimately returns a document with those exact keys gets a Point instead of a Map. The PR acknowledges this and the detection is appropriately tightened (requires the crs key and a recognized value or numeric srid), so it is the least-bad option given Cypher has no Point class - just flagging it as an unavoidable, observable behavior change for that key shape.

2. toPointStructure runs on every scalar (micro-nit). In toPackStreamValue, the point probe (line ~113) executes before the String/Number/Boolean/temporal/UUID branches, so every scalar in a result set pays one extra method call + instanceof Map before reaching its real branch. It is a cheap early-return, so cost is negligible, but folding the point check into the existing Map branch would avoid it on the hot path and read a bit cleaner.

3. One SemanticError classification is a slight stretch. CypherSemanticValidator (line ~959) throws CommandSemanticException("UndefinedVariable: Aggregation in ORDER BY is not projected"), now mapped to SemanticError. It is swept in because it reuses the UndefinedVariable: prefix, but it is really an aggregation-projection rule. The classification is arguably still correct (it is semantic in Neo4j), so this is fine - just noting the message prefix is doing double duty and, per the PR's own "certify only ERR-002" stance, this one wasn't independently verified against real Neo4j.

Test coverage

Strong. Outbound (Duration, cartesian/WGS-84-3D Point, 2-hop/backward/cycle Path), inbound decode (Duration, Point2D/3D, malformed-degrades-to-opaque), full encode->wire->decode->re-encode round trip, negative point-detection cases, and the semantic-vs-syntax exception distinction are all exercised. The docs/superpowers/ artifacts follow an existing repo convention, so no concern there.

Nice work - the defensive decoding and the protocol-version documentation in particular are above par.