💀 Deprecate fPRM Permission API; superseded by owner facet chunks#3071
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeprecate legacy fPRM/Permission and replace uses with ResolvedOwnership and explicit owner-facet fields; update EntryBuilder, conversion paths, CLI commands, pna add_metadata translation, archive writer, and tests accordingly. ChangesOwner-facet migration and Permission deprecation
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 deprecates the fPRM chunk and the Permission struct in favor of new owner facet chunks (fUId, fGId, fONm, fGNm, fOSi, fGSi, fMOd). The changes include adding #[deprecated] attributes to the relevant types and methods in EntryBuilder and Metadata, along with #[allow(deprecated)] annotations to internal implementations and tests to maintain compatibility. No review comments were provided, so I have no feedback to offer regarding the review process.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/archive/write.rs (1)
531-585:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize owner-facet chunks in
write_file_entry.Line 569 writes deprecated
fPRM, but the function never emitsfUId/fGId/fONm/fGNm/fOSi/fGSi/fMOd. This silently drops metadata written via the replacement API when callers useArchive::write_file/SolidArchive::write_file.💡 Suggested patch
pub(crate) fn write_file_entry<W, F>( @@ if let Some(p) = metadata.permission { (ChunkType::fPRM, p.to_bytes()).write_chunk_in(inner)?; } + if let Some(v) = metadata.owner_uid { + (ChunkType::fUId, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.owner_gid { + (ChunkType::fGId, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.owner_user_name { + (ChunkType::fONm, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.owner_group_name { + (ChunkType::fGNm, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.owner_user_sid { + (ChunkType::fOSi, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.owner_group_sid { + (ChunkType::fGSi, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.permission_mode { + (ChunkType::fMOd, v.to_bytes()).write_chunk_in(inner)?; + } + if let Some(v) = metadata.link_target_type { + (ChunkType::fLTP, v.to_bytes()).write_chunk_in(inner)?; + } let context = get_writer_context(option)?;🤖 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/archive/write.rs` around lines 531 - 585, write_file_entry currently writes deprecated fPRM but omits the owner-related chunks (fUId, fGId, fONm, fGNm, fOSi, fGSi, fMOd), so owner/facet metadata is dropped; update write_file_entry to check metadata.owner fields and emit the corresponding chunks (ChunkType::fUId, ::fGId, ::fONm, ::fGNm, ::fOSi, ::fGSi, ::fMOd) using the same .write_chunk_in(inner)? pattern (and appropriate .to_bytes() conversions or byte slices) — place these writes alongside the permission write (after the fPRM block) so callers of Archive::write_file / SolidArchive::write_file preserve owner user id, group id, owner/group names, sids and mode. Ensure you reference the function write_file_entry, the metadata variable, and the ChunkType enum names when adding the emissions.pna/src/ext/entry_builder.rs (1)
54-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
add_metadatashould copy owner facets andpermission_mode.Line 60 only forwards deprecated
permission; owner-facet fields are not copied, so metadata migration paths can lose data when this helper is used.💡 Suggested patch
fn add_metadata(&mut self, metadata: &Metadata) -> &mut Self { self.created(metadata.created()) .modified(metadata.modified()) .accessed(metadata.accessed()) .permission(metadata.permission().cloned()) + .owner_uid(metadata.owner_uid()) + .owner_gid(metadata.owner_gid()) + .owner_user_name(metadata.owner_user_name().cloned()) + .owner_group_name(metadata.owner_group_name().cloned()) + .owner_user_sid(metadata.owner_user_sid().cloned()) + .owner_group_sid(metadata.owner_group_sid().cloned()) + .permission_mode(metadata.permission_mode()) .link_target_type(metadata.link_target_type()) }🤖 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 `@pna/src/ext/entry_builder.rs` around lines 54 - 62, add_metadata currently only forwards the deprecated permission() and omits owner facets and the new permission_mode, causing data loss; update the add_metadata helper to also set the new permission_mode (call .permission_mode(metadata.permission_mode())) and copy owner facet fields from Metadata (e.g. .owner_user(metadata.owner_user().cloned()) and .owner_group(metadata.owner_group().cloned()) or the appropriate owner-* builder methods your EntryBuilder exposes), and retain or map the deprecated .permission(metadata.permission().cloned()) only for backward compatibility.
🤖 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.
Outside diff comments:
In `@lib/src/archive/write.rs`:
- Around line 531-585: write_file_entry currently writes deprecated fPRM but
omits the owner-related chunks (fUId, fGId, fONm, fGNm, fOSi, fGSi, fMOd), so
owner/facet metadata is dropped; update write_file_entry to check metadata.owner
fields and emit the corresponding chunks (ChunkType::fUId, ::fGId, ::fONm,
::fGNm, ::fOSi, ::fGSi, ::fMOd) using the same .write_chunk_in(inner)? pattern
(and appropriate .to_bytes() conversions or byte slices) — place these writes
alongside the permission write (after the fPRM block) so callers of
Archive::write_file / SolidArchive::write_file preserve owner user id, group id,
owner/group names, sids and mode. Ensure you reference the function
write_file_entry, the metadata variable, and the ChunkType enum names when
adding the emissions.
In `@pna/src/ext/entry_builder.rs`:
- Around line 54-62: add_metadata currently only forwards the deprecated
permission() and omits owner facets and the new permission_mode, causing data
loss; update the add_metadata helper to also set the new permission_mode (call
.permission_mode(metadata.permission_mode())) and copy owner facet fields from
Metadata (e.g. .owner_user(metadata.owner_user().cloned()) and
.owner_group(metadata.owner_group().cloned()) or the appropriate owner-* builder
methods your EntryBuilder exposes), and retain or map the deprecated
.permission(metadata.permission().cloned()) only for backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5829cb4-503a-45d7-bebd-fd42b8799468
📒 Files selected for processing (7)
lib/src/archive.rslib/src/archive/write.rslib/src/chunk/types.rslib/src/entry.rslib/src/entry/builder.rslib/src/entry/meta.rspna/src/ext/entry_builder.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
xtask/src/main.rs (1)
522-532: 💤 Low valueConsider setting default permissions when
unix_mode()is absent.When
unix_mode()returnsNone(common for ZIP files created on Windows), no ownership or permission metadata is set on the entry. This preserves accuracy but may leave entries with no permissions in the resulting archive.This is likely acceptable for a conversion tool, but worth noting that Windows-originated ZIPs will have entries without permission metadata.
🤖 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 522 - 532, The apply_zip_owner function currently only sets owner and permission metadata when entry.unix_mode() is Some, leaving entries from Windows ZIPs without any owner/permission metadata; add an else branch in apply_zip_owner (referencing builder and entry.unix_mode()) to set sensible defaults: set owner_uid and owner_gid to 0 (libpna::OwnerUid::from(0) and libpna::OwnerGid::from(0)) and set permission_mode to a reasonable default based on whether the entry is a directory (e.g., 0o755) or a file (e.g., 0o644) — detect directories via entry.is_dir() or entry.name().ends_with('/') and call builder.permission_mode(libpna::PermissionMode::from(...)) accordingly so Windows-originated entries get predictable permissions.
🤖 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 111-149: The code currently synthesizes uid/gid = 0 by using
own.uid.unwrap_or(0) / own.gid.unwrap_or(0) when owner.user/owner.group is None;
change these matches to return Option<u64> (and Option<String> for names)
instead of raw u64 so we preserve absence instead of inventing root. Update the
owner.user match branches to produce uid_opt: Some(u64::MAX) for Name, Some(id)
for ID, Some(sys.uid) for System (if present) and None for the None case unless
own.uid.is_some(); do the same for group -> gid_opt; then call
metadata.with_owner_uid(uid_opt.map(pna::OwnerUid::from)) and
metadata.with_owner_gid(gid_opt.map(pna::OwnerGid::from)) and similarly pass
Option<String> through owner_user_name/group_name helper functions so no 0 is
written when the source lacks owner facets.
In `@cli/src/command/core.rs`:
- Around line 2090-2116: The current branch only applies owner overrides when
source ownership exists (uses own.is_empty()), which prevents
--uid/--gid/--uname/--gname from taking effect for entries without owner facets;
change the conditional to trigger whenever any override is supplied (replace the
if condition to check uid.is_some() || gid.is_some() || uname.is_some() ||
gname.is_some()) so you always build new_uid/new_gid/new_uname/new_gname using
the provided values or falling back to own.*, and then call
metadata.with_owner_uid(...).with_owner_gid(...).with_owner_user_name(...).with_owner_group_name(...).with_permission_mode(own.mode.map(...)).with_owner_user_sid(...).with_owner_group_sid(...
) exactly as before (using ResolvedOwnership::from_metadata for fallback).
In `@cli/src/command/extract.rs`:
- Around line 1605-1619: The code currently skips calling restore_owner when
ResolvedOwnership::from_metadata(item.metadata()) is empty, which prevents
explicit extraction overrides (--uid/--gid/--uname/--gname) from being applied;
change the logic so restore_owner(path, &ownership, options) is invoked whenever
keep_options.owner_strategy is OwnerStrategy::Preserve (i.e., when the user
requested preservation/overrides), regardless of ownership.is_empty(), while
still keeping the mode restoration branch conditional on ownership.mode (leave
the existing DataKind::SymbolicLink and ownership.mode checks intact); update
the block around ResolvedOwnership::from_metadata, the ownership.is_empty()
guard, and the OwnerStrategy::Preserve match to always call restore_owner when
preserve is selected so override-only restores work.
In `@cli/src/command/list.rs`:
- Around line 825-840: The code builds uname/gname by treating missing ownership
fields as present and rendering uid/gid 0; change the logic in the uname/gname
construction (the tuple around row.ownership and variable o) to first detect
whether any ownership info exists (e.g., o.uname.is_some() or o.uid.is_some()
for user, and o.gname.is_some() or o.gid.is_some() for group) and return
Cow::default() when none exist; otherwise, when options.numeric_owner is true
only format and emit the numeric uid/gid if o.uid/o.gid is Some (do not fall
back to 0), and when numeric_owner is false prefer o.uname/o.gname if Some else
fall back to formatting the numeric only if present—update the conditional
expressions around o.uname, o.uid, o.gname, and o.gid accordingly so unknown
ownership is rendered as empty instead of "0".
In `@cli/tests/cli/stdio/option_same_permissions.rs`:
- Around line 673-677: The assertions incorrectly treat uid/gid == 0 as missing;
update the checks to accept any Some(...) by replacing
m.owner_uid().is_some_and(|u| u.get() > 0) with m.owner_uid().is_some() and
similarly replace m.owner_gid().is_some_and(|g| g.get() > 0) with
m.owner_gid().is_some() so the owner presence uses .is_some() alongside
m.owner_user_name().is_some() and m.owner_group_name().is_some().
---
Nitpick comments:
In `@xtask/src/main.rs`:
- Around line 522-532: The apply_zip_owner function currently only sets owner
and permission metadata when entry.unix_mode() is Some, leaving entries from
Windows ZIPs without any owner/permission metadata; add an else branch in
apply_zip_owner (referencing builder and entry.unix_mode()) to set sensible
defaults: set owner_uid and owner_gid to 0 (libpna::OwnerUid::from(0) and
libpna::OwnerGid::from(0)) and set permission_mode to a reasonable default based
on whether the entry is a directory (e.g., 0o755) or a file (e.g., 0o644) —
detect directories via entry.is_dir() or entry.name().ends_with('/') and call
builder.permission_mode(libpna::PermissionMode::from(...)) accordingly so
Windows-originated entries get predictable permissions.
🪄 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: 465d5637-3ba6-45fa-934f-b920710cb3d9
📒 Files selected for processing (43)
cli/src/command/acl.rscli/src/command/chmod.rscli/src/command/chown.rscli/src/command/core.rscli/src/command/core/mtree.rscli/src/command/core/permission.rscli/src/command/diff.rscli/src/command/extract.rscli/src/command/list.rscli/src/command/strip.rscli/src/ext.rscli/tests/cli/chmod.rscli/tests/cli/chmod/edge_cases.rscli/tests/cli/chmod/glob_pattern.rscli/tests/cli/chmod/keep_solid.rscli/tests/cli/chmod/multiple_clauses.rscli/tests/cli/chmod/numeric.rscli/tests/cli/chmod/password.rscli/tests/cli/chmod/password_file.rscli/tests/cli/chmod/symbolic_all.rscli/tests/cli/chmod/symbolic_combined.rscli/tests/cli/chmod/symbolic_group.rscli/tests/cli/chmod/symbolic_other.rscli/tests/cli/chmod/symbolic_user.rscli/tests/cli/chmod/unsolid.rscli/tests/cli/chown/empty_files.rscli/tests/cli/chown/group_only.rscli/tests/cli/chown/option_no_owner_lookup.rscli/tests/cli/chown/option_no_owner_lookup_numeric.rscli/tests/cli/chown/user_and_group.rscli/tests/cli/chown/user_only.rscli/tests/cli/create/numeric_owner.rscli/tests/cli/create/symlink_metadata.rscli/tests/cli/create/user_group.rscli/tests/cli/extract/option_keep_permission.rscli/tests/cli/list/trailing_slash.rscli/tests/cli/stdio/mtree.rscli/tests/cli/stdio/option_same_permissions.rscli/tests/cli/stdio/symlink.rscli/tests/cli/strip.rscli/tests/cli/utils/archive.rspna/src/ext/entry_builder.rsxtask/src/main.rs
Comment on lines
+2090
to
+2116
| let own = crate::ext::ResolvedOwnership::from_metadata(&metadata); | ||
| if (uid.is_some() || gid.is_some() || uname.is_some() || gname.is_some()) && !own.is_empty() | ||
| { | ||
| let new_perm = pna::Permission::new( | ||
| uid.map(u64::from).unwrap_or_else(|| perm.uid()), | ||
| uname.clone().unwrap_or_else(|| perm.uname().to_string()), | ||
| gid.map(u64::from).unwrap_or_else(|| perm.gid()), | ||
| gname.clone().unwrap_or_else(|| perm.gname().to_string()), | ||
| perm.permissions(), | ||
| ); | ||
| metadata = metadata.with_permission(Some(new_perm)); | ||
| let new_uid = uid.map(u64::from).or(own.uid); | ||
| let new_gid = gid.map(u64::from).or(own.gid); | ||
| let new_uname = uname | ||
| .clone() | ||
| .or_else(|| own.uname.clone()) | ||
| .unwrap_or_default(); | ||
| let new_gname = gname | ||
| .clone() | ||
| .or_else(|| own.gname.clone()) | ||
| .unwrap_or_default(); | ||
| metadata = metadata | ||
| .with_owner_uid(new_uid.map(pna::OwnerUid::from)) | ||
| .with_owner_gid(new_gid.map(pna::OwnerGid::from)) | ||
| .with_owner_user_name(crate::command::core::permission::owner_name_opt(&new_uname)) | ||
| .with_owner_group_name(crate::command::core::permission::owner_group_name_opt( | ||
| &new_gname, | ||
| )) | ||
| .with_permission_mode(own.mode.map(pna::PermissionMode::from)) | ||
| .with_owner_user_sid(own.user_sid.clone().map(|s| { | ||
| pna::OwnerUserSid::new(s).expect("rescued sid within owner-facet bound") | ||
| })) | ||
| .with_owner_group_sid(own.group_sid.clone().map(|s| { | ||
| pna::OwnerGroupSid::new(s).expect("rescued sid within owner-facet bound") | ||
| })); |
Contributor
There was a problem hiding this comment.
Owner overrides shouldn't depend on source ownership being present.
The !own.is_empty() check makes --uid / --gid / --uname / --gname ineffective when importing archive entries that were created without owner facets. This path already has enough information to synthesize the requested facets, so it should be gated on “override supplied” only and use own just for fallback values.
🤖 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 2090 - 2116, The current branch only
applies owner overrides when source ownership exists (uses own.is_empty()),
which prevents --uid/--gid/--uname/--gname from taking effect for entries
without owner facets; change the conditional to trigger whenever any override is
supplied (replace the if condition to check uid.is_some() || gid.is_some() ||
uname.is_some() || gname.is_some()) so you always build
new_uid/new_gid/new_uname/new_gname using the provided values or falling back to
own.*, and then call
metadata.with_owner_uid(...).with_owner_gid(...).with_owner_user_name(...).with_owner_group_name(...).with_permission_mode(own.mode.map(...)).with_owner_user_sid(...).with_owner_group_sid(...
) exactly as before (using ResolvedOwnership::from_metadata for fallback).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/migrate.rs`:
- Around line 117-124: The code calls OwnerUserSid::new and OwnerGroupSid::new
inside the map and uses expect, which can panic on malformed archive metadata;
change these to validate and return an io::Error instead of panicking: replace
the .map(|s| pna::OwnerUserSid::new(s).expect(...)) and the group equivalent
with code that maps the Option through OwnerUserSid::new / OwnerGroupSid::new to
a Result and, on Err, converts the error into a std::io::Error (e.g.,
ErrorKind::InvalidData) and returns it from the enclosing function (the
migration path that builds the value passed into with_owner_user_sid and
with_owner_group_sid) so malformed SID values produce a propagated io::Error
rather than panic.
🪄 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: c2890525-868e-4fe3-9b8c-3def0e38cd67
📒 Files selected for processing (7)
cli/src/command/core.rscli/src/command/migrate.rscli/tests/cli/migrate.rscli/tests/cli/migrate/fprm_to_owner_facet.rscli/tests/cli/stdio.rscli/tests/cli/stdio/archive_source_uid_override.rscli/tests/cli/stdio/symlink.rs
💤 Files with no reviewable changes (1)
- cli/tests/cli/stdio/symlink.rs
✅ Files skipped from review due to trivial changes (2)
- cli/tests/cli/stdio.rs
- cli/tests/cli/migrate.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/src/command/core.rs
Comment on lines
+117
to
+124
| .with_owner_user_sid( | ||
| own.user_sid | ||
| .map(|s| pna::OwnerUserSid::new(s).expect("rescued sid within owner-facet bound")), | ||
| ) | ||
| .with_owner_group_sid( | ||
| own.group_sid | ||
| .map(|s| pna::OwnerGroupSid::new(s).expect("rescued sid within owner-facet bound")), | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid panic on malformed SID values during migration.
Line 119 and Line 123 use expect(...) on data derived from archive metadata. A malformed input can panic the process; this path should return an io::Error instead.
Suggested non-panicking fix
- let new_meta = pna::Metadata::new()
+ let owner_user_sid = own
+ .user_sid
+ .map(pna::OwnerUserSid::new)
+ .transpose()
+ .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
+ let owner_group_sid = own
+ .group_sid
+ .map(pna::OwnerGroupSid::new)
+ .transpose()
+ .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
+
+ let new_meta = pna::Metadata::new()
.with_created(created)
.with_modified(modified)
.with_accessed(accessed)
.with_link_target_type(link_target_type)
@@
- .with_owner_user_sid(
- own.user_sid
- .map(|s| pna::OwnerUserSid::new(s).expect("rescued sid within owner-facet bound")),
- )
- .with_owner_group_sid(
- own.group_sid
- .map(|s| pna::OwnerGroupSid::new(s).expect("rescued sid within owner-facet bound")),
- );
+ .with_owner_user_sid(owner_user_sid)
+ .with_owner_group_sid(owner_group_sid);🤖 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/migrate.rs` around lines 117 - 124, The code calls
OwnerUserSid::new and OwnerGroupSid::new inside the map and uses expect, which
can panic on malformed archive metadata; change these to validate and return an
io::Error instead of panicking: replace the .map(|s|
pna::OwnerUserSid::new(s).expect(...)) and the group equivalent with code that
maps the Option through OwnerUserSid::new / OwnerGroupSid::new to a Result and,
on Err, converts the error into a std::io::Error (e.g., ErrorKind::InvalidData)
and returns it from the enclosing function (the migration path that builds the
value passed into with_owner_user_sid and with_owner_group_sid) so malformed SID
values produce a propagated io::Error rather than panic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/command/chmod.rs (1)
100-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow numeric
chmodto create a mode from scratch.Lines 103-105 currently bail out before applying
Mode::Numeric, sochmod 644does nothing on entries that don't already carryfMOdor legacy permission metadata. Only symbolic clauses need a current mode.Suggested fix
fn transform_entry<T>(entry: NormalEntry<T>, mode: &Mode) -> NormalEntry<T> { let metadata = entry.metadata().clone(); let own = crate::ext::ResolvedOwnership::from_metadata(&metadata); - let Some(cur_mode) = own.mode else { - return entry.with_metadata(metadata); - }; - let new_mode = mode.apply_to(cur_mode); + let new_mode = match (mode, own.mode) { + (Mode::Numeric(mode), _) => *mode, + (Mode::Clause(_), Some(cur_mode)) => mode.apply_to(cur_mode), + (Mode::Clause(_), None) => return entry.with_metadata(metadata), + }; let metadata = metadata .with_permission(None) .with_owner_uid(own.uid.map(pna::OwnerUid::from))🤖 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/chmod.rs` around lines 100 - 132, In transform_entry, don't unconditionally return when own.mode is None; instead detect whether the requested Mode is purely symbolic (e.g., Mode variants that need a base mode) and only bail out in that case—if the requested Mode is numeric (Mode::Numeric or equivalent) build new_mode directly from the numeric value and continue to set metadata; update the early-return logic around own.mode and Mode to allow creating a mode from scratch for numeric modes while preserving the current behavior for symbolic Mode variants in the transform_entry function.
♻️ Duplicate comments (1)
cli/src/command/chown.rs (1)
109-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't short-circuit
chownon metadata-less entries.Line 112 still returns the entry unchanged when it has no existing owner/mode metadata, which makes
pna experimental chown ...a no-op instead of materializing the requested owner facets on fresh entries.Suggested fix
fn transform_entry<T>(entry: NormalEntry<T>, owner: &Ownership) -> NormalEntry<T> { let metadata = entry.metadata().clone(); let own = crate::ext::ResolvedOwnership::from_metadata(&metadata); - if own.is_empty() { - return entry.with_metadata(metadata); - } let (uid, uname): (Option<u64>, String) = match owner.user.as_ref() { Some(OwnerSpecifier::Name(uname)) => (Some(u64::MAX), uname.clone()), Some(OwnerSpecifier::ID(uid)) => (Some(*uid), String::new()), Some(OwnerSpecifier::System(user)) => ( Some(user.uid().unwrap_or(u64::MAX)),🤖 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/chown.rs` around lines 109 - 151, The transformer currently returns early in transform_entry when ResolvedOwnership::from_metadata(&metadata) is empty, making chown a no-op for entries without existing owner/mode; remove that early return and always build new metadata so requested owner facets are materialized on fresh entries. Keep using the same owner-spec matching logic (owner.user / owner.group) to compute uid/uname and gid/gname, but when own is empty treat own.uid/own.gid/own.uname/own.gname/own.mode/own.* as absent (None/default) so the .with_owner_* and .with_permission_mode calls set values from the owner arg or None as appropriate; update transform_entry (and its use of metadata, own, uid/gid, uname/gname) to always call entry.with_metadata(new_metadata) instead of returning early.
🤖 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/extract.rs`:
- Around line 1609-1612: The new branch calls restore_owner(...) for
"override-only" cases but resolve_owner still synthesizes missing uid/gid to 0,
causing unintended root assignment; change the logic so restore_owner is only
invoked when the target identity actually exists (i.e., archive or CLI provided
a UID/GID) by updating should_restore_owner or the check around
OwnerStrategy::Preserve to require presence (Some) of the specific ownership
side rather than relying on resolve_owner's 0 fallback, or alternatively modify
resolve_owner to return None for missing uid/gid instead of 0 so restore_owner
receives None and skips applying root — ensure changes reference
OwnerStrategy::Preserve, keep_options.owner_strategy, should_restore_owner,
restore_owner, and resolve_owner so the untouched side remains None unless
explicitly supplied.
---
Outside diff comments:
In `@cli/src/command/chmod.rs`:
- Around line 100-132: In transform_entry, don't unconditionally return when
own.mode is None; instead detect whether the requested Mode is purely symbolic
(e.g., Mode variants that need a base mode) and only bail out in that case—if
the requested Mode is numeric (Mode::Numeric or equivalent) build new_mode
directly from the numeric value and continue to set metadata; update the
early-return logic around own.mode and Mode to allow creating a mode from
scratch for numeric modes while preserving the current behavior for symbolic
Mode variants in the transform_entry function.
---
Duplicate comments:
In `@cli/src/command/chown.rs`:
- Around line 109-151: The transformer currently returns early in
transform_entry when ResolvedOwnership::from_metadata(&metadata) is empty,
making chown a no-op for entries without existing owner/mode; remove that early
return and always build new metadata so requested owner facets are materialized
on fresh entries. Keep using the same owner-spec matching logic (owner.user /
owner.group) to compute uid/uname and gid/gname, but when own is empty treat
own.uid/own.gid/own.uname/own.gname/own.mode/own.* as absent (None/default) so
the .with_owner_* and .with_permission_mode calls set values from the owner arg
or None as appropriate; update transform_entry (and its use of metadata, own,
uid/gid, uname/gname) to always call entry.with_metadata(new_metadata) instead
of returning early.
🪄 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: 5eefe8f3-bb8d-4d50-a835-25abbf2a0387
📒 Files selected for processing (9)
cli/src/command/chmod.rscli/src/command/chown.rscli/src/command/core/permission.rscli/src/command/extract.rscli/src/ext.rscli/tests/cli/chmod.rscli/tests/cli/chown/user_only.rslib/src/archive/write.rsxtask/src/main.rs
💤 Files with no reviewable changes (1)
- xtask/src/main.rs
ChanTsune
force-pushed
the
lib/deprecate-fprm
branch
2 times, most recently
from
0ba40f6 to
a4f8c75
Compare
ChanTsune
force-pushed
the
lib/deprecate-fprm
branch
from
a4f8c75 to
79fd5ea
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChanTsune
force-pushed
the
lib/deprecate-fprm
branch
from
ac30413 to
f810c33
Compare
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
2 participants
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.
Summary by CodeRabbit
Deprecated
Changed behavior
New Features
Tests