Skip to content

Conversation

@hrntknr
Copy link
Member

@hrntknr hrntknr commented Oct 22, 2025

Summary

#83

Type of Change

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • refactor: A code change that neither fixes a bug nor adds a feature
  • perf: A code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies
  • ci: Changes to our CI configuration files and scripts
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit

Related Issues

Copilot AI review requested due to automatic review settings October 22, 2025 20:17
Copy link
Contributor

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 adds support for manual TLS certificate management, allowing users to provide their own certificate and key files instead of relying solely on automatic ACME-based certificate provisioning. The implementation includes automatic certificate reloading when files change.

Key Changes:

  • New FileReloader component that monitors certificate/key files and reloads them when modified
  • CLI flags --tls-cert-file and --tls-key-file for specifying custom certificates
  • Updated TLS logic to support both automatic ACME provisioning and manual certificate management

Reviewed Changes

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

Show a summary per file
File Description
pkg/tlsreload/file_reloader.go New file watcher that detects certificate/key file changes and reloads them atomically
pkg/mcp-proxy/main.go Updated server initialization to support manual TLS mode with HTTP→HTTPS redirect logic
main.go Added CLI flags for certificate/key file paths and updated autoTLS logic
docs/docs/quickstart.md Updated TLS configuration section to document manual certificate option
docs/docs/configuration.md Added documentation for new TLS certificate and key file options
README.md Updated quick reference to mention manual certificate option
Comments suppressed due to low confidence (1)

pkg/mcp-proxy/main.go:1

  • The autoTLS parameter logic is incorrect. When both tlsCertFile and tlsKeyFile are provided, this evaluates to true, but manual TLS should not enable automatic TLS. The condition should be !noAutoTLS && tlsCertFile == "" && tlsKeyFile == \"\" to enable autoTLS only when manual certificates are not provided and noAutoTLS is false.
package mcpproxy

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

Comment on lines 372 to +373
}
return errors.New("TLS is enabled, but tlsAcceptTOS is not set to true. Please explicitly agree to the TOS")
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

[nitpick] The else block has been removed, changing the control flow. The previous structure with an explicit else clause was clearer. Consider keeping the else block for better readability, or add a blank line before the return statement to separate the conditional logic.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Oct 22, 2025

Codecov Report

❌ Patch coverage is 0% with 155 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/tlsreload/file_reloader.go 0.00% 74 Missing ⚠️
pkg/mcp-proxy/main.go 0.00% 73 Missing ⚠️
main.go 0.00% 8 Missing ⚠️

📢 Thoughts on this report? Let us know!

@hrntknr hrntknr merged commit f888826 into main Oct 22, 2025
8 checks passed
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