🛡️ Sentinel: Fix metadata length parsing confusion#3043
Merged
ChanTsune merged 3 commits intoMay 10, 2026
Conversation
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughAdds byte-length-bounded core types and domain wrappers, replaces raw strings/bytes in ExtendedAttribute and Permission with bounded types, and updates CLI, xtask, docs, and tests to use the new constructors and conversions. ChangesBounded Metadata Type System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a vulnerability where metadata lengths for uname, gname, and extended attributes were silently truncated during serialization. The changes introduce explicit length validation using try_from and update serialization methods to return io::Result. Feedback from the reviewer suggests optimizing these methods by validating lengths before allocating memory to prevent potential OOM issues. Additionally, the reviewer pointed out that the new use of unwrap() in into_chunks introduces a potential panic point that should be documented or addressed through a safer API.
| } | ||
|
|
||
| fn into_chunks(self) -> Vec<RawChunk> { | ||
| self.try_into_chunks().unwrap() |
Contributor
There was a problem hiding this comment.
The use of unwrap() here introduces a potential panic point if the entry contains metadata with names exceeding the format limits (e.g., a uname > 255 bytes). While try_into_chunks provides a safe alternative, the existing into_chunks (and by extension EntryPart::from) now becomes fallible via panic. In a library context, it's generally preferable to propagate errors, although this might require breaking changes to the public EntryPart API. Consider documenting this panic behavior or providing a TryFrom implementation for EntryPart.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/src/entry/meta.rs (1)
419-438: 💤 Low valueMissing success-boundary test for exactly 255-byte
uname/gname.The added tests verify the error path at 256 bytes but never exercise the maximum valid length (255 bytes), so the happy path at the boundary is unconfirmed.
🧪 Suggested additional tests
+ #[test] + fn permission_max_uname_len() { + let perm = Permission::new(1000, "a".repeat(255), 100, "group1".into(), 0o644); + assert!(perm.to_bytes().is_ok()); + } + + #[test] + fn permission_max_gname_len() { + let perm = Permission::new(1000, "user1".into(), 100, "a".repeat(255), 0o644); + assert!(perm.to_bytes().is_ok()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/entry/meta.rs` around lines 419 - 438, Add tests that assert a 255-byte uname and a 255-byte gname are accepted: create Permission instances using Permission::new with "a".repeat(255) for uname and for gname, call perm.to_bytes().unwrap() and then Permission::try_from_bytes(...) to assert equality (round-trip), and include these as new #[test] functions alongside permission_too_long_uname and permission_too_long_gname to verify the boundary succeeds.lib/src/entry/attr.rs (1)
77-97: ⚡ Quick winNo error-path tests for
xattr name too long/xattr value too long.
meta.rsgainspermission_too_long_unameandpermission_too_long_gname;attr.rsgains no equivalent tests, leaving theu32overflow branches into_bytesuncovered.🧪 Suggested tests
+ #[test] + fn xattr_name_too_long() { + // usize > u32::MAX is not constructible on 32-bit platforms; use a large but + // still feasible size to exercise the try_from path on 64-bit. + #[cfg(target_pointer_width = "64")] + { + let name = "a".repeat(u32::MAX as usize + 1); + let xattr = ExtendedAttribute::new(name, b"value".into()); + assert!(xattr.to_bytes().is_err()); + } + } + + #[test] + fn xattr_value_too_long() { + #[cfg(target_pointer_width = "64")] + { + let value = vec![0u8; u32::MAX as usize + 1]; + let xattr = ExtendedAttribute::new("name".into(), value); + assert!(xattr.to_bytes().is_err()); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/entry/attr.rs` around lines 77 - 97, Add unit tests that exercise the overflow branches in Attr::to_bytes by constructing Attr-like values whose name.len() and value.len() exceed u32::MAX (so u32::try_from fails) and asserting the call returns an io::Error with ErrorKind::InvalidData and the message containing "xattr name too long" and "xattr value too long" respectively; implement two tests (e.g., xattr_name_too_long and xattr_value_too_long) that invoke to_bytes() and check the error, referencing the to_bytes method and the self.name / self.value fields in assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/entry.rs`:
- Around line 810-812: NormalEntry::into_chunks currently unwraps
try_into_chunks and can panic on oversized uname/gname/xattr; change
construction of EntryPart to be fallible by removing the panic path and exposing
the error: stop using into_chunks().unwrap(), implement TryFrom<T:
SealedEntryExt> for EntryPart that calls try_into_chunks() and returns
io::Result<EntryPart>, and update/recommend callers to use
EntryPart::try_from(entry)? instead of EntryPart::from(entry). Ensure
NormalEntry::into_chunks either returns Result by delegating to try_into_chunks
or is removed in favor of only the try_* path so no unwrap remains.
In `@lib/src/entry/attr.rs`:
- Around line 113-115: The test module `mod tests` in attr.rs has an extra
trailing blank line before its closing `}` which causes the formatter/CI to
fail; open the `mod tests` block and remove the blank line immediately preceding
the final `}` so the closing brace follows the last test code line directly,
then run `cargo fmt`/the project's formatter to confirm the formatting check
passes.
---
Nitpick comments:
In `@lib/src/entry/attr.rs`:
- Around line 77-97: Add unit tests that exercise the overflow branches in
Attr::to_bytes by constructing Attr-like values whose name.len() and value.len()
exceed u32::MAX (so u32::try_from fails) and asserting the call returns an
io::Error with ErrorKind::InvalidData and the message containing "xattr name too
long" and "xattr value too long" respectively; implement two tests (e.g.,
xattr_name_too_long and xattr_value_too_long) that invoke to_bytes() and check
the error, referencing the to_bytes method and the self.name / self.value fields
in assertions.
In `@lib/src/entry/meta.rs`:
- Around line 419-438: Add tests that assert a 255-byte uname and a 255-byte
gname are accepted: create Permission instances using Permission::new with
"a".repeat(255) for uname and for gname, call perm.to_bytes().unwrap() and then
Permission::try_from_bytes(...) to assert equality (round-trip), and include
these as new #[test] functions alongside permission_too_long_uname and
permission_too_long_gname to verify the boundary succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 452ed4c4-c9e7-41f4-ac82-edb5bac33c48
📒 Files selected for processing (5)
.jules/sentinel.mdlib/src/archive/write.rslib/src/entry.rslib/src/entry/attr.rslib/src/entry/meta.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/command/chown.rs`:
- Around line 132-138: transform_entry currently uses UserName::try_from(...)
and GroupName::try_from(...) with .expect(), which will panic for long
user-supplied names; change transform_entry to be fallible (return
io::Result<...>) and replace both .expect() calls with try conversions that map
the TryFromError into an io::Error (e.g.,
io::Error::new(io::ErrorKind::InvalidInput, "owner/group name exceeds 255
bytes") ) using the ? operator so the error is returned instead of panicking;
update the callers that invoke transform_entry (the sites that previously
ignored fallibility) to propagate or handle the io::Result (using ? at those
call sites) so the validation error flows back to the CLI instead of aborting.
In `@cli/src/command/core.rs`:
- Around line 1013-1019: The code currently calls
pna::UserName::try_from(uname).expect(...) and
pna::GroupName::try_from(gname).expect(...), which panics on >255 byte names;
change these to propagate errors through the function's io::Result by replacing
the expect calls with try_from and mapping their Err to an io::Error (e.g.,
io::ErrorKind::InvalidInput with a clear message) so entry.permission(...) is
only called with valid UserName/GroupName; locate the conversion sites in
core.rs (the entry.permission(...) call and the pna::UserName::try_from /
pna::GroupName::try_from invocations) and return Err(...) on conversion failure
instead of panicking.
- Around line 2051-2058: transform_normal_entry currently uses expect on
pna::UserName::try_from and pna::GroupName::try_from which will panic for
owner/group names >255 bytes; change this to propagate a proper error instead of
panicking: replace the expect calls when building new_perm so that you match or
? the Result from UserName::try_from and GroupName::try_from, return a
descriptive Err (or map to the function's existing error type) indicating
"owner/group name too long" and include the offending name, ensuring
transform_normal_entry rejects the archive entry gracefully rather than
panicking.
In `@cli/src/command/core/mtree.rs`:
- Around line 485-491: Change the code to propagate conversion errors instead of
panicking: update the enclosing function (the one that constructs
EntryBuilder/entry) to return io::Result<EntryBuilder>, replace the two expect()
calls around pna::UserName::try_from(uname) and pna::GroupName::try_from(gname)
with fallible conversions that map the TryFrom error into an io::Error and
return Err(...) so entry.permission(...) receives valid values or the function
returns an error; apply the same pattern for the other conversions referenced at
the similar site (lines around 553-555) so no TryFrom uses expect().
In `@cli/src/command/xattr.rs`:
- Around line 311-318: The code currently panics when converting oversized xattr
name/value inside the map that builds pna::ExtendedAttribute (using
pna::XattrName::try_from and pna::XattrValue::try_from) by calling expect;
change this to return a proper error instead of panicking: replace the expect
calls with fallible conversions that propagate a descriptive error (using ? or
map_err to convert the TryFrom error into the command's error type) and ensure
the map closure returns Result<pna::ExtendedAttribute, _> so upstream can handle
and report the error rather than panic.
In `@xtask/src/main.rs`:
- Around line 376-382: The current code panics by calling expect on
UserName::try_from and GroupName::try_from inside libpna::Permission::new;
instead propagate the conversion errors through the existing convert_entry
Result: replace the expect(...) calls with the `?` operator (or map_err to the
convert_entry error type) so UserName::try_from(uname) and
GroupName::try_from(gname) return their Err up the call stack, then pass the
resulting values into libpna::Permission::new; ensure the function signature and
returned error type from convert_entry accommodate the propagated conversion
error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7aa742d-cf5d-4e97-8b7f-2e000979705e
📒 Files selected for processing (25)
cli/src/command/chmod.rscli/src/command/chown.rscli/src/command/core.rscli/src/command/core/mtree.rscli/src/command/xattr.rscli/src/utils/os/unix/fs/xattrs.rscli/tests/cli/list/trailing_slash.rscli/tests/cli/utils/archive.rscli/tests/cli/xattr/get/option_match_encoding.rscli/tests/cli/xattr/set/basic.rscli/tests/cli/xattr/set/option_base64.rscli/tests/cli/xattr/set/option_hex.rscli/tests/cli/xattr/set/option_remove.rscli/tests/cli/xattr/set/overwrite.rscli/tests/cli/xattr/set/set_and_remove.rslib/src/archive.rslib/src/entry.rslib/src/entry/attr.rslib/src/entry/builder.rslib/src/entry/meta.rslib/src/util.rslib/src/util/bounded.rslib/src/util/bounded/bytes.rslib/src/util/bounded/str.rsxtask/src/main.rs
✅ Files skipped from review due to trivial changes (5)
- lib/src/util.rs
- cli/tests/cli/xattr/set/option_hex.rs
- cli/tests/cli/xattr/set/overwrite.rs
- lib/src/entry.rs
- lib/src/entry/builder.rs
Comment on lines
2051
to
2058
| let resolved_uname = uname.clone().unwrap_or_else(|| perm.uname().to_string()); | ||
| let resolved_gname = gname.clone().unwrap_or_else(|| perm.gname().to_string()); | ||
| let new_perm = pna::Permission::new( | ||
| uid.map(u64::from).unwrap_or_else(|| perm.uid()), | ||
| uname.clone().unwrap_or_else(|| perm.uname().to_string()), | ||
| pna::UserName::try_from(resolved_uname).expect("uname must fit within 255 bytes"), | ||
| gid.map(u64::from).unwrap_or_else(|| perm.gid()), | ||
| gname.clone().unwrap_or_else(|| perm.gname().to_string()), | ||
| pna::GroupName::try_from(resolved_gname).expect("gname must fit within 255 bytes"), | ||
| perm.permissions(), |
Contributor
There was a problem hiding this comment.
Panic when transforming archive entries with oversized owner names.
The transform_normal_entry function processes entries from input archives. If an archive (potentially created by a different tool or maliciously crafted) contains owner names exceeding 255 bytes, the transformation will panic instead of rejecting the entry gracefully.
This affects the @archive inclusion feature and archive transformation operations.
🔧 Proposed fix
let new_perm = pna::Permission::new(
uid.map(u64::from).unwrap_or_else(|| perm.uid()),
- pna::UserName::try_from(resolved_uname).expect("uname must fit within 255 bytes"),
+ pna::UserName::try_from(resolved_uname)
+ .map_err(|e| io::Error::new(io::ErrorKind::InvalidData,
+ format!("uname in archive exceeds 255 bytes: {}", e)))?,
gid.map(u64::from).unwrap_or_else(|| perm.gid()),
- pna::GroupName::try_from(resolved_gname).expect("gname must fit within 255 bytes"),
+ pna::GroupName::try_from(resolved_gname)
+ .map_err(|e| io::Error::new(io::ErrorKind::InvalidData,
+ format!("gname in archive exceeds 255 bytes: {}", e)))?,
perm.permissions(),
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/command/core.rs` around lines 2051 - 2058, transform_normal_entry
currently uses expect on pna::UserName::try_from and pna::GroupName::try_from
which will panic for owner/group names >255 bytes; change this to propagate a
proper error instead of panicking: replace the expect calls when building
new_perm so that you match or ? the Result from UserName::try_from and
GroupName::try_from, return a descriptive Err (or map to the function's existing
error type) indicating "owner/group name too long" and include the offending
name, ensuring transform_normal_entry rejects the archive entry gracefully
rather than panicking.
Comment on lines
+376
to
+382
| libpna::Permission::new( | ||
| uid, | ||
| libpna::UserName::try_from(uname).expect("uname must fit within 255 bytes"), | ||
| gid, | ||
| libpna::GroupName::try_from(gname).expect("gname must fit within 255 bytes"), | ||
| mode, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify panic path and call chain for build_permission
rg -n -C3 'fn build_permission|UserName::try_from|GroupName::try_from|expect\(".*255 bytes' xtask/src/main.rs
rg -n -C3 'let permission = build_permission\(header, &path\)' xtask/src/main.rsRepository: ChanTsune/Portable-Network-Archive
Length of output: 898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get wider context around line 297 to see the containing function
sed -n '250,310p' xtask/src/main.rs | cat -n
# Search for all call sites of build_permission
rg -n 'build_permission' xtask/src/main.rs
# Get the full signature and context of convert_entry
rg -n -A 50 'fn convert_entry' xtask/src/main.rs | head -60Repository: ChanTsune/Portable-Network-Archive
Length of output: 5130
Avoid panicking on oversized tar owner/group names.
At Lines 378 and 380, expect(...) can abort conversion on crafted tar metadata. Since convert_entry already returns Result, propagate the error instead of panicking.
💡 Proposed fix (propagate error instead of panic)
-fn build_permission(header: &tar::Header, path: &str) -> libpna::Permission {
+fn build_permission(
+ header: &tar::Header,
+ path: &str,
+) -> Result<libpna::Permission, Box<dyn std::error::Error>> {
@@
- libpna::Permission::new(
- uid,
- libpna::UserName::try_from(uname).expect("uname must fit within 255 bytes"),
- gid,
- libpna::GroupName::try_from(gname).expect("gname must fit within 255 bytes"),
- mode,
- )
+ let uname = libpna::UserName::try_from(uname)
+ .map_err(|_| format!("{path}: uname exceeds 255 bytes"))?;
+ let gname = libpna::GroupName::try_from(gname)
+ .map_err(|_| format!("{path}: gname exceeds 255 bytes"))?;
+ Ok(libpna::Permission::new(uid, uname, gid, gname, mode))
}- let permission = build_permission(header, &path);
+ let permission = build_permission(header, &path)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xtask/src/main.rs` around lines 376 - 382, The current code panics by calling
expect on UserName::try_from and GroupName::try_from inside
libpna::Permission::new; instead propagate the conversion errors through the
existing convert_entry Result: replace the expect(...) calls with the `?`
operator (or map_err to the convert_entry error type) so
UserName::try_from(uname) and GroupName::try_from(gname) return their Err up the
call stack, then pass the resulting values into libpna::Permission::new; ensure
the function signature and returned error type from convert_entry accommodate
the propagated conversion error.
github-actions
Bot
added
cli
ChanTsune
force-pushed
the
sentinel-fix-metadata-truncation-10749376436631233274
branch
from
b6e3ef5 to
7fbfcae
Compare
Introduce BoundedString<const MAX: usize> and BoundedBytes<const MAX: usize> as crate-internal helpers for variable-length fields whose maximum byte length is constrained by a fixed-width on-the-wire length prefix. Casting a usize length into a narrower integer at serialization time silently truncates oversized inputs and corrupts framing; bounding the length at construction time encodes the format constraint in the type system and keeps serialization infallible. The types are pub inside a pub(crate) module so their API surface is at publishable-crate quality (TryFrom variants, symmetric PartialEq, From back to inner, Default, repr(transparent), #[non_exhaustive] LengthExceeded with accessors, doctest examples) while remaining invisible to external libpna consumers.
Replace the String / Vec<u8> fields and constructor parameters of Permission and ExtendedAttribute with UserName, GroupName, XattrName, and XattrValue. The 255-byte and u32::MAX limits required by the fPRM and xATR length prefixes are now type-level invariants checked once at construction. Permission::to_bytes and ExtendedAttribute::to_bytes can therefore downcast lengths to u8 / u32 infallibly, eliminating the silent truncation that previously occurred when serializing oversized names. API change: Permission::new and ExtendedAttribute::new now take the new types directly. Existing call sites construct them via TryFrom; CLI and xtask sites wrap construction in expect() so out-of-range input fails fast instead of silently corrupting the archive framing. LengthExceeded is also re-exported from the crate root since it appears as the Error associated type of the new TryFrom impls.
github-actions
Bot
added
break
ChanTsune
force-pushed
the
sentinel-fix-metadata-truncation-10749376436631233274
branch
from
26c02a0 to
9053e81
Compare
Move byte-length enforcement out of the consumer-side .expect() panic and into the layer that produces each value, classified by source: - CLI arguments (--uname, --gname, --name on uname/gname/xattr-set): clap value_parser now turns the input straight into UserName, GroupName, or XattrName, so out-of-range input is rejected by the argument parser with a user-visible error before any I/O happens. - chown's compound `user:group` argument: RawOwnership::FromStr rejects names whose byte length exceeds the u8 length prefix. - Mtree manifest, NSS lookup, and xattr-restore dump file: convert the retrieved value with TryFrom and surface LengthExceeded as io::ErrorKind::InvalidData on the existing io::Result-returning code path. resolve_name() in mtree.rs is generic over UserName / GroupName so the bound is enforced at the conversion boundary. - Permission and ExtendedAttribute fields downstream of CLI parsing (PermissionStrategyResolver, OwnerOptions, CreationPermissionStrategyResolver, ExtractionPermissionStrategyResolver, SetAttrStrategy::Apply.name) hold the bounded newtype directly so the consumer no longer needs a fallible conversion. The remaining .expect() call sites (chmod, Windows SecurityDescriptor, OS xattr ABI, ustar tar header) are documented with the contract that makes the conversion provably safe.
github-actions
Bot
added
break
ChanTsune
deleted the
sentinel-fix-metadata-truncation-10749376436631233274
branch
This was referenced May 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Severity: HIGH
💡 Vulnerability: Metadata length parsing confusion via silent truncation.
🎯 Impact: Attacker-controlled long names could cause subsequent metadata fields (UID, GID) to be misaligned, enabling metadata injection.
🔧 Fix: Enforced format constraints at the serialization boundary using
u8::try_fromandu32::try_from, propagating errors up the stack.✅ Verification: Added regression tests in
lib/src/entry/meta.rsandlib/src/entry/attr.rs. Verified all tests and lints pass.PR created automatically by Jules for task 10749376436631233274 started by @ChanTsune
Summary by CodeRabbit
Improvements
Documentation
Tests