Skip to content

refactor(storage): centralize sanitized Git subprocess execution - #4934

Merged
me2seeks merged 1 commit into
apache:mainfrom
seekskyworld:refactor/4929-storage-git-exec
Sep 8, 2026
Merged

refactor(storage): centralize sanitized Git subprocess execution#4934
me2seeks merged 1 commit into
apache:mainfrom
seekskyworld:refactor/4929-storage-git-exec

Conversation

@seekskyworld

Copy link
Copy Markdown
Contributor

Fixes #4929

Summary

  • add a storage-local Git execution helper for text and byte output
  • centralize repository environment sanitization and retain explicit temporary index support
  • migrate worktree, workspace identity, and project catalog Git calls

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

  • npm --workspace @maka/core run build
  • npm --workspace @maka/storage run build
  • npx biome check (changed files)
  • git diff --check
  • node --test packages/storage/dist/tests/git-exec.test.js packages/storage/dist/tests/git-worktree-child-executor.test.js packages/storage/dist/tests/workspace-identity.test.js packages/storage/dist/tests/project-catalog.test.js (45 passed)

Signed-off-by: seekskyworld <djh1813553759@gmail.com>
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Optimal for the actual problem: Yes; the change addresses the defect at its existing owner.
  2. 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.
  3. Low-quality tests that can be deleted or replaced: None identified.
  4. Deeper refactor: No deeper refactor is needed.
  5. Ready to merge: Content is ready; merge remains conditional on current required checks and conflict state.
  6. 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.

@me2seeks
me2seeks merged commit 71ecf20 into apache:main Sep 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(storage): centralize sanitized Git subprocess execution

3 participants