Update socket scan reach to include the same flags as socket scan create --reach

[mtorp]

This PR updates socket scan reach to:

1. Include the same reachability-specific flags as socket scan create --reach.
2. Use the --manifests-tar-hash reachability workflow, which is what ensures that SBOMs are computed by Socket instead of the client side SBOM computation from the Coana CLI.

The main difference between socket scan create --reach and socket scan reach is that socket scan create --reach creates a full scan whereas socket scan reach just writes the reachability results to .socket.facts.json.

[claude]

Claude finished @mtorp's task —— View job

---

Pull Request Review

I'll review this PR focusing on code quality, potential bugs, performance, security, and architecture.

Todo List:

• Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
• Analyze the refactoring of reachability analysis into separate modules
• Review new files: perform-reachability-analysis.mts and reachability-flags.mts
• Examine modifications to existing command files
• Check test coverage and test quality
• Look for potential bugs, security issues, and performance concerns
• Provide comprehensive feedback with specific suggestions

Code Review Analysis

This PR performs a significant refactoring of the reachability analysis functionality, extracting shared code into reusable modules. The changes are well-structured and follow good software engineering principles.

🎯 Architecture & Design

Excellent modularization effort:

• ✅ Good: Extracted reachabilityFlags into a shared module, eliminating 37 lines of duplication from cmd-scan-create.mts
• ✅ Good: Created performReachabilityAnalysis() as a reusable function, removing ~166 lines of duplicated logic
• ✅ Good: Enhanced cmd-scan-reach to match feature parity with cmd-scan-create --reach

🔍 Code Quality Issues

1. Inconsistent Ecosystem Processing
src/commands/scan/perform-reachability-analysis.mts:151-153

'--ecosystems',
...convertToCoanaEcosystems(reachabilityOptions.reachEcosystems),

vs original in handle-create-new-scan.mts:

convertToCoanaEcosystems(reachabilityOptions.reachEcosystems).join(' ')

Issue: The refactored version spreads the array, while the original joined with spaces. This could cause different behavior.

2. Potential Type Coercion Issues
src/commands/scan/cmd-scan-reach.mts:194-197

reachAnalysisTimeout: Number(reachAnalysisTimeout),
reachAnalysisMemoryLimit: Number(reachAnalysisMemoryLimit),

Issue: Number(undefined) returns NaN, which may not be the intended behavior. Consider using nullish coalescing:

reachAnalysisTimeout: reachAnalysisTimeout ?? 0,
reachAnalysisMemoryLimit: reachAnalysisMemoryLimit ?? 8192,

3. Missing Parameter Validation
src/commands/scan/perform-reachability-analysis.mts:63-108
Issue: The function doesn't validate required parameters when uploadManifests is true. Consider adding validation:

if (uploadManifests && (!orgSlug || !packagePaths)) {
  return {
    ok: false,
    message: 'Organization slug and package paths are required for manifest upload',
    cause: 'Missing required parameters for reachability analysis'
  }
}

🧪 Testing

✅ Good: Comprehensive test coverage for new command flags, including:

• Individual flag validation
• Comma-separated values handling
• Multiple flag instances
• Invalid ecosystem validation
• Combined flag usage

🚀 Performance & Security

No significant concerns identified:

• Manifest filtering logic correctly excludes previous .socket.facts.json files
• Environment variables are properly scoped
• No sensitive data exposure in logs

🔧 Minor Improvements

1. Environment Variable Assignment
src/commands/scan/perform-reachability-analysis.mts:167-172

if (repoName) {
  env['SOCKET_REPO_NAME'] = repoName
}
if (branchName) {
  env['SOCKET_BRANCH_NAME'] = branchName
}

Suggestion: Use more concise assignment:

Object.assign(env, {
  ...(repoName && { SOCKET_REPO_NAME: repoName }),
  ...(branchName && { SOCKET_BRANCH_NAME: branchName }),
})

2. Magic Number
src/commands/scan/reachability-flags.mts:13

default: 8192,

Suggestion: Extract to a named constant for better maintainability.

📝 Summary

This is a solid refactoring that improves code organization and eliminates duplication. The main concerns are:

1. Critical: Fix ecosystem processing inconsistency to maintain behavior parity
2. Important: Handle undefined values in number coercion properly
3. Minor: Add parameter validation for better error handling

The architectural decisions are sound, test coverage is excellent, and the code follows established patterns in the codebase.

Recommendation: Approve with minor fixes 🟡

---

[pvdz]

Nit: the dryrun check happens after the checkCommandInput call below, usually the last step of these cmd- files.
This way all the tests can test input validation. As long as there is no network activity or write mutations, dry run can continue. Note that determineOrgSlug will pass on the dryRun state, for example, and skip network in that case.
Most of the time the cmd- files don't do write mutation or network access (or can do without in dryRun) up to the point where they call the handle file.

[mtorp]

Thanks, good catch! I've moved the bail now.

[pvdz]

Looks odd to me since there's no usage removal but I guess these weren't used before?

[mtorp]

They are in use. Just imported from reachability-flags.mts instead so they can be shared between scan create --reach and scan reach.