Skip to content

fix: execute git/npm subcommands with argv arrays - #127

Merged
pyramation merged 1 commit into
mainfrom
feat/argv-command-execution
Sep 1, 2026
Merged

fix: execute git/npm subcommands with argv arrays#127
pyramation merged 1 commit into
mainfrom
feat/argv-command-execution

Conversation

@pyramation

Copy link
Copy Markdown
Contributor

Summary

Three call sites built shell command strings out of caller-supplied values, so any metacharacter in a branch name, repo URL, clone destination, package name, or git config key was re-parsed by /bin/sh instead of being passed through as data. All three now use execFileSync with an argv array (no shell), plus -- before positional operands so values starting with - can't be read as flags.

genomic/src/git/git-cloner.ts:

-const command = `git clone${branchArgs}${singleBranchArgs}${depthArgs} ${url} ${destination}`;
-execSync(command, { stdio, encoding: 'utf-8' });
+const args = ['clone'];
+if (branch) args.push('--branch', branch);
+if (singleBranch) args.push('--single-branch');
+args.push('--depth', String(depth), '--', url, destination);
+execFileSync('git', args, { stdio, encoding: 'utf-8' });

genomic/src/utils/npm-version-check.ts: npm view ${packageName} versionexecFileSync('npm', ['view', packageName, 'version'], ...).

inquirerer/src/resolvers/git.ts: git config --global ${key}execFileSync('git', ['config', '--global', '--', key], ...).

Behavior is otherwise unchanged (same flags, same order, same stdio/encoding options). Tests that mocked child_process.execSync and asserted on the command string were updated to mock execFileSync and assert on the argv array; getNpmWhoami still uses execSync('npm whoami') (constant string) so that mock stays in place alongside the new one. Added a genomic case asserting a destination containing ; arrives as one argv entry.

Link to Devin session: https://app.devin.ai/sessions/bb89d57636c84c7383305191a303b1d4
Open in Devin Desktop: https://app.devin.ai/desktop/session/bb89d57636c84c7383305191a303b1d4?variant=devin
Requested by: @pyramation

Replace interpolated shell strings with execFileSync argument arrays in
GitCloner, npm-version-check, and the git config resolver so branch names,
URLs, destinations, package names, and config keys are passed as single argv
entries instead of being re-parsed by the shell.
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@pyramation
pyramation merged commit e592bed into main Sep 1, 2026
61 checks passed
@tenki-reviewer

tenki-reviewer Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review complete. No blocking issues — approved ✅; 1 nitpick below.

🧹 Nitpicks (1) — 🟢 1 low
  • 🟢 Assert full argv in metacharacter clone test (create-gen.test.ts:610) — The metacharacter destination test (packages/genomic/__tests__/create-gen.test.ts:610-615) asserts only args[args.length - 1], whereas the two sibling clone tests assert the complete argv array with toEqual.

This change replaces string-based execSync calls with execFileSync (file + args array) in the git cloner, npm version check, and the inquirerer git config resolver, and rewrites the corresponding jest tests to mock and assert the new argv-array signatures. The migration is largely consistent and the tests were updated to match the new call shape.

Files Change
packages/genomic/src/git/git-cloner.ts, packages/inquirerer/src/resolvers/git.ts Migrate git clone/config subprocess calls to execFileSync with argv arrays and -- separators.
packages/genomic/src/utils/npm-version-check.ts Convert npm view invocation to execFileSync with an argv array.
packages/genomic/__tests__/create-gen.test.ts, inquirerer/__tests__/*.test.ts Update jest mocks and assertions to the new execFileSync signatures.

Note: the sweep diff-reader pass did not publish (step-limit failure); findings are drawn from the adjudicated scanner passes. One low-severity test-hardening suggestion was kept.

Reviewed commit: 5768bb2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant