Skip to content

Conversation

@fslds
Copy link

@fslds fslds commented Aug 8, 2025

Reasoning:
Not all use-cases have credentials set in the environment necessarily, especially when used as a library. Credentials can also come from secret files, vaults, etc and may not be exposed in the environment.

Some practices even discourage putting credentials in the environment as various crash dumps may include them, potentially causing credential leaks.

This is a fully backwards compatible minor change, exposing the credentials (client_id and client_secret in the __init__ functions of both the FalconClient and the FalconMCPServer, with a default of None.

@fslds fslds marked this pull request as ready for review August 8, 2025 19:56
@carlosmmatos carlosmmatos added the enhancement New feature or request label Aug 11, 2025
@carlosmmatos carlosmmatos requested a review from protiumx August 11, 2025 11:05
@carlosmmatos
Copy link
Collaborator

Hi @fslds,

Thanks for this PR! This is a solid enhancement that addresses a real need for library usage and better credential management practices. The implementation looks good and follows sensible Python patterns.

There are a few things we need to address before merging:

Missing Test Coverage
The main blocker is that we don't have tests for the new functionality. We'll need tests in both test_client.py and test_server.py to cover:

  • Client initialization with directly passed credentials
  • Credential precedence (direct params should override environment variables)
  • Server properly passing credentials through to the client

Error Message Update
The error message in FalconClient.__init__ still only mentions environment variables. It should be updated to mention both approaches:

raise ValueError(
    "Falcon API credentials not provided. Either pass client_id and client_secret "
    "parameters or set FALCON_CLIENT_ID and FALCON_CLIENT_SECRET environment variables."
)

Documentation
We should add an example to the README showing library usage with direct credentials, especially for secret management integration scenarios. Something showing how this works with HashiCorp Vault or similar would be valuable.

Collaboration Offer
If you're not familiar with our testing patterns or would prefer help with any of these changes, I'm happy to collaborate on this PR or provide guidance. Just let me know what would be most helpful.

This change will be really useful for enterprise deployments where credentials come from secret management systems rather than environment variables. The backward compatibility is great too.

Let me know if you have questions about any of these requirements.

@fslds
Copy link
Author

fslds commented Aug 14, 2025

Hey @carlosmmatos - good feedback, I'll work on those points and update the PR once done here!

@protiumx
Copy link
Collaborator

protiumx commented Nov 7, 2025

Hi @fslds, thank you for the PR. As @carlosmmatos mentioned this is a very nice addition. Let us know if you would like to proceed adding tests and docs updates or if you would prefer we take over it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants