refactor(system): add system.secrets functions#57
Merged
Conversation
- createEmbeddedSecretConfig - createReferencedSecretConfig - readConfiguredSecretValue Closes: #55
Contributor
Reviewer's GuideThis PR adds new system.secrets helper functions for creating secret configurations and reading their values, and introduces a SecretException type in the gateway secrets package with corresponding stub updates. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new helpers in system.secrets currently just return empty PyObject instances or a dummy Plaintext; consider aligning the implementations with the docstrings (e.g., using the provided JSON/provider/secret names) so the returned structures actually represent the described SecretConfig.
- readConfiguredSecretValue mixes
ValueErrorandSecretExceptionfor basic validation and has a typo in the error message; it would be clearer to use consistent exception types/messages for invalid JSON vs real secret access issues. - Docstrings for createEmbeddedSecretConfig/createReferencedSecretConfig describe returning dictionaries while the type hints and implementations use PyObject; consider reconciling the documented return type with the actual and stubbed types for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helpers in system.secrets currently just return empty PyObject instances or a dummy Plaintext; consider aligning the implementations with the docstrings (e.g., using the provided JSON/provider/secret names) so the returned structures actually represent the described SecretConfig.
- readConfiguredSecretValue mixes `ValueError` and `SecretException` for basic validation and has a typo in the error message; it would be clearer to use consistent exception types/messages for invalid JSON vs real secret access issues.
- Docstrings for createEmbeddedSecretConfig/createReferencedSecretConfig describe returning dictionaries while the type hints and implementations use PyObject; consider reconciling the documented return type with the actual and stubbed types for clarity.
## Individual Comments
### Comment 1
<location path="src/system/secrets.py" line_range="164-165" />
<code_context>
+ """
+ if not secretConfig:
+ raise ValueError("There is no JSON.")
+ if not isinstance(secretConfig, dict):
+ raise SecretException("There is a problem eading the secret.")
+ return PyPlaintext(Plaintext())
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Type check makes any value returned from createEmbeddedSecretConfig/createReferencedSecretConfig fail, causing SecretException to always be raised.
readConfiguredSecretValue requires secretConfig to be a dict, but createEmbeddedSecretConfig and createReferencedSecretConfig currently return a bare PyObject. Any secretConfig produced by these helpers will therefore fail the isinstance(secretConfig, dict) check and always raise SecretException. Either align the helpers to return a dict/dict-like object, or relax the type check to validate the expected structure instead, so callers can successfully use the new API end-to-end.
</issue_to_address>
### Comment 2
<location path="src/system/secrets.py" line_range="165" />
<code_context>
+ if not secretConfig:
+ raise ValueError("There is no JSON.")
+ if not isinstance(secretConfig, dict):
+ raise SecretException("There is a problem eading the secret.")
+ return PyPlaintext(Plaintext())
+
</code_context>
<issue_to_address>
**nitpick (typo):** Fix the typo in the SecretException message.
The exception message contains a typo: "eading" should be "reading" to keep error logs and handling clear.
Suggested implementation:
```python
raise SecretException("There is a problem reading the secret.")
```
I only see part of the function in the snippet. Make sure this `SecretException` line appears directly after the `if not isinstance(secretConfig, dict):` check in `src/system/secrets.py`, and that no other occurrences of the typo ("eading the secret.") remain elsewhere in the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+164
to
+165
| if not isinstance(secretConfig, dict): | ||
| raise SecretException("There is a problem eading the secret.") |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Type check makes any value returned from createEmbeddedSecretConfig/createReferencedSecretConfig fail, causing SecretException to always be raised.
readConfiguredSecretValue requires secretConfig to be a dict, but createEmbeddedSecretConfig and createReferencedSecretConfig currently return a bare PyObject. Any secretConfig produced by these helpers will therefore fail the isinstance(secretConfig, dict) check and always raise SecretException. Either align the helpers to return a dict/dict-like object, or relax the type check to validate the expected structure instead, so callers can successfully use the new API end-to-end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #55
Summary by Sourcery
Introduce new system secrets configuration helpers and supporting exception type.
New Features:
Enhancements: