fix(ui): PR #209 review round — current-screen guards, pending-aware settle, one body guard (T-971)
Every screen-targeted intent now routes through one _require_current_screen() check and returns the structured error shape instead of silently mutating an off-screen viewer (hoshe's finding: scroll_rung from the reach screen fired real IPC and reported ok). The reference driver's fixed 4-frame settle becomes is_pending()-aware with a 600-frame bound, the keep-waiting decision extracted as a pure testable function — restoring the proven eyeball-driver discipline. The terrain_reference guard moves into AtlasApp._on_body_selected(), the shared tail for double-click, Enter, AND the intent path — closing a pre-existing click/Enter divergence hoshe caught this PR formalizing; the intent layer pre-checks via the new SystemScreen.find_body() and reports structured errors for unknown ids and terrain-less bodies. after_test() resets AtlasAgentBridge.current_app (tyre's freed-pending footgun). Suites 58/58 + 14/14; full suite 3,638. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
# =============================================================================
|
||||
|
||||
Reference in New Issue
Block a user