feat: warn when NominalClient is created without a workspace RID

[seanmreidy]

Summary

• Adds a UserWarning in NominalClient.from_token when no workspace_rid is provided
• Warning message: "NominalClient will soon require a workspace RID. Any client which doesn't
  have a workspace RID specified will fail."

Background

get_default_workspace returns a workspace if the user belongs to exactly one workspace, or if
the org has a default workspace configured. This is the first step toward making
workspace_rid a required parameter.

Test plan

• Warning emitted when calling NominalClient.from_token(token, url) without workspace_rid
• No warning when workspace_rid is provided
• NominalClient.create(url, token) also triggers the warning (routes through from_token)
• NominalClient.from_profile triggers the warning when the profile has no workspace_rid

[github-actions]

Missing Test Coverage: This warning functionality needs comprehensive tests to ensure it works correctly. Consider adding tests using pytest.warns() to verify:

1. Warning is emitted when workspace_rid=None
2. No warning when workspace_rid is provided
3. Warning message and category are correct
4. Stacklevel correctly points to user code

The warning should also be tested for the create() and from_profile() code paths mentioned in the PR description.

[seanmreidy]

Seems a bit excessive for just a warning message

[github-actions]

Review Summary

This PR successfully adds a deprecation warning when NominalClient is created without a workspace_rid. The implementation is clean and follows Python warning best practices.

✅ What's Working Well

• Correct Warning Implementation: Uses UserWarning with clear, informative message
• Proper Code Placement: Warning is emitted before main logic, ensuring it always triggers
• Good Message Content: Explains both what will change and the consequences
• Follows Patterns: Consistent with existing codebase patterns and imports

❗ Critical Issues to Address

1. Missing Test Coverage

The PR lacks tests for the warning behavior. This is essential for a user-facing change that generates warnings. Consider adding tests to verify:

• Warning is emitted when workspace_rid=None
• No warning when workspace_rid is provided
• Warning uses correct message and category
• All code paths work: from_token(), create(), from_profile()

2. Stacklevel Verification Needed

The stacklevel=2 setting should be tested across all call paths:

• Direct from_token() calls → should point to user code
• create() → from_token() → should point to user's create() call
• from_profile() → from_token() → should point to user's from_profile() call

📝 Recommendations

1. Add comprehensive test coverage using pytest.warns() context manager
2. Test stacklevel behavior manually or with debug logging
3. Consider edge cases like empty string vs None values
4. Update any relevant documentation if this deprecation timeline should be documented

🎯 Test Plan Verification

The PR mentions several test scenarios that should be implemented as actual tests:

• Warning emitted when calling NominalClient.from_token(token, url) without workspace_rid ✅ (implemented)
• No warning when workspace_rid is provided ⚠️ (needs test)
• NominalClient.create(url, token) also triggers the warning ⚠️ (needs test)
• NominalClient.from_profile triggers warning when profile has no workspace_rid ⚠️ (needs test)

The core implementation is solid, but test coverage would significantly strengthen this change and ensure it works as expected for all users.

[devin-ai-integration]

Devin Review found 1 potential issue.

View 1 additional finding in Devin Review.

[cursor]

Cursor Bugbot has reviewed your changes using default mode and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 631494b. Configure here.}

[seanmreidy]

@claude

[github-actions]

Claude finished @seanmreidy's task —— View job

---

Analysis Complete - Providing Comprehensive Review

I've analyzed the PR and the feedback from multiple reviewers. Here's my comprehensive assessment of the current implementation and the issues that need to be addressed:

🚨 Critical Issues Identified

1. Incorrect stacklevel for Indirect Calls

The current implementation uses stacklevel=2 in from_token(), but this is incorrect when called via create() or from_profile(). The warning will point to internal library code instead of the user's actual call site.

• Direct call: NominalClient.from_token() → stacklevel=2 ✅ correct
• Indirect call: NominalClient.create() → from_token() → stacklevel=2 ❌ points to line 239 in create()
• Indirect call: NominalClient.from_profile() → from_token() → stacklevel=2 ❌ points to line 153 in from_profile()

2. Spurious Warning from as_user() Function

The experimental as_user() function (nominal/experimental/impersonation/__init__.py:13-21) calls from_token() without any warning suppression. Users who create a client without workspace_rid and then call as_user() will get a second warning they cannot act upon.

3. Missing Comprehensive Test Coverage

The PR lacks proper test coverage for the warning functionality. The existing tests were modified to work around the warnings rather than properly testing them.

📋 Detailed Analysis

Current Implementation Flow:

# All paths lead to from_token() which emits the warning
NominalClient.from_token()     # Direct - stacklevel=2 works
NominalClient.create()         # → calls from_token() - stacklevel=2 incorrect  
NominalClient.from_profile()   # → calls from_token() - stacklevel=2 incorrect
as_user()                      # → calls from_token() - causes spurious warning

Test Issues Found:

• test_workspace_resolution.py:48,58 - Tests expect the warning but don't verify message/category
• test_clientsbunch.py:225 - Changed from None to fake RID to avoid warning instead of fixing root cause

🔧 Recommended Solutions

Option 1: Move Warning to Each Entry Point (Recommended)

# Remove warning from from_token(), add to each public method:

@classmethod  
def from_token(cls, ...):
    if workspace_rid is None:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)
    # ... rest of method

