refactor(storage): centralize sanitized Git subprocess execution - #4934
Conversation
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 1feaf39d00b1286eb441afbecf1979e82d701cbe.
I found no P0-P3 correctness, ownership, race, repository-selection, timeout, or temporary-index issue in the five-file diff. packages/storage/src/git-exec.ts:20-80 preserves the prior timeout and buffer behavior, isolates Git from ambient repository variables, and retains explicit temporary-index support. The worktree executor, project catalog, and workspace identity callers use the helper with their previous limits; the new test covers both text and byte output with invalid ambient Git variables.
The exact-head hosted label and test checks passed, and the merge-tree and diff check are clean. I could not run the local Storage typecheck because this checkout has no usable tsc, and I did not run an additional Windows smoke test. The refactor/product decision remains for human maintainers; this is a technical review only.
Automated review notice: This is an AI-assisted review and does not replace independent human review.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review by OpenAI Codex, operated by me2seeks, at 1feaf39d00b1286eb441afbecf1979e82d701cbe. This is an automated technical assessment, not an independent human review. Approval is submitted at the operator's explicit direction.
No blocking correctness findings. Compared all migrated Git process options, repository-variable cleanup and temporary-index injection. Existing 3-second / 64 KiB catalog and workspace limits and 2-minute executor limits remain unchanged; byte results retain the previous Uint8Array copy. The exact-source test launched real Git with poisoned GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR and GIT_INDEX_FILE and passed for both text and byte APIs.
- Optimal for the actual problem: Yes; the change addresses the defect at its existing owner.
- Production code that can be deleted: A nonblocking P3: runGit and runGitBytes now only forward identical arguments to the imported helpers; direct calls or import aliases can remove both local wrappers and their options-type import without moving policy elsewhere.
- Low-quality tests that can be deleted or replaced: None identified.
- Deeper refactor: No deeper refactor is needed.
- Ready to merge: Content is ready; merge remains conditional on current required checks and conflict state.
- Residual risks / verification: Focused real-Git test passed from exact bundled sources. Full worktree lifecycle suite, typecheck and Windows execution were not rerun locally. No protected-area effect identified in this diff.
Fixes #4929
Summary
Motivation
Repeated subprocess setup could diverge across security-sensitive Git calls. A single helper keeps repository selection isolated from ambient Git variables while preserving existing timeouts, buffer limits, and error behavior.
Validation