feat(lsp): return optional from PositionMapper boundary methods

[16bit-ykiko]

Summary

• to_position and to_offset now return std::optional instead of asserting on out-of-range input, since they receive untrusted LSP client input
• Internal helpers (line_of, line_start, etc.) keep their asserts — validation at the trust boundary, not everywhere
• Added [[unlikely]] on all nullopt return paths
• Added 3 new test cases for out-of-bounds: offset beyond document, line beyond document, character beyond line

Test plan

• All 14 language_position tests pass
• Full test suite: 647/647 pass

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Enhanced position mapping validation to handle out-of-range offsets and positions gracefully.

• Tests

  • Added comprehensive test coverage for boundary conditions and invalid scenarios across different character encodings.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PositionMapper class methods to_position() and to_offset() were refactored to return std::optional instead of raw values. These methods now include bounds validation and return std::nullopt for invalid inputs rather than relying on assertions or undefined behavior.

Changes

Cohort / File(s)	Summary
Header & Declaration include/eventide/ipc/lsp/position.h	Updated method signatures to return std::optional<protocol::Position> and std::optional<std::uint32_t>. Added #include <optional> dependency.
Implementation Logic src/ipc/lsp/position.cpp	Added bounds validation to to_position() for offset range and computed line/column validity. Replaced assert-based checks in to_offset() with runtime validation returning std::nullopt for invalid positions, UTF-8 errors, and encoding mismatches.
Test Coverage tests/unit/ipc/lsp/position_tests.cpp	Updated existing tests to handle nullable returns via .has_value() checks and dereferencing. Added new test cases for out-of-range offsets, invalid line/character indices, and EOF boundary conditions across encodings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A mapper once lived, rough and bare,
With methods that'd crash without care!
Now optional-wrapped, with bounds held tight,
Each byte checked thrice—what a delight!
No more asserts in the dead of night. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly and specifically describes the main change: converting PositionMapper boundary methods to return optional types instead of asserting on invalid input.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/lsp-position-optional

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/ipc/lsp/position.cpp (1)

244-246: Defensive check appears unreachable but harmless.

Given the guard at line 239 (offset > content.size()), any offset <= content.size() will always satisfy line_start(line) + column <= line_end_exclusive(line) since column = offset - line_start(line) and offset cannot exceed the line's valid range when within content bounds.

This check is dead code but serves as a defensive safety net. Consider removing it for clarity, or adding a comment explaining it guards against future invariant breakage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ipc/lsp/position.cpp` around lines 244 - 246, The extra guard
`if(line_start(line) + column > line_end_exclusive(line))` is unreachable given
the prior `offset > content.size()` check but exists as a defensive safety net;
update the `position.cpp` logic around the function that computes position from
`offset` (referencing `line_start(line)`, `line_end_exclusive(line)`, `column`,
and `offset`) by either removing the dead check or, preferably, keeping it and
adding a concise comment clarifying it is a defensive invariant check to protect
against future invariant breakage or corrupted input, so readers know why the
seemingly unreachable branch remains.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/ipc/lsp/position.cpp`:
- Around line 244-246: The extra guard `if(line_start(line) + column >
line_end_exclusive(line))` is unreachable given the prior `offset >
content.size()` check but exists as a defensive safety net; update the
`position.cpp` logic around the function that computes position from `offset`
(referencing `line_start(line)`, `line_end_exclusive(line)`, `column`, and
`offset`) by either removing the dead check or, preferably, keeping it and
adding a concise comment clarifying it is a defensive invariant check to protect
against future invariant breakage or corrupted input, so readers know why the
seemingly unreachable branch remains.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 19d63e47-4489-4ab0-b7a2-c9f36acdd99c

📥 Commits

Reviewing files that changed from the base of the PR and between 37dc1ed and cb8ebe8.

📒 Files selected for processing (3)

• include/eventide/ipc/lsp/position.h
• src/ipc/lsp/position.cpp
• tests/unit/ipc/lsp/position_tests.cpp