@classmethod
def create(cls, ...):  
    if workspace_rid is None:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)
    return cls.from_token(...)  # Warning already emitted

@classmethod  
def from_profile(cls, ...):
    if prof.workspace_rid is None:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)  
    return cls.from_token(..., _workspace_warning_emitted=True)

Option 2: Dynamic stacklevel (Alternative)

Add private parameter to from_token() and adjust stacklevel dynamically.

🧪 Required Test Coverage

The implementation needs tests for:

• Warning emitted when workspace_rid=None in from_token()
• Warning emitted when workspace_rid=None in create()
• Warning emitted when profile has no workspace_rid in from_profile()
• No warning when workspace_rid is provided
• Warning message and category are correct
• as_user() doesn't cause spurious warnings
• Stacklevel correctly points to user code for all paths

🎯 Action Items

1. Fix stacklevel issue - Implement Option 1 above
2. Fix as_user() spurious warning - Add warning suppression parameter
3. Add comprehensive test suite - Cover all scenarios with pytest.warns()
4. Revert test workarounds - Fix underlying issues instead of avoiding them

The core concept is solid, but the implementation needs these fixes to work correctly for all user scenarios.

---

[seanmreidy]

@claude

[github-actions]

Claude finished @seanmreidy's task —— View job

---

Comprehensive PR Review: Workspace RID Warning Implementation

After reviewing the current implementation and extensive feedback from multiple reviewers, I've identified several critical issues that need to be addressed before this PR can be merged safely.

🚨 Critical Issues Identified

1. Incorrect stacklevel for Indirect Calls

Lines: nominal/core/client.py:194

The current implementation uses stacklevel=2 universally, but this is incorrect for indirect calls:

• Direct call: NominalClient.from_token() → stacklevel=2 ✅ Correct (points to user code)
• Indirect via create(): User calls create() → create() calls from_token() → stacklevel=2 ❌ Wrong (points to line 240 in create() method, not user code)
• Indirect via from_profile(): User calls from_profile() → from_profile() calls from_token() → stacklevel=2 ❌ Wrong (points to line 153 in from_profile() method, not user code)

Impact: Users see warnings pointing to internal library code instead of their actual call sites, making the warning confusing and unhelpful.

2. Spurious Warning from as_user() Function

Lines: nominal/experimental/impersonation/__init__.py:13-21

The as_user() function calls from_token() with _workspace_warning_emitted=True (line 21), which correctly suppresses the warning. However, the reviewers identified this was a fix for a spurious warning issue where users would get a second warning they couldn't act upon.

Current Status: ✅ Fixed - The implementation correctly passes _workspace_warning_emitted=True in as_user().

3. Missing Comprehensive Test Coverage

Lines: tests/e2e/test_workspace_resolution.py:48,58

The tests expect warnings but don't properly verify:

• Warning message content
• Warning category (UserWarning)
• That warnings are NOT emitted when workspace_rid is provided
• Coverage for all entry points (create(), from_profile(), as_user())

Current tests only verify that warnings are emitted, not their correctness or completeness.

4. Test Workarounds Instead of Proper Testing

Lines: tests/core/test_clientsbunch.py:225

The test was changed from workspace_rid=None to workspace_rid="ri.workspace.main.workspace.test" to avoid the warning rather than properly testing the warning behavior.

🔧 Recommended Solutions

Option 1: Move Warning to Each Entry Point (Recommended)

Remove the warning from from_token() and add it to each public method with correct stacklevel=2:

@classmethod  
def from_token(cls, ..., _workspace_warning_emitted: bool = False):
    # Remove warning logic from here
    if workspace_rid is None and not _workspace_warning_emitted:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)
    # ... rest of method

@classmethod
def create(cls, ...):  
    if workspace_rid is None:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)
    return cls.from_token(..., _workspace_warning_emitted=True)

@classmethod  
def from_profile(cls, ...):
    if prof.workspace_rid is None:
        warnings.warn("NominalClient will soon require...", UserWarning, stacklevel=2)  
    return cls.from_token(..., _workspace_warning_emitted=True)

Option 2: Dynamic stacklevel

Add a private parameter to adjust stacklevel dynamically, though this is more complex.

🧪 Required Test Coverage

Add comprehensive tests for:

• Warning emitted when workspace_rid=None in from_token()
• Warning emitted when workspace_rid=None in create()
• Warning emitted when profile has no workspace_rid in from_profile()
• No warning when workspace_rid is provided for all methods
• Warning message and category are correct
• as_user() doesn't cause spurious warnings (already working)
• Stacklevel correctly points to user code for all paths

🎯 Implementation Quality

Positive aspects:

• ✅ Uses UserWarning (correct category)
• ✅ Clear, informative warning message
• ✅ Proper conditional logic
• ✅ as_user() correctly suppresses warning

Issues to address:

• ❌ Stacklevel inconsistency across call paths
• ❌ Insufficient test coverage
• ❌ Test workarounds instead of proper verification

📝 Verdict

The core concept and implementation approach are solid, but the stacklevel issue is a critical user experience problem that makes warnings confusing and unhelpful. The missing test coverage also means we can't verify the fix works correctly across all scenarios.

Recommendation: Implement Option 1 (move warnings to entry points) and add comprehensive test coverage before merging.

---