fix: guard desktop_state null crash in switch_app/resize_app; fix click default#282
fix: guard desktop_state null crash in switch_app/resize_app; fix click default#282FelixIsaac wants to merge 1 commit into
Conversation
…ault - switch_app and resize_app both access self.desktop_state without checking if it's been populated. desktop_state is None at init and only set after get_state() is called. Calling App-Tool(switch) or App-Tool(resize) before State-Tool crashes with AttributeError, hanging the stdio transport and timing out all subsequent tool calls. - click() default clicks=2 (double-click) disagreed with the Click-Tool definition in main.py which exposes default=1. Align to single-click. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review by Qodo
Context used✅ Compliance rules (platform):
15 rules 1. resize_app missing docstring
|
PR Summary by QodoGuard uninitialized desktop_state in switch/resize; align click default to single WalkthroughsDescription• Prevent App-Tool switch/resize from crashing when desktop_state is uninitialized. • Lazily populate desktop_state via get_state() before accessing active_app/apps. • Align Desktop.click() default clicks with Click-Tool default (single click). Diagramgraph TD
A["App-Tool"] --> B["Desktop.app()"] --> C["switch_app()/resize_app()"] --> D["get_state() if desktop_state None"] --> E["desktop_state"]
F["Click-Tool"] --> G["Desktop.click()"] --> H["pyautogui.click()"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Centralize guard via ensure_state() helper
2. Initialize desktop_state eagerly at startup
Recommendation: The PR’s lazy guard is the right minimal-risk fix for a crash that breaks the stdio transport. If additional methods start depending on desktop_state, consider introducing a small ensure_state() helper (or moving the guard into Desktop.app() for switch/resize paths) to keep the invariant in one place. File ChangesBug fix (1)
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan |
| return bounding_rectangle.xcenter(),bounding_rectangle.ycenter() | ||
|
|
||
| def click(self,loc:tuple[int,int],button:str='left',clicks:int=2): | ||
| def click(self,loc:tuple[int,int],button:str='left',clicks:int=1): |
There was a problem hiding this comment.
1. click() uses 'left' 📘 Rule violation ✧ Quality
The updated click() signature sets the default button value using single quotes ('left'). This
violates the requirement that all string literals use double quotes, which helps keep string style
consistent across the codebase.
Agent Prompt
## Issue description
A modified line introduces/retains a single-quoted string literal (`'left'`) in `click()`'s default argument, but compliance requires double quotes for all string literals.
## Issue Context
This is in a function signature line changed in this PR, so it must comply with the string-literal quoting rule.
## Fix Focus Areas
- src/desktop/service.py[257-257]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return bounding_rectangle.xcenter(),bounding_rectangle.ycenter() | ||
|
|
||
| def click(self,loc:tuple[int,int],button:str='left',clicks:int=2): | ||
| def click(self,loc:tuple[int,int],button:str='left',clicks:int=1): |
There was a problem hiding this comment.
2. click() missing return type 📘 Rule violation ✧ Quality
The modified click() method lacks an explicit return type annotation. This violates the requirement that all function signatures include return type hints, reducing static checkability.
Agent Prompt
## Issue description
The `click()` method signature is missing an explicit return type annotation.
## Issue Context
Per compliance, all function/method signatures must include parameter and return type hints; `click()` appears to return nothing and should be annotated as `-> None`.
## Fix Focus Areas
- src/desktop/service.py[257-259]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The Click-Tool definition exposed clicks=1 but Desktop.click() defaulted to clicks=2, causing unexpected double-clicks when callers relied on the default. Aligns the service layer to match the tool's documented behavior. Resolves the click-default mismatch from PR #282; null-guard fixes for resize_app/switch_app were already covered by the prior refactor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit c20bbfb |
| if self.desktop_state is None: | ||
| self.get_state() |
There was a problem hiding this comment.
1. resize_app missing docstring 📘 Rule violation ✧ Quality
resize_app and switch_app are public methods whose first statement is now the added `if self.desktop_state is None:` guard, so neither begins with a Google-style docstring as required. This violates the documentation requirement for public functions, reducing API clarity, maintainability, and readability for tool/service consumers.
Agent Prompt
## Issue description
`resize_app` and `switch_app` are public methods but do not have a Google-style docstring as the first statement in the function body because a `desktop_state` guard was added above where a docstring should be.
## Issue Context
This PR introduced an `if self.desktop_state is None:` guard at the top of both `resize_app` and `switch_app`, making the first statement a non-docstring line and leaving no Google-style docstring preceding it, which violates PR Compliance ID 222802.
## Fix Focus Areas
- src/desktop/service.py[141-146]
- src/desktop/service.py[212-217]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if self.desktop_state is None: | ||
| self.get_state() | ||
| active_app=self.desktop_state.active_app |
There was a problem hiding this comment.
2. Get_state remove may crash 🐞 Bug ☼ Reliability
resize_app()/switch_app() now call get_state() when desktop_state is None, but get_state() builds apps from one get_apps() call and active_app from get_active_app() which calls get_apps() again; if those snapshots differ, apps.remove(active_app) can raise ValueError and crash the call. Because App is a dataclass, equality includes fields like depth and size, making this mismatch plausible even when the window handle is the same.
Agent Prompt
## Issue description
`resize_app()` and `switch_app()` now call `self.get_state()` when `self.desktop_state` is `None`. However, `get_state()` can still raise `ValueError` at `apps.remove(active_app)` because `apps` and `active_app` are computed from two separate `get_apps()` snapshots, and `App` is a dataclass so equality compares all fields (including `depth`/`size`). This can still crash the tool call even after the new null guard.
## Issue Context
- `get_state()` calls `get_apps()` and then `get_active_app()`, and `get_active_app()` calls `get_apps()` again.
- The object returned by the second `get_apps()` call may not compare equal to the object in the first list (e.g., `depth` changes if enumeration order changes), so `apps.remove(active_app)` can fail.
## Fix Focus Areas
- src/desktop/service.py[52-60]
- src/desktop/service.py[79-88]
## Suggested fix
- Avoid calling `get_apps()` twice inside `get_state()`.
- Option A (preferred): compute the foreground window handle once, then pick `active_app` from the already-fetched `apps` list by matching `handle`, and filter `apps` by `handle` rather than `remove()` by full-object equality.
- Option B: change `get_active_app()` to accept a precomputed `apps` list (single snapshot) and return the matching object from that list.
- (Optional hardening) If `get_state()` fails, catch exceptions in `resize_app()`/`switch_app()` and return an error tuple instead of raising.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Thank you! All three fixes have been applied to main — the null guards for |
Summary
switch_appandresize_appboth accessself.desktop_statewithout checking if it has been populated.desktop_stateis initialised toNoneinDesktop.__init__and only set afterget_state()is called. CallingApp-Toolwithmode=switchormode=resizebeforeState-ToolraisesAttributeError: 'NoneType' object has no attribute 'active_app', which hangs thestdioMCP transport and causes all subsequent tool calls to time out indefinitely.Desktop.click()inservice.pydefaulted toclicks=2(double-click) while theClick-Tooldefinition inmain.pyexposesclicks=1. Aligned both to single-click.Changes
resize_app: callself.get_state()ifself.desktop_state is Nonebefore accessing itswitch_app: same guardDesktop.click(): change defaultclicks=2→clicks=1Test plan
App-Tool(mode=switch, name=...)without a priorState-Toolcall — should switch window cleanly instead of hangingApp-Tool(mode=resize)without a priorState-Toolcall — should return "No active app found" or resize cleanlyClick-Toolwith defaultclicksparam produces a single click, not a double-click🤖 Generated with Claude Code