Skip to content

Commit 32ecdd9

Browse files
nullvariantclaude
andauthored
fix(submodule): preserve leading whitespace in git submodule status parsing (#172)
* fix(submodule): preserve leading whitespace in git submodule status parsing The gitExec() function trims stdout, which removes the leading status character from git submodule status output. This causes the first submodule to fail regex matching since the status character (' ', '-', or '+') is part of the expected format. Changes: - Add gitExecRaw() function that returns raw stdout without trimming - Update listSubmodules() to use gitExecRaw() instead of gitExec() - Remove stdout.trim() from line splitting to preserve all status chars - Add tests for single submodule, multiple submodules, and nested submodules (3-level hierarchy with depth 0-3) 🖥️ IDE: [Cursor](https://cursor.sh) 🔌 Extension: [Claude Code](https://claude.ai/download) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Model-Raw: claude-opus-4-5-20251101 * test(secureExec): add gitExecRaw error handling test Add test coverage for gitExecRaw() error path to achieve 100% coverage on the new function. Tests invalid git command and success cases. 🖥️ IDE: [Cursor](https://cursor.sh) 🔌 Extension: [Claude Code](https://claude.ai/download) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Model-Raw: claude-opus-4-5-20251101 --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent d2469a6 commit 32ecdd9

4 files changed

Lines changed: 284 additions & 5 deletions

File tree

extensions/git-id-switcher/src/secureExec.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,38 @@ export async function gitExec(args: string[], cwd?: string): Promise<GitExecResu
552552
}
553553
}
554554

555+
/**
556+
* Git command wrapper that preserves raw stdout (no trimming)
557+
*
558+
* Use this for commands with structured output where whitespace is significant,
559+
* such as `git submodule status` where the leading character indicates status.
560+
*
561+
* @param args - Git command arguments (e.g., ['submodule', 'status'])
562+
* @param cwd - Working directory for the command
563+
* @returns GitExecResult with raw stdout (not trimmed)
564+
*
565+
* @example
566+
* // Get submodule status (preserves leading status character)
567+
* const result = await gitExecRaw(['submodule', 'status'], '/path/to/repo');
568+
* if (result.success) {
569+
* // stdout: " a1b2c3d... vendor/lib (main)\n f6a1b2c... tools/cli (main)\n"
570+
* // Leading space indicates initialized submodule
571+
* const lines = result.stdout.split('\n');
572+
* }
573+
*/
574+
export async function gitExecRaw(args: string[], cwd?: string): Promise<GitExecResult> {
575+
try {
576+
const { stdout } = await secureExec('git', args, { cwd });
577+
return { success: true, stdout: stdout };
578+
} catch (error) {
579+
const errorObj = error instanceof Error ? error : new Error(String(error));
580+
if (!(error instanceof TimeoutError)) {
581+
securityLogger.logCommandError('git', args, errorObj, cwd);
582+
}
583+
return { success: false, error: errorObj };
584+
}
585+
}
586+
555587
/**
556588
* SSH-add command wrapper
557589
*

extensions/git-id-switcher/src/submodule.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*/
1111

1212
import { getWorkspace } from './vscodeLoader';
13-
import { gitExec } from './secureExec';
13+
import { gitExec, gitExecRaw } from './secureExec';
1414
import { validateSubmodulePath, normalizeAndValidatePath } from './pathUtils';
1515
import { securityLogger } from './securityLogger';
1616

@@ -134,7 +134,7 @@ export async function listSubmodules(workspacePath: string): Promise<Submodule[]
134134

135135
const validatedWorkspacePath = workspaceValidation.normalizedPath;
136136

137-
const result = await gitExec(['submodule', 'status'], validatedWorkspacePath);
137+
const result = await gitExecRaw(['submodule', 'status'], validatedWorkspacePath);
138138

139139
if (!result.success) {
140140
// SECURITY: Log unexpected errors (except ENOENT for non-git directories)
@@ -154,7 +154,7 @@ export async function listSubmodules(workspacePath: string): Promise<Submodule[]
154154
return [];
155155
}
156156

157-
const lines = stdout.trim().split('\n');
157+
const lines = stdout.split('\n');
158158
const submodules: Submodule[] = [];
159159

