Skip to content

do not pass default branch and repo name to coana CLI#723

Merged
mtorp merged 3 commits intomainfrom
dont-pass-default-branch-and-repo
Sep 1, 2025
Merged

do not pass default branch and repo name to coana CLI#723
mtorp merged 3 commits intomainfrom
dont-pass-default-branch-and-repo

Conversation

@mtorp
Copy link
Contributor

@mtorp mtorp commented Sep 1, 2025

This change ensures that Coana won't try to fetch cached analysis configuration for the default project. The default repo and branch are likely used for very different projects, so we shouldn't be using cached configuration.

  • edit: Also upgrades Coana to 14.12.10 to fix various minor bugs related to the reachability analysis.

@mtorp mtorp requested a review from barslev September 1, 2025 11:02
@claude
Copy link

claude bot commented Sep 1, 2025

Claude finished @mtorp's task —— View job


Code Review Complete ✅

Reviewing PR #723: "do not pass default branch and repo name to coana CLI"

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze the modified file: src/commands/scan/perform-reachability-analysis.mts
  • Review code changes for quality, bugs, performance, security, and design
  • Provide comprehensive feedback with specific suggestions

Review Summary

Overall Assessment: ✅ APPROVED

This is a well-reasoned and correctly implemented fix that prevents unintended configuration sharing between different projects.

Code Analysis

The Change:

- if (repoName) {
+ if (repoName && repoName !== constants.SOCKET_DEFAULT_REPOSITORY) {
    env['SOCKET_REPO_NAME'] = repoName
  }
- if (branchName) {
+ if (branchName && branchName !== constants.SOCKET_DEFAULT_BRANCH) {
    env['SOCKET_BRANCH_NAME'] = branchName
  }

What it does:

  • Previously: Always passed repoName and branchName to Coana CLI if they were truthy
  • Now: Only passes them if they differ from the default fallback values ('socket-default-repository' and 'socket-default-branch')

Review Feedback

Strengths:

  1. Clear Problem & Solution: The change addresses a real issue where different projects could inadvertently share cached Coana configuration through default values.

  2. Good Documentation: The inline comment clearly explains the reasoning (// do not pass default repo and branch name to coana to mixing buckets).

  3. Consistent Implementation: Both repoName and branchName are handled identically, maintaining consistency.

  4. Safe Change: This is a conservative fix that won't break existing functionality - it only prevents passing meaningless default values.

  5. Proper Use of Constants: Uses the defined constants (SOCKET_DEFAULT_REPOSITORY, SOCKET_DEFAULT_BRANCH) rather than hardcoding strings.

💬 Minor Suggestions:

  1. Typo in Comment (perform-reachability-analysis.mts:170):

    - // do not pass default repo and branch name to coana to mixing
    + // do not pass default repo and branch name to coana to avoid mixing
  2. Consider Logging: For debugging purposes, you might consider adding a debug log when skipping default values:

    if (repoName && repoName !== constants.SOCKET_DEFAULT_REPOSITORY) {
      env['SOCKET_REPO_NAME'] = repoName
    } else if (repoName === constants.SOCKET_DEFAULT_REPOSITORY) {
      // Optional: Add debug logging
      // debug('Skipping default repository name to avoid config mixing')
    }

    (Though this may be overkill for this change)

🔍 Technical Assessment:

  • Security: ✅ No security implications
  • Performance: ✅ Negligible performance impact (adds two string comparisons)
  • Architecture: ✅ Follows existing patterns and doesn't introduce architectural changes
  • Error Handling: ✅ No additional error handling needed
  • Testing: The change is simple enough that existing tests should cover the functionality

Conclusion

This is a straightforward, well-implemented fix that addresses a legitimate concern about configuration isolation between projects. The change is minimal, safe, and properly documented.

Recommendation: Merge after fixing the minor typo in the comment.


@socket-security-staging
Copy link

socket-security-staging bot commented Sep 1, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updated@​coana-tech/​cli@​14.12.6 ⏵ 14.12.10851007896100

View full report

@socket-security
Copy link

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updated@​coana-tech/​cli@​14.12.6 ⏵ 14.12.1086 +110079 +196 +1100

View full report

@mtorp mtorp merged commit 25f35f5 into main Sep 1, 2025
17 checks passed
@mtorp mtorp deleted the dont-pass-default-branch-and-repo branch September 1, 2025 12:01
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.

2 participants