Import unification PR 6: extract the shared import phases - #1444
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e guidance, stale test comments Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… residual per-file guard asymmetry; PR 6b carved out Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First extraction commit of PR 6 (pure-identical hoists, verified byte-identical by comment-stripped diff before the move): - _make_stop_check(runner, job): module-level factory; the pause-aware nonblocking-probe rationale (cancellation_requested vs is_cancelled) moves verbatim into its docstring — the two nested defs were byte-identical, comment included. - _build_destination_context(db, params) -> _DestContext(destination, mount_baseline, path_under_any_source, path_under_destination, fold_basename): the normalize -> mount-baseline -> source-guard -> containment-predicates -> basename-fold sequence, extracted as ONE function because the ordering is load-bearing (baseline captured immediately after normalization, before discovery — PR #1396 review). fold_basename is unused on the local path until PR 7 (free and harmless). Comment merge: kept the local path's fuller versions of the normalize/baseline/source-guard/containment blocks (they carry the scanner._ensure_folder, 2026-07-30-incident, and probe-walk-up rationale) and folded in the remote-only facts (route sets destination to mount_path/subpath; rsync-still-pushes-to-NAS consequence; _linkable_twin_dirs scoping; SMB/FAT in the case-insensitivity list); fixed the known_mounted_roots -> known_mount_roots typo. The remote _fold_basename comment (PR #1113 review) moves verbatim. - Deleted the remote-only _missing_mount_root wrapper; both paths now call _missing_archive_mount_root(destination) directly at the batch guard. The wrapper's Task 2.7 shadow-directory rationale block moves verbatim to the remote call site (merged with the shorter call-site comment it referenced); the local call-site comment's stale reference to the wrapper is repointed. - Both function-level pipeline_job imports drop the three names now used only inside the builder. No behavior change; vireo/tests/test_import_job.py: 225 passed, 1 skipped (identical to baseline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map commit 2 of PR 6:
- _make_emitter(job, runner, state, eta) -> _emit: ONE body for both
paths, clearing _IMPORT_ETA_PROGRESS_KEYS +
_IMPORT_TRANSFER_PROGRESS_KEYS unconditionally. The transfer-keys
clear is a proven no-op on the local path (the keys are written only
by the remote-only _emit_transfer and never seeded by JobRunner,
whose progress dicts are exactly {current, total, current_file} —
jobs.py:552/626/1042; push_event mirroring never carries them
locally); the proof is a comment in the emitter. _emit_transfer
stays nested in the remote fn (transport core). Comment merge: kept
the local path's folder_counts rationale wording ("not just from the
terminal result") in the factory docstring, and rewrote the folders
snapshot comment's "Mirrors the local path" as "Both paths send the
folder table — spec decision 1".
- _plan_import(db, params, emit, state) -> _ImportPlan (NamedTuple, 11
fields): discovery (with nested _discovery_onerror touching only
state.discovery_errors + log), source-snapshot capture, selection,
checker construction, folder-planning timestamps, batch grouping —
the 78-line block previously verified identical by comment-stripped
diff. ORDERING preserved: _capture_source_snapshots before
_apply_selection (the retry-signature rationale comment moved with
the code — kept the remote fn's FULL version; the local copy was the
abbreviated pointer at it). Also kept the local-only section
comments (enumeration-error rationale, timestamps and grouping
headers) and neutralized both "Shared with the local/remote path"
variants to "Shared by both copy paths".
- Both call sites destructure the plan BY NAME (same anti-transposition
discipline as _Selection, restated on _ImportPlan); plan.files and
plan.timestamps are consumed inside the planner and deliberately not
re-bound as locals.
No behavior change; vireo/tests/test_import_job.py: 225 passed,
1 skipped (identical to baseline).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map commit 3 of PR 6: _finalize_import(job, runner, db, state, params, *, discovered, include_paths, source_snapshots, deselected, vanished_paths, appeared, remote_unverified=False) -> dict — terminal status/summary/update_step, discovery-error unsafe entries, partial_scope, the flag-gated remote_unverified append (exact remote predicate reproduced: 'remote_unverified and discovered > 0'), the unverified_duplicate entry, _append_selection_unsafe, safe_to_format and unverified_duplicates_only (each with 'and not remote_unverified'), and the shared 19-key result dict. The local path calls with the default (every remote_unverified term degenerates to a no-op — the local path hash-verifies every copy); the remote path passes remote_unverified=not params.verify_by_hash at the call site. unsafe_files entry order is unchanged on both paths: the honesty-gate append sits where the remote had it and is gated off locally. Comment merge (noted per plan): kept the local path's fuller versions of the discovery-error rationale, the safe_to_format/partial_scope pill-honesty block, and the JobRunner 'ok' mixed-outcome convention (the remote's one-line 'Scope narrowing (same rules as the local path)' header and inline 'ok' are superseded); kept the remote's photo_ids after-import-chaining comment (PR #1113 ledger) and its honesty-gate + unverified_duplicates_only-unreachability comments; repointed the source_snapshots comment's 'assignment above the copy loop' at _plan_import. The two result dicts were otherwise identical ('ok' differed only in line-wrapping). No behavior change; vireo/tests/test_import_job.py: 225 passed, 1 skipped (identical to baseline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map commit 4 of PR 6 — strings only, emits/counters identical since
PR 2:
1. Sticky-guard reason: local adopts the REMOTE wording ('archive
claim was rolled back' — covers adoptions, which 'landing' did not).
2. Stale-mount reason: remote adopts the LOCAL wording ('copying here
would write to the local disk under a stale mount point' — names
the actual failure mode; the remote's 'cataloging here would read a
local shadow' described only the scan half).
3. Per-file mount-lost reason: both adopt the merged wording
'detached while this batch was in progress (the directory persists
but the share is gone, so neither further writes nor a duplicate
match against it can be trusted)'.
Grepped tests for all six old strings first: no assertion pins (the
mount-detach pins assert 'local shadow'/'detached before', untouched),
but two test COMMENTS quoted the old per-file wordings (test lines
~11584, ~11658) — updated to the merged wording, zero assertion
changes.
vireo/tests/test_import_job.py: 225 passed, 1 skipped (identical to
baseline).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map commit 5 of PR 6: - _ImportBatchState (module-level, after _ImportRunState): rel, dest_folder (filled post-preflight), landed, dup_dirs, dup_skips, reclassified_landed_paths, mount_lost, dest_read_cancelled, plus the remote-only trio to_transfer / claimed_basenames / queued_src_hashes (default factories; commented as unused-on-local until PR 7). The per-declaration incident comments from BOTH paths moved onto the dataclass fields, merged where they diverged (dup_skips: kept one text with all three Codex review ids; mount_lost: local's 2026-07-26 blast-radius rationale + remote's per-file-decision rationale; dest_read_cancelled: both helper names and both PR #1423 review ids; landed: local's WC-extraction sentence + remote's card-side-verified_hash sentence). - Both per-batch blocks now construct batch_st = _ImportBatchState(rel=rel, dest_folder=dest_folder); the nine batch names were renamed to batch_st.<name> mechanically (5b discipline) via a tokenize-based rewrite scoped to the two function line ranges: 82 NAME-token replacements, comments/strings untouched, attribute accesses skipped. Audit: zero bare occurrences of the nine names remain in either function range (token scan) and zero symtable entries for them in either function or any nested scope. - _batch_preflight(state, emit, db, *, rel, batch, queued, ctx, missing_root_check) -> dest_folder | None: sticky mount-loss refusal -> dest-under-source -> missing-mount-root -> stale-mount -> makedirs -> folder-status promotion, extracted as ONE function with the 2026-07-30 incident ordering stated as load-bearing in its docstring. Comment merges: sticky guard kept the local wording ('copies that never completed'); dest-under-source keeps BOTH ledger entries (local PR #1107 makedirs-on-card rationale + remote PR #1113 cand_mount rationale); missing-mount-root merges the Task 2.7 shadow-directory block with the local 2026-07-30 root-owned-/Volumes account; stale-mount keeps the local Linux-/mnt text + remote's rsync-keeps-pushing sentence; makedirs and promotion keep the local fuller versions. Both paths pass missing_root_check=lambda: _missing_archive_mount_root(destination). Remote's ssh_dest compute stays behind in the remote fn (transport). - _rollback_on_mount_loss(state, batch_st, rel, verified_counted_for_copies, extra_rollback=None): dup_skips rollback -> extra_rollback() -> landed rollback via _reclassify_landed_failed -> state.mount_ever_lost LAST, with the ordering stated as load-bearing (the sticky flag asserts the rollback happened, so an exception partway through must not leave later batches refused while this batch's counts stand). The final post-loop probe stays at both call sites with its PR #1423 skip-on-dest_read_cancelled comments. The remote passes a _drop_queued_transfers closure for its to_transfer queue ('detached before this file was transferred' wording unchanged); its remote-specific 'only adoptions can be in landed here' note stays at the remote call site. Unified shape drops dup_dirs on mount loss even when dup_skips is empty (the remote shape — the safe direction; local's 'and dup_skips' guard was incidental). vireo/tests/test_import_job.py: 225 passed, 1 skipped; -k 'mount_detach or dest_read_cancel or behavior': 15 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…PR 6b The gate returns _GATE_SKIPPED/_GATE_PROCEED/_GATE_CANCELLED verdicts; both callers map them with the same three-line block, and CANCELLED maps to break (the PR #1423 wedged-mount contract, documented on the constants). The catalog tail extracts as five shared functions (_catalog_scan_and_prescan, _invalidate_changed_and_sweep, _fill_wc_overrides, _link_twins_and_emit, _extract_deferred_working_copies), adopting the LOCAL loop shape where the two sides differed only in form (remote iterated landed_paths / prebuilt changed_candidates). Comment merges: the shared scan call carries BOTH the local PR #1107 pairing-context block and the remote PR #1398 not-incremental incident block; the landed_paths PR #1113 rationale comes from the remote side; WC-fill and link-emit comments merge both wordings. The stamping loops stay in each function (spec decision 11 / PR 6b). Also: _rollback_on_mount_loss drops its redundant rel param (batch_st.rel); missing_root_check lambdas close over ctx.destination. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds an implementation plan for shared import-phase extraction, updates the design specification with stamping divergences and sequencing, and revises two mount-detachment test comments. ChangesImport unification documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the repository path portable.
Line 15 hard-codes
/Users/julius/conductor/workspaces/vireo/nagoya. Other contributors and CI cannot use this path. Refer to the repository root without an absolute path, and derive it in commands when needed.🤖 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 `@docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md` at line 15, Remove the hard-coded local repository path from the plan’s “Repo root” entry. Refer to the repository root generically and instruct commands to derive or use the current working directory when needed, while preserving the branch, baseline commit, test command, and per-task commit requirements.
🤖 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 `@docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md`:
- Line 5: Correct the commit-count wording in the Goal statement of the
import-unification plan so it matches Tasks 1 through 7 and the PR summary:
state seven suite-green commits unless the plan explicitly combines one task
into another commit.
- Around line 58-63: Update the planned helper signatures to match the runtime
contracts: remove rel from _rollback_on_mount_loss because it reads
batch_st.rel, and include verified_counted_for_copies in
_catalog_scan_and_prescan because scan-failure reclassification requires it.
Ensure the plan’s call-site instructions use these corrected signatures, or
explicitly list the corresponding runtime API and caller updates if changing the
helpers is intended.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md`:
- Line 15: Remove the hard-coded local repository path from the plan’s “Repo
root” entry. Refer to the repository root generically and instruct commands to
derive or use the current working directory when needed, while preserving the
branch, baseline commit, test command, and per-task commit requirements.
🪄 Autofix
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 Plus
Run ID: 29d62858-385a-4be3-9ab9-c9ba3bc3e3df
📒 Files selected for processing (4)
docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.mddocs/superpowers/specs/2026-08-06-import-path-unification-design.mdvireo/import_job.pyvireo/tests/test_import_job.py
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Extract every now-identical phase of the two import functions into shared module-level functions (six suite-green commits, least-risky first), shrinking both functions to their transport cores — plus record two newly-found divergences in the spec. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the commit-count claim.
The goal says “six suite-green commits,” but Tasks 1 through 7 each define a commit. The PR summary also states seven commits. Change “six” to “seven,” or identify the task that shares a commit.
🤖 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 `@docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md` at
line 5, Correct the commit-count wording in the Goal statement of the
import-unification plan so it matches Tasks 1 through 7 and the PR summary:
state seven suite-green commits unless the plan explicitly combines one task
into another commit.
Comment on lines
+58
to
+63
| - [ ] Extract `_rollback_on_mount_loss(state, batch_st, rel, verified_counted_for_copies, extra_rollback=None)` from R2314-2369 / L4196-4247: final probe stays at the call site; the function does dup_skips rollback → `extra_rollback()` if provided (remote passes the `to_transfer` closure) → landed rollback via `_reclassify_landed_failed` → `state.mount_ever_lost = batch_st.mount_lost` LAST. Ordering comment required. | ||
| - [ ] Full file → 225/1; also `-k "mount_detach or dest_read_cancel or behavior"` and report. Commit: `"Batch state object; extract batch preflight and mount-loss rollback"` | ||
|
|
||
| ### Task 7: Duplicate gate + catalog tail (map commit 6) | ||
| - [ ] `_duplicate_gate(state, batch_st, *, source_file, rel, checker, db, params, ctx, stop_requested) -> verdict` from R1973-2118 / L3793-3961 (verified identical; 4 non-semantic hunks). Verdict: a tiny enum or module constants `_GATE_SKIPPED` (caller `continue`s), `_GATE_PROCEED`, `_GATE_CANCELLED` (caller `break`s — BOTH `DestReadCancelled` exits map here; see the control-flow hazard in Context). Both callers' mappings must be structurally identical — write them as the same three-line block. | ||
| - [ ] Extract the catalog-tail quartet (all verified identical): `_catalog_scan_and_prescan(state, batch_st, db, params, scan, destination, dest_folder, rel)` (gate + landed_paths + pre-scan capture + scan call + scan-failure rollback + `_invalidate_new_images`) from R2602-2658 / L4265-4332; `_invalidate_changed_and_sweep(state, batch_st, db, params, raw_companion_invalidations)` (diff loop + companion invalidation + commit + sweep) from R2936-3011 / L4548-4613; `_fill_wc_overrides(state, batch_st, params, dest_folder)` from R3012-3033 / L4622-4648 — NOTE: these three blocks are SEMANTICALLY identical but differ in loop shape (remote iterates `landed_paths`/builds a `changed_candidates` comprehension; local iterates `landed` with alias variables) — adopt the LOCAL form in the shared function and say so in the commit message; the parity matrix is the check. Twin-linking and WC-extract ARE byte-identical; `_link_twins_and_emit(state, batch_st, db, workspace_id, emit, rel, queued)` from R3034-3057 / L4649-4673 + the deferred `_extract_working_copies` block R3059-3077 / L4674-4704 as `_extract_deferred_working_copies(state, params, runner, job)`. The STAMPING LOOP (R2687-2935 / L4352-4546, incl. local `_rehash_dest_or_none`) STAYS IN EACH FUNCTION — it is PR 6b's align-then-extract; leave a one-line comment at each pointing at spec decision 11. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the planned helper signatures with the runtime contracts.
The supplied vireo/import_job.py helpers use different signatures:
_rollback_on_mount_lossreadsbatch_st.rel; it does not acceptrel._catalog_scan_and_prescanreadsbatch_st.dest_folderand requiresverified_counted_for_copiesfor scan-failure reclassification.
If implementers copy the plan literally, the catalog path can lose the counter-accounting dependency or fail at the call boundary. Update the plan, or explicitly document the intended API changes and all required call-site updates.
Proposed signature correction
- `_rollback_on_mount_loss(state, batch_st, rel, verified_counted_for_copies, extra_rollback=None)`
+ `_rollback_on_mount_loss(state, batch_st, verified_counted_for_copies, extra_rollback=None)`
- `_catalog_scan_and_prescan(state, batch_st, db, params, scan, destination, dest_folder, rel)`
+ `_catalog_scan_and_prescan(state, batch_st, db, params, scan, destination, rel, verified_counted_for_copies)`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [ ] Extract `_rollback_on_mount_loss(state, batch_st, rel, verified_counted_for_copies, extra_rollback=None)` from R2314-2369 / L4196-4247: final probe stays at the call site; the function does dup_skips rollback → `extra_rollback()` if provided (remote passes the `to_transfer` closure) → landed rollback via `_reclassify_landed_failed` → `state.mount_ever_lost = batch_st.mount_lost` LAST. Ordering comment required. | |
| - [ ] Full file → 225/1; also `-k "mount_detach or dest_read_cancel or behavior"` and report. Commit: `"Batch state object; extract batch preflight and mount-loss rollback"` | |
| ### Task 7: Duplicate gate + catalog tail (map commit 6) | |
| - [ ] `_duplicate_gate(state, batch_st, *, source_file, rel, checker, db, params, ctx, stop_requested) -> verdict` from R1973-2118 / L3793-3961 (verified identical; 4 non-semantic hunks). Verdict: a tiny enum or module constants `_GATE_SKIPPED` (caller `continue`s), `_GATE_PROCEED`, `_GATE_CANCELLED` (caller `break`s — BOTH `DestReadCancelled` exits map here; see the control-flow hazard in Context). Both callers' mappings must be structurally identical — write them as the same three-line block. | |
| - [ ] Extract the catalog-tail quartet (all verified identical): `_catalog_scan_and_prescan(state, batch_st, db, params, scan, destination, dest_folder, rel)` (gate + landed_paths + pre-scan capture + scan call + scan-failure rollback + `_invalidate_new_images`) from R2602-2658 / L4265-4332; `_invalidate_changed_and_sweep(state, batch_st, db, params, raw_companion_invalidations)` (diff loop + companion invalidation + commit + sweep) from R2936-3011 / L4548-4613; `_fill_wc_overrides(state, batch_st, params, dest_folder)` from R3012-3033 / L4622-4648 — NOTE: these three blocks are SEMANTICALLY identical but differ in loop shape (remote iterates `landed_paths`/builds a `changed_candidates` comprehension; local iterates `landed` with alias variables) — adopt the LOCAL form in the shared function and say so in the commit message; the parity matrix is the check. Twin-linking and WC-extract ARE byte-identical; `_link_twins_and_emit(state, batch_st, db, workspace_id, emit, rel, queued)` from R3034-3057 / L4649-4673 + the deferred `_extract_working_copies` block R3059-3077 / L4674-4704 as `_extract_deferred_working_copies(state, params, runner, job)`. The STAMPING LOOP (R2687-2935 / L4352-4546, incl. local `_rehash_dest_or_none`) STAYS IN EACH FUNCTION — it is PR 6b's align-then-extract; leave a one-line comment at each pointing at spec decision 11. | |
| - [ ] Extract `_rollback_on_mount_loss(state, batch_st, verified_counted_for_copies, extra_rollback=None)` from R2314-2369 / L4196-4247: final probe stays at the call site; the function does dup_skips rollback → `extra_rollback()` if provided (remote passes the `to_transfer` closure) → landed rollback via `_reclassify_landed_failed` → `state.mount_ever_lost = batch_st.mount_lost` LAST. Ordering comment required. | |
| - [ ] Full file → 225/1; also `-k "mount_detach or dest_read_cancel or behavior"` and report. Commit: `"Batch state object; extract batch preflight and mount-loss rollback"` | |
| ### Task 7: Duplicate gate + catalog tail (map commit 6) | |
| - [ ] `_duplicate_gate(state, batch_st, *, source_file, rel, checker, db, params, ctx, stop_requested) -> verdict` from R1973-2118 / L3793-3961 (verified identical; 4 non-semantic hunks). Verdict: a tiny enum or module constants `_GATE_SKIPPED` (caller `continue`s), `_GATE_PROCEED`, `_GATE_CANCELLED` (caller `break`s — BOTH `DestReadCancelled` exits map here; see the control-flow hazard in Context). Both callers' mappings must be structurally identical — write them as the same three-line block. | |
| - [ ] Extract the catalog-tail quartet (all verified identical): `_catalog_scan_and_prescan(state, batch_st, db, params, scan, destination, rel, verified_counted_for_copies)` (gate + landed_paths + pre-scan capture + scan call + scan-failure rollback + `_invalidate_new_images`) from R2602-2658 / L4265-4332; `_invalidate_changed_and_sweep(state, batch_st, db, params, raw_companion_invalidations)` (diff loop + companion invalidation + commit + sweep) from R2936-3011 / L4548-4613; `_fill_wc_overrides(state, batch_st, params, dest_folder)` from R3012-3033 / L4622-4648 — NOTE: these three blocks are SEMANTICALLY identical but differ in loop shape (remote iterates `landed_paths`/builds a `changed_candidates` comprehension; local iterates `landed` with alias variables) — adopt the LOCAL form in the shared function and say so in the commit message; the parity matrix is the check. Twin-linking and WC-extract ARE byte-identical; `_link_twins_and_emit(state, batch_st, db, workspace_id, emit, rel, queued)` from R3034-3057 / L4649-4673 + the deferred `_extract_working_copies` block R3059-3077 / L4674-4704 as `_extract_deferred_working_copies(state, params, runner, job)`. The STAMPING LOOP (R2687-2935 / L4352-4546, incl. local `_rehash_dest_or_none`) STAYS IN EACH FUNCTION — it is PR 6b's align-then-extract; leave a one-line comment at each pointing at spec decision 11. |
🤖 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 `@docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md`
around lines 58 - 63, Update the planned helper signatures to match the runtime
contracts: remove rel from _rollback_on_mount_loss because it reads
batch_st.rel, and include verified_counted_for_copies in
_catalog_scan_and_prescan because scan-failure reclassification requires it.
Ensure the plan’s call-site instructions use these corrected signatures, or
explicitly list the corresponding runtime API and caller updates if changing the
helpers is intended.
Bind loop-scoped batch_st and rel as default arguments on the inline _drop_queued_transfers callback so ruff no longer flags B023 (function definition does not bind loop variable). The callback is invoked synchronously inside _rollback_on_mount_loss in the same batch iteration, so late-binding was never a real hazard — the default-arg binding just makes that explicit and quiets the four warnings that failed the lint job.
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.
What this is
PR 6 of the import-path-unification project. Plan:
docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md(in this PR). Every phase the two import functions still shared as hand-mirrored copies is now ONE module-level function; the two functions shrank from 1,836/1,634 lines to 1,010/710 — what remains is the transport cores (collision/adopt walk, rsync transfer) and the stamping loops (deliberately deferred, see below).Methodology
A fresh side-by-side phase map (comment-stripped diff of every phase pair) verified what was extractable verbatim vs parameterizable vs divergent, and the plan sequenced seven suite-green commits least-risky first. The suite count (225 passed, 1 skipped) is identical at every commit — bisectable throughout.
Extracted (shared functions, both paths now call the same code)
_build_destination_context(normalize → mount baseline → source guard → containment predicates, ordering load-bearing per the 2026-07-30 incident) +_make_stop_check; the remote_missing_mount_rootwrapper deleted._make_emitter— ONE_emitbody; the transfer-keys clear is a proven no-op locally (the keys are written only by the remote-only_emit_transferand never seeded by JobRunner; proof in a comment)._plan_import(discovery → snapshots → selection → checker → timestamps → batching; was a 78-line empty diff)._finalize_import(the entire 135-line tail; oneremote_unverified=Falseparameter)._ImportBatchState+_batch_preflight(all six batch guards as ONE ordered function) +_rollback_on_mount_loss(with the remoteto_transferhook asextra_rollback)._duplicate_gatewith_GATE_SKIPPED/_GATE_PROCEED/_GATE_CANCELLEDverdicts — the CANCELLED→breakmapping contract is documented on the constants (mapping it tocontinuewould reintroduce the PR Bound destination hash reads so Stop can interrupt a dead SMB mount #1423 wedged-mount pin); both callers use the same three-line mapping block._catalog_scan_and_prescan,_invalidate_changed_and_sweep,_fill_wc_overrides,_link_twins_and_emit,_extract_deferred_working_copies(local loop-shape adopted where the sides differed only in form).Deliberately NOT extracted (recorded in the spec, updated in this PR)
file_hashon scan-NULL rows; the remote doesn't (plus a zero-byte normalization-convention split). That's a behavior decision, not a refactor — carved out as PR 6b (align-then-extract), spec decision table row 11. Both loops carry a pointer comment.same_file/dest-under-source guard has no remote counterpart; PR 7 must decide keep/port/drop deliberately.Comment ledger
Comments moved verbatim with their code; where the paths' comments diverged they were merged keeping both sides' unique content (e.g. the shared scan call now carries the local PR #1107 pairing-context block AND the remote PR #1398 not-incremental incident block). Citation counts audited: every PR/Codex id present in the deleted blocks survives in the shared functions.
Process note
Most of this PR ran through the usual subagent-per-task + two-stage-review pipeline (Tasks 0-6, four review passes, all approved — including a domination-analysis proof that dropping local's incidental
and dup_skipsguard is behavior-neutral). Task 7 (gate + catalog tail) was completed inline after the account spend limit interrupted the implementer; its splices were verified by structural-marker scripting, citation audits, and the full parity net. Extra review attention on commit 790aa6e is welcome.Tests
vireo/tests/test_import_job.py: 225 passed, 1 skipped — identical at all seven commits. Full parity net (behavior/adoption/renamed_twin/zero_byte/dest_read_cancel/mount_detach/pairs): 25 passed.test_api_exiftool_status_reports_missing).🤖 Generated with Claude Code
Summary by CodeRabbit