This repository was archived by the owner on Oct 2, 2025. It is now read-only.
fix: Prevent undefined org in trace submissions [jules] - #691
Open
jmoseley wants to merge 2 commits into
Open
Conversation
Closes #664 The CheckpointManager in packages/gensx-core/src/checkpoint.ts could allow trace submissions with an 'undefined' string or an empty string as the organization ID if checkpoints were enabled and the org ID was not properly set or was explicitly set to "undefined". This change enhances the validation in the CheckpointManager constructor. It now explicitly checks if `this.org` is an empty string or the literal string "undefined" when checkpoints are enabled (i.e., an API key is present). If either condition is met, an error is thrown, preventing the client from attempting to send traces with an invalid org ID. Added new test cases to packages/gensx-core/tests/checkpoint.test.tsx to cover scenarios where: - org is undefined, an empty string, or the string "undefined" with checkpoints enabled (should throw). - org is valid with checkpoints enabled (should not throw). - org is problematic but checkpoints are disabled (should not throw).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
jmoseley
changed the title
| if (this.checkpointsEnabled && (this.org === "" || this.org === "undefined")) { | ||
| throw new Error( | ||
| "Organization not set. Set it via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", | ||
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
Contributor
There was a problem hiding this comment.
Suggested change
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", | |
| "Organization not set or is invalid. A valid organization must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
|
|
||
| test("throws error if apiKey is present and org is undefined", () => { | ||
| expect(() => new CheckpointManager({ apiKey: "test-key", org: undefined })).toThrow( | ||
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
Contributor
There was a problem hiding this comment.
Suggested change
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", | |
| "Organization not set or is invalid. A valid organization must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
|
|
||
| test("throws error if apiKey is present and org is an empty string", () => { | ||
| expect(() => new CheckpointManager({ apiKey: "test-key", org: "" })).toThrow( | ||
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
Contributor
There was a problem hiding this comment.
Suggested change
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", | |
| "Organization not set or is invalid. A valid organization must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
|
|
||
| test("throws error if apiKey is present and org is 'undefined' string", () => { | ||
| expect(() => new CheckpointManager({ apiKey: "test-key", org: "undefined" })).toThrow( | ||
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
Contributor
There was a problem hiding this comment.
Suggested change
| "Organization not set or is invalid ('undefined' string). A valid organization ID must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", | |
| "Organization not set or is invalid. A valid organization must be set via constructor options, GENSX_ORG environment variable, or in ~/.config/gensx/config when checkpoints are enabled. You can disable checkpoints by setting GENSX_CHECKPOINTS=false or unsetting GENSX_API_KEY.", |
dereklegenzoff
approved these changes
May 20, 2025
Test: Enhance CheckpointManager org validation tests This commit significantly expands the test coverage for the `org` validation logic within the `CheckpointManager` constructor in `packages/gensx-core/src/checkpoint.ts`. The previous tests primarily focused on validation when `apiKey` and `org` were passed directly as constructor options. These new tests ensure robustness by also verifying behavior when these configurations are sourced from environment variables (`GENSX_API_KEY`, `GENSX_ORG`) and from the global configuration file. Key additions and changes: - Restructured tests into sub-suites for clarity: "using constructor options", "using environment variables", and "using config file". - Added comprehensive test cases for scenarios where `org` is invalid (empty, "undefined" string, or not set) when checkpoints are enabled via environment variables or a mocked config file. - Verified correct behavior when `org` and `apiKey` are valid via these alternative sources. - Tested the defined override hierarchy: Constructor options > Environment variables > Config file. This includes ensuring that if a higher-priority source provides an invalid `org` (while enabling checkpoints), an error is thrown even if a lower-priority source had a valid `org`. - Employed mocking and careful management of environment variables within tests to ensure isolation and accuracy. These enhanced tests provide greater confidence that the `CheckpointManager` correctly validates the organization ID regardless of how it's configured, preventing issues like #664 under a wider range of conditions.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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 #664
The CheckpointManager in packages/gensx-core/src/checkpoint.ts could allow trace submissions with an 'undefined' string or an empty string as the organization ID if checkpoints were enabled and the org ID was not properly set or was explicitly set to "undefined".
This change enhances the validation in the CheckpointManager constructor. It now explicitly checks if
this.orgis an empty string or the literal string "undefined" when checkpoints are enabled (i.e., an API key is present). If either condition is met, an error is thrown, preventing the client from attempting to send traces with an invalid org ID.Added new test cases to packages/gensx-core/tests/checkpoint.test.tsx to cover scenarios where: