WIP: feat: add support for options.profile on aws clients #1493
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
sxlijin
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to a6422c0 in 1 minute and 48 seconds
More details
- Looked at
183lines of code in8files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. integ-tests/typescript/tests/providers/aws.test.ts:334
- Draft comment:
Remove 'it.only' in invalid profile test to avoid limiting test run. - Reason this comment was not posted:
Marked as duplicate.
2. tools/build:222
- Draft comment:
Ensure switching test target from vertex to aws is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This comment violates our rules. It starts with "Ensure..." which is explicitly called out as likely not useful. It's asking the author to verify their intention, which is against our guidelines. The author deliberately made this change, and we should assume they know what they're doing unless there's strong evidence of a problem.
Maybe the test target change could break some important test coverage? Maybe there's a specific reason vertex.test.ts needs to run?
Without strong evidence that running vertex.test.ts is required or that this change causes problems, we should trust the author's judgment. The change appears deliberate and straightforward.
Delete this comment. It's asking for verification of intention which violates our rules, and we don't have evidence that the change is problematic.
3. tools/uv.lock:3
- Draft comment:
Raising 'requires-python' to '>=3.12' may break compatibility with older python versions; confirm intention. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This is a dependency/version management change. The comment asks for confirmation of intention, which violates our rules. Lock files are typically generated files that reflect deliberate version choices. The author likely intentionally updated the Python version requirement.
The Python version change could have significant implications for the project's compatibility. Maybe this deserves special attention?
While Python version changes are important, our rules explicitly state not to comment on dependency changes or ask for confirmation of intentions. Lock files especially are deliberately generated.
Delete the comment as it violates our rules about commenting on dependency changes and asking for confirmation of intentions.
4. integ-tests/typescript/tests/providers/aws.test.ts:191
- Draft comment:
Remove 'it.only' here to avoid skipping other tests. 'it.only' should not be committed. - Reason this comment was not posted:
Marked as duplicate.
5. integ-tests/typescript/tests/providers/aws.test.ts:334
- Draft comment:
Remove 'it.only' in the invalid profile test; leaving .only will limit test execution. - Reason this comment was not posted:
Marked as duplicate.
6. tools/build:221
- Draft comment:
Verify that switching the test command from 'vertex.test.ts' to 'aws.test.ts' is deliberate and doesn’t inadvertently skip vertex provider tests. - Reason this comment was not posted:
Marked as duplicate.
7. tools/uv.lock:1
- Draft comment:
Ensure the upgrade to Python >=3.12 and removal of conditional markers (e.g., for termcolor) aligns with all target environments. - Reason this comment was not posted:
Marked as duplicate.
8. integ-tests/baml_src/clients.baml:246
- Draft comment:
Typographical issue: The client definition 'client Invalid{' is missing a space before the opening brace. For consistency and readability, please change it to 'client Invalid {'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. integ-tests/python/baml_client/inlinedbaml.py:350
- Draft comment:
Typo: In the declaration for 'client Invalid{', it would be more readable to add a space between 'Invalid' and '{'. Please change it to 'client Invalid {' to maintain consistency with the other client declarations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. fiddle-examples/chat-roles.baml:10
- Draft comment:
Typographical error: The return type of function 'ClassifyMessage2' is declared as 'Category' while the defined enum is named 'Category2'. Please update the function's return type to 'Category2' (or adjust the enum name) to align them. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. fiddle-examples/chat-roles.baml:4
- Draft comment:
Typographical error: The function 'ClassifyMessage2' is declared to return 'Category', but the defined enum is 'Category2'. Please update the return type to 'Category2' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. tools/build:273
- Draft comment:
Typographical error: In the comment on line 273, 'schanges' should be corrected to 'changes' (and consider using 'consumes' if that fits the intent). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_FNYMgQzWeZ6HBj6B
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -188,12 +188,12 @@ describe('AWS Provider', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('should support AWS profile configuration', async () => { | |||
| it.only('should support AWS profile configuration', async () => { | |||
There was a problem hiding this comment.
Remove 'it.only' in profile tests to ensure full test suite runs.
Suggested change
| it.only('should support AWS profile configuration', async () => { | |
| it('should support AWS profile configuration', async () => { |
sxlijin
changed the title
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a199d09 in 1 minute and 54 seconds
More details
- Looked at
79lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. engine/baml-lib/llm-client/src/clients/aws_bedrock.rs:230
- Draft comment:
Removing the cfg attributes for wasm and always resolving profile is intentional? Ensure that enabling profile resolution in wasm does not break limitations (e.g., file system access) in the playground. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:155
- Draft comment:
The WASM branch now uses the provided profile (if any) by setting it on the credentials chain builder. Confirm that the builder.profile_name(profile) call produces the intended effect in the playground. Also, consider removing/commenting out leftover debug code. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment asks the PR author to confirm the effect of a specific call, which violates the rule against asking for confirmation of intention or behavior. However, it also suggests considering the removal of leftover debug code, which is a valid suggestion. The comment is partially useful, but the part asking for confirmation should be removed.
3. engine/baml-runtime/src/internal/llm_client/primitive/aws/wasm.rs:44
- Draft comment:
Profile file support is now enabled by uncommenting the .profile_files() call. Verify that the hard-coded config content and the file access in WASM work as intended in the playground, given the filesystem limitations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the behavior of the code, which violates the rule against asking for confirmation or verification of behavior. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:155
- Draft comment:
The updated block now logs debug information and uses WasmAwsCreds with a profile name when no explicit credentials are provided. Consider cleaning up by removing commented-out alternative calls and clarifying the note about the 'static lifetime' requirement. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_NiyMOqKtTezU7itt
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -231,16 +231,16 @@ impl UnresolvedAwsBedrock { | |||
| } | |||
| }; | |||
|
|
|||
| #[cfg(not(target_arch = "wasm32"))] | |||
| // #[cfg(not(target_arch = "wasm32"))] | |||
There was a problem hiding this comment.
The cfg attributes for wasm are now commented out so that the profile is always resolved. Consider removing the commented-out cfg checks entirely once the feature is stable to reduce confusion.
Suggested change
| // #[cfg(not(target_arch = "wasm32"))] |
fix profile loading, we were previously respecting the wrong one omg it works ok commented out change is fine vscode ext works again, but not triggering WasmAwsCreds more stuff finally stop freaking out about json files rename to load_env one more rename one more atom
sxlijin
force-pushed
the
sam/aws-profile
branch
from
a199d09 to
7a4ecce
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 7a4ecce in 2 minutes and 34 seconds
More details
- Looked at
270lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. engine/baml-runtime/src/internal/llm_client/primitive/aws/wasm.rs:194
- Draft comment:
The ProvideCredentials implementation currently returns hardcoded fake credentials. Ensure this is intended as a temporary placeholder and that production use is properly secured. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment points out a real security concern - hardcoded fake credentials in production could be dangerous. However, the comment is phrased as a verification request ("Ensure this is intended...") which violates our rules. Additionally, the code is in a WASM-specific implementation which suggests this may be intentional for browser environments where real AWS credentials wouldn't be used.
I might be wrong about the WASM context - perhaps real AWS credentials are needed even in browser environments. Also, even if this is intentional, having fake credentials could still be a security risk worth flagging.
While security is important, the comment's phrasing as a verification request makes it not actionable. If there was a real security issue, it should be stated directly rather than asking for confirmation.
Delete the comment. While it identifies a potential security concern, it's phrased as a verification request rather than an actionable issue, and the WASM context suggests this may be intentional behavior.
2. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:151
- Draft comment:
Replace the unprofessional debug message with a more appropriate log (avoid profanity). - Reason this comment was not posted:
Marked as duplicate.
3. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:159
- Draft comment:
The call to aws_config::defaults(BehaviorVersion::latest()) is made without capturing its result. Consider removing it or assigning it, if needed. - Reason this comment was not posted:
Marked as duplicate.
4. typescript/playground-common/src/shared/baml-project-panel/atoms.ts:88
- Draft comment:
The awsCreds are loaded and logged but not integrated into the runtime/environment configuration. Confirm if additional handling is needed for profile support in the playground. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm if additional handling is needed for profile support, which violates the rule against asking for confirmation or ensuring behavior. It does not provide a specific suggestion or point out a clear issue.
5. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:207
- Draft comment:
Typo: In the comment explaining the secret key check, 'remainer' should be corrected to 'remainder'. - Reason this comment was not posted:
Comment was on unchanged code.
6. engine/baml-runtime/src/types/runtime_context.rs:48
- Draft comment:
There appears to be a typographical error in the non-wasm branch definition of AwsCredProvider. The type declaration ends with an extra '>' (i.e.,...Result<HashMap<String, String>>>>should likely be corrected to...Result<HashMap<String, String>>>). Please fix this extra bracket. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment suggests removing a bracket, but looking at the parallel wasm implementation and counting the brackets carefully, 5 brackets are actually correct: 1 for HashMap, 1 for Result, 1 for BoxFuture, 1 for Box, and 1 for Option. The comment is suggesting a change that would make the code incorrect.
Could I be miscounting the brackets? Should I be considering other factors beyond just matching the wasm version?
I've triple-checked the bracket count and mapped each closing bracket to its opening construct. The 5 brackets are definitely correct and necessary for this type definition.
The comment is incorrect - it suggests removing a bracket that is actually needed. The PR change adding the bracket was correct.
7. typescript/playground-common/src/shared/baml-project-panel/atoms.ts:319
- Draft comment:
Typo found in the comment. The word 'somehting' should be corrected to 'something' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_yovLItWpTCWkruPV
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| if let Some(profile) = self.properties.profile.as_ref() { | ||
| builder = builder.profile_name(profile); | ||
| } | ||
| log::debug!("Building wasm aws credentials chain - UNCONDITIONALLY BECAUSE BULLSHIT $ENV_VAR UNCONDITIONAL SUBSTITUTION"); |
There was a problem hiding this comment.
The debug log message here contains profanity, which is unprofessional. Consider rephrasing it to a more neutral message.
Suggested change
| log::debug!("Building wasm aws credentials chain - UNCONDITIONALLY BECAUSE BULLSHIT $ENV_VAR UNCONDITIONAL SUBSTITUTION"); | |
| log::debug!("Building wasm aws credentials chain - due to unconditional $ENV_VAR substitution"); |
|
|
||
| // Set region if specified | ||
| let loader = { | ||
| aws_config::defaults(BehaviorVersion::latest()); |
There was a problem hiding this comment.
The non-WASM branch calls aws_config::defaults(...) without assigning its result. Likely, you meant to capture the returned loader before proceeding with credentials configuration.
Suggested change
| aws_config::defaults(BehaviorVersion::latest()); | |
| let loader = aws_config::defaults(BehaviorVersion::latest()); |
| &aws_secret_access_key.api_key.expose_secret()[1..] | ||
| )); | ||
| _ => { | ||
| log::debug!( |
There was a problem hiding this comment.
The log message in the non-WASM branch uses wording referring to a WASM credentials chain. This may be confusing; ensure the log clearly indicates which branch is executing.
| const vscodeEnv = (await vscode.loadEnv())?.envVars ?? {} | ||
|
|
||
| const awsCreds = await vscode.loadAwsCreds() | ||
| console.log('awsCreds', awsCreds) |
There was a problem hiding this comment.
Avoid leaving console.log statements (e.g. logging awsCreds) in production code. Consider using a proper logger or removing it.
Suggested change
| console.log('awsCreds', awsCreds) |
| // let datetime: DateTime<Utc> = datetime_str.parse().expect("Invalid datetime format"); | ||
| // let expires_after = | ||
| // web_time::UNIX_EPOCH + web_time::Duration::from_secs(datetime.timestamp() as u64); | ||
| ProvideCredentials::ready(Ok(Credentials::new( |
There was a problem hiding this comment.
WasmAwsCreds currently returns hardcoded fake credentials. Please add a TODO or implement proper profile-based credentials support.
| } else { | ||
| use futures::future::BoxFuture; | ||
| pub type BamlSrcReader = Option<Box<fn(&str) -> BoxFuture<'static, Result<Vec<u8>>>>>; | ||
| pub type AwsCredProvider = Option<Box<fn(Option<&str>) -> BoxFuture<'static, Result<HashMap<String, String>>>>>>; |
There was a problem hiding this comment.
The type definition for AwsCredProvider appears to have an extra '>' in the closing brackets. Please verify and correct the type signature.
Suggested change
| pub type AwsCredProvider = Option<Box<fn(Option<&str>) -> BoxFuture<'static, Result<HashMap<String, String>>>>>>; | |
| pub type AwsCredProvider = Option<Box<fn(Option<&str>) -> BoxFuture<'static, Result<HashMap<String, String>>>>>; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes three issues:
aws_config::default_providerright now, aka when the user provides no cues about aws creds in the baml client optionsImplementation notes:
AWS_PROFILEcan be backed by either IAM role profiles or SSO exchanges~/.aws/credentials~/.aws/sso/(I'm unclear on how exactly the Rust SDK hooks into this yet, nor if an override point exists here)std::fs::readin wasm, we instead just implementProvideCredentialsin wasm and force it to delegate tonpm:@aws-sdk/credential-providersin the proxy, that feels a lot more doable. however, protocol will also need to support forwarding env vars over (and shimming them on the node side). we may also need some shenanigans to make sure bedrock works on promptfiddle if we do this.fixes #1488
(currently WIP)
Important
Enhances AWS profile support across the system, including VSCode extension and runtime, with updated tests and dependencies.
options.profileinaws_client.rsandaws_bedrock.rs.LoadAwsCredsRequestandLoadAwsCredsResponseinvscode-rpc.ts.WebviewPanelHostto handle AWS credentials loading.baml.envVarFileandbaml.envVarCommandsettings inpackage.json.WebviewPanelHost.ts.extension.tsto handle new AWS profile configurations.aws.test.tsto include tests for AWS profile and credential handling.buildscript to run AWS tests.@aws-sdk/credential-providersand@smithy/typesinpackage.jsonfiles.Cargo.lockfor Rust dependencies.This description was created by
for 7a4ecce. It will automatically update as commits are pushed.