Conversation
A shell command named status would be shadowed by a parent class like AsyncDriverClient with the method status. Add __getattribute__ to ShellClient to intercept configured shell method names before descriptor resolution. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughRefactored dynamic method invocation in ShellClient: changed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py`:
- Around line 43-63: The __getattribute__ override currently returns a lambda
for any name in _methods which can shadow critical client methods; add a
reserved-names safeguard: define a set of reserved names (including core methods
call, streamingcall, stream, log_stream, open_stream, close; client methods cli;
attributes portal, stack, children, description, methods_description; metadata
uuid, labels, name; AsyncClient/stub attrs stub, log_level, logger, status) and,
inside __getattribute__, after loading _methods and before returning the lambda
from _call_method, check if name is in that reserved set and if so do not treat
it as a dynamic shell method (fall back to object.__getattribute__); ensure this
uses the existing symbols _methods, _call_method and __getattribute__ so
existing logic remains but avoids shadowing reserved names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f83a8c2a-7f8f-4329-8f0e-3e03256a6ada
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.pypython/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py
| def __getattribute__(self, name): | ||
| if not name.startswith("_"): | ||
| d = object.__getattribute__(self, "__dict__") | ||
| methods = d.get("_methods") | ||
|
|
||
| # Lazy-load on first access; guard prevents recursion | ||
| # since self.call() re-enters __getattribute__ | ||
| if methods is None and not d.get("_loading_methods"): | ||
| d["_loading_methods"] = True | ||
| try: | ||
| methods = set(object.__getattribute__(self, "call")("get_methods")) | ||
| d["_methods"] = methods | ||
| except Exception: | ||
| logger.debug("Failed to lazy-load shell methods", exc_info=True) | ||
| finally: | ||
| d.pop("_loading_methods", None) | ||
|
|
||
| if methods and name in methods: | ||
| return lambda *args, **kwargs: object.__getattribute__(self, "_call_method")(name, *args, **kwargs) | ||
|
|
||
| return object.__getattribute__(self, name) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find public methods/attributes in DriverClient that could be shadowed
ast-grep --pattern $'class DriverClient($_):
$$$
def $_($$$):
$$$
$$$
'
# Also search for property definitions
rg -n '@property' python/packages/jumpstarter/jumpstarter/client/Repository: jumpstarter-dev/jumpstarter
Length of output: 8422
🏁 Script executed:
# Read the ShellClient implementation around lines 43-63 with full context
cat -n python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py | head -80Repository: jumpstarter-dev/jumpstarter
Length of output: 3452
🏁 Script executed:
# Find and read the get_methods() implementation
rg -A 10 'def get_methods' python/packages/jumpstarter-driver-shell/Repository: jumpstarter-dev/jumpstarter
Length of output: 1440
🏁 Script executed:
# Check if ShellClient has any existing safeguards or method filtering
rg -B 5 -A 15 'class ShellClient' python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 711
🏁 Script executed:
# Find AsyncDriverClient to see if there are additional public methods/properties
fd -t f 'async' python/packages/jumpstarter/jumpstarter/client/ | head -5Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Check the full parent class hierarchy of DriverClient
rg -B 5 'class AsyncDriverClient' python/packages/jumpstarter/jumpstarter/client/Repository: jumpstarter-dev/jumpstarter
Length of output: 525
🏁 Script executed:
# Look for all public attributes and methods in DriverClient and related classes
rg -n '^\s+(def |[a-z_]+:)' python/packages/jumpstarter/jumpstarter/client/base.py | head -40Repository: jumpstarter-dev/jumpstarter
Length of output: 870
🏁 Script executed:
# Check how shell methods are configured - look for self.methods in driver
rg -B 5 -A 5 'self.methods' python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py | head -40Repository: jumpstarter-dev/jumpstarter
Length of output: 1767
🏁 Script executed:
# Look for how ShellDriver is instantiated to understand if method names are user-configurable
rg -n 'class ShellDriver' python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Check the complete ShellDriver init to understand method configuration
cat -n python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py | head -50Repository: jumpstarter-dev/jumpstarter
Length of output: 2274
🏁 Script executed:
# Verify what "report", "uuid", "labels" might be - check for mixins or other parent classes
rg -n 'def report|def uuid|def labels|resources' python/packages/jumpstarter/jumpstarter/client/ | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 306
🏁 Script executed:
# Verify the full method resolution order for ShellClient
python3 << 'EOF'
import sys
sys.path.insert(0, 'python/packages/jumpstarter')
sys.path.insert(0, 'python/packages/jumpstarter-driver-shell')
from jumpstarter_driver_shell.client import ShellClient
from jumpstarter.client import DriverClient
# Get all public attributes and methods from parent classes
public_members = set()
for cls in ShellClient.__mro__:
if cls is object:
continue
for name in dir(cls):
if not name.startswith('_'):
public_members.add(name)
print("Public members in ShellClient hierarchy:")
for name in sorted(public_members):
print(f" {name}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 785
🏁 Script executed:
# Check if there's any existing validation or safeguard in get_methods or driver configuration
rg -B 10 -A 10 'def get_methods' python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 902
🏁 Script executed:
# Check AsyncDriverClient for additional methods/properties
rg -A 50 'class AsyncDriverClient' python/packages/jumpstarter/jumpstarter/client/core.py | head -80Repository: jumpstarter-dev/jumpstarter
Length of output: 1780
🏁 Script executed:
# Search for "report", "uuid", "labels" definitions in client hierarchy
rg -n 'def report|uuid|labels|resources' python/packages/jumpstarter/jumpstarter/client/core.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 435
🏁 Script executed:
# Look for mixins or parent classes that might add additional methods
rg -n 'class.*Mixin|class Driver' python/packages/jumpstarter/jumpstarter/driver.py | head -10Repository: jumpstarter-dev/jumpstarter
Length of output: 160
🏁 Script executed:
# Find Metadata class definition to understand what properties it adds
rg -B 5 -A 20 'class Metadata' python/packages/jumpstarter/jumpstarter/client/core.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# List all public methods in DriverClient that would be shadowed
cat -n python/packages/jumpstarter/jumpstarter/client/base.py | sed -n '20,150p'Repository: jumpstarter-dev/jumpstarter
Length of output: 5306
🏁 Script executed:
# Test the actual shadowing scenario - create a test case
cat > /tmp/test_shadowing.py << 'EOF'
# Simulate what happens when ShellClient.__getattribute__ intercepts "call"
class Parent:
def call(self, *args):
return "parent_call_result"
def close(self):
return "parent_close_result"
class Child(Parent):
def __init__(self):
self._methods = {"call", "status"} # User configured "call" as a shell method
def __getattribute__(self, name):
if not name.startswith("_"):
methods = object.__getattribute__(self, "_methods")
if methods and name in methods:
return lambda *args, **kwargs: f"shell_{name}_called"
return object.__getattribute__(self, name)
# Test
c = Child()
print(f"c.call() = {c.call()}") # Should call shell_call, not Parent.call
print(f"c.close() = {c.close()}") # Should call Parent.close
EOF
python3 /tmp/test_shadowing.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 131
🏁 Script executed:
# Find Metadata class to see what properties it adds
rg -B 2 -A 30 'class Metadata' python/packages/jumpstarter/jumpstarter/client/core.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Get a comprehensive list of all public methods and attributes in the client hierarchy
cat > /tmp/find_public.py << 'EOF'
import ast
import sys
# Read DriverClient definition
with open('python/packages/jumpstarter/jumpstarter/client/base.py', 'r') as f:
tree = ast.parse(f.read())
public_members = set()
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef) and node.name in ['DriverClient', 'AsyncDriverClient']:
for item in node.body:
if isinstance(item, ast.FunctionDef) and not item.name.startswith('_'):
public_members.add(item.name)
# Read core.py for AsyncDriverClient/Metadata
with open('python/packages/jumpstarter/jumpstarter/client/core.py', 'r') as f:
content = f.read()
tree = ast.parse(content)
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef) and node.name in ['Metadata', 'AsyncDriverClient']:
for item in node.body:
if isinstance(item, ast.FunctionDef) and not item.name.startswith('_'):
public_members.add(item.name)
elif isinstance(item, (ast.Assign, ast.AnnAssign)):
if isinstance(item, ast.Assign):
for target in item.targets:
if isinstance(target, ast.Name) and not target.id.startswith('_'):
public_members.add(target.id)
else:
if isinstance(item.target, ast.Name) and not item.target.id.startswith('_'):
public_members.add(item.target.id)
print("All public members that could be shadowed:")
for name in sorted(public_members):
print(f" {name}")
EOF
python3 /tmp/find_public.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 217
🏁 Script executed:
# Check Metadata class specifically
rg -B 3 -A 40 'class Metadata' python/packages/jumpstarter/jumpstarter/common/metadata.py 2>/dev/null || rg -r 'class Metadata' python/packages/jumpstarter/Repository: jumpstarter-dev/jumpstarter
Length of output: 329
Missing safeguard for reserved method names could break client functionality.
The __getattribute__ override intercepts all non-underscore attributes and returns a shell method callable if the name is in _methods. However, if a user configures a shell method with the same name as a critical parent class method (e.g., "call", "streamingcall", "close", "stream"), the lambda will shadow that method and break the client. This is a real shadowing risk that has been verified.
Per the driver code, get_methods() returns all configured method names without filtering. The proposed approach of adding a reserved names check is sound, but the suggested list needs to be more complete to cover all public methods and properties:
- Core methods:
call,streamingcall,stream,log_stream,open_stream,close - Client methods:
cli - Attributes/Fields:
portal,stack,children,description,methods_description - Metadata properties:
uuid,labels,name - AsyncClient attributes:
stub,log_level,logger,status
🛡️ Suggested reserved names filter
`@dataclass`(kw_only=True)
class ShellClient(DriverClient):
_methods: set[str] | None = None
+ _RESERVED_NAMES: frozenset = frozenset({
+ "call", "streamingcall", "cli", "close", "stream",
+ "log_stream", "open_stream", "portal", "stack", "children",
+ "description", "methods_description", "uuid", "labels", "name",
+ "stub", "log_level", "logger", "status",
+ })
...
def __getattribute__(self, name):
if not name.startswith("_"):
d = object.__getattribute__(self, "__dict__")
methods = d.get("_methods")
# Lazy-load on first access; guard prevents recursion
# since self.call() re-enters __getattribute__
if methods is None and not d.get("_loading_methods"):
d["_loading_methods"] = True
try:
methods = set(object.__getattribute__(self, "call")("get_methods"))
+ reserved = object.__getattribute__(self, "_RESERVED_NAMES")
+ if methods & reserved:
+ logger.warning(
+ f"Shell methods {methods & reserved} shadow reserved names and will be ignored"
+ )
+ methods -= reserved
d["_methods"] = methods
except Exception:
logger.debug("Failed to lazy-load shell methods", exc_info=True)
finally:
d.pop("_loading_methods", None)
- if methods and name in methods:
+ if methods and name in methods and name not in object.__getattribute__(self, "_RESERVED_NAMES"):
return lambda *args, **kwargs: object.__getattribute__(self, "_call_method")(name, *args, **kwargs)
return object.__getattribute__(self, name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py`
around lines 43 - 63, The __getattribute__ override currently returns a lambda
for any name in _methods which can shadow critical client methods; add a
reserved-names safeguard: define a set of reserved names (including core methods
call, streamingcall, stream, log_stream, open_stream, close; client methods cli;
attributes portal, stack, children, description, methods_description; metadata
uuid, labels, name; AsyncClient/stub attrs stub, log_level, logger, status) and,
inside __getattribute__, after loading _methods and before returning the lambda
from _call_method, check if name is in that reserved set and if so do not treat
it as a dynamic shell method (fall back to object.__getattribute__); ensure this
uses the existing symbols _methods, _call_method and __getattribute__ so
existing logic remains but avoids shadowing reserved names.
There was a problem hiding this comment.
@mangelajo yeah, this is a difficult problem, I don't know how likely it is to use call in the exporter config, but close doesn't sound very unlikely
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
yeah, probably we should add a checker on the exporter config with a banned list of names that can't be used.
There was a problem hiding this comment.
@coderabbitai how can we effectively maintain such a lease without forgetting to add new methods?
There was a problem hiding this comment.
@bennyz Great question! The most reliable way to avoid maintaining a manual list is to derive the reserved names programmatically from the MRO at class-definition time. This means the set auto-updates whenever parent classes gain new public methods — no manual bookkeeping required.
Here's the approach:
`@dataclass`(kw_only=True)
class ShellClient(DriverClient):
_methods: set[str] | None = None
`@classmethod`
def _parent_public_attrs(cls) -> frozenset[str]:
"""Dynamically derive reserved names from the parent class hierarchy.
Skips ShellClient itself (mro[0]) so only parent names are reserved.
"""
return frozenset(
attr
for parent in cls.__mro__[1:] # skip ShellClient
for attr in vars(parent)
if not attr.startswith("_")
)
def __getattribute__(self, name):
if not name.startswith("_"):
d = object.__getattribute__(self, "__dict__")
methods = d.get("_methods")
if methods is None and not d.get("_loading_methods"):
d["_loading_methods"] = True
try:
methods = set(object.__getattribute__(self, "call")("get_methods"))
# Strip any names that would shadow parent class attrs
reserved = object.__getattribute__(self, "_parent_public_attrs")()
shadowed = methods & reserved
if shadowed:
logger.warning(
"Shell method(s) %s shadow reserved client names and will be ignored",
shadowed,
)
methods -= reserved
d["_methods"] = methods
except Exception:
logger.debug("Failed to lazy-load shell methods", exc_info=True)
finally:
d.pop("_loading_methods", None)
if methods and name in methods:
return lambda *args, **kwargs: object.__getattribute__(self, "_call_method")(name, *args, **kwargs)
return object.__getattribute__(self, name)Why this works without ongoing maintenance:
cls.__mro__[1:]skipsShellClientbut walks every ancestor (DriverClient,AsyncDriverClient,object, mixins, etc.).vars(parent)inspects each class's own__dict__, so only directly-defined names are included (no duplicates from inheritance, no surprises).- If a new public method is added to any ancestor, it's automatically in
reservedthe next time the code runs — nothing to update.
Optional: cache it to avoid recomputing on every __getattribute__ call:
# After the class body:
ShellClient._RESERVED_NAMES: frozenset[str] = ShellClient._parent_public_attrs()Then reference _RESERVED_NAMES directly instead of calling _parent_public_attrs() each time. The tradeoff is that the cache is frozen at import time, but since parent classes don't change at runtime, that's perfectly fine.
🧠 Learnings used
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Each driver package must include a `client.py` file in `<package>/jumpstarter_driver_<name>/client.py`
2 participants
A shell command named status would be shadowed by a parent class like AsyncDriverClient with the method status.
Add getattribute to ShellClient to intercept configured shell method names before descriptor resolution.
A configuration with:
would confuse
j vm statusand try to call AsyncDriverClient#status