Batch installer copies to reduce validation overhead

[justin808]

Why

Copy-mode installation starts one rsync process per managed child directory. Batching those paths removes repeated startup cost. Part of #726; suite sharding remains separate.

What changed

Copy the existing source paths in one rsync invocation into a temporary directory on the destination filesystem, then replace managed entries after copying succeeds. A failed copy preserves existing managed entries and unrelated content and propagates its error.

Replacement proceeds sequentially: a failed rename can leave the current entry missing while later entries retain their old content. A process-group termination can leave inactive hidden staging; the reproduced case preserved installed content and allowed a successful retry. This does not claim whole-install atomicity.

How to review and verify

Review copy_children_preserving_unrelated and its six behavioral tests. Three alternating baseline/patched Linux runs of the existing test_copy_mode_preserves_unrelated_agent_files case measured medians of 3.722 seconds versus 1.727 seconds: 53.6% faster. This measures one installer case; full hosted CI savings remain unmeasured.

Test plan

• Six focused copy tests, 22 assertions; fresh paired benchmark on the integrated payload.
• Clean committed local bin/validate, including installer and stack suites: PASS.
• Complete repository lint and applicable independent review evidence.
• Current-head full hosted validation: PASS; current-head Codex completed and all six review threads are resolved.
• Changelog: deferred_to_update_changelog.

Agent details

Current candidate and evidence

Candidate 37bc79ef540afe05bccbdd8e96b3c5e50d6630a7 integrates main 3111d3d9164f59465fc04a79e9a71b075e275788. The clean merge brings in only the two previously reviewed preflight files from main. The installer feature patch is byte-for-byte identical to the approved 1b1952e patch against its prior base. Current-main CHANGELOG.md is preserved. The original feature review and independent incoming-main review remain applicable; no feature change or conflict resolution was introduced. Clean committed local full validation passed on this candidate. Current-head full hosted validation also passed. CI readiness is READY; the integrated base remains current and all six review threads are resolved.

Three alternating Linux pairs used this candidate's payload for both versions, replacing only the baseline installer with main's version. Baseline seconds: 3.863142627, 3.722252418, 3.665247419. Candidate seconds: 1.727238584, 1.802217876, 1.697127585. Median 3.722 to 1.727 seconds: 53.6% faster, with nonoverlapping ranges. Docker was offline with a 2-CPU quota and 3 GB memory while local validation also ran on the host. These are paired single-case measurements, not exclusive-runner or whole-job results.

Review provenance: Codex completed on current 37bc79e. The completed Claude review repeated the known staging-residue limitation; its disposition links the prior independent reproduction. All six threads are resolved. The independent local feature/integration review remains applicable because the installer feature is unchanged and the incoming main files were separately reviewed.

QA Evidence

• QA lane: independent review and behavioral CLI tests.
• Scope checked: batching, managed replacement, unrelated entries, symlinks, empty input, errors and partial multi-item copy failure.
• Tested at: 37bc79ef540afe05bccbdd8e96b3c5e50d6630a7.
• Automated checks: focused tests, paired benchmark, lint and clean committed local full validation passed. Current-head hosted full validation passed.
• Manual checks: three alternating baseline/patched Linux installer-case pairs.
• User-visible UI change: no.
• Visual evidence: not applicable: installer CLI change.
• Interaction change: no application interaction change.
• Interaction evidence: not applicable: installer CLI change.
• Visual fix: no.
• Negative control: not applicable: no visual fix.
• Performance evidence: existing installer-case median 3.722 to 1.727 seconds, 53.6% improvement; full hosted savings unmeasured.
• Findings: independently reviewed feature unchanged; documented sequential replacement and staging-residue limitations retained.
• QA required: yes.
• QA required rationale: performance optimization must preserve installer behavior.
• QA lane status: satisfied.
• Release-blocking status: clear.
• Process-gap disposition: script.

Decision log

