Align MCP with merged OpenRCT2 #26675 plugin APIs - #2
Conversation
A bin slot is only full (visibly full / emptiable by a handyman) once its 2-bit edge slot reaches 0, not whenever the status byte is below 255. Update the _is_litter_bin_full fallback to do a per-slot check, prefer the native isAdditionFull getter when present, and sync the ride-builder API type stub.
Use ride reliability/occupancy/income, context.gameSpeed, and isAdditionFull; polyfill rect map/guest scans since those APIs did not land.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe update expands Ride Builder APIs and type definitions, enriches ride and time status data, adds spatial guest and map-element queries, changes refurbishment occupancy handling, improves litter-bin fullness detection, and wires these capabilities through MCP tools with focused tests. ChangesOpenRCT2 integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPTool
participant MapRegion
participant RideBuilderClient
participant RideBuilderPlugin
MCPTool->>MapRegion: request element type and rectangle
MapRegion->>RideBuilderClient: call getElementsInRect with bounded coordinates
RideBuilderClient->>RideBuilderPlugin: execute rectangle scan
RideBuilderPlugin-->>RideBuilderClient: return serialized elements
RideBuilderClient-->>MapRegion: return elements payload
MapRegion-->>MCPTool: return type, bounds, count, and elements
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp-server/openrct2_mcp/ride_ops.py (1)
283-308: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
_merge_maintenanceduplicates_merge_ride_builder_maintenancefrom bridge_fast.py.Both functions implement identical merge logic (same key pairs, same
is Noneguard, sameactiveBreakdown/stationQueueTimeshandling). Consider extracting a shared helper to avoid drift as new fields are added.Additionally,
_load_ride_builder_maintenance(line 370) now delegates to_load_ride_builder_maintenance_index— a private function imported across module boundaries (line 12). If this helper is shared, consider making it public (dropping the underscore) or moving it to a shared utility module.♻️ Proposed shared helper
# In bridge_fast.py — rename to public: -def _merge_ride_builder_maintenance( +def merge_ride_builder_maintenance( summary: dict[str, Any], ride_builder_row: dict[str, Any] | None, ) -> dict[str, Any]: ... -def _load_ride_builder_maintenance_index(ride_builder: RideBuilderClient) -> dict[int, dict[str, Any]]: +def load_ride_builder_maintenance_index(ride_builder: RideBuilderClient) -> dict[int, dict[str, Any]]: ...# In ride_ops.py — reuse instead of duplicating: -from openrct2_mcp.bridge_fast import ..., _load_ride_builder_maintenance_index +from openrct2_mcp.bridge_fast import ..., load_ride_builder_maintenance_index, merge_ride_builder_maintenance -def _merge_maintenance( - maintenance: dict[str, Any], - ride_builder_row: dict[str, Any] | None, -) -> dict[str, Any]: - """Prefer ride-builder maintenance stats when the bridge omits reliability.""" - merged = dict(maintenance) - if ride_builder_row is None: - return merged - for key, rb_key in ( - ... - ): - if merged.get(key) is None and ride_builder_row.get(rb_key) is not None: - merged[key] = ride_builder_row.get(rb_key) - if ride_builder_row.get("activeBreakdown"): - merged["active_breakdown"] = True - station_times = ride_builder_row.get("stationQueueTimes") - if station_times and merged.get("station_queue_times") is None: - merged["station_queue_times"] = station_times - return merged +# _merge_maintenance callers now use merge_ride_builder_maintenance directlyAlso applies to: 370-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/ride_ops.py` around lines 283 - 308, Extract the duplicated maintenance merge logic from _merge_maintenance and _merge_ride_builder_maintenance into one shared helper, preserving the existing field mappings, None checks, activeBreakdown handling, and stationQueueTimes behavior. Update both callers to use that helper, and avoid cross-module imports of private _load_ride_builder_maintenance_index by renaming it to a public symbol or relocating it to the shared utility module.
🧹 Nitpick comments (6)
mcp-server/tests/test_bridge_fast_api115.py (1)
1-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests are correct; consider adding a
Nonerow edge case.The two tests cover the main merge behaviors (fill gaps, preserve bridge values). A test for
ride_builder_row=Nonewould verify the early-return path and guard against future changes that break the None guard.✅ Suggested additional test
+ def test_returns_summary_unchanged_when_row_is_none(self): + summary = {"id": 5, "reliability": 80.0, "guest_count": 3} + merged = _merge_ride_builder_maintenance(summary, None) + self.assertEqual(merged, summary) + self.assertIs(merged, summary) # should return the same object🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/tests/test_bridge_fast_api115.py` around lines 1 - 31, Add a unittest for _merge_ride_builder_maintenance that passes a None ride-builder row, and assert it returns the original summary unchanged, covering the early-return guard without altering the existing merge behavior tests.mcp-server/tests/test_time_tools.py (1)
22-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a fallback-path test.
The happy-path test is solid, but there's no coverage for when
ride_builderisNoneor when the plugin call raises an exception — both should fall back toknown_speedwithgame_speed_source: "session". A regression in the fallback path could go undetected.✅ Suggested additional test
+ def test_falls_back_to_session_when_plugin_fails(self): + from openrct2_mcp.time_tools import game_time_status + from openrct2_mcp.connection import GameSpeed + + class _FailingRideBuilder: + def call(self, endpoint: str): + raise RuntimeError("plugin unavailable") + + status = game_time_status( + self._FakeGame(), + known_speed=GameSpeed.FAST, + ride_builder=_FailingRideBuilder(), + ) + self.assertEqual(status["game_speed"], int(GameSpeed.FAST)) + self.assertEqual(status["game_speed_source"], "session") + + def test_falls_back_when_no_ride_builder(self): + from openrct2_mcp.time_tools import game_time_status + from openrct2_mcp.connection import GameSpeed + + status = game_time_status( + self._FakeGame(), + known_speed=GameSpeed.NORMAL, + ride_builder=None, + ) + self.assertEqual(status["game_speed_source"], "session") + self.assertIsNone(status.get("game_speed_note"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/tests/test_time_tools.py` around lines 22 - 41, Add fallback-path coverage to GameTimeStatusTests for game_time_status when ride_builder is absent and when its plugin call raises an exception; assert both cases use known_speed with game_speed_source set to "session", while preserving the existing plugin-success test.mcp-server/openrct2_mcp/ride_ops.py (1)
373-448: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDouble-loading of maintenance index and ride data in
list_refurbish_candidates.
list_rides_fast(called at line 387) internally calls_load_ride_builder_maintenance_indexand fetches each ride's raw data viaget_ride_raw. Thenlist_refurbish_candidatesseparately calls_load_ride_builder_maintenance(line 383, which delegates to the same index loader) andget_ride_rawagain per ride (line 393). This doubles the network calls to the ride-builder plugin and bridge for every invocation.Consider reusing the summaries already returned by
list_rides_fast(which now contain merged maintenance fields) instead of re-fetching and re-merging.♻️ Proposed refactor: reuse merged summaries
def list_refurbish_candidates( game: RCT2, ride_builder: RideBuilderClient, *, limit: int = 20, reliability_threshold: float = DEFAULT_RELIABILITY_REFURBISH_THRESHOLD, downtime_threshold: float = DEFAULT_DOWNTIME_REFURBISH_THRESHOLD, rides_only: bool = True, ) -> dict[str, Any]: """List rides ranked for refurbish using downtime and reliability together.""" - rb_rows = _load_ride_builder_maintenance(ride_builder) - reliability_available = any(row.get("reliability") is not None for row in rb_rows.values()) + summaries = list_rides_fast(game, ride_builder) + reliability_available = any(s.get("reliability") is not None for s in summaries) candidates: list[dict[str, Any]] = [] - for summary in list_rides_fast(game, ride_builder): + for summary in summaries: classification = str(summary.get("classification") or "").lower() if rides_only and classification in ("stall", "facility"): continue ride_id = summary["id"] - raw = get_ride_raw(game, ride_id) or {} - maintenance = _merge_maintenance( - ride_maintenance_from_raw(raw), - rb_rows.get(ride_id), - ) + # summary already has merged maintenance fields from list_rides_fast + maintenance = { + "downtime": summary.get("downtime"), + "reliability": summary.get("reliability"), + "age_months": summary.get("age_months"), + "breakdown": summary.get("breakdown"), + "active_breakdown": summary.get("active_breakdown"), + } score, reasons = refurbish_need_score( maintenance, reliability_threshold=reliability_threshold, downtime_threshold=downtime_threshold, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/ride_ops.py` around lines 373 - 448, Update list_refurbish_candidates to reuse the maintenance fields already returned in each summary by list_rides_fast, removing the separate _load_ride_builder_maintenance call and per-ride get_ride_raw fetch. Derive reliability_available from those summaries and pass each summary’s merged maintenance data to refurbish_need_score and recommend_refurbish while preserving candidate ranking, thresholds, and output behavior.plugins/ride-builder/src/ride-builder.js (2)
199-211: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
readReliabilityPercent's early return skips the normalization applied to the fallback keys.The fallback keys (
reliabilityPercentage,reliability_percentage) are defensively clamped/converted (value <= 100 ? value : Math.round(value / 655.35)), but the newride.reliabilityfast-path trusts the.d.tscontract ("0–100") unconditionally. If a future/variant OpenRCT2 build ever exposesreliabilityon the same raw 0–65535 scale as the legacy fields, this path would silently return an out-of-range percentage instead of correcting it like the fallback does.♻️ Suggested fix: route through the same normalization
function readReliabilityPercent(ride) { - if (typeof ride.reliability === "number" && Number.isFinite(ride.reliability)) { - return ride.reliability; - } - const keys = ["reliabilityPercentage", "reliability_percentage"]; + const keys = ["reliability", "reliabilityPercentage", "reliability_percentage"]; for (let i = 0; i < keys.length; i++) { const value = ride[keys[i]]; if (typeof value === "number" && Number.isFinite(value)) { return value <= 100 ? value : Math.round(value / 655.35); } } return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ride-builder/src/ride-builder.js` around lines 199 - 211, Update readReliabilityPercent so ride.reliability uses the same normalization as reliabilityPercentage and reliability_percentage: return it unchanged when at most 100, otherwise convert it with the existing 655.35 scaling and rounding. Preserve the finite-number validation and fallback-key behavior.
365-395: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
handleGetElementsInRect's native branch bypasses the summarizers that the fallback and the guest-rect handler both use.
handleGetGuestsInRectmaps both the native (map.getGuestsInRect) and fallback (scanGuestsInRect) paths throughserializeGuestNearTile, so callers get a consistent shape either way.handleGetElementsInRect, however, returns rawmap.getElementsInRect(type, bounds)output when that native function exists, but routes the fallback throughsummarizeFootpathElement/summarizeTrackElement/summarizeEntranceElement(which addtileX/tileYand filter fields likeisAdditionFull). Today this is dead code since the native function was never merged per the PR description, but if it lands later with a different element shape, downstream consumers (map_region.py, the MCP tool) would silently receive a differently-shaped payload.♻️ Suggested fix: normalize the native path too (adjust field extraction once the native shape is known)
if (typeof map.getElementsInRect === "function") { - return map.getElementsInRect(type, bounds); + return map.getElementsInRect(type, bounds).map(({ tileX, tileY, element }) => { + if (type === "footpath") return summarizeFootpathElement(tileX, tileY, element); + if (type === "track") return summarizeTrackElement(tileX, tileY, element); + return summarizeEntranceElement(tileX, tileY, element); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ride-builder/src/ride-builder.js` around lines 365 - 395, Normalize the native result in handleGetElementsInRect before returning it, using the same type-specific summarizers as scanElementsInRect: summarizeFootpathElement, summarizeTrackElement, or summarizeEntranceElement. Ensure both native and fallback paths return the same summarized shape, including tile coordinates and filtered fields.mcp-server/openrct2_mcp/server.py (1)
203-235: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
getGameSpeedround trip.
openrct2_statuscallsride_builder.call("getGameSpeed")to extractapiVersion, then immediately callsgame_time_status(..., ride_builder=ride_builder), which (pertime_tools.py) calls the same"getGameSpeed"endpoint again just to readgameSpeedfrom the same payload shape. This is a redundant plugin round trip on every status check, and introduces a small window where the two calls could theoretically observe different speeds if the speed changes between them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/server.py` around lines 203 - 235, Update openrct2_status to make a single getGameSpeed call, reuse its payload for both plugin_api_version and game_time_status, and adjust the game_time_status interface or invocation as needed so it does not issue a second ride_builder request while preserving existing status values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Line 12: Eliminate the duplicated maintenance merge logic by renaming
bridge_fast’s _merge_ride_builder_maintenance to public
merge_ride_builder_maintenance and updating ride_ops’ _merge_maintenance callers
to use it. Rename _load_ride_builder_maintenance_index to public
load_ride_builder_maintenance_index and update the ride_ops import and
references accordingly, preserving existing merge behavior.
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 259-269: Update normalizeRectBounds to enforce a hard maximum
rectangle width and height before returning normalized bounds, rejecting
oversized requests with null. Ensure this validation applies to all callers,
including scanElementsInRect, scanGuestsInRect, and native rectangle scans, so
raw socket clients cannot trigger unbounded synchronous tile iteration.
---
Outside diff comments:
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Around line 283-308: Extract the duplicated maintenance merge logic from
_merge_maintenance and _merge_ride_builder_maintenance into one shared helper,
preserving the existing field mappings, None checks, activeBreakdown handling,
and stationQueueTimes behavior. Update both callers to use that helper, and
avoid cross-module imports of private _load_ride_builder_maintenance_index by
renaming it to a public symbol or relocating it to the shared utility module.
---
Nitpick comments:
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Around line 373-448: Update list_refurbish_candidates to reuse the maintenance
fields already returned in each summary by list_rides_fast, removing the
separate _load_ride_builder_maintenance call and per-ride get_ride_raw fetch.
Derive reliability_available from those summaries and pass each summary’s merged
maintenance data to refurbish_need_score and recommend_refurbish while
preserving candidate ranking, thresholds, and output behavior.
In `@mcp-server/openrct2_mcp/server.py`:
- Around line 203-235: Update openrct2_status to make a single getGameSpeed
call, reuse its payload for both plugin_api_version and game_time_status, and
adjust the game_time_status interface or invocation as needed so it does not
issue a second ride_builder request while preserving existing status values.
In `@mcp-server/tests/test_bridge_fast_api115.py`:
- Around line 1-31: Add a unittest for _merge_ride_builder_maintenance that
passes a None ride-builder row, and assert it returns the original summary
unchanged, covering the early-return guard without altering the existing merge
behavior tests.
In `@mcp-server/tests/test_time_tools.py`:
- Around line 22-41: Add fallback-path coverage to GameTimeStatusTests for
game_time_status when ride_builder is absent and when its plugin call raises an
exception; assert both cases use known_speed with game_speed_source set to
"session", while preserving the existing plugin-success test.
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 199-211: Update readReliabilityPercent so ride.reliability uses
the same normalization as reliabilityPercentage and reliability_percentage:
return it unchanged when at most 100, otherwise convert it with the existing
655.35 scaling and rounding. Preserve the finite-number validation and
fallback-key behavior.
- Around line 365-395: Normalize the native result in handleGetElementsInRect
before returning it, using the same type-specific summarizers as
scanElementsInRect: summarizeFootpathElement, summarizeTrackElement, or
summarizeEntranceElement. Ensure both native and fallback paths return the same
summarized shape, including tile coordinates and filtered fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5579337-28bf-45c4-a5cc-e979866ec75e
📒 Files selected for processing (13)
mcp-server/openrct2_mcp/bridge_fast.pymcp-server/openrct2_mcp/guest_intel.pymcp-server/openrct2_mcp/map_region.pymcp-server/openrct2_mcp/ride_ops.pymcp-server/openrct2_mcp/scenery_tools.pymcp-server/openrct2_mcp/server.pymcp-server/openrct2_mcp/time_tools.pymcp-server/tests/test_bridge_fast_api115.pymcp-server/tests/test_map_region_api115.pymcp-server/tests/test_scenery_tools.pymcp-server/tests/test_time_tools.pyplugins/ride-builder/lib/openrct2.d.tsplugins/ride-builder/src/ride-builder.js
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp-server/openrct2_mcp/ride_ops.py (1)
283-309: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate merge logic across
ride_ops.pyandbridge_fast.py.
_merge_maintenancehere is functionally identical tobridge_fast._merge_ride_builder_maintenance(same field list, same "fill only when missing" semantics). This PR extended both copies in parallel with the same 6 new keys — exactly the drift risk duplication creates._load_ride_builder_maintenanceright below already shows the intended pattern (delegate to thebridge_fasthelper);_merge_maintenanceshould do the same.♻️ Proposed fix
-def _merge_maintenance( - maintenance: dict[str, Any], - ride_builder_row: dict[str, Any] | None, -) -> dict[str, Any]: - """Prefer ride-builder maintenance stats when the bridge omits reliability.""" - merged = dict(maintenance) - if ride_builder_row is None: - return merged - for key, rb_key in ( - ("downtime", "downtime"), - ("reliability", "reliability"), - ("age_months", "age"), - ("breakdown", "breakdown"), - ("guest_count", "guestCount"), - ("is_empty", "isEmpty"), - ("income_per_hour", "incomePerHour"), - ("profit", "profit"), - ("queue_time", "queueTime"), - ): - if merged.get(key) is None and ride_builder_row.get(rb_key) is not None: - merged[key] = ride_builder_row.get(rb_key) - if ride_builder_row.get("activeBreakdown"): - merged["active_breakdown"] = True - station_times = ride_builder_row.get("stationQueueTimes") - if station_times and merged.get("station_queue_times") is None: - merged["station_queue_times"] = station_times - return merged +def _merge_maintenance( + maintenance: dict[str, Any], + ride_builder_row: dict[str, Any] | None, +) -> dict[str, Any]: + """Prefer ride-builder maintenance stats when the bridge omits reliability.""" + return _merge_ride_builder_maintenance(maintenance, ride_builder_row)(requires importing
_merge_ride_builder_maintenancefrombridge_fast, alongside_load_ride_builder_maintenance_index)Also applies to: 369-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/ride_ops.py` around lines 283 - 309, Replace the duplicated field-by-field logic in _merge_maintenance with a delegation to bridge_fast._merge_ride_builder_maintenance, importing that helper alongside _load_ride_builder_maintenance_index. Preserve the existing None handling and return behavior, including the unchanged result when ride_builder_row is absent.
🧹 Nitpick comments (4)
mcp-server/tests/test_map_region_api115.py (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for width/height clamping behavior.
The clamping logic (
max(1, min(width, MAX_REGION_SIDE))) inget_elements_in_rectis untested. Edge cases likewidth=0,width=-5, andwidth=100should be covered to verify the bounds are correctly clamped to 1 and 40 respectively.♻️ Suggested additional tests
def test_clamps_oversized_width_and_height(self): rb = MagicMock() rb.call.return_value = [] result = get_elements_in_rect(rb, "footpath", 0, 0, 100, 100) rb.call.assert_called_once_with( "getElementsInRect", { "type": "footpath", "bounds": {"minX": 0, "minY": 0, "maxX": 39, "maxY": 39}, }, ) def test_clamps_zero_width_and_height(self): rb = MagicMock() rb.call.return_value = [] result = get_elements_in_rect(rb, "track", 5, 5, 0, 0) rb.call.assert_called_once_with( "getElementsInRect", { "type": "track", "bounds": {"minX": 5, "minY": 5, "maxX": 5, "maxY": 5}, }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/tests/test_map_region_api115.py` around lines 24 - 26, Add tests alongside test_rejects_unknown_element_type for get_elements_in_rect covering oversized dimensions and zero or negative dimensions, asserting the backend call clamps each side to MAX_REGION_SIDE (40) and the minimum side length of 1, including the expected bounds.mcp-server/openrct2_mcp/guest_intel.py (1)
97-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the exception instead of silently swallowing it.
The broad
except Exception: passmakes it impossible to distinguish between "plugin path legitimately unavailable" and "plugin path has a bug that always fails." If the plugin scan consistently errors, users will silently get the slower bridge-scan heuristic with no indication. At minimum, log the exception at debug or warning level before falling back.♻️ Suggested refactor
except Exception as exc: - pass + logger.debug("ride-builder getGuestsInRect failed, falling back to bridge scan: %s", exc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/guest_intel.py` around lines 97 - 98, Update the broad exception handler in the plugin scan flow to log the caught exception at debug or warning level before falling back, while preserving the existing fallback behavior. Use the surrounding plugin path detection function or scan symbol to locate this handler, and include enough context to distinguish a missing plugin path from a scan failure.Source: Linters/SAST tools
plugins/ride-builder/src/ride-builder.js (1)
365-381: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNative-API fallback for elements skips the summarization guests get.
handleGetGuestsInRect's native path maps results throughserializeGuestNearTileto keep a consistent shape.handleGetElementsInRect's native path (map.getElementsInRect) returns the raw result unmodified — inconsistent with the tile-scan fallback's summarized shape (tileX,tileY,additionStatus,isAdditionFull, etc.). Currently unreachable (nativemap.getElementsInRectdoesn't exist per the comment above), but will silently change the response contract once it lands.♻️ Proposed fix: summarize the native result too
if (typeof map.getElementsInRect === "function") { - return map.getElementsInRect(type, bounds); + const summarize = type === "footpath" ? summarizeFootpathElement + : type === "track" ? summarizeTrackElement + : summarizeEntranceElement; + return map.getElementsInRect(type, bounds).map(el => summarize(el.tileX ?? el.x, el.tileY ?? el.y, el)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ride-builder/src/ride-builder.js` around lines 365 - 381, Update the native path in handleGetElementsInRect to map each map.getElementsInRect result through the same element summarization used by scanElementsInRect, preserving the consistent tileX, tileY, additionStatus, isAdditionFull, and related response shape across both paths.mcp-server/openrct2_mcp/ride_ops.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-module import of a private helper.
_load_ride_builder_maintenance_indexis imported here with its underscore intact, signalling module-private inbridge_fast.pyyet now used cross-module. Consider dropping the leading underscore inbridge_fast.py(or re-exporting explicitly) to make the intended public surface clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/ride_ops.py` at line 12, Make the maintenance-index helper’s cross-module API intentional: rename _load_ride_builder_maintenance_index in bridge_fast.py to a public name and update its callers, including ride_ops.py, or explicitly re-export it through bridge_fast.py while preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp-server/openrct2_mcp/map_region.py`:
- Around line 44-45: Normalize the response’s elements field to an empty list
whenever elements is not a list, matching the existing count behavior. Update
the elements assignment alongside the count calculation so non-list payloads
such as None produce count 0 and elements [].
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Around line 101-121: Update _ride_occupancy_from_plugin to validate that the
result returned by ride_builder.call is a dictionary before accessing guestCount
or isEmpty. If the payload is not a dict, return (None, None), preserving the
existing fallback behavior for plugin failures and malformed responses.
- Around line 214-233: Update the wait-timeout handling in the ride operation
loop so a ride that remains occupied records or reports a meaningful
occupancy-based failure before returning the timeout error. Ensure the final
diagnostic does not fall back to “unknown error” when
_ride_occupancy_from_plugin indicates guests remain, while preserving existing
_attempt() error reporting for other failure paths.
In `@mcp-server/openrct2_mcp/server.py`:
- Around line 1355-1373: Update the docstring for get_map_elements_in_rect_tool
to replace the Unicode multiplication sign in “40×40” with the ASCII “x”,
preserving the rest of the documentation unchanged.
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 259-269: Update normalizeRectBounds, used by scanElementsInRect
and scanGuestsInRect, to clamp each rectangle’s X and Y span to the plugin’s
established maximum before returning normalized bounds. Ensure direct
getElementsInRect/getGuestsInRect callers, including uncapped radius requests,
cannot trigger scans beyond that limit while preserving valid bounds and
existing min/max normalization.
---
Outside diff comments:
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Around line 283-309: Replace the duplicated field-by-field logic in
_merge_maintenance with a delegation to
bridge_fast._merge_ride_builder_maintenance, importing that helper alongside
_load_ride_builder_maintenance_index. Preserve the existing None handling and
return behavior, including the unchanged result when ride_builder_row is absent.
---
Nitpick comments:
In `@mcp-server/openrct2_mcp/guest_intel.py`:
- Around line 97-98: Update the broad exception handler in the plugin scan flow
to log the caught exception at debug or warning level before falling back, while
preserving the existing fallback behavior. Use the surrounding plugin path
detection function or scan symbol to locate this handler, and include enough
context to distinguish a missing plugin path from a scan failure.
In `@mcp-server/openrct2_mcp/ride_ops.py`:
- Line 12: Make the maintenance-index helper’s cross-module API intentional:
rename _load_ride_builder_maintenance_index in bridge_fast.py to a public name
and update its callers, including ride_ops.py, or explicitly re-export it
through bridge_fast.py while preserving the existing behavior.
In `@mcp-server/tests/test_map_region_api115.py`:
- Around line 24-26: Add tests alongside test_rejects_unknown_element_type for
get_elements_in_rect covering oversized dimensions and zero or negative
dimensions, asserting the backend call clamps each side to MAX_REGION_SIDE (40)
and the minimum side length of 1, including the expected bounds.
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 365-381: Update the native path in handleGetElementsInRect to map
each map.getElementsInRect result through the same element summarization used by
scanElementsInRect, preserving the consistent tileX, tileY, additionStatus,
isAdditionFull, and related response shape across both paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5579337-28bf-45c4-a5cc-e979866ec75e
📒 Files selected for processing (13)
mcp-server/openrct2_mcp/bridge_fast.pymcp-server/openrct2_mcp/guest_intel.pymcp-server/openrct2_mcp/map_region.pymcp-server/openrct2_mcp/ride_ops.pymcp-server/openrct2_mcp/scenery_tools.pymcp-server/openrct2_mcp/server.pymcp-server/openrct2_mcp/time_tools.pymcp-server/tests/test_bridge_fast_api115.pymcp-server/tests/test_map_region_api115.pymcp-server/tests/test_scenery_tools.pymcp-server/tests/test_time_tools.pyplugins/ride-builder/lib/openrct2.d.tsplugins/ride-builder/src/ride-builder.js
Dedupe maintenance merge helpers, reuse ride summaries for refurbish ranking, avoid double getGameSpeed, and harden ride-builder rect scans.
Normalize non-list map payloads, clamp plugin rect bounds, validate occupancy responses, and improve timeout diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ride-builder/src/ride-builder.js (1)
226-241: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve unknown occupancy as unknown.
When
guestCountis unavailable andride.isEmptyis not a boolean, Line [241] returnsfalsebecausenull === 0is false. Downstream bridge/refurbishment code usesNoneto represent unknown occupancy, so this incorrectly reports an empty-state result as authoritative.Suggested fix
- const guestCount = typeof ride.guestCount === "number" ? ride.guestCount : null; + const guestCount = Number.isFinite(ride.guestCount) ? ride.guestCount : null; ... - isEmpty: typeof ride.isEmpty === "boolean" ? ride.isEmpty : guestCount === 0, + isEmpty: typeof ride.isEmpty === "boolean" + ? ride.isEmpty + : guestCount == null ? null : guestCount === 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ride-builder/src/ride-builder.js` around lines 226 - 241, The occupancy fallback in the ride mapping must preserve an unknown state: update the isEmpty calculation in the returned object so it uses ride.isEmpty when boolean, returns true or false only when guestCount is numeric, and otherwise returns null/None rather than comparing null to zero.
🧹 Nitpick comments (1)
mcp-server/openrct2_mcp/time_tools.py (1)
102-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatch
ConnectionErrorhere instead ofException.RideBuilderClient.call()only raisesConnectionErroron request failure, so this keeps the fallback working without swallowing unrelated bugs. The failure test should raiseConnectionErrorto match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/openrct2_mcp/time_tools.py` around lines 102 - 106, Update the exception handler around ride_builder.call("getGameSpeed") to catch only ConnectionError, allowing unrelated exceptions to propagate; also update the associated failure test to raise ConnectionError instead of a generic exception.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp-server/openrct2_mcp/server.py`:
- Around line 212-218: In the getGameSpeed handling within openrct2_status,
catch only ConnectionError for transport failures and set speed_payload and
plugin_api to None there. Before accessing apiVersion, require speed_payload to
be a dict, while preserving the integer check; allow malformed payloads and
unrelated exceptions to propagate instead of silently suppressing them.
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 259-276: Update normalizeRectBounds to validate bounds.minX,
bounds.maxX, bounds.minY, and bounds.maxY as finite integers before applying
Math.min or Math.max. Return null for strings, booleans, null, fractional, or
otherwise invalid coordinates, while preserving the existing normalization and
maximum-side logic for valid integer bounds.
---
Outside diff comments:
In `@plugins/ride-builder/src/ride-builder.js`:
- Around line 226-241: The occupancy fallback in the ride mapping must preserve
an unknown state: update the isEmpty calculation in the returned object so it
uses ride.isEmpty when boolean, returns true or false only when guestCount is
numeric, and otherwise returns null/None rather than comparing null to zero.
---
Nitpick comments:
In `@mcp-server/openrct2_mcp/time_tools.py`:
- Around line 102-106: Update the exception handler around
ride_builder.call("getGameSpeed") to catch only ConnectionError, allowing
unrelated exceptions to propagate; also update the associated failure test to
raise ConnectionError instead of a generic exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 511f1c89-a7f1-4c74-bdb3-cef306382f90
📒 Files selected for processing (10)
mcp-server/openrct2_mcp/bridge_fast.pymcp-server/openrct2_mcp/guest_intel.pymcp-server/openrct2_mcp/map_region.pymcp-server/openrct2_mcp/ride_ops.pymcp-server/openrct2_mcp/server.pymcp-server/openrct2_mcp/time_tools.pymcp-server/tests/test_bridge_fast_api115.pymcp-server/tests/test_map_region_api115.pymcp-server/tests/test_time_tools.pyplugins/ride-builder/src/ride-builder.js
🚧 Files skipped from review as they are similar to previous changes (6)
- mcp-server/tests/test_map_region_api115.py
- mcp-server/tests/test_bridge_fast_api115.py
- mcp-server/openrct2_mcp/bridge_fast.py
- mcp-server/openrct2_mcp/guest_intel.py
- mcp-server/openrct2_mcp/map_region.py
- mcp-server/openrct2_mcp/ride_ops.py
Catch only ConnectionError for plugin speed reads, require integer rect bounds, and keep unknown isEmpty as null.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp-server/openrct2_mcp/time_tools.py`:
- Line 11: Alias the project-local ConnectionError as PluginConnectionError in
mcp-server/openrct2_mcp/time_tools.py at line 11, then update its related except
and raise sites to use the alias. Apply the same import alias and exception-site
updates in mcp-server/tests/test_time_tools.py at line 35.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44590e9f-110b-4eb8-9e32-0156217ee808
📒 Files selected for processing (4)
mcp-server/openrct2_mcp/server.pymcp-server/openrct2_mcp/time_tools.pymcp-server/tests/test_time_tools.pyplugins/ride-builder/src/ride-builder.js
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp-server/openrct2_mcp/server.py
- plugins/ride-builder/src/ride-builder.js
| from pyrct2.client import RCT2 | ||
|
|
||
| from openrct2_mcp.connection import ensure_paused, ensure_unpaused | ||
| from openrct2_mcp.connection import ConnectionError, ensure_paused, ensure_unpaused |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## time_tools.py\n'
cat -n mcp-server/openrct2_mcp/time_tools.py | sed -n '1,220p'
printf '\n## test_time_tools.py\n'
cat -n mcp-server/tests/test_time_tools.py | sed -n '1,220p'
printf '\n## ConnectionError imports/usages\n'
rg -n --no-heading '\bConnectionError\b' mcp-server/openrct2_mcp mcp-server/testsRepository: BenDaSpur/openrct2-mcp
Length of output: 10484
Alias the project-local ConnectionError. This shadows the builtin in both mcp-server/openrct2_mcp/time_tools.py and mcp-server/tests/test_time_tools.py; import it as PluginConnectionError and update the except / raise sites.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 11-11: Import ConnectionError is shadowing a Python builtin
(A004)
📍 Affects 2 files
mcp-server/openrct2_mcp/time_tools.py#L11-L11(this comment)mcp-server/tests/test_time_tools.py#L35-L35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp-server/openrct2_mcp/time_tools.py` at line 11, Alias the project-local
ConnectionError as PluginConnectionError in
mcp-server/openrct2_mcp/time_tools.py at line 11, then update its related except
and raise sites to use the alias. Apply the same import alias and exception-site
updates in mcp-server/tests/test_time_tools.py at line 35.
Source: Linters/SAST tools
1 participant
Summary
reliability,guestCount/isEmpty, income/profit,station.queueTime,context.gameSpeed, andisAdditionFull.getElementsInRect/getGuestsInRectdid not land in that PR).openrct2.d.tsto develop and update refurbish/list/status tooling to prefer the new metrics.Test plan
pytest tests/test_bridge_fast_api115.py tests/test_map_region_api115.py tests/test_time_tools.py tests/test_ride_ops.py tests/test_scenery_tools.pyopenrct2_statusreportsplugin_api_versionand plugingame_speedlist_rides/list_refurbish_candidates_toolshow reliability;refurbish_ride_toolwaits usingguestCount/isEmptyget_map_elements_in_rect_toolandsample_guests_near_tile_toolreturn data via polyfillSummary by CodeRabbit