[rominf]

Follow-up to #660, covering the non-blocking review notes there.

• Duplicated guard (jussielo): moved_one_level_down and the symlink filter in entries_without_top_level_symlink now share one is_absolute helper. It also serves the target check.
• Nesting (jussielo): moving_an_entry_down_keeps_its_symlink_target_in_place now also moves links nested one and two levels deep, and pins that each gains exactly one ../.
• Guard branches (juhovainio): the same test now checks both spellings (/ and @OUT) of an absolute name and of an absolute target, so all four guard branches are pinned without relying on the property sweep.

The nested cases showed that #660's doc comments, and my reply there, claimed more than the move does:

• Still true: one ../ keeps an escaping target escaping, to the same place, at any depth, and keeps a top-level link pointing exactly where it did.
• Not true: a nested link whose target stays inside the extraction root still stays inside, but not always at the same path. sub/l -> . resolves to sub before the move and to a after it.

The escape properties depend only on the part that holds. The comments and the test now state and check exactly that.

Each new assertion fails under a mutation of the code it guards:

• per-depth ../;
• no ../;
• each guard narrowed to one spelling;
• the helper narrowed either way.

Test-only change: no user-observable CLI behaviour changes, so no Gherkin scenario is needed (AGENTS.md §3).

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 64f04d6

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (single file, test-only).
Follow-up to PR #660, addressing its two deferred non-blocking review notes (duplicated absolute-path guard; missing nested-symlink test case) — fully implements both, nothing left outstanding from #660.

Blocking

None

Non-blocking

• moved_one_level_down's absolute-name early return is never tested together with a symlink target — apps/rocm/src/comfyui.rs:4913 (absolute-name cases use Kind::Dir only), apps/rocm/src/comfyui.rs:4900 (absolute-target cases use the relative name "l" only).
  No test constructs an Entry that is both name-absolute and Kind::Symlink. The function's real contract for that case is "return the entry completely untouched, including the target" (the early return happens before the target is ever inspected). A plausible mutation — hoisting the target ../-splice above the name-absolute check, or otherwise decoupling it from the early return — would prepend ../ to a relative target on an absolute-named symlink entry, and every current assertion would still pass: the Kind::Dir cases never have a target to corrupt, and the Kind::Symlink cases with a relative name never exercise the name-absolute branch. Verified by hand-tracing both guard branches against every existing assertion; no test combination intersects them. (Possibly caught non-deterministically by the separate property-fuzz test via entries()'s link_name strategy, but that's shrink/probability-dependent, not a guarantee — and it's outside this diff.)
  Confidence 85 · logic (test-coverage gap) · Fix: add one case with an absolute name (either spelling) and Kind::Symlink(relative_target), asserting the target is byte-for-byte unchanged.

Decisions for the author

• comfyui.rs is 5055 lines, and its test modules (including archive_properties) make up a large fraction of it — non-blocking-improvement. AGENTS.md §6 asks new subsystems to default to their own file rather than growing a shared one; this diff adds only ~41 net lines and isn't the cause of the file's size, so it's not this PR's job to fix, but a future pass extracting mod archive_properties (and sibling test modules) into their own file(s) would be in the spirit of that guardrail.

Positive signals

• The new nested-link assertions correctly stop short of asserting full before/after path equality for a non-escaping nested target, matching the actual lexical-resolution semantics (sub/l -> . really does resolve differently before and after the move) rather than overclaiming invariance the code doesn't provide — and the doc comments were corrected to match, which is a real, useful walk-back of PR #660's own doc comment rather than a cosmetic tweak.
• is_absolute cleanly dedupes what were two independently-maintained copies of the same "leading / or @" check (verified identical via De Morgan's law against both call sites), directly resolving a reviewer-flagged duplication from #660.

Deployment notes

None — test-only change behind #[cfg(all(test, unix))]; no production code, CLI behavior, docs, or configuration touched.

What this covered

The whole diff (e4a20d9d…64f04d6a, base origin/main, head PR commit 64f04d6a), read completely: the changed is_absolute/moved_one_level_down/entries_without_top_level_symlink functions, the extended unit test and its resolve()/components() helpers, plus the surrounding archive_properties module (the other property tests, the #[ignore]d known-defect tests, and property()) for context on what these test helpers feed into. Ran as two independent blind workers (code-correctness; agent-instruction-adherence + history + prior-review-comments) plus consolidation, impact analysis, and design review, and independently hand-verified the path-resolution math (including the sub/l -> . example and an escaping two-level-deep case) rather than taking the PR's claims on faith.

CI: all required checks green (clippy, commit-signatures+sign-off, coverage, build/test on Linux and Windows, CodeQL, E2E self-hosted lanes including Strix Halo Ubuntu/Windows/WSL2). One lane, E2E tests (MI300X), showed QUEUED with no conclusion at review time — looks like a stuck/offline self-hosted runner rather than anything caused by this diff (the change is Unix-test-only and touches no MI300X-specific path); not treated as blocking, but worth a nudge if it's still queued.

Did not run: no additional "prior-changes" pass beyond the dedicated worker's search, which found both of PR #660's review comments fully addressed by this PR. Not reconciled against any GitHub discussion already on #684 itself — this review was produced blind from the code and the linked PR #660, per the review-code method.

[rominf]

On the automated notes:

• Absolute-named symlink: agreed, that combination is not pinned yet. I will add the case in a small follow-up: an absolute name, in either spelling, with a relative symlink target, asserting the target is unchanged. This approved PR lands as it is.
• Size of comfyui.rs: agreed it is large. This PR adds about 40 lines and does not cause it. Moving mod archive_properties and its sibling test modules into their own files is a separate refactor.

[rominf]

The absolute-named symlink case is in #692.