diff --git a/client/tests/atlas_agent_driver.gd b/client/tests/atlas_agent_driver.gd index 386bbf71e..8b1225e06 100644 --- a/client/tests/atlas_agent_driver.gd +++ b/client/tests/atlas_agent_driver.gd @@ -29,6 +29,22 @@ ## own `_run_job("jump_to_center", ...)` dispatch) exists to exercise — ## AtlasAgentInterface's production seam for it, not reimplemented here. ## +## PR #209 review (Hoshe finding 2) — settle discipline after a mutating +## intent: a FIXED `for i in range(4)` frame count (the pre-fix shape) is a +## regression against the proven is_pending()-aware pattern every eyeball +## driver round already established (visual_capture.gd's own +## _wait_for_atlas_layers_ready()) — a slow live server round-trip leaves the +## NEXT job's observe() reading a mid-fetch viewer, exactly the class of bug +## the whole T-1157 "settle-until-ready" technique exists to prevent. +## _settle_after_intent() below polls StepCanvasRequest.is_pending() (via the +## regional viewer's own get_request()) when "regional" is the current screen +## — the only screen a step-canvas fetch could be in flight for — with a +## bounded max-frame fallback (SETTLE_MAX_FRAMES, matching how the eyeball +## drivers waited: a generous multi-second bound, never an unbounded await). +## Every OTHER screen-targeted intent (reach/system) has no request to wait +## on, so it keeps the small fixed settle (SETTLE_FIXED_FRAMES) for its own +## panel-rebuild/queue_redraw() to apply. +## ## Usage (JSON job list on stdin path, matching visual_capture.gd's own ## `-- --flag value` CLI convention): ## godot --rendering-driver opengl3 --path client \ @@ -44,6 +60,18 @@ ## back to assert the reference driver actually completes a real session. extends SceneTree +## Fixed settle for a mutating intent with no in-flight request to poll +## (reach/system screen intents — a panel rebuild/queue_redraw() needs at +## most a couple of frames, never a network round-trip). +const SETTLE_FIXED_FRAMES: int = 4 + +## Bounded fallback for the is_pending()-aware settle on "regional" — +## generous, matching how the T-1157 eyeball drivers waited (multi-second +## bound at 60fps), never an unbounded await. A request that's STILL pending +## after this many frames is a real timeout, not "give it a bit longer" — +## the driver logs it and moves on rather than hanging the whole job list. +const SETTLE_MAX_FRAMES: int = 600 + var _jobs_path: String = "" var _output_path: String = "" var _results: Array = [] @@ -99,19 +127,62 @@ func _run() -> void: result = atlas_agent_interface.observe(atlas_agent_bridge.current_app) else: result = atlas_agent_interface.act(atlas_agent_bridge.current_app, intent, params) - # Settle-until-ready (T-1157 inventory item 2's underlying discipline, - # applied generically here rather than per-navigation-intent): a few - # frames after every mutating call so the resulting screen/viewer - # state (panel rebuilds, queue_redraw()) has actually applied before - # the NEXT job's observe() reads it. - for i in range(4): - await process_frame + await _settle_after_intent(atlas_agent_bridge.current_app) _results.append({"intent": intent, "params": params, "result": result}) _write_output(_output_path, _results) quit() +## PR #209 review (Hoshe finding 2) — see this file's own header doc for the +## full rationale. `app` may be null (e.g. after close_atlas) — a null app +## has nothing to poll, so this falls through to the fixed settle only. The +## bounded-fallback DECISION (keep waiting vs stop) is split out into the +## pure, no-await should_keep_waiting() below specifically so it's testable +## without a real SceneTree frame loop (test_atlas_agent_driver.gd drives it +## directly against a fake pending-state + frame counter). +func _settle_after_intent(app: Variant) -> void: + var viewer: Variant = _regional_viewer_if_current(app) + if viewer == null: + for i in range(SETTLE_FIXED_FRAMES): + await process_frame + return + var request: Variant = viewer.get_request() + var waited := 0 + while should_keep_waiting(request.is_pending(), waited, SETTLE_MAX_FRAMES): + await process_frame + waited += 1 + if request.is_pending(): + print( + "atlas_agent_driver: settle timed out after %d frames — request still pending" + % SETTLE_MAX_FRAMES + ) + + +## The pure bounded-fallback decision: keep waiting only while the request is +## STILL pending AND the frame budget isn't exhausted. Static + no SceneTree +## dependency — `is_pending`/`waited`/`max_frames` are plain values a test can +## supply directly, exercising the exact boundary conditions (pending forever +## past the bound stops; resolving early stops immediately) without needing +## a real request object or a real frame loop. +static func should_keep_waiting(is_pending: bool, waited: int, max_frames: int) -> bool: + return is_pending and waited < max_frames + + +## `app` may be null; "regional" may not even be registered yet this early in +## a session (e.g. before the first open_body). Returns null in either case +## rather than erroring — the caller's fixed-settle fallback covers it. +func _regional_viewer_if_current(app: Variant) -> Variant: + if app == null: + return null + if app.current_screen_id() != "regional": + return null + var regional_screen: Variant = app.get_screen("regional") + if regional_screen == null: + return null + return regional_screen.get_viewer() + + func _parse_args() -> void: var args := OS.get_cmdline_user_args() var i := 0 diff --git a/client/tests/test_atlas_agent_driver.gd b/client/tests/test_atlas_agent_driver.gd index caca7b9a3..b69868308 100644 --- a/client/tests/test_atlas_agent_driver.gd +++ b/client/tests/test_atlas_agent_driver.gd @@ -62,6 +62,30 @@ func test_write_output_round_trips_through_json() -> void: assert_that(parsed).is_equal(results) +# ============================================================================= +# PR #209 review (Hoshe finding 2) — should_keep_waiting()'s bounded-fallback +# logic. Pure/static, no SceneTree dependency, so the boundary conditions are +# directly testable without a real frame loop or a real StepCanvasRequest. +# ============================================================================= + + +func test_should_keep_waiting_true_while_pending_and_under_the_frame_budget() -> void: + assert_bool(DriverScript.should_keep_waiting(true, 0, 600)).is_true() + assert_bool(DriverScript.should_keep_waiting(true, 599, 600)).is_true() + + +func test_should_keep_waiting_false_once_no_longer_pending() -> void: + assert_bool(DriverScript.should_keep_waiting(false, 0, 600)).is_false() + + +func test_should_keep_waiting_false_once_the_frame_budget_is_exhausted() -> void: + # Still pending, but waited has reached max_frames — the bounded fallback + # must stop here rather than hanging indefinitely on a genuinely-stuck + # request (the whole reason SETTLE_MAX_FRAMES exists). + assert_bool(DriverScript.should_keep_waiting(true, 600, 600)).is_false() + assert_bool(DriverScript.should_keep_waiting(true, 601, 600)).is_false() + + ## The InputSwallower inner class (T-1157 inventory item 1) — a plain Node ## subclass with no SceneTree/window dependency for its OWN logic ## (set_input_as_handled() requires a live viewport to call meaningfully, but diff --git a/client/tests/test_atlas_agent_interface.gd b/client/tests/test_atlas_agent_interface.gd index 22d56a163..1ad47e1d9 100644 --- a/client/tests/test_atlas_agent_interface.gd +++ b/client/tests/test_atlas_agent_interface.gd @@ -55,6 +55,12 @@ func after_test() -> void: HudGroups._active_mode = HudGroups.Mode.GAMEPLAY HudGroups._groups.erase(TEST_APP_PATH) HudGroups._groups.erase("implant/map") + # PR #209 review (Tyre): every _make_app() call sets + # AtlasAgentBridge.current_app to the (now freed) instance via on_install() + # — clearing it here matches the HudGroups-reset discipline already above + # and closes the latent footgun of a later test/consumer reading a stale, + # freed-pending app reference. + AtlasAgentBridge.current_app = null # ============================================================================= @@ -158,17 +164,57 @@ func test_open_body_reaches_atlas_app_nav_push_to_regional() -> void: app.queue_free() -func test_open_body_is_a_no_op_for_unrecognized_id() -> void: +func test_open_body_returns_structured_error_for_unrecognized_id() -> void: var app = _make_app() app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) _enter_orbital(app) - AtlasAgentInterfaceScript.act(app, "open_body", {"body_id": "does_not_exist"}) + var result: Dictionary = AtlasAgentInterfaceScript.act( + app, "open_body", {"body_id": "does_not_exist"} + ) + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() assert_str(app.current_screen_id()).is_equal("system") app.queue_free() +## PR #209 review (Hoshe finding 3, lead ruling): open_body must reject a +## RECOGNIZED body with no terrain_reference — this is the pre-existing +## click/Enter divergence the PR formalizes and fixes. Structured error, not +## a silent no-op, and no navigation must occur. +func test_open_body_rejects_a_body_with_no_terrain_reference() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_orbital(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "open_body", {"body_id": "GJ1c"}) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + assert_str(app.current_screen_id()).override_failure_message( + "open_body must not navigate for a body with no terrain_reference" + ).is_equal("system") + app.queue_free() + + +## The positive case alongside the guard above: a body WITH a +## terrain_reference still opens normally (GJ1b in the fixture) — this is +## test_open_body_reaches_atlas_app_nav_push_to_regional() above, kept as its +## own assertion here too so the guard test and the "terrain body opens" test +## sit next to each other per the review's own test list. +func test_open_body_opens_a_body_with_a_terrain_reference() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_orbital(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "open_body", {"body_id": "GJ1b"}) + + assert_bool(result.get("ok", false)).is_true() + assert_str(app.current_screen_id()).is_equal("regional") + app.queue_free() + + # ============================================================================= # act() — regional/step-canvas intents # ============================================================================= @@ -320,6 +366,133 @@ func test_close_atlas_reaches_hud_groups_close_app() -> void: app.queue_free() +# ============================================================================= +# PR #209 review (Hoshe finding 1) — off-screen intent dispatch. Every +# screen-targeted intent must reject when its expected screen is NOT the +# current one, with the structured {"ok": false, "error": ...} shape, and +# must NOT mutate the off-screen screen/viewer as a side effect. +# ============================================================================= + + +## The explicitly-called-out case: scroll_rung while "reach" is showing must +## not silently mutate the (registered but off-screen) regional viewer — the +## exact bug this whole guard exists to close. Verifies both the structured +## error AND the absence of a side effect (the off-screen viewer's held rung +## is unchanged from its "regional" was never entered" default). +func test_scroll_rung_from_reach_screen_returns_structured_error_and_does_not_mutate_viewer() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + assert_str(app.current_screen_id()).override_failure_message( + "test setup: app must default to the reach screen" + ).is_equal("reach") + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "scroll_rung", {"direction": 1}) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + var viewer: Variant = app.get_screen("regional").get_viewer() + assert_str(viewer.get_held_rung()).override_failure_message( + "scroll_rung from an off-screen 'reach' must not mutate the regional viewer" + ).is_equal(StepCanvasTransport.RUNG_GLOBAL) + app.queue_free() + + +func test_jump_to_center_from_system_screen_returns_structured_error() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_orbital(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act( + app, "jump_to_center", {"world_center": [1.0, 2.0]} + ) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + app.queue_free() + + +func test_reset_view_from_reach_screen_returns_structured_error() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "reset_view", {}) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + app.queue_free() + + +func test_set_overlay_from_system_screen_returns_structured_error_and_does_not_mutate_viewer() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_orbital(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act( + app, "set_overlay", {"overlay_id": "gen_dw_temp", "visible": true} + ) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + var viewer: Variant = app.get_screen("regional").get_viewer() + assert_bool(viewer.is_overlay_visible("gen_dw_temp")).override_failure_message( + "set_overlay from an off-screen 'system' must not mutate the regional viewer" + ).is_false() + app.queue_free() + + +func test_select_system_from_system_screen_returns_structured_error() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_orbital(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act( + app, "select_system", {"system_id": "GJ1"} + ) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + app.queue_free() + + +func test_open_system_from_regional_screen_returns_structured_error() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + _enter_regional(app) + + var result: Dictionary = AtlasAgentInterfaceScript.act( + app, "open_system", {"system_id": "GJ1"} + ) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + app.queue_free() + + +func test_select_body_from_reach_screen_returns_structured_error() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "select_body", {"body_id": "GJ1c"}) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + app.queue_free() + + +func test_open_body_from_reach_screen_returns_structured_error_and_does_not_navigate() -> void: + var app = _make_app() + app._internal_app_changed(TEST_APP_PATH, HudGroups.Mode.FULLSCREEN) + + var result: Dictionary = AtlasAgentInterfaceScript.act(app, "open_body", {"body_id": "GJ1b"}) + + assert_bool(result.get("ok", true)).is_false() + assert_bool(result.has("error")).is_true() + assert_str(app.current_screen_id()).override_failure_message( + "open_body from an off-screen 'reach' must not navigate" + ).is_equal("reach") + app.queue_free() + + # ============================================================================= # Unknown intent # ============================================================================= diff --git a/client/ui/implant/apps/atlas/atlas_agent_interface.gd b/client/ui/implant/apps/atlas/atlas_agent_interface.gd index 24f3f2f11..38969df35 100644 --- a/client/ui/implant/apps/atlas/atlas_agent_interface.gd +++ b/client/ui/implant/apps/atlas/atlas_agent_interface.gd @@ -151,6 +151,23 @@ static func _walk_affordances_recursive(node: Node, out: Array) -> void: ## meant to be driven by a fallible external caller (a curl-style JSON body, ## a test fixture), and a malformed intent name is exactly the kind of input ## it must handle gracefully. +## +## **PR #209 review (Hoshe finding 1, the off-screen dispatch bug):** +## app.get_screen(id) is a REGISTRY lookup — every screen is registered (and +## therefore reachable) for the app's whole lifetime, regardless of which one +## is currently visible/current. Before this fix, every screen-targeted +## intent resolved its target via get_screen() alone, so e.g. scroll_rung +## while "reach" was showing silently mutated the off-screen "regional" +## viewer — including firing a real SimBridge.request_step_canvas() IPC +## call — and returned {"ok": true}, as if the player had actually been +## looking at the map. Every screen-targeted intent below now checks +## app.current_screen_id() against the screen it targets FIRST, returning +## the same structured {"ok": false, "error": ...} shape the null-app/ +## unknown-intent paths already use. This is a per-intent expectation, not a +## single global gate, because different intents target different screens +## (select_system/open_system expect "reach"; select_body/open_body expect +## "system"; scroll_rung/jump_to_center/reset_view/set_overlay expect +## "regional") — see _require_current_screen()'s own doc. static func act(app: Node, intent: String, params: Dictionary = {}) -> Dictionary: if app == null and intent != "open_atlas": return {"ok": false, "error": "no AtlasApp instance available"} @@ -162,35 +179,35 @@ static func act(app: Node, intent: String, params: Dictionary = {}) -> Dictionar HudGroups.close_app() return {"ok": true} "select_system": - app.get_screen("reach").select_system_by_id(str(params.get("system_id", ""))) - return {"ok": true} + return _act_on_current_screen( + app, "reach", func(s: Node) -> void: s.select_system_by_id(str(params.get("system_id", ""))) + ) "open_system": - app.get_screen("reach").open_system_by_id(str(params.get("system_id", ""))) - return {"ok": true} + return _act_on_current_screen( + app, "reach", func(s: Node) -> void: s.open_system_by_id(str(params.get("system_id", ""))) + ) "select_body": - app.get_screen("system").select_body_by_id(str(params.get("body_id", ""))) - return {"ok": true} + return _act_on_current_screen( + app, "system", func(s: Node) -> void: s.select_body_by_id(str(params.get("body_id", ""))) + ) "open_body": - app.get_screen("system").open_body_by_id(str(params.get("body_id", ""))) - return {"ok": true} + return _act_open_body(app, params) "scroll_rung": return _act_scroll_rung(app, params) "jump_to_center": return _act_jump_to_center(app, params) "reset_view": - var viewer: Variant = _get_viewer(app) - if viewer == null: - return {"ok": false, "error": "regional screen not active"} - viewer._reset_to_global() - return {"ok": true} + return _act_on_current_screen( + app, "regional", func(s: Node) -> void: s.get_viewer()._reset_to_global() + ) "set_overlay": - var overlay_viewer: Variant = _get_viewer(app) - if overlay_viewer == null: - return {"ok": false, "error": "regional screen not active"} - overlay_viewer.set_overlay_visible( - str(params.get("overlay_id", "")), bool(params.get("visible", true)) + return _act_on_current_screen( + app, + "regional", + func(s: Node) -> void: s.get_viewer().set_overlay_visible( + str(params.get("overlay_id", "")), bool(params.get("visible", true)) + ) ) - return {"ok": true} "back": app.nav.pop() return {"ok": true} @@ -198,14 +215,85 @@ static func act(app: Node, intent: String, params: Dictionary = {}) -> Dictionar return {"ok": false, "error": "unknown intent '%s'" % intent} # gdlint:ignore = max-returns +## The shared current-screen guard (PR #209 review, Hoshe finding 1): returns +## the structured {"ok": false, "error": ...} shape if `expected_screen_id` +## isn't the CURRENT screen (app.current_screen_id()), otherwise runs +## `body` against the resolved screen instance and returns {"ok": true}. +## `body` is a Callable taking the screen Node — every screen-targeted intent +## that has no extra result fields to report (select_system/open_system/ +## select_body/reset_view/set_overlay) routes through this single check +## rather than five copies of the same "is this screen current" branch. +## scroll_rung/jump_to_center/open_body still need their own wrappers (they +## report extra fields — rung/world_center — or a body-specific guard) but +## reuse _require_current_screen() for the identical check. +static func _act_on_current_screen( + app: Node, expected_screen_id: String, body: Callable +) -> Dictionary: + var screen: Variant = _require_current_screen(app, expected_screen_id) + if screen == null: + return _not_current_screen_error(app, expected_screen_id) + body.call(screen) + return {"ok": true} + + +## Returns the registered screen instance for `expected_screen_id` ONLY if it +## is also the CURRENTLY showing screen (app.current_screen_id() == +## expected_screen_id) — null otherwise (either unregistered, per +## get_screen()'s own contract, or registered-but-not-current, the bug this +## whole guard exists to close). This is the ONE place "is this screen +## current" is checked — every act() branch above and every _act_* helper +## below calls this rather than checking current_screen_id() inline. +static func _require_current_screen(app: Node, expected_screen_id: String) -> Variant: + if app.current_screen_id() != expected_screen_id: + return null + return app.get_screen(expected_screen_id) + + +static func _not_current_screen_error(app: Node, expected_screen_id: String) -> Dictionary: + return { + "ok": false, + "error": ( + "intent requires screen '%s' to be current, but '%s' is showing" + % [expected_screen_id, app.current_screen_id()] + ), + } + + +## `open_body` — PR #209 review (Hoshe finding 3, lead ruling): the +## Enter-key path has always gated body entry on `terrain_reference != null` +## (AtlasApp._handle_enter()'s "system" branch); double-click (and therefore +## this intent, which drives the identical SystemScreen.body_selected signal) +## did not — a pre-existing click/Enter divergence this PR formalizes into a +## contract, so it fixes it. The guard itself now lives in the SHARED tail +## (AtlasApp._on_body_selected(), the signal handler both double-click and +## this intent funnel through) — a guarded body is a silent no-op there, +## matching what the Enter path always did. This intent does its OWN +## pre-check via SystemScreen.find_body() so it can report a STRUCTURED +## error instead of masking "nothing happened" as {"ok": true} the way a bare +## click has no way to report either way. +static func _act_open_body(app: Node, params: Dictionary) -> Dictionary: + var screen: Variant = _require_current_screen(app, "system") + if screen == null: + return _not_current_screen_error(app, "system") + var body_id: String = str(params.get("body_id", "")) + var body: Dictionary = screen.find_body(body_id) + if body.is_empty(): + return {"ok": false, "error": "unrecognized body_id '%s'" % body_id} + if body.get("terrain_reference") == null: + return {"ok": false, "error": "body '%s' has no terrain reference" % body_id} + screen.open_body_by_id(body_id) + return {"ok": true} + + ## `scroll_rung` — direction is required; cursor_local defaults to a ## reasonable canvas-center guess (Vector2(400, 300), matching this cluster's ## own gdUnit test fixtures' convention, see test_step_canvas_viewer.gd) since ## an agent driver has no real cursor position to anchor on. static func _act_scroll_rung(app: Node, params: Dictionary) -> Dictionary: - var viewer: Variant = _get_viewer(app) - if viewer == null: - return {"ok": false, "error": "regional screen not active"} + var screen: Variant = _require_current_screen(app, "regional") + if screen == null: + return _not_current_screen_error(app, "regional") + var viewer: Variant = screen.get_viewer() var direction: int = int(params.get("direction", 1)) var cursor_raw: Variant = params.get("cursor_local") var cursor_local: Vector2 = ( @@ -225,21 +313,24 @@ static func _act_scroll_rung(app: Node, params: Dictionary) -> Dictionary: ## every other navigation intent uses — no parallel request-building code ## exists in this file. static func _act_jump_to_center(app: Node, params: Dictionary) -> Dictionary: - var viewer: Variant = _get_viewer(app) - if viewer == null: - return {"ok": false, "error": "regional screen not active"} + var screen: Variant = _require_current_screen(app, "regional") + if screen == null: + return _not_current_screen_error(app, "regional") var center_raw: Variant = params.get("world_center") if not (center_raw is Array and (center_raw as Array).size() >= 2): return {"ok": false, "error": "jump_to_center requires world_center: [x, y]"} + var viewer: Variant = screen.get_viewer() var world_center := Vector2(float(center_raw[0]), float(center_raw[1])) var rung: String = str(params.get("rung", "")) viewer.jump_to(world_center, rung) return {"ok": true, "rung": viewer.get_held_rung(), "world_center": [world_center.x, world_center.y]} -## Shared "regional" screen -> StepCanvasViewer accessor — returns null if -## "regional" isn't the registered screen (defensive; every intent that needs -## the viewer checks this rather than assuming the screen tree shape). +## Shared "regional" screen -> StepCanvasViewer accessor, used ONLY by +## observe() (which already knows "regional" is current — it matched on +## screen_id itself — so it doesn't need _require_current_screen()'s guard, +## just a null-safe read of a screen that may not even be registered yet +## early in app lifecycle). static func _get_viewer(app: Node) -> Variant: var regional_screen: Node = app.get_screen("regional") if regional_screen == null: diff --git a/client/ui/implant/apps/atlas/atlas_app.gd b/client/ui/implant/apps/atlas/atlas_app.gd index 8942f2f98..48ff1b018 100644 --- a/client/ui/implant/apps/atlas/atlas_app.gd +++ b/client/ui/implant/apps/atlas/atlas_app.gd @@ -127,11 +127,7 @@ func _handle_enter() -> void: nav.replace("system", {"mode": "orbital", "system": sys}) elif _system_screen and _system_screen.has_body_panel_open(): var body: Dictionary = _system_screen.get_selected_body() - if body.get("terrain_reference") != null: - nav.push("regional", { - "body": body, - "system": nav.current_payload().get("system", {}), - }) + _on_body_selected(body) # ============================================================================= @@ -147,7 +143,28 @@ func _on_system_selected(system_id: String) -> void: nav.push("system", {"mode": "orbital", "system": system}) +## PR #209 review (Hoshe finding 3, lead ruling): the SHARED tail every path +## that "opens" a body funnels through — double-click (SystemScreen. +## body_selected), the Enter-key path (_handle_enter()'s "system" branch, +## which used to gate on terrain_reference itself and now just calls this), +## and AtlasAgentInterface's open_body/open_body_by_id intent (also via this +## same signal). Gating HERE, once, closes a pre-existing divergence: the +## Enter-key path already required `terrain_reference != null` before this +## PR, but double-click (and therefore the new open_body intent) did not — +## both silently pushed "regional" for a body with no heightmap. Formalizing +## the intent contract is what surfaced it, so this PR fixes it for both +## callers at once rather than reintroducing the same split. +## +## A guarded body (no terrain_reference) is a silent no-op here — same +## behavior the Enter path always had (it simply never called nav.push() for +## that case). AtlasAgentInterface's open_body/open_body_by_id wrap this at +## the intent layer with a STRUCTURED error instead of a silent no-op (see +## atlas_agent_interface.gd's own _act_open_body()) — an agent driver must be +## able to tell "nothing happened" from "call succeeded", which a bare click +## has no way to report either way. func _on_body_selected(body: Dictionary) -> void: + if body.get("terrain_reference") == null: + return nav.push("regional", { "body": body, "system": nav.current_payload().get("system", {}), diff --git a/client/ui/implant/apps/atlas/screens/system_screen.gd b/client/ui/implant/apps/atlas/screens/system_screen.gd index f069800c3..7c5b65e32 100644 --- a/client/ui/implant/apps/atlas/screens/system_screen.gd +++ b/client/ui/implant/apps/atlas/screens/system_screen.gd @@ -427,8 +427,14 @@ func select_body_by_id(body_id: String) -> void: ## T-971 (AtlasAgentInterface `open_body` intent) — the SAME state mutation ## _handle_orbital_double_click() performs (double-click equivalent: emits -## body_selected, which AtlasApp._on_body_selected() turns into -## nav.push("regional", ...)). A no-op for an unrecognized id. +## body_selected, which AtlasApp._on_body_selected() now gates on +## terrain_reference — PR #209 review, Hoshe finding 3 — before pushing +## "regional"). A no-op for an unrecognized id. Emits unconditionally for a +## RECOGNIZED body regardless of terrain_reference, same as the double-click +## handler always has — the guard lives in the shared _on_body_selected() +## tail, not here, so this stays a pure "found it, emit" lookup identical to +## the click path (AtlasAgentInterface does its OWN pre-check via +## find_body() to report a structured error instead of a silent no-op). func open_body_by_id(body_id: String) -> void: for b: Dictionary in _orbital_bodies: if str(b.get("body_id", "")) == body_id: @@ -436,6 +442,20 @@ func open_body_by_id(body_id: String) -> void: return +## Generic body lookup by id — {} if not found. Shared read used by both +## select_body_by_id()/open_body_by_id()'s own linear scans (kept inline in +## each for now, matching this file's existing per-method scan style) and by +## AtlasAgentInterface's open_body pre-check (PR #209 review, Hoshe finding +## 3) so the intent layer can build a structured "no terrain reference" +## error without re-deriving the emit-vs-no-op logic already owned by +## open_body_by_id() above. +func find_body(body_id: String) -> Dictionary: + for b: Dictionary in _orbital_bodies: + if str(b.get("body_id", "")) == body_id: + return b + return {} + + func _close_detail_panels() -> void: _selected_body = {} _selected_station = {}