Skip to content

refactor(#5082): make writeTo() the sole header authority in PackStreamStructure - #5112

Merged
robfrank merged 7 commits into
mainfrom
feat/5082-bolt-writeto-header-authority
Jul 8, 2026
Merged

refactor(#5082): make writeTo() the sole header authority in PackStreamStructure#5112
robfrank merged 7 commits into
mainfrom
feat/5082-bolt-writeto-header-authority

Conversation

@robfrank

@robfrank robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #5082. Follow-up to #5001 (PR #5059). Part of the Bolt certification epic #4882 / tracking issue #4890.

Removes getSignature() and getFieldCount() from the PackStreamStructure interface and all seven implementers, making writeTo() the sole authority for Bolt structure-header emission. After #5001 added Bolt 5.x version-gated encoding, these no-arg accessors became inconsistent with writeTo() on version-gated structs (they cannot see the negotiated version - only the writer carries it), leaving a latent footgun: any future writeStructureHeader(getSignature(), getFieldCount()) call site would emit a header/body mismatch and corrupt the stream on a 5.x connection.

No wire-behavior change on any negotiated Bolt version (3.0-5.4) - this is dead-accessor removal, not a re-encode. No writeTo() body logic changed.

Changes

Production

  • PackStreamStructure reduced to a single-method contract: void writeTo(PackStreamWriter).
  • BoltPointStructure.writeTo (the only internal consumer of the writeStructureHeader(getSignature(), getFieldCount()) idiom) now inlines its version-invariant signature (z == null ? 0x58 : 0x59) and field count (3/4) locally.
  • BoltNode, BoltRelationship, BoltUnboundRelationship, BoltPath, BoltTemporalStructure, BoltDateTimeStructure drop the two now-unused overrides; their writeTo() bodies already wrote headers with inline literals and are untouched.

Not touched (verified different types, each keeps its own methods): PackStreamReader.StructureValue and BoltMessage (the inbound-decode + message paths in BoltStructureMapper.fromInboundStructure / BoltMessage.parse).

Tests - accessor assertions on interface implementers converted to stronger, wire-truth assertions:

  • BoltStructureTest (Node/Rel/UnboundRel/Path): assert the serialized TINY_STRUCT marker + signature bytes.
  • BoltTypeRoundTripTest: TYPE-011 relies on the existing wire round-trip (drops redundant pre-wire asserts); TYPE-012 Point uses getZ() null-ness (the sole determinant of the 2D/3D signature).
  • Bolt4998PathMappingTest: TYPE-012 Point → getZ().
  • BoltDateTimeStructureTest needed no change - all its getSignature() calls are on the PackStreamReader.StructureValue returned by its roundTrip()/mapperRoundTrip() helpers, which already serialize through writeTo().

Verification

  • mvn -pl bolt verify - BUILD SUCCESS, 275 unit tests pass.
  • mvn -pl bolt verify -DskipITs=false - real-driver ITs (neo4j-java-driver against a live server across negotiated Bolt 3.0-5.4, incl. BoltProtocolIT, RemoteBoltDatabaseIT, BoltVersionNegotiationTest, temporal/point/path/counters ITs) all pass. The one flake observed - BoltProtocolIT.concurrentSessions failing with User 'root' is not allowed to update schema - is a pre-existing concurrent-schema-creation race unrelated to serialization; it passed on isolated re-run.
  • Confirmed no residual getSignature()/getFieldCount() references remain in structure/ or the interface.

Acceptance criteria

  • PackStreamStructure no longer exposes getSignature() / getFieldCount(); writeTo() is the only header contract.
  • No @Override accessor remains on any of the seven implementers.
  • No behavioral change on the wire for any negotiated Bolt version (3.0-5.4).
  • mvn -pl bolt verify green.

🤖 Generated with Claude Code

robfrank and others added 4 commits July 8, 2026 09:51
…treamStructure

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gnature/getFieldCount

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…amStructure

Remove getSignature()/getFieldCount() from the PackStreamStructure interface and
all seven implementers; inline the header write in BoltPointStructure.writeTo (its
only internal consumer). writeTo() is now the single source of truth for structure
headers, which the version-gated structs already require since only the writer
carries the negotiated Bolt version. No wire change on any version (3.0-5.4).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mergify

mergify Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

  • Queue this pull request

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

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 gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Bolt protocol serialization layer by removing the getSignature() and getFieldCount() methods from the PackStreamStructure interface and all of its implementers, establishing writeTo() as the sole authority for structure-header emission. Corresponding unit tests have been updated to assert on serialized wire bytes or specific properties instead of the removed accessors. Feedback suggests removing a redundant assertion on p.getZ() in BoltTypeRoundTripTest.

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.

Comment thread bolt/src/test/java/com/arcadedb/bolt/BoltTypeRoundTripTest.java Outdated
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Code Review: PR 5112 — writeTo() sole header authority

Reviewed the full diff, the remaining getSignature()/getFieldCount() call sites, and the writer's default version. This is a clean, low-risk refactor and I'd be comfortable merging it.

Correctness

  • The removal is complete and the boundary is drawn correctly. The three surviving getSignature()/getFieldCount() call sites (BoltMessage.parse, BoltStructureMapper.fromInboundStructure, BoltNetworkExecutor) all operate on PackStreamReader.StructureValue / BoltMessage, which are distinct types that keep their own accessors, so nothing in the inbound-decode path is affected. Verified by grep against bolt/src/main.
  • BoltPointStructure.writeTo inlining is behavior-identical: signature/fieldCount are computed from the same 'z == null' predicate the accessors used, at the same call site. No re-encode.
  • Deleting the accessors on the version-gated structs (BoltNode/BoltRelationship/BoltUnboundRelationship/BoltDateTimeStructure) is the right call: they returned the Bolt-4.x base shape while writeTo() emits the 5.x shape, so they were genuinely inconsistent and only safe because nothing consumed them. Collapsing to a single-method contract removes the footgun rather than papering over it.

Test coverage

  • The conversions are strictly stronger, not weaker: BoltStructureTest now pins the actual emitted TINY_STRUCT marker (0xB0 | fieldCount) + signature byte via a real writeTo(), which is the wire truth the old accessor asserts only approximated.
  • Confirmed the wireHeader helper's implicit assumption: PackStreamWriter's default boltMajorVersion = 4, so the 3/5/3 field-count expectations are correct for the un-negotiated default.
  • Moving the Point 2D/3D assertions to getZ() null-ness is sound since z is the sole determinant of the signature; the round-trip ITs still guard the actual 0x58/0x59 bytes on the wire.

Minor observations (non-blocking)

  1. wireHeader reads only header[0]/header[1] — fine given every struct here is a TINY_STRUCT (< 16 fields, single-byte marker). If a structure ever grew past 15 fields the marker would become STRUCT_8/STRUCT_16 and the offsets would shift, but that isn't reachable for these Bolt types. Just noting the assumption isn't asserted.
  2. Design/plan docs (~508 lines) under docs/superpowers/ read as internal agent-workflow artifacts (checkbox task lists, worktree paths, 'REQUIRED SUB-SKILL' directives) rather than user-facing docs. Worth deciding whether they belong in repo history; not a code concern either way.

Style / conventions

  • final on locals/params, single-child if without braces, no System.out, no Claude author attribution in source, all consistent with CLAUDE.md.

Nice work. The 'no wire-behavior change, dead-accessor removal' framing is accurate, and the compile-time safety net (any missed caller becomes a build error) plus the real-driver ITs make this about as safe as a refactor gets.

Fold the 2D/3D signature-intent comment onto the existing z-value assertions
instead of a separate redundant getZ() check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Both observations are non-blocking and neither needs a code change - reasoning below.

1. wireHeader reading only header[0]/header[1]. The single-byte-marker assumption is structurally guaranteed, not merely true for today's struct set. PackStreamWriter.writeStructureHeader only emits a TINY_STRUCT marker and throws IOException("Structure field count too large") for fieldCount >= 16:

if (fieldCount < 16) {
  out.writeByte(TINY_STRUCT | fieldCount);
} else {
  throw new IOException("Structure field count too large: " + fieldCount);
}
out.writeByte(signature);

So any serializable Bolt structure has a one-byte marker at header[0] and the signature at header[1] by construction - a STRUCT_8/STRUCT_16 header can never reach the wire through this writer. On top of that, the existing assertThat(header[0]).isEqualTo((byte)(0xB0 | n)) equality would fail loudly first if the marker ever shifted. I'd rather not assert a can't-happen invariant in every test and add noise, so leaving as-is.

2. docs/superpowers/ spec + plan artifacts. Intentional and consistent with the repo convention - the spec/plan pair is committed within the feature PR here (same as the sibling Bolt cert PRs #5006/#5016/#5059/#5085). Happy to drop or relocate them if you'd prefer they stay out of history; maintainer's call.

The independent confirmation of the boltMajorVersion = 4 default and the inbound-decode boundary matches what the diff relies on.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review: refactor(#5082) make writeTo() the sole header authority

Reviewed the full diff plus the surrounding bolt code. This is a clean, low-risk dead-accessor removal. I verified the core claims independently:

Correctness verified

  • All 7 implementers of PackStreamStructure had their getSignature()/getFieldCount() overrides removed; none remain. ✅
  • The only production dispatch, PackStreamWriter.writeValue (line 295-296), routes structures exclusively through writeTo(this) - it never touched the removed accessors, so this is genuinely inert on the wire. ✅
  • The remaining getSignature()/getFieldCount() references in BoltNetworkExecutor, BoltMessage.parse, BoltStructureMapper.fromInboundStructure, and PackStreamReader.StructureValue all operate on StructureValue/BoltMessage (different types), confirmed by reading each call site. Correctly left untouched. ✅
  • BoltPointStructure.writeTo inlining preserves exact behavior: signature = z == null ? SIGNATURE_2D : SIGNATURE_3D, fieldCount = z == null ? 3 : 4 matches the old accessors byte-for-byte.
  • The new wireHeader() test helper uses a default PackStreamWriter (boltMajorVersion = 4), so the asserted field counts (3/5/3) correctly pin the Bolt 4.x path.

Nits / optional

  1. Unit coverage doesn't guard the actual footgun. The motivation is the 5.x version-gated header mismatch (Node 3->4, Rel 5->8, DateTime 'F'/'f'->'I'/'i'), but the new wireHeader assertions only exercise the default 4.x writer. The 5.x header shape is only covered by the real-driver ITs. A cheap, fast unit guard would be a writer.boltMajorVersion(5) variant asserting header[0] == (byte)(0xB0 | 4) for a Node and (0xB0 | 8) for a Relationship - that locks the version-gated counts at the unit level where they're most likely to regress silently. Not blocking, since ITs cover it.

  2. ~508 lines of planning docs committed under docs/superpowers/plans/ and docs/superpowers/specs/. If these are intended as permanent design records, fine; if they were ephemeral agent scratch artifacts, they may not belong in the repo. Worth a conscious call.

  3. Minor: the final commit folds the getZ() intent comment onto the value assertions, which reads well. No concern.

Style/CLAUDE.md compliance: final used on the new helper's params/locals, single-method interface with clear Javadoc, no leftover debug, no @author/Claude attribution in source. Tests converted to stronger wire-truth assertions - a genuine improvement over the old construction smoke-checks.

Overall: LGTM. The refactor does exactly what it claims with zero wire-behavior risk. Consider adding the 5.x unit assertion (nit 1) since that's the specific case the change exists to protect.

@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/gemini review

@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@claude please re-review the updated head (6f49a87) - the redundant getZ() assertions flagged in the first pass have been collapsed onto the existing value assertions.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @robfrank's task in 59s —— View job


Re-review: updated head 6f49a871a

  • Gather context (diff since last review, latest commit 6f49a87)
  • Verify the redundant getZ() assertions were properly collapsed
  • Re-review full diff for any other issues
  • Post final review

Verified the fix in 6f49a87 (bolt/src/test/java/com/arcadedb/bolt/BoltTypeRoundTripTest.java):

  • type012_cartesianPointNative: the standalone assertThat(p.getZ()).isNull(); // z absent -> ... line is gone. The intent comment is now folded onto the remaining assertThat(p.getZ()).isNull() at the end of the test (line 116). No duplicate assertion remains.
  • type012_wgs84Point3DNative: same treatment - the standalone assertThat(p.getZ()).isNotNull(); // z present -> ... line is removed, and the comment is folded onto assertThat(p.getZ()).isEqualTo(100.0) (line 132).

This is exactly what was described in the commit message and matches Gemini's original suggestion (fold the comment rather than leave a bare comment or duplicate check). Both tests still assert getZ()'s null-ness (the sole determinant of the Point2D/Point3D signature per the PR's stated rationale), just once instead of twice.

No other files changed in this commit - git diff origin/main...HEAD --stat confirms 6f49a87 only touches BoltTypeRoundTripTest.java (+2/-6 net across the two tests).

Rest of the diff: unchanged since the earlier review passes (PackStreamStructure reduced to the single writeTo() method, all seven implementers dropped their accessor overrides, BoltPointStructure.writeTo inlines the signature/field-count computation, BoltStructureTest's wireHeader() helper correctly asserts on real serialized bytes). Nothing new to flag there.

Note: I wasn't able to run mvn -pl bolt test in this session (sandboxed shell blocked the Maven invocation), so this is a static/code-reading verification rather than a fresh test run. The diff is small and mechanical enough that I'm confident in the result; the PR description's own mvn -pl bolt verify run (275 tests, BUILD SUCCESS) plus the earlier review passes already exercised this.

LGTM - the flagged nit is resolved, no new issues introduced.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Bolt serialization layer by removing the getSignature() and getFieldCount() methods from the PackStreamStructure interface and its seven implementing classes, establishing writeTo() as the sole authority for structure-header emission. The tests have been updated to assert on serialized wire bytes instead of using the removed accessors. The review feedback suggests enhancing the test coverage in BoltTypeRoundTripTest by explicitly verifying the serialized wire bytes (the TINY_STRUCT marker and signature) for both 2D and 3D points to prevent silent regressions.

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.

Comment thread bolt/src/test/java/com/arcadedb/bolt/BoltTypeRoundTripTest.java Outdated
Comment thread bolt/src/test/java/com/arcadedb/bolt/BoltTypeRoundTripTest.java Outdated
…level

Add a boltMajorVersion(5) wireHeader variant and assert Node 3->4, Relationship
5->8, UnboundRelationship 3->4 element_id header widening - the exact case the
removed accessors got wrong - so a version-gate regression fails fast in unit
tests rather than only in the real-driver ITs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Nit 1 (5.x unit coverage) implemented in b67f0a7 - it's the right call, since 4.x-only unit assertions left the exact regression the PR targets guarded only by the ITs.

Added versionGatedStructuresEmitBolt5HeaderShape in BoltStructureTest, driven by a new wireHeader(struct, boltMajorVersion) overload that serializes at boltMajorVersion(5):

  • BoltNode -> header[0] == (byte)(0xB0 | 4) (3 -> 4, adds element_id)
  • BoltRelationship -> (byte)(0xB0 | 8) (5 -> 8, adds element ids)
  • BoltUnboundRelationship -> (byte)(0xB0 | 4) (3 -> 4, adds element_id)

The BoltDateTimeStructure 'F'/'f' -> 'I'/'i' signature flip is already unit-covered by BoltDateTimeStructureTest (its roundTrip helper serializes at major 5), so the version-gated set is now fully pinned at the unit level. mvn -pl bolt test -Dtest=BoltStructureTest green (55 tests).

Nit 2 (superpowers docs) is the same point from the first pass - intentional per the repo convention (committed within the feature PR, as with the sibling Bolt cert PRs); maintainer's call on whether to keep them in history.

@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/gemini review

@robfrank

robfrank commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@claude one more pass on the latest head (b67f0a7) - added the Bolt 5.x version-gated header-shape unit guard you suggested (Node 3->4, Rel 5->8, UnboundRel 3->4).

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @robfrank's task in 1m 27s —— View job


Re-review: latest head b67f0a774

  • Gather context (diff since last review, latest commit b67f0a7)
  • Review the new Bolt 5.x version-gated header-shape unit guard
  • Re-check rest of diff for regressions
  • Attempt mvn -pl bolt test -Dtest=BoltStructureTest (blocked - see note below)
  • Post final review

Verified b67f0a7 (bolt/src/test/java/com/arcadedb/bolt/BoltStructureTest.java, +26/-1, only file touched by this commit per git show --stat):

  • New wireHeader(s, boltMajorVersion) overload correctly wraps the existing default via new PackStreamWriter().boltMajorVersion(boltMajorVersion) - PackStreamWriter.boltMajorVersion(int) is a real builder method (PackStreamWriter.java:90), not new API surface invented for the test.
  • New test versionGatedStructuresEmitBolt5HeaderShape asserts, at boltMajorVersion(5):
    • BoltNode -> header[0] == (0xB0 | 4) - matches BoltNode.writeTo()'s writer.getBoltMajorVersion() >= 5 branch, which calls writeStructureHeader(SIGNATURE, 4) (id, labels, properties, elementId).
    • BoltRelationship -> header[0] == (0xB0 | 8) - matches its >= 5 branch, writeStructureHeader(SIGNATURE, 8) (id, startNodeId, endNodeId, type, properties, elementId, startNodeElementId, endNodeElementId).
    • BoltUnboundRelationship -> header[0] == (0xB0 | 4) - matches its >= 5 branch, writeStructureHeader(SIGNATURE, 4) (id, type, properties, elementId).
    • Each also asserts header[1] equals the class's SIGNATURE constant, unaffected by the version gate.
  • Constructor call sites in the new test match each class's actual constructor arity/order (BoltNode(id, labels, properties, elementId), BoltRelationship(id, start, end, type, properties, elementId, startElementId, endElementId), BoltUnboundRelationship(id, type, properties, elementId)), and all needed types (BoltNode, BoltRelationship, BoltUnboundRelationship, List, Map) are already imported in the file - compiles cleanly by inspection.
  • This closes exactly the gap flagged in my prior pass: the version-gated header shape (the specific regression this PR exists to prevent) is now pinned at the unit level, not just via the real-driver ITs.

Rest of the diff: unchanged since the previous re-review of 6f49a87 - git diff origin/main...HEAD --stat matches the PR's stated file list 1:1, no other files moved.

Note on verification: as in the prior pass, the sandboxed shell in this session blocks mvn invocation entirely (requires interactive approval unavailable here), so this is a static/code-reading verification (cross-checking the test's byte assertions against each writeTo() implementation line-by-line) rather than a fresh build run. Given the change is a small, self-contained test addition and the arithmetic checks out exactly against the source, I'm confident in the result. robfrank's comment states mvn -pl bolt test -Dtest=BoltStructureTest passed locally (55 tests).

LGTM - the 5.x unit guard is correct and closes the coverage gap from the earlier review round. No new issues found.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the PackStreamStructure interface and its seven implementers by removing the getSignature() and getFieldCount() methods, making writeTo() the sole authority for Bolt structure-header emission. The signature and field count logic for BoltPointStructure has been inlined directly into its writeTo() method. Additionally, the unit tests have been updated to assert on the serialized wire bytes or specific properties instead of using the removed accessors, and design/plan documentation has been added. There are no review comments, so I have no feedback to provide.

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.

BoltPointStructure.writeTo is the one production header path this PR inlined, so
pin its emitted marker + signature bytes directly (Point2D 0xB3/0x58, Point3D
0xB4/0x59) alongside the getZ() value checks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codacy-production

codacy-production Bot commented Jul 8, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

🟢 Coverage 100.00% diff coverage · -7.54% coverage variation

Metric Results
Coverage variation -7.54% coverage variation
Diff coverage 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (38489b0) 136228 101912 74.81%
Head commit (6ca0816) 168054 (+31826) 113043 (+11131) 67.27% (-7.54%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#5112) 3 3 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

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

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review: refactor(#5082) - make writeTo() the sole header authority in PackStreamStructure

Reviewed the full diff and cross-checked against the current source. This is a well-scoped, low-risk cleanup and the reasoning in the PR description holds up against the code. Recommend merge.

What is good

  • Correct problem framing. The removed accessors (getSignature()/getFieldCount()) genuinely could not be version-correct: they are no-arg interface methods, while the 5.x header shape depends on writer.getBoltMajorVersion(). Confirmed the only serialization path is PackStreamWriter.writeValue -> ((PackStreamStructure) value).writeTo(this) (PackStreamWriter.java:295-296), so the accessors were dead for outbound encoding. Removing them eliminates a real footgun rather than just trimming code.
  • Behavior-preserving by construction. No writeTo() body logic changed. The one internal consumer (BoltPointStructure.writeTo) inlines a version-invariant signature/count locally - identical bytes.
  • Scope verification is accurate. Confirmed exactly 7 implementers and that the only remaining getSignature() caller in main source is BoltStructureMapper.fromInboundStructure (BoltStructureMapper.java:579), which operates on PackStreamReader.StructureValue - a different type, correctly left untouched. No orphaned callers anywhere in the repo.
  • Tests got stronger, not weaker. Swapping construction-time accessor asserts for on-the-wire header-byte asserts pins the thing that actually matters. The marker encoding in the tests (0xB0 | fieldCount) matches writeStructureHeader (PackStreamWriter.java:265-272), and all field counts (3/4/5/8) are safely < 16.
  • versionGatedStructuresEmitBolt5HeaderShape is the standout addition. It pins the Node 3->4 / Rel 5->8 / UnboundRel 3->4 widening at the unit level - exactly the case the old accessors got wrong - so a version-gate regression now fails fast instead of only surfacing in the real-driver ITs. Good instinct.

Minor / non-blocking

  • Interface Javadoc could name the version accessor. PackStreamStructure.writeTo new comment says only the writer carries the negotiated Bolt version. A pointer to PackStreamWriter.getBoltMajorVersion() would help the next reader connect the contract to the mechanism. Purely a nicety.
  • wireHeader indexes header[0]/header[1] unguarded. Fine given every writeTo() emits a header first, but a one-line assertThat(header.length).isGreaterThanOrEqualTo(2) would give a clearer failure if a future struct ever regressed to emitting nothing. Optional.
  • Committed planning docs (docs/superpowers/specs + plans, ~508 lines) are consistent with the established convention in this repo (59 existing files there), so no objection; flagging only for awareness since they dominate the line count.

Verification I ran

  • Confirmed no residual getSignature()/getFieldCount() references in structure/ or the interface.
  • Confirmed PackStreamWriter.boltMajorVersion(int) returns the builder and defaults to 4, matching the test helper usage.
  • Confirmed the sole writeTo dispatch and the 7-implementer set.

Nice, surgical change with a net improvement in test fidelity.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review: refactor of PackStreamStructure — writeTo() as sole header authority

Reviewed the full diff, the two remaining getSignature()/getFieldCount() call sites, PackStreamWriter, and the DateTime test coverage. This is a clean, low-risk, well-motivated refactor. Recommendation: approve.

What is good

  • Correct problem framing. Removing the no-arg accessors is the right fix. Since only the writer carries the negotiated Bolt version, a no-arg getSignature()/getFieldCount() structurally cannot agree with writeTo() on version-gated structs (Node 3->4, Rel 5->8, DateTime F/f->I/i at Bolt >=5). Collapsing to a single-method contract removes the footgun by construction, not by convention.
  • Behavior-preserving. No writeTo() body logic changed; the only internal consumer (BoltPointStructure.writeTo) inlines its version-invariant signature/field-count locally. Any missed caller would be a compile error, not a runtime surprise.
  • Verified untouched call sites are genuinely different types. BoltNetworkExecutor:402 (message.getSignature() on BoltMessage) and BoltStructureMapper:579 (structure.getSignature() on PackStreamReader.StructureValue) are the inbound-decode/message paths and are correctly left alone.
  • Tests got stronger, not weaker. Converting accessor asserts to raw-wire-byte asserts (0xB0 | fieldCount marker + signature byte) pins the actual emitted header. Confirmed TINY_STRUCT = 0xB0 matches the 0xB0 | n used in the tests.
  • versionGatedStructuresEmitBolt5HeaderShape is a valuable addition - it pins the Bolt-5 header widening (Node->4, Rel->8, UnboundRel->4) at the unit level, exactly the case the removed accessors got wrong. Good regression guard.

Minor notes (non-blocking)

  • DateTime 5.x coverage confirmed present. BoltDateTimeStructure is version-gated but has no new unit test in this PR; that is fine - BoltDateTimeStructureTest already round-trips through writeTo() at both major 4 and 5 and asserts the resulting signature (F/f vs I/i). Coverage is real, just not new here.
  • docs/superpowers/plans and docs/superpowers/specs are AI-generated planning artifacts (checkbox task lists, worktree paths). Worth a maintainer decision on whether these belong in the main tree long-term - they do not affect the build, but are process scaffolding rather than user/dev docs. No action required for this PR.
  • Consistency nit (optional): BoltPointStructure.writeTo now computes signature/fieldCount as locals, while BoltNode/BoltRelationship/BoltUnboundRelationship inline the same distinctions directly in the writeStructureHeader(...) args across an if (major >= 5) branch. Both are fine; not worth churning.

Test coverage

Adequate and improved. Wire-byte assertions at both Bolt 4.x and 5.x, plus the existing real-driver ITs across negotiated 3.0-5.4, cover the behavior-preservation claim. The one flake called out in the description (concurrentSessions schema-update race) is pre-existing and unrelated to serialization.

Nice, surgical change with the reasoning fully documented.

@robfrank
robfrank merged commit a854e5d into main Jul 8, 2026
25 of 30 checks passed
@robfrank
robfrank deleted the feat/5082-bolt-writeto-header-authority branch July 8, 2026 08:46
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.44%. Comparing base (38489b0) to head (6ca0816).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5112      +/-   ##
============================================
- Coverage     65.83%   65.44%   -0.39%     
+ Complexity      909      906       -3     
============================================
  Files          1692     1692              
  Lines        136228   136216      -12     
  Branches      29113    29114       +1     
============================================
- Hits          89680    89141     -539     
- Misses        34355    34877     +522     
- Partials      12193    12198       +5     

☔ 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.

robfrank added a commit that referenced this pull request Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bolt: make writeTo() the sole header authority in PackStreamStructure (drop getFieldCount()/getSignature())

1 participant