Skip to content

Fix AsPath for String not normalizing paths#954

Merged
kixelated merged 1 commit intomainfrom
fix/910-aspath-string-normalization
Feb 13, 2026
Merged

Fix AsPath for String not normalizing paths#954
kixelated merged 1 commit intomainfrom
fix/910-aspath-string-normalization

Conversation

@kixelated
Copy link
Collaborator

Summary

  • Fixes OriginConsumer::announced panics if prefix does not match #910OriginConsumer::announced panics if prefix does not match
  • AsPath for String and AsPath for &String bypassed Path::new() and directly borrowed the raw string, so trailing slashes (e.g. "some_prefix/") were preserved without normalization
  • This caused OriginConsumerNotify::announce/reannounce/unannounce to panic when strip_prefix failed on the unnormalized root
  • Fixed by routing both impls through Path::new(), matching the &str impl behavior

Test plan

  • Added test_with_root_trailing_slash_consumer — consumer with trailing-slash String root receives announcements
  • Added test_with_root_trailing_slash_producer — producer with trailing-slash String root publishes correctly
  • Added test_with_root_trailing_slash_unannounce — unannounce also works with trailing-slash root
  • All 145 existing moq-lite tests still pass

🤖 Generated with Claude Code

AsPath for String/&String bypassed Path::new() and directly borrowed the
raw string, so trailing slashes were preserved. This caused
OriginConsumerNotify to panic when strip_prefix failed on the
unnormalized root.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) February 13, 2026 12:33
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

Walkthrough

This pull request introduces regression tests for with_root path handling when prefixes contain trailing slashes and are provided as owned Strings. Tests cover consumer-side, producer-side, and unannounce path scenarios to ensure path normalization and correct root behavior. Additionally, the implementation of AsPath for String and borrowed String types is modified to use Path::new instead of Cow::Borrowed, altering ownership semantics. No public API signatures are changed, and all modifications remain internal to the module.

🚥 Pre-merge checks | ✅ 5 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing AsPath for String not normalizing paths, which directly addresses the panic issue mentioned in issue #910.
Description check ✅ Passed The description is well-detailed and directly related to the changeset, explaining the root cause of the panic, the fix applied, and the test coverage added.
Linked Issues check ✅ Passed The PR successfully addresses issue #910 by fixing the AsPath implementations to normalize paths through Path::new(), preventing panics when strip_prefix encounters unnormalized trailing slashes.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing issue #910: path normalization in AsPath implementations and regression tests for the trailing-slash scenarios, with no extraneous modifications.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/910-aspath-string-normalization

No actionable comments were generated in the recent review. 🎉


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

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

@kixelated kixelated merged commit 430c847 into main Feb 13, 2026
1 check passed
@kixelated kixelated deleted the fix/910-aspath-string-normalization branch February 13, 2026 12:49
@moq-bot moq-bot bot mentioned this pull request Feb 12, 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.

OriginConsumer::announced panics if prefix does not match

1 participant