Take the measured installer improvement first. Suite sharding and #726's full-job improvement target remain outside this PR.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-09T22:03:48.196185Z	37bc79e	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 91fa8911-58f5-436d-82c9-f2706c514ded

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11116e5cc1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[justin808]

At 93bcb7f7583176527d58092db6a7c6858f496ac5, the confirmed batched-copy failure regression is fixed. Both duplicate reports are resolved; the optional ARG_MAX suggestion is declined. Updated-head CI and review remain separate pending gates.

Review decisions and evidence

The regression wrote partial data and returned 23: before correction it damaged existing managed entries; afterward all existing entries and unrelated content remain, staging is removed, and 23 propagates. Six focused tests/22 assertions, all nine real copy-mode cases, complete lint, and independent review pass. Three new alternating Linux benchmark pairs measure median 2.835s baseline versus 0.880s patched, 68.9% faster for this one existing installer case. Full hosted runtime savings remain unmeasured. Replacement is sequential; no whole-install atomicity is claimed.

• 3967097414: MUST_FIX fixed; replied and resolved.
• 3967097751: OPTIONAL declined; replied and resolved.
• 3967107943: MUST_FIX fixed; replied and resolved.

No ARG_MAX failure is observed at current pack size; optional chunking is not tracked and no follow-up issue was created. Codex and Claude original-wave feedback is accounted for; CodeRabbit explicitly skipped review.

Future full-PR scans should start after this comment unless check all reviews is requested.

[claude]

Review

Solid, well-scoped change. copy_children_preserving_unrelated now stages the full batch with one rsync call and only swaps each destination in afterward via same-filesystem mv, which both delivers the claimed batching win and meaningfully shrinks the failure window versus the old per-item rm + rsync (which had no error checking at all on the rsync call). The new bin/installer-copy-test.rb suite exercises the behaviors that matter here — batching, symlink source/dest handling, empty input, full-batch failure, and partial-batch failure — and is proportionate to how much more failure-handling logic the function now has.

One correctness note left as an inline comment: the per-item rm -rf "$destination" || mv ... step (bin/install-agent-workflows:3280) still leaves a small gap versus the PR description's "failed copy preserves existing managed entries" claim — if mv fails immediately after rm -rf succeeds for the item currently being swapped, that entry ends up deleted with nothing replacing it. This is a narrow, low-probability edge case (same-filesystem rename is normally near-atomic), not a blocker, but the safety guarantee isn't quite absolute for the in-flight item.

No security or scope concerns — the diff is the minimum needed to batch the copy and add regression coverage for the new failure paths.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b1952e5e3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

Reviewed copy_children_preserving_unrelated (bin/install-agent-workflows) and the new bin/installer-copy-test.rb suite.

Design: batching the per-child rsync calls into one invocation into a same-filesystem staging directory, then swapping entries in via mv, is a sound and minimal way to achieve the stated goal (reduce rsync process-startup overhead) while also fixing the old code's lack of any error checking on rsync. The benchmark evidence in the PR description supports the claimed speedup.

Scope: the diff is proportionate — no new flags, modes, or config surface, just the one function plus the tests that exercise its new failure paths (batching, symlinks, empty input, full-batch failure, partial-batch failure). No concerns there.

Already discussed and resolved: the narrow window where rm -rf "$destination" succeeds but the following mv fails, leaving that one entry deleted rather than replaced, was already raised and accepted as a documented, low-probability limitation (not whole-install atomicity) — no need to relitigate that here.

New finding (left as an inline comment): interrupted runs (the process-group-termination case the PR description mentions reproducing) leave the .agent-workflows-copy.XXXXXX staging directory behind under the managed target directory with no cleanup path on a later run — unlike the other staging mechanisms already in this file, which each have their own recovery/cleanup handling. Worth a look since it's a real, reproducible gap rather than a hypothetical one.

No security issues found (source paths are always directory-prefixed, so no rsync option-injection risk from item basenames; nothing here handles untrusted input).