Skip to content

Conversation

kofort9
Copy link
Contributor

@kofort9 kofort9 commented Oct 18, 2025

Description

Fixes a panic that occurs when running MCP servers with environment variables when no secrets manager is configured.

Related to #1469

The Problem

When running thv run <server> with environment variables in an environment without a configured secrets manager (e.g., GitHub Actions), the code panics with a nil pointer dereference at pkg/runner/env.go:117.

The Fix

Added a nil check before calling secretsManager.GetSecret() to gracefully handle the case where the secrets manager is not available.

Impact

Before: Panic when running MCP servers without secrets manager
After: Graceful handling - falls through to environment variables or prompts

This maintains backward compatibility:

  • Users with secrets manager configured: Works as before
  • Users without secrets manager: Now works correctly with environment variables

Testing

  • Built and tested locally with environment variables
  • No panic occurs
  • go vet passes, gofmt compliant

This fixes a panic that occurs when running MCP servers via GitHub Actions
with environment variables when no secrets manager is configured.

The code now gracefully handles the case where secretsManager is nil,
allowing both Actions-based (env vars) and Docker-based (secrets manager)
workflows to coexist.

Fixes stacklok#1469
gofmt fixed a formatting issue with an extra blank line in the nil check code.
@kofort9 kofort9 mentioned this pull request Oct 18, 2025
@JAORMX JAORMX requested a review from Copilot October 18, 2025 04:16
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a nil pointer dereference panic that occurs when running MCP servers with environment variables in environments where no secrets manager is configured (e.g., GitHub Actions).

Key Changes:

  • Added nil check before accessing secretsManager.GetSecret() to prevent panic
  • Gracefully falls through to environment variable prompts when secrets manager is unavailable

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

codecov bot commented Oct 18, 2025

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.31%. Comparing base (bd12db9) to head (9f66c56).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/runner/env.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2252      +/-   ##
==========================================
- Coverage   53.33%   53.31%   -0.02%     
==========================================
  Files         231      231              
  Lines       29529    29536       +7     
==========================================
- Hits        15749    15747       -2     
- Misses      12647    12656       +9     
  Partials     1133     1133              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX JAORMX requested a review from Copilot October 18, 2025 07:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Add more context to explain why secrets manager might be null:
- Setup incomplete
- Missing provider

This provides clearer guidance for users and developers.
Break long log message into multiple lines to match file formatting pattern.
JAORMX
JAORMX previously approved these changes Oct 18, 2025
@JAORMX JAORMX requested a review from Copilot October 18, 2025 16:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@JAORMX JAORMX merged commit 042a6d6 into stacklok:main Oct 18, 2025
23 checks passed
@kofort9 kofort9 deleted the fix/env-var-nil-pointer-1469 branch October 18, 2025 21:11
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