160160
for (const line of lines) {

extensions/git-id-switcher/src/test/secureExec.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import * as os from 'node:os';
5050
import {
5151
secureExec,
5252
gitExec,
53+
gitExecRaw,
5354
sshAgentExec,
5455
sshKeygenExec,
5556
TimeoutError,
@@ -1249,6 +1250,38 @@ async function testGitExecErrorLogging(): Promise<void> {
12491250
console.log('✅ gitExec error handling tests passed!');
12501251
}
12511252

1253+
/**
1254+
* Test gitExecRaw error handling
1255+
*
1256+
* Coverage target: gitExecRaw() error path (lines 578-584)
1257+
* Tests that gitExecRaw properly handles errors and returns Result type.
1258+
*/
1259+
async function testGitExecRawErrorHandling(): Promise<void> {
1260+
console.log('Testing gitExecRaw error handling...');
1261+
1262+
// Test 1: Invalid git command should return failure Result
1263+
{
1264+
const result = await gitExecRaw(['--invalid-flag-xyz-123']);
1265+
assert.strictEqual(result.success, false, 'Invalid flag should fail');
1266+
if (!result.success) {
1267+
assert.ok(result.error instanceof Error, 'Error should be Error instance');
1268+
assert.strictEqual(typeof result.error.message, 'string', 'Error message should be string');
1269+
}
1270+
}
1271+
1272+
// Test 2: Success case should return raw stdout (no trimming)
1273+
{
1274+
const result = await gitExecRaw(['--version']);
1275+
assert.strictEqual(result.success, true, 'git --version should succeed');
1276+
if (result.success) {
1277+
assert.ok(result.stdout.startsWith('git version'), 'Should return git version');
1278+
// Note: gitExecRaw does NOT trim, so trailing newline should be preserved
1279+
}
1280+
}
1281+
1282+
console.log('✅ gitExecRaw error handling tests passed!');
1283+
}
1284+
12521285
/**
12531286
* Test timeout error handling verification
12541287
*
@@ -1332,6 +1365,7 @@ export async function runSecureExecTests(): Promise<void> {
13321365
await testCommandBlockedLogsToAuditTrail();
13331366
await testGitExecResultType();
13341367
await testGitExecErrorLogging();
1368+
await testGitExecRawErrorHandling();
13351369
await testActualTimeoutBehavior();
13361370

13371371
console.log('\n✅ All secure execution tests passed!\n');

extensions/git-id-switcher/src/test/submodule.test.ts

Lines changed: 215 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,8 +701,8 @@ async function testSetIdentityForSubmodulesWithRepo(): Promise<void> {
701701
* Test listSubmodules with git repo that has no submodules (empty stdout path)
702702
*
703703
* Coverage target: listSubmodules() empty stdout path (line 153-155)
704-
* Note: The full success path (lines 157-168) requires fixing a bug in gitExec
705-
* where stdout.trim() removes the leading status character from submodule output.
704+
* Note: The success path test is now in testListSubmodulesWithSubmodule().
705+
* Bug fix: gitExecRaw() added to preserve raw stdout.
706706
*/
707707
async function testListSubmodulesEmptyResult(): Promise<void> {
708708
console.log('Testing listSubmodules with repo having no submodules...');
@@ -731,6 +731,210 @@ async function testListSubmodulesEmptyResult(): Promise<void> {
731731
}
732732
}
733733

734+
/**
735+
* Test listSubmodules with a real submodule
736+
*
737+
* Coverage target: listSubmodules() success path (lines 152, 157-167)
738+
* This test creates a temporary git repo with a submodule to verify
739+
* that the parsing logic works correctly after fixing the gitExec trim bug.
740+
*/
741+
async function testListSubmodulesWithSubmodule(): Promise<void> {
742+
console.log('Testing listSubmodules with real submodule...');
743+
744+
const { execSync } = await import('node:child_process');
745+
// Use realpathSync to resolve /tmp -> /private/tmp on macOS
746+
const parentDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'parent-repo-')));
747+
const childDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'child-repo-')));
748+
749+
try {
750+
// 1. Create child repo (will become submodule)
751+
execSync('git init', { cwd: childDir, stdio: 'ignore' });
752+
execSync('git config user.email "test@example.com"', { cwd: childDir, stdio: 'ignore' });
753+
execSync('git config user.name "Test"', { cwd: childDir, stdio: 'ignore' });
754+
fs.writeFileSync(path.join(childDir, 'README.md'), '# Child Repo');
755+
execSync('git add .', { cwd: childDir, stdio: 'ignore' });
756+
execSync('git commit -m "Initial commit"', { cwd: childDir, stdio: 'ignore' });
757+
758+
// 2. Create parent repo
759+
execSync('git init', { cwd: parentDir, stdio: 'ignore' });
760+
execSync('git config user.email "test@example.com"', { cwd: parentDir, stdio: 'ignore' });
761+
execSync('git config user.name "Test"', { cwd: parentDir, stdio: 'ignore' });
762+
fs.writeFileSync(path.join(parentDir, 'README.md'), '# Parent Repo');
763+
execSync('git add .', { cwd: parentDir, stdio: 'ignore' });
764+
execSync('git commit -m "Initial commit"', { cwd: parentDir, stdio: 'ignore' });
765+
766+
// 3. Add child as submodule (use -c protocol.file.allow=always for git >= 2.38.1)
767+
execSync(`git -c protocol.file.allow=always submodule add ${childDir} vendor/child`, { cwd: parentDir, stdio: 'ignore' });
768+
execSync('git commit -m "Add submodule"', { cwd: parentDir, stdio: 'ignore' });
769+
770+
// 4. Call listSubmodules
771+
const submodules = await listSubmodules(parentDir);
772+
773+
// 5. Assertions
774+
assert.ok(Array.isArray(submodules), 'Should return an array');
775+
assert.strictEqual(submodules.length, 1, 'Should find exactly 1 submodule');
776+
777+
const sub = submodules[0];
778+
assert.strictEqual(sub.path, 'vendor/child', 'Submodule path should be vendor/child');
779+
assert.strictEqual(sub.initialized, true, 'Submodule should be initialized');
780+
assert.ok(sub.commitHash.match(/^[a-f0-9]{40}$/), 'Commit hash should be 40 hex chars');
781+
assert.ok(sub.absolutePath.endsWith('vendor/child'), 'Absolute path should end with vendor/child');
782+
783+
console.log('✅ listSubmodules with real submodule tests passed!');
784+
} finally {
785+
fs.rmSync(parentDir, { recursive: true, force: true });
786+
fs.rmSync(childDir, { recursive: true, force: true });
787+
}
788+
}
789+
790+
/**
791+
* Test listSubmodules with multiple submodules
792+
*
793+
* Coverage target: listSubmodules() success path with multiple entries
794+
* This test verifies that the gitExecRaw fix works correctly when
795+
* there are multiple submodules (the original bug only affected the first).
796+
*/
797+
async function testListSubmodulesWithMultipleSubmodules(): Promise<void> {
798+
console.log('Testing listSubmodules with multiple submodules...');
799+
800+
const { execSync } = await import('node:child_process');
801+
// Use realpathSync to resolve /tmp -> /private/tmp on macOS
802+
const parentDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'parent-multi-')));
803+
const childDir1 = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'child1-')));
804+
const childDir2 = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'child2-')));
805+
const childDir3 = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'child3-')));
806+
807+
try {
808+
// 1. Create child repos (will become submodules)
809+
for (const [childDir, name] of [
810+
[childDir1, 'child1'],
811+
[childDir2, 'child2'],
812+
[childDir3, 'child3'],
813+
] as const) {
814+
execSync('git init', { cwd: childDir, stdio: 'ignore' });
815+
execSync('git config user.email "test@example.com"', { cwd: childDir, stdio: 'ignore' });
816+
execSync('git config user.name "Test"', { cwd: childDir, stdio: 'ignore' });
817+
fs.writeFileSync(path.join(childDir, 'README.md'), `# ${name}`);
818+
execSync('git add .', { cwd: childDir, stdio: 'ignore' });
819+
execSync('git commit -m "Initial commit"', { cwd: childDir, stdio: 'ignore' });
820+
}
821+
822+
// 2. Create parent repo
823+
execSync('git init', { cwd: parentDir, stdio: 'ignore' });
824+
execSync('git config user.email "test@example.com"', { cwd: parentDir, stdio: 'ignore' });
825+
execSync('git config user.name "Test"', { cwd: parentDir, stdio: 'ignore' });
826+
fs.writeFileSync(path.join(parentDir, 'README.md'), '# Parent Repo');
827+
execSync('git add .', { cwd: parentDir, stdio: 'ignore' });
828+
execSync('git commit -m "Initial commit"', { cwd: parentDir, stdio: 'ignore' });
829+
830+
// 3. Add children as submodules (use -c protocol.file.allow=always for git >= 2.38.1)
831+
execSync(`git -c protocol.file.allow=always submodule add ${childDir1} vendor/lib1`, { cwd: parentDir, stdio: 'ignore' });
832+
execSync(`git -c protocol.file.allow=always submodule add ${childDir2} vendor/lib2`, { cwd: parentDir, stdio: 'ignore' });
833+
execSync(`git -c protocol.file.allow=always submodule add ${childDir3} tools/cli`, { cwd: parentDir, stdio: 'ignore' });
834+
execSync('git commit -m "Add submodules"', { cwd: parentDir, stdio: 'ignore' });
835+
836+
// 4. Call listSubmodules
837+
const submodules = await listSubmodules(parentDir);
838+
839+
// 5. Assertions
840+
assert.ok(Array.isArray(submodules), 'Should return an array');
841+
assert.strictEqual(submodules.length, 3, 'Should find exactly 3 submodules');
842+
843+
// Verify all submodules are found (order may vary)
844+
const paths = submodules.map(s => s.path).sort();
845+
assert.deepStrictEqual(paths, ['tools/cli', 'vendor/lib1', 'vendor/lib2'], 'Should find all submodule paths');
846+
847+
// Verify all are initialized with valid commit hashes
848+
for (const sub of submodules) {
849+
assert.strictEqual(sub.initialized, true, `Submodule ${sub.path} should be initialized`);
850+
assert.ok(sub.commitHash.match(/^[a-f0-9]{40}$/), `Submodule ${sub.path} should have valid commit hash`);
851+
}
852+
853+
console.log('✅ listSubmodules with multiple submodules tests passed!');
854+
} finally {
855+
fs.rmSync(parentDir, { recursive: true, force: true });
856+
fs.rmSync(childDir1, { recursive: true, force: true });
857+
fs.rmSync(childDir2, { recursive: true, force: true });
858+
fs.rmSync(childDir3, { recursive: true, force: true });
859+
}
860+
}
861+
862+
/**
863+
* Test listSubmodulesRecursive with real nested submodules
864+
*
865+
* Coverage target: listSubmodulesRecursive() with nested submodules
866+
* This test verifies that nested submodules are correctly detected at all depth levels.
867+
* Creates a 3-level hierarchy: root → level1 → level2
868+
*/
869+
async function testListSubmodulesRecursiveWithRealRepos(): Promise<void> {
870+
console.log('Testing listSubmodulesRecursive with nested submodules...');
871+
872+
const { execSync } = await import('node:child_process');
873+
// Use realpathSync to resolve /tmp -> /private/tmp on macOS
874+
const rootDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'root-')));
875+
const level1Dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'level1-')));
876+
const level2Dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'level2-')));
877+
878+
try {
879+
// 1. Create level2 repo (deepest)
880+
execSync('git init', { cwd: level2Dir, stdio: 'ignore' });
881+
execSync('git config user.email "test@example.com"', { cwd: level2Dir, stdio: 'ignore' });
882+
execSync('git config user.name "Test"', { cwd: level2Dir, stdio: 'ignore' });
883+
fs.writeFileSync(path.join(level2Dir, 'README.md'), '# Level 2');
884+
execSync('git add .', { cwd: level2Dir, stdio: 'ignore' });
885+
execSync('git commit -m "Initial commit"', { cwd: level2Dir, stdio: 'ignore' });
886+
887+
// 2. Create level1 repo with level2 as submodule
888+
execSync('git init', { cwd: level1Dir, stdio: 'ignore' });
889+
execSync('git config user.email "test@example.com"', { cwd: level1Dir, stdio: 'ignore' });
890+
execSync('git config user.name "Test"', { cwd: level1Dir, stdio: 'ignore' });
891+
fs.writeFileSync(path.join(level1Dir, 'README.md'), '# Level 1');
892+
execSync('git add .', { cwd: level1Dir, stdio: 'ignore' });
893+
execSync('git commit -m "Initial commit"', { cwd: level1Dir, stdio: 'ignore' });
894+
execSync(`git -c protocol.file.allow=always submodule add ${level2Dir} nested/level2`, { cwd: level1Dir, stdio: 'ignore' });
895+
execSync('git commit -m "Add level2 submodule"', { cwd: level1Dir, stdio: 'ignore' });
896+
897+
// 3. Create root repo with level1 as submodule
898+
execSync('git init', { cwd: rootDir, stdio: 'ignore' });
899+
execSync('git config user.email "test@example.com"', { cwd: rootDir, stdio: 'ignore' });
900+
execSync('git config user.name "Test"', { cwd: rootDir, stdio: 'ignore' });
901+
fs.writeFileSync(path.join(rootDir, 'README.md'), '# Root');
902+
execSync('git add .', { cwd: rootDir, stdio: 'ignore' });
903+
execSync('git commit -m "Initial commit"', { cwd: rootDir, stdio: 'ignore' });
904+
execSync(`git -c protocol.file.allow=always submodule add ${level1Dir} libs/level1`, { cwd: rootDir, stdio: 'ignore' });
905+
execSync('git commit -m "Add level1 submodule"', { cwd: rootDir, stdio: 'ignore' });
906+
907+
// 3.5. Initialize nested submodules recursively
908+
execSync('git -c protocol.file.allow=always submodule update --init --recursive', { cwd: rootDir, stdio: 'ignore' });
909+
910+
// 4. Test with depth=1 (should find only level1)
911+
const depth1Results = await listSubmodulesRecursive(rootDir, 1);
912+
assert.strictEqual(depth1Results.length, 1, 'Depth 1 should find 1 submodule');
913+
assert.strictEqual(depth1Results[0].path, 'libs/level1', 'Should find level1');
914+
915+
// 5. Test with depth=2 (should find level1 and level2)
916+
const depth2Results = await listSubmodulesRecursive(rootDir, 2);
917+
assert.strictEqual(depth2Results.length, 2, 'Depth 2 should find 2 submodules');
918+
const paths = depth2Results.map(s => s.path).sort();
919+
assert.ok(paths.includes('libs/level1'), 'Should find level1');
920+
assert.ok(paths.includes('nested/level2'), 'Should find level2');
921+
922+
// 6. Test with depth=3 (should still find 2, no more nesting)
923+
const depth3Results = await listSubmodulesRecursive(rootDir, 3);
924+
assert.strictEqual(depth3Results.length, 2, 'Depth 3 should still find 2 submodules (no deeper nesting)');
925+
926+
// 7. Test with depth=0 (should find nothing)
927+
const depth0Results = await listSubmodulesRecursive(rootDir, 0);
928+
assert.strictEqual(depth0Results.length, 0, 'Depth 0 should find 0 submodules');
929+
930+
console.log('✅ listSubmodulesRecursive with nested submodules tests passed!');
931+
} finally {
932+
fs.rmSync(rootDir, { recursive: true, force: true });
933+
fs.rmSync(level1Dir, { recursive: true, force: true });
934+
fs.rmSync(level2Dir, { recursive: true, force: true });
935+
}
936+
}
937+
734938
/**
735939
* Run all submodule tests
736940
*/
@@ -775,6 +979,15 @@ export async function runSubmoduleTests(): Promise<void> {
775979
// listSubmodules empty result path with real git repo
776980
await testListSubmodulesEmptyResult();
777981

982+
// listSubmodules success path with real submodule
983+
await testListSubmodulesWithSubmodule();
984+
985+
// listSubmodules with multiple submodules (verifies gitExecRaw fix)
986+
await testListSubmodulesWithMultipleSubmodules();
987+
988+
// listSubmodulesRecursive with nested submodules (verifies depth handling)
989+
await testListSubmodulesRecursiveWithRealRepos();
990+
778991
console.log('\n✅ All submodule tests passed!\n');
779992
} catch (error) {
780993
console.error('\n❌ Test failed:', error);

0 commit comments

Comments
 (0)