fix: populate hop_count on terminal GET events

[sanity]

Summary

Production telemetry bug: hop_count is currently None on the vast majority of terminal GET events because it's computed at log time via op_manager.get_current_hop(id), which returns None whenever the operation has been cleaned up before the response is logged. As a result every dashboard / telemetry / benchmark that looks at GET routing depth is silently working with empty data.

This PR threads the hop count through the wire (positional field on GetMsg::Response) so the originator has a populated value when it constructs GetSuccess / GetNotFound log events. Also fills in hop_count at the relay that emits an exhaustion NotFound (it knows its own max_htl - htl).

After this fix, hop_count is populated on 100% of inline terminal GET events in deterministic-simulation testing across N ∈ {20, 50, 100, 200} (verified during the hop-count benchmark sweep — see #4237).

Scope

Inline GET responses only. Specifically:

• ✅ GetMsg::Response{Found, value: Some(state)} → emits GetSuccess with populated hop_count
• ✅ GetMsg::Response{NotFound} → emits GetNotFound with populated hop_count (interpreted as exhaustion depth, not path-to-storer)
• ❌ GetMsg::ResponseStreaming (large-payload GETs above streaming threshold) — does NOT carry hop_count. Tracked in telemetry: hop_count not populated on streaming GET successes (GetMsg::ResponseStreaming) #4249.
• ❌ GetMsg::Response{Found, value: None} (failure branch) — GetEvent::GetFailure exists in tracing but has no live emission site in the current tree.
• ❌ PUT / UPDATE / SUBSCRIBE all have the same get_current_hop() bug. Tracked in telemetry: populate hop_count on PutSuccess and SubscribeSuccess events #4248.

Wire-format compatibility

Adds a positional field to a bincode-serialized type. Bincode does not honour #[serde(default)] for positional encoding, so cross-version compatibility is gated by MIN_COMPATIBLE_VERSION at the handshake layer:

• Bumps crates/core/Cargo.toml version and min-compatible-version both from 0.2.62 → 0.2.63.
• Bumps crates/fdev/Cargo.toml freenet path dep correspondingly.
• Verified at crates/core/src/transport/connection_handler.rs:2584-2609: handshake gracefully rejects mixed-version peers in both directions via ack_error (no crash).

Test plan

• cargo build -p freenet --features "testing" --tests clean
• test_get_msg_response_hop_count_roundtrip (new) — asserts the new field roundtrips through bincode for Found/NotFound across values {0,1,4,10,64}.
• classify_response_found_preserves_hop_count (new) — asserts the classifier propagates the wire-carried hop_count into Terminal::InlineFound unchanged across the same set of values. Catches a regression where a refactor synthesised 0 (or dropped the field) on the relay bubble-up path.
• Existing 68 GET unit tests still pass.
• test_hop_count_populated_on_terminal_get_events (simulation-level) — kept in tree but #[ignore]d, tracking test: un-ignore test_hop_count_populated_on_terminal_get_events once a GET-producing workload is wired through TestConfig #4250 to un-ignore once a deterministic GET-producing workload is wired through TestConfig.

Defence-in-depth

• Tracing-layer extraction clamps peer-supplied hop_count to op_manager.ring.max_hops_to_live so a malicious or buggy peer can't pollute telemetry with usize::MAX.

Review history

This PR went through a Full multi-perspective review (code-first / testing / skeptical / big-picture / Codex). Findings addressed in commits on this branch:

• Initial rule-review wire-compat / regression-test warnings → b3527ea2
• Full-tier review findings (classifier preservation test, bound clamp, NotFound semantics doc, stale line-number nit) → 9299f54e
• Codex re-review (test reference + streaming-gap acknowledgement) → this commit + the issue links above.

Origin

Extracted from PR #4237 (hop-count sweep benchmark for the Freenet whitepaper). #4237 remains draft for the benchmark scaffolding; this PR is the production fix portion.

[AI-assisted - Claude]

[github-actions]

I have all the context I need. Let me write up the review.

Rule Review: Regression test gap in tracing.rs fix path

Rules checked: git-workflow.md, code-style.md, testing.md, operations.md
Files reviewed: 7

---

Warnings

• crates/core/src/tracing.rs:1302–1355 — The core bug fix (switching from op_manager.get_current_hop(id) to reading hop_count directly from the wire message) is not covered by any enabled test. test_get_msg_response_hop_count_roundtrip verifies bincode round-trip of the field, and classify_response_found_preserves_hop_count verifies the classifier propagates it — but neither asserts anything about the EventKind produced by from_msg_to_event. If the tracing.rs change were reverted to op_manager.get_current_hop(), both unit tests would still pass and the bug would silently reappear. The only test that covers the end-to-end path (test_hop_count_populated_on_terminal_get_events) is #[ignore]d. The rule requires a test that "would catch this exact bug if reintroduced." (rule: testing.md — fix: PRs must include a regression test specific enough to catch the exact bug)

---

Info

• crates/core/src/node/op_state_manager.rs:615 — New doc comment embeds a hardcoded line number (tracing.rs:1271) and a PR reference (PR #4245). Line numbers go stale with every surrounding edit; PR references belong in commit history, not code. Both will mislead future readers. (rule: code-style.md — comments explain WHY not WHAT; don't reference callers or tickets in code)

• crates/core/src/operations/get.rs:78 — #[serde(default)] on hop_count: usize has no effect for bincode (positional encoding ignores serde defaults). The accompanying comment explains this, but the attribute itself implies a backward-compat guarantee that doesn't exist for the actual serializer. Removing it would make the intent clearer and match the pattern used for the subscribe field's existing comment in the same file. (rule: code-style.md — don't add constructs that misrepresent behavior)

---

Summary: One warning around the untested tracing path — the wire-format and classifier tests are solid, but a direct unit test asserting that from_msg_to_event produces hop_count: Some(n) (rather than None) would close the gap. Two minor style notes on the doc comment and the no-op serde attribute.

---

Rule review against .claude/rules/. WARNING findings block merge. ⚠️ 1 warning(s) — fix or add review-override label

[sanity]

Comprehensive PR Review: #4245

Summary

• PR Title: fix: populate hop_count on terminal GET events
• Type: fix (wire-format change)
• Review tier: Full (wire format is a high-risk surface)
• Reviewers run: freenet:code-first, freenet:testing, freenet:skeptical, freenet:big-picture, Codex (twice — once on initial submission, once on the fix commits)
• HEAD reviewed: 85557cf7 (post fix commits)

---

Code-First Analysis

Independent understanding: PR adds positional hop_count: usize to GetMsg::Response. Storer fills max_htl - htl, relay bubble-up preserves it verbatim, HTL-exhaustion NotFound fills its own max_htl - htl. Tracing-layer extraction reads from the wire instead of the stub op_manager.get_current_hop().

Alignment: matches the PR description for inline Response. Two scope gaps that the description correctly acknowledges:

• GetMsg::ResponseStreaming not addressed (streaming GETs unchanged).
• GetFailure event has no live emission site in the tree — the "Success/NotFound/Failure" framing is partly aspirational.

Wire-compat: verified at handshake — connection_handler.rs:2584-2609 cleanly rejects mixed 0.2.62↔0.2.63 in both directions via ack_error.

---

Testing Assessment

Test Type	Status	Notes
Unit (wire roundtrip)	✅	test_get_msg_response_hop_count_roundtrip in get.rs — Found/NotFound across {0,1,4,10,64}
Unit (classifier)	✅	classify_response_found_preserves_hop_count in op_ctx_task.rs — pins the bubble-up path
Simulation	⚠️	test_hop_count_populated_on_terminal_get_events is #[ignore]d; tracks #4250
E2E	N/A

Regression Test: present and adequate at the unit level. Initial testing-reviewer concern about "presence-only" assertion has been addressed: there are now two unit tests pinning both the wire format and the classifier-side propagation. The blocking ask was the classifier pin — done.

Remaining gap: no end-to-end integration test exercises the storer→relay→originator chain. The two unit tests catch the most likely regression modes (field dropped, classifier synthesised 0, refactor lost the field) but not "a relay overwrites with its own htl in relay_send_found". Mitigation: the skeptical reviewer verified by code reading that all relay_send_found callers correctly forward downstream_hop_count. Tracking #4250 covers building the simulation harness for end-to-end coverage.

---

Skeptical Findings

Concern	Severity	Disposition
Wire-format handshake mixed-version crash risk	HIGH (theoretical)	Verified safe by reviewer at connection_handler.rs:2584-2609 — graceful ack_error rejection in both directions
Relay-preservation invariant could regress	HIGH (test gap)	All current GetMsg::Response construction sites verified to use the correct source (skeptical); classifier pin test added; relay_send_found end-to-end coverage tracked in #4250
Peer-supplied hop_count = usize::MAX pollutes telemetry	MED	Addressed: clamped to op_manager.ring.max_hops_to_live at both extraction sites in tracing.rs
NotFound.hop_count analytics ambiguity	MED	Addressed: docstring clarifies it's exhaustion depth, not path-to-storer
usize overflow / off-by-one at HTL=0 boundary	LOW	Reviewer-verified math is correct; comment-clarification ask not acted on
Old hop_count: 0 literals in tests don't assert preservation	LOW	Untouched — pre-existing; not regressed

---

Big Picture

• Goal alignment: yes. PR does exactly what its title and body claim.
• Scope: tightly scoped, all 7 changed files justified.
• Removed code: none destructive. get_current_hop() stub kept for the remaining PUT caller; documentation updated to reflect current state.
• Counterpart bugs (PutSuccess/UpdateSuccess/SubscribeSuccess): confirmed to exist via the same root cause. Tracked in #4248.
• Streaming gap: tracked in #4249.
• #[ignore]d test follow-up: tracked in #4250.

---

Documentation

• Code docs: complete. hop_count field has a load-bearing docstring at get.rs:96-112 explaining semantics and wire-compat handling.
• op_state_manager.rs:613 stub docstring updated to reflect that only PUT still consumes it.
• tracing.rs:1322-1333 clarifies that NotFound hop_count is exhaustion depth.

---

Recommendations

Must Fix (Blocking)

All blocking findings from the initial review were addressed in the fix commits (9299f54e, c040837e, 85557cf7).

Should Fix (Important)

All "should fix" items either addressed in-PR or tracked as follow-up issues (#4248, #4249, #4250).

Consider (Suggestions)

1. Off-by-one chain-of-reasoning comment at op_ctx_task.rs:1967-1970 could be tightened (low-priority readability).
2. Several existing hop_count: 0 literals in pre-existing tests (get.rs:283, 304, 366, 384; op_ctx_task.rs:2481, 2497, 2582) could carry non-zero values and assert preservation — would catch a future struct-shape refactor that loses the field. Not blocking.

False positives dismissed

• Codex flagged fdev binstall URLs pointing at v0.2.62 — crates/fdev/Cargo.toml:13-16 has an explicit comment that release.yml rewrites these URLs on each version bump (issue #3995). Not a finding.
• Big-picture reviewer's "29 file scope" measurement (using three-dot diff) was against a stale local main — actual PR is the expected 7 files.

---

Verdict

State: Ready to Merge
HEAD SHA reviewed: 85557cf7

All blocking findings addressed in the fix commits on this branch:

• Testing-reviewer's classifier-preservation pin → classify_response_found_preserves_hop_count
• Skeptical M1 (bound clamp) → tracing.rs clamp to max_hops_to_live
• Skeptical M2 (NotFound semantics) → docstring
• Big-picture stale-line-number nit → fixed
• Codex re-review's ignore-reason missing tracking issue → references #4250
• Streaming gap, non-GET counterparts, sim-test un-ignore → tracked in #4249/#4248/#4250

The wire-format change is the most consequential part. The skeptical reviewer verified the handshake guards are correct in both directions, and every GetMsg::Response construction site uses the correct hop_count source. The unit tests pin the two most-likely-to-regress paths (wire roundtrip + classifier propagation).

[AI-assisted - Claude]