Test spring#3
Conversation
…dules, new cruise-to-kill intent, map/config/launch updates, and corresponding Lua API/endpoint + CMake changes
…clear nav history on STARTED
|
抱歉——我无法在当前回复中生成完整且通过验证的隐藏 review stack 区块，因为该区块必须精确且一次性包含所有 300+ 个 rangeId。请允许我分批生成（例如把 PR 拆成 2–3 部分），或提供更小的范围集合，我将立即产出符合规范的、可验证的审阅堆栈与 Walkthrough。 ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lua/api.lua (1)
37-49: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win不要用默认值吞掉缺失的导航配置。
restart_navigation的参数已经按结构化配置声明了，这里再把缺失字段默默降级成false/empty，会把调用方的拼写错误或漏参直接吞掉，最后静默启动到错误的地图/传感配置。这里更适合在入口一次性校验必填项，并对global_map做边界校验后立即报错。建议修改
function api.restart_navigation(config) - config = config or {} + assert(config ~= nil, "config is required") + assert(config.launch_livox ~= nil, "config.launch_livox is required") + assert(config.launch_odin1 ~= nil, "config.launch_odin1 is required") + assert(config.use_sim_time ~= nil, "config.use_sim_time is required") + assert( + type(config.global_map) == "string" and config.global_map:match("^[%w_-]+$"), + "config.global_map is invalid" + ) local filename, msg = util.search_setup_resource() if not filename then error(msg) end - local launch_livox = tostring(config.launch_livox or "false") - local launch_odin1 = tostring(config.launch_odin1 or "false") - local global_map = tostring(config.global_map or "empty") - local use_sim_time = tostring(config.use_sim_time or "false") + local launch_livox = tostring(config.launch_livox) + local launch_odin1 = tostring(config.launch_odin1) + local global_map = tostring(config.global_map) + local use_sim_time = tostring(config.use_sim_time)As per coding guidelines, "Perform comprehensive validation during initialization rather than runtime checks - validate all configuration parameters and constructor arguments upfront." Also, "Do not add redundant validation checks for function arguments that are already documented in type annotations ... and prioritize raising errors over using fallback/default values."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/api.lua` around lines 37 - 49, The restart_navigation function currently masks missing or misspelled config fields by coercing them to default strings (e.g., launch_livox, launch_odin1, global_map, use_sim_time), which can silently start navigation with wrong settings; update api.restart_navigation to perform explicit upfront validation of the config table: require that required keys exist (e.g., global_map) and have acceptable values, validate global_map against allowed names or non-empty string, and validate booleans for launch_livox, launch_odin1, use_sim_time; if any validation fails, raise an error immediately instead of converting to default values and remove the current tostring(... or "...") fallbacks so callers get explicit errors for missing/invalid parameters.
🟠 Major comments (17)
src/lua/intent/competion/cruise-to-kill.lua-1-8 (1)
1-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick win缺少
__index元方法，实例将无法继承intent上定义的任何方法。Lua 面向对象的标准模式要求在将表用作元表之前设置
intent.__index = intent。当前代码中setmetatable(details, intent)将intent作为元表，但由于intent.__index为nil，Lua 在实例上查找键时不会沿元表链向上追溯。一旦后续在intent上添加任何方法（如function intent:execute()），通过实例调用时将静默失败，引发运行时错误。🐛 建议修复
local intent = {} +intent.__index = intent function intent.new() local details = { a = 1, b = 2, } return setmetatable(details, intent) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/competion/cruise-to-kill.lua` around lines 1 - 8, The intent table is used as a metatable but lacks the __index metamethod so instances created by intent.new won't inherit methods; update the constructor to set intent.__index = intent before calling setmetatable (or set the metatable to { __index = intent }) so that calls on instances (e.g., methods defined as function intent:execute()) correctly resolve via the metatable; modify intent.new and/or the intent table initialization accordingly to ensure proper method inheritance.godot-mock/models/cube_material.tres-7-8 (1)
7-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
albedo_color为纯黑将使纹理完全不可见Godot 官方文档明确指出：「Albedo color and texture can be used together as they are multiplied.」
Color(0, 0, 0, 1)与任何纹理相乘结果均为黑色，white_wood.png贴图将完全失效，Cube 会渲染为纯黑色。应将albedo_color改为白色Color(1, 1, 1, 1)，使纹理按原色显示。🐛 建议修复
-albedo_color = Color(0, 0, 0, 1) +albedo_color = Color(1, 1, 1, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/models/cube_material.tres` around lines 7 - 8, albedo_color is currently set to Color(0,0,0,1) which multiplies the albedo_texture (ExtResource("1")) to pure black; change the albedo_color property to Color(1,1,1,1) so the texture (white_wood.png referenced by albedo_texture / ExtResource("1")) displays at its original colors.src/lua/endpoint/mock.lua-28-29 (1)
28-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick win不要在这里绕过环境初始化并直接打印启动成功。
mock模式这里直接跑ros2 launch，但没有复用search_setup_resource()/source流程，而且完全忽略了native.run()的返回值。当前进程如果没预先加载 ROS 环境，初始化会失败，但日志仍然会显示“launched”。建议修改
- native.run("ros2 launch rmcs-navigation static.launch.yaml &") - action:info("static.launch.yaml launched") + local setup, err = native.search_setup_resource() + if not setup then + error(err) + end + + local ok, run_err = + native.run(string.format("source %q && ros2 launch rmcs-navigation static.launch.yaml &", setup)) + if not ok then + error(run_err) + end + + action:info("static.launch.yaml launched")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/endpoint/mock.lua` around lines 28 - 29, 当前在 mock 模式中直接调用 native.run("ros2 launch ...") 并立即 action:info("static.launch.yaml launched")，这会绕过 search_setup_resource()/source 环境初始化且不检查 native.run 返回值；请改为复用 search_setup_resource()/source 的初始化流程以确保 ROS 环境已加载，然后调用 native.run 并检查其返回状态/错误，只有在 native.run 表示成功后才调用 action:info("static.launch.yaml launched")，在失败时记录具体错误并返回/抛出失败以避免误报；相关标识符：native.run、search_setup_resource、source、action:info。src/lua/train_map.lua-74-79 (1)
74-79: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win地图格子的取值需要在加载时收紧到合法区域 ID。
这里现在只校验了
row[x]是number。NaN、小数或者未定义的 region 编号都会通过初始化校验，然后被locate()原样返回给后续状态机。既然这个模块已经在加载阶段做数据校验，建议把格子值也收紧成“有限整数 + 已知区域枚举”。建议修改
local function validate_data(data) assert(type(data) == "table", "region map data should be a table") assert(is_positive_integer(data.width), "region map data.width should be a positive integer") assert(is_positive_integer(data.height), "region map data.height should be a positive integer") assert(is_finite_number(data.resolution) and data.resolution > 0, "region map data.resolution should be positive") assert(type(data.origin) == "table", "region map data.origin should be a table") assert(is_finite_number(data.origin.x), "region map data.origin.x should be finite") assert(is_finite_number(data.origin.y), "region map data.origin.y should be finite") assert(type(data.rows) == "table", "region map data.rows should be a table") + local names = data.names or RegionName for y = 1, data.height do local row = data.rows[y] assert(type(row) == "table", "region map row " .. y .. " should be a table") for x = 1, data.width do - assert(type(row[x]) == "number", "region map cell " .. y .. "," .. x .. " should be a number") + local cell = row[x] + assert(is_finite_number(cell) and math.floor(cell) == cell, "region map cell " .. y .. "," .. x .. " should be an integer") + assert(names[cell] ~= nil, "region map cell " .. y .. "," .. x .. " should be a known region id") end end endAs per coding guidelines, "Perform comprehensive validation during initialization rather than runtime checks - validate all configuration parameters and constructor arguments upfront."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/train_map.lua` around lines 74 - 79, The loop validating cells in train_map.lua is too permissive (only checks type(row[x]) == "number"); tighten it to reject NaN/floats/unknown IDs by ensuring each cell value in data.rows[y][x] is a finite integer and is present in the known region enumeration (the module's region enum/table) at initialization; if a value is a valid integer ID, coerce/normalize it to an integer, otherwise raise an assertion/error so invalid values never reach locate() or the runtime state machine. Ensure you update the validation block that iterates over data.height/data.width and reference the module's region enum (the canonical region lookup table) when checking membership.src/lua/api.lua-60-75 (1)
60-75:⚠️ Potential issue | 🟠 Major多步启动脚本中的早期失败会被静默吞掉，导致半启动状态被误判为成功。
这段代码中，多条命令仅通过换行符连接，没有设置任何错误处理。当
source、screen -X quit或前面的 launch 命令失败时，后续步骤仍会继续执行。由于util.run()只检查最后一条命令的退出状态，即使早期命令失败，只要最后一条命令成功，整个操作就会被视为成功。结合>/dev/null 2>&1的输出重定向，失败信息完全被隐藏，无法用于调试。建议添加
set -euo pipefail以启用严格错误处理，同时为screen -X quit添加|| true以处理会话不存在的情况：建议修改
local command = [[ +set -euo pipefail source %q - -screen -S rmcs-navigation -X quit - +screen -S rmcs-navigation -X quit || true screen -dmS rmcs-navigation screen -S rmcs-navigation -X screen bash -lc "ros2 launch foxglove_bridge foxglove_bridge_launch.xml" - configs=%q screen -S rmcs-navigation -X screen bash -lc "ros2 launch rmcs-navigation sensor.launch.yaml $configs" sleep 1 screen -S rmcs-navigation -X screen bash -lc "ros2 launch rmcs-navigation motion.launch.yaml $configs" ]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/api.lua` around lines 60 - 75, The multi-line shell constructed in the local variable command silently swallows early failures; update the command string built for packed_command to enable strict shell failure handling by prepending "set -euo pipefail" and make the "screen -S rmcs-navigation -X quit" invocation tolerant of a missing session by appending "|| true". Keep the rest of the commands and the util.run(...) invocation but ensure the formatted string still uses filename and configs (the existing variables) so that any failed intermediate command causes the whole packed_command to exit nonzero and be observable by util.run.src/lua/intent/competion/forward-press.lua-21-26 (1)
21-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick win把
mode和duration在构造期校验完整这里现在只保证
mode是字符串，但initial_state()会把任何未知值都当成one_step；同时duration直接走默认值/透传，后面到elapsed() >= self.duration才会暴露类型或边界问题。配置写错时应直接失败，不要静默降级成另一种行为。可参考的修正
function M.new(args) assert(type(args) == "table", "args should be a table") assert(type(args.mode) == "string", "args.mode should be a string") + assert( + args.mode == "one_step" or args.mode == "two_step", + "args.mode should be 'one_step' or 'two_step'" + ) assert(type(args.switch_interval) == "number", "args.switch_interval should be a number") assert(args.switch_interval > 0, "args.switch_interval should be positive") + if args.duration ~= nil then + assert(type(args.duration) == "number", "args.duration should be a number") + assert(args.duration > 0, "args.duration should be positive") + end return setmetatable({ mode = args.mode, switch_interval = args.switch_interval,As per coding guidelines, "
src/lua/**/*.lua: Perform comprehensive validation during initialization rather than runtime checks - validate all configuration parameters and constructor arguments upfront. If validation fails, raise errors immediately instead of using default values, as using defaults can obscure bugs during debugging."Also applies to: 28-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/competion/forward-press.lua` around lines 21 - 26, The constructor should validate both mode and duration immediately instead of silently defaulting: update the initialization path around initial_state(mode) and the constructor that sets self.duration to check that mode is exactly "one_step" or "two_step" (and map to State.one_step/State.two_step accordingly) and that duration is a finite positive number within allowed bounds (reject nil, non-number, NaN, <=0 or out-of-range) and raise an error if validation fails; ensure you perform these checks during creation (the function that calls initial_state and assigns self.duration) so misconfigured inputs fail fast rather than falling back to defaults.src/lua/endpoint/competition-test.lua-88-106 (1)
88-106:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift测试规则坐标全部置零会让导航意图失真
这里把公路区、浮动路、前压、守家等一组本应互相区分的目标点都写成了
(0, 0)。这样start_cruise、keep_cruise、guard_home、forward_press等任务虽然还能切状态，但实际导航会反复指向同一个点，测试结果很难反映真实路径行为。如果这只是占位数据，建议至少改成从 map/option 读取真实点位，或者明确隔离成不会被误用的 mock 配置。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/endpoint/competition-test.lua` around lines 88 - 106, The test config sets many navigation waypoints (e.g., rule.resupply_zone.ours, rule.road_zone_begin.ours, rule.one_step_begin.ours, rule.central_highland_middle, rule.fluctuant_road_final.them) to {x=0.0,y=0.0}, which causes navigation tasks (start_cruise, keep_cruise, guard_home, forward_press) to all point to the same location; update the fixture so these symbols read real coordinates from the map/options API (or a dedicated test-map loader) or replace them with clearly-named mock-only placeholders that the navigation code will ignore, and remove duplicate keys (e.g., duplicate rule.one_step_begin.ours) to ensure each waypoint is distinct and not accidentally used by production navigation logic.src/lua/endpoint/train-test.lua-381-386 (1)
381-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick win低弹药分支没有接入逃生/恢复状态机
cross_road和keep_cruise只在low_health()时进入escape，recover也只看health_ready()。这样前面配置的bullet_limit/bullet_ready在这条状态机里实际上不会生效：低弹药时会继续巡航，恢复阶段也可能在补弹前就重新出发。可参考的修正
- if condition.low_health() then + if condition.low_health() or condition.low_bullet() then runtime.escape_route = select_escape_route() clear_current_intent() handle:set_next(State.escape) return end- if condition.low_health() then + if condition.low_health() or condition.low_bullet() then runtime.escape_route = select_escape_route() clear_current_intent() handle:set_next(State.escape) return end- if condition.health_ready() then + if condition.health_ready() and condition.bullet_ready() then handle:set_next(State.cross_road) endAlso applies to: 411-416, 465-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/endpoint/train-test.lua` around lines 381 - 386, The low-ammo branch currently only checks condition.low_health() and routes to State.escape, so the bullet-based transitions (bullet_limit/bullet_ready) never trigger; update the state-entry logic in the functions/blocks using condition.low_health(), cross_road, keep_cruise and the recover/health_ready() checks so that when bullet_ready() is false or bullet_count < bullet_limit you route into the escape/recover flow: call select_escape_route(), set runtime.escape_route, clear_current_intent(), and handle:set_next(State.escape) (or State.recover when appropriate) instead of continuing cruise; ensure any existing recover() or health_ready() logic respects bullet_ready() before allowing a return to cruise.godot-mock/scene/sim_bullet.gd-44-46 (1)
44-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick win默认碰撞体创建条件过于宽松了。
这里按
get_child_count()判断，会把“已有自定义子节点但没有CollisionShape3D”的情况误判为已初始化；这种子弹会正常飞行，但永远打不中任何装甲。建议至少按CollisionShape3D/MeshInstance3D是否存在分别判断，而不是看总子节点数。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/sim_bullet.gd` around lines 44 - 46, The current initialization uses get_child_count() to decide whether to call _create_default_shape_and_mesh(), which misclassifies nodes that have custom children but lack a CollisionShape3D or MeshInstance3D; change the logic in the ready/initialization path to explicitly check for presence of a CollisionShape3D and a MeshInstance3D (e.g., using has_node_of_type or iterating children and checking is_class("CollisionShape3D") / is_class("MeshInstance3D")) and only call _create_default_shape_and_mesh() to create whichever specific component(s) are missing instead of relying on get_child_count().godot-mock/scene/battlefiled.tscn-661-664 (1)
661-664:⚠️ Potential issue | 🟠 Major | ⚡ Quick win这里把
OutpostCube的专用尺寸覆盖掉了。
outpost_cube.tscn已经给 outpost 配好了更大的cube_size/hitbox_size，但这里又把它们改回0.5。这样主战场里的 outpost 会比预期小很多，受击盒和显示高度也会跟专用配置不一致。建议修改
-cube_size = Vector3(0.5, 0.5, 0.5) -hitbox_size = Vector3(0.5, 0.5, 0.5) +cube_size = Vector3(1.5, 1.5, 1.5) +hitbox_size = Vector3(1.6, 1.6, 1.6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/battlefiled.tscn` around lines 661 - 664, OutpostCube 的 cube_size 和 hitbox_size 在场景覆盖成了 0.5，覆盖了专用场景 outpost_cube.tscn 中配置的更大尺寸，导致主战场里据点显示和受击盒不正确；请恢复或移除这一覆盖：在节点 OutpostCube 中删除或修改当前 cube_size 和 hitbox_size 条目，使其不覆盖 outpost_cube.tscn 的默认值（或将值改为 outpost_cube.tscn 中定义的尺寸），确保 Transform/节点实例继续使用 outpost_cube.tscn 的专用配置。src/lua/util/blackboard_logger.lua-5-17 (1)
5-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick win先在
attach()阶段校验 blackboard 结构。这里的字段缺失会等到后台循环第一次执行时才以
nil访问报错，排查会比较绕。建议在append_task()前一次性确认scheduler、blackboard.user/game/play/meta/referee和blackboard.referee.robots_hp都存在，并立即抛错。建议修改
function BlackboardLogger.attach(scheduler, blackboard) local request = require("util.scheduler").request + assert(scheduler ~= nil and scheduler.append_task ~= nil, "scheduler.append_task is required") + assert(blackboard ~= nil, "blackboard is required") + assert(blackboard.user ~= nil, "blackboard.user is required") + assert(blackboard.game ~= nil, "blackboard.game is required") + assert(blackboard.play ~= nil, "blackboard.play is required") + assert(blackboard.meta ~= nil, "blackboard.meta is required") + assert(blackboard.referee ~= nil, "blackboard.referee is required") + assert(blackboard.referee.robots_hp ~= nil, "blackboard.referee.robots_hp is required") scheduler:append_task(function()As per coding guidelines, "Perform comprehensive validation during initialization rather than runtime checks - validate all configuration parameters and constructor arguments upfront. If validation fails, raise errors immediately instead of using default values, as using defaults can obscure bugs during debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/util/blackboard_logger.lua` around lines 5 - 17, 在 BlackboardLogger.attach 中，在调用 scheduler:append_task 之前对传入的 scheduler 和 blackboard 的必需字段做一次完整校验：确认 scheduler 非空且支持 append_task（或 request），并且 blackboard.user、blackboard.game、blackboard.play、blackboard.meta、blackboard.referee，以及 blackboard.referee.robots_hp 都存在且类型合理；若任一校验失败立即抛出错误（不要等到后台循环首次访问时才报 nil），以便尽早暴露配置问题；定位修改点为 BlackboardLogger.attach 和 append_task 调用之前的初始化逻辑并加入这些显式断言/错误抛出。godot-mock/scene/sim_armor.gd-38-40 (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick win不要用
get_child_count()代替碰撞体存在检查。只要节点下面已经挂了任意一个子节点，这里就会跳过默认
CollisionShape3D的创建；结果是脚本存在，但装甲板可能根本没有受击体积。这里应该检查“是否已有CollisionShape3D”，而不是“是否有任何子节点”。建议修改
- if get_child_count() == 0: + var has_collision_shape := false + for child in get_children(): + if child is CollisionShape3D: + has_collision_shape = true + break + if not has_collision_shape: _create_default_shape()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/sim_armor.gd` around lines 38 - 40, 不要用 get_child_count() 来判断是否存在碰撞体；改为遍历子节点检查是否已存在 CollisionShape3D 实例。定位到当前使用 get_child_count() 的分支，替换为一个检查子节点类型的逻辑（遍历 get_children() 并用 `child is CollisionShape3D` 判断），只有当没有找到任何 CollisionShape3D 时才调用 _create_default_shape()；保留原有的 _create_default_shape() 调用点不变。src/cxx/context.cc-57-128 (1)
57-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick win补齐 mock YAML 到新增输入字段的同步。
这些输入在
mock模式下虽然都make_and_bind_directly()了，但Context::from()仍只同步旧的 5 个键。结果就是本次新增的 blackboard 字段在 mock 路径里会一直回落到0/UNKNOWN，看起来“接上了”但实际上拿不到数据。建议修改
auto Context::from(const std::string& raw) noexcept -> std::expected<void, std::string> { try { auto root = YAML::Load(raw); if (!root.IsMap()) return std::unexpected{"context yaml root must be a map"}; try_sync(game_stage, root, "game_stage"); + try_sync(stage_remain_time, root, "stage_remain_time"); + try_sync(sync_timestamp, root, "sync_timestamp"); + try_sync(robot_id, root, "robot_id"); try_sync(robot_health, root, "robot_health"); try_sync(robot_bullet, root, "robot_bullet"); + try_sync(robots_hp, root, "robots_hp"); + try_sync(switch_right, root, "switch_right"); + try_sync(switch_left, root, "switch_left"); try_sync(red_score, root, "red_score"); try_sync(blue_score, root, "blue_score"); + + // 其余本次新增、且会被 lua_sync()/logger 消费的字段也需要按同样方式补齐。 return {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/context.cc` around lines 57 - 128, The mock YAML synchronization in Context::from() is missing the newly added blackboard inputs so fields created via make_input (e.g., "/referee/game/stage", "/referee/shooter/cooling", "/referee/ally/hero_hp", "/remote/switch/right", etc.) stay at 0/UNKNOWN in mock mode; update Context::from() to include all new keys in the mock sync list (the same keys passed to make_input/make_and_bind_directly) so the mock YAML populates those fields (add the new "/referee/..." and "/remote/..." entries to the existing mock sync array/map used by Context::from()).src/cxx/sim_sidecar.cc-417-424 (1)
417-424:⚠️ Potential issue | 🟠 Major | ⚡ Quick win补齐
user.gold的状态通路。Godot 端在
sim.input里持续发送user.gold，但这里的状态模型、入站解析和写回 Lua 黑板都没有这个字段。结果就是 Lua 一直看到缺失/陈旧的金币值，任何依赖金币的决策都会和场景实际状态脱节。建议补丁
struct User { double chassis_power_limit = 0.0; double x = 0.0; double y = 0.0; double yaw = 0.0; std::optional<double> health; std::optional<double> bullet; + std::optional<double> gold; };if (state.user.health) { user["health"] = *state.user.health; } if (state.user.bullet) { user["bullet"] = *state.user.bullet; } + if (state.user.gold) { + user["gold"] = *state.user.gold; + }if (user["health"] && !user["health"].IsNull()) { state.user.health = parse_double(user["health"]); } if (user["bullet"] && !user["bullet"].IsNull()) { state.user.bullet = parse_double(user["bullet"]); } + if (user["gold"] && !user["gold"].IsNull()) { + state.user.gold = parse_double(user["gold"]); + }Also applies to: 878-901, 1113-1145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/sim_sidecar.cc` around lines 417 - 424, The User struct and the code paths that parse sim.input and write to the Lua blackboard are missing the gold field; add a double gold (or std::optional<double> gold if optional) to struct User, update the input parsing logic that constructs/updates User (the sim.input parsing function/block) to read user.gold from incoming messages, and ensure the Lua blackboard write/update function (the code that pushes user state back into Lua) includes the gold field so Lua sees current values. Also propagate gold in any intermediate state-update or serialization/deserialization helpers that reference User.godot-mock/scene/sim_sidecar_client.gd-127-130 (1)
127-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick win断线后这里不会再发起连接。
connect_to_host()只在_ready()调用了一次。服务端晚于 Godot 启动，或 sidecar 中途重启后，这里只会清空本地状态并return，客户端会一直停在离线状态，除非重载场景。Also applies to: 177-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/sim_sidecar_client.gd` around lines 127 - 130, connect_to_host() is only called once in _ready(), so if the server starts later or the sidecar restarts the client never reconnects; extract the connect logic into a reusable function (e.g., _attempt_connect or _connect_to_server) that calls tcp.connect_to_host(host, port), and replace the one-off call in _ready() with a call to that helper; on failure or in your TCP-disconnected handler (e.g., _on_tcp_disconnected or wherever you clear local state) schedule repeated retries using a timer or get_tree().create_timer(RECONNECT_DELAY) and call the helper again until success; apply the same retry hook to the other connection block you noted (the logic around lines 177-204) so both initial connect attempts and disconnect events trigger reconnection attempts.godot-mock/scene/character_body_3d.gd-207-220 (1)
207-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick win把黑板同步到
0 HP时也要进入死亡态。这里仅更新了
hp，没有同步更新is_dead。如果 UI/Lua 把血量改成 0，这个角色仍然会继续移动、导航和开火，只是is_alive()变成了false。建议补丁
func sync_resources_from_blackboard( health: Variant, bullet_value: Variant, gold_value: Variant = null ) -> void: if health != null: hp = clampi(int(round(float(health))), 0, max_health) + if hp <= 0 and not is_dead: + _enter_death_state() + return if bullet_value != null: bullet = maxi(0, int(round(float(bullet_value)))) if gold_value != null: gold = maxi(0, int(round(float(gold_value)))) _update_health_bar()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/character_body_3d.gd` around lines 207 - 220, The sync_resources_from_blackboard function updates hp but doesn't set the death state; modify sync_resources_from_blackboard so after clamping/setting hp it also updates the character's death flag/state (e.g., set is_dead = true or call the existing death-handling method) when hp <= 0, and ensure you still call _update_health_bar() — reference sync_resources_from_blackboard, is_dead (or the class's death method), is_alive(), and _update_health_bar() to locate where to add this check.godot-mock/scene/enemy.gd-172-196 (1)
172-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick win上帝视角分支把移动轴写反了。
当前这行会把“前后”映射到 X，把“左右”映射到 Z，和上面的注释相反；切到外部相机后，WASD 的移动方向会整体错 90°。
建议补丁
- var desired := Vector3(-input_forward, 0.0, -input_right) + var desired := Vector3(input_right, 0.0, -input_forward)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/scene/enemy.gd` around lines 172 - 196, 在 _compute_desired_planar_movement 中，上帝视角分支把轴映射写反了：当前把前后映射到 X、左右映射到 Z（var desired := Vector3(-input_forward, 0.0, -input_right)），应当使用世界轴把前后映射到 Z、左右映射到 X。修复方法是在该函数中替换该 desired 赋值为把 input_right 放到 X，把 input_forward 放到 Z（保留符号以使正前为 -Z），并保持后续归一化与乘以 move_speed 不变，以与 fpv 分支行为一致。
🟡 Minor comments (5)
src/lua/AGENTS.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win建议将疑问句中的“或者”改为“还是”以符合中文表达习惯。
当前句式是选择问句，用“还是”更自然、专业，可读性更好。
✍️ 建议修改
-每当你想写一个辅助函数时，请仔细斟酌：它的使用范围有多大？是否大到足以变成一个在全项目可见的标准函数？我不能内联逻辑或者使用一个局部的函数来解决吗？ +每当你想写一个辅助函数时，请仔细斟酌：它的使用范围有多大？是否大到足以变成一个在全项目可见的标准函数？我不能内联逻辑还是使用一个局部的函数来解决吗？🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/AGENTS.md` at line 7, In AGENTS.md update the sentence "我不能内联逻辑或者使用一个局部的函数来解决吗？" to use the more natural conjunction "还是" instead of "或者" so it reads "我不能内联逻辑还是使用一个局部的函数来解决吗？"; locate this sentence in the AGENTS.md content and replace the single word to improve Chinese readability.src/lua/intent/competion/cruise-to-kill.lua-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win目录名疑似拼写错误：
competion→competition。文件路径
src/lua/intent/competion/cruise-to-kill.lua中competion缺少字母i，可能导致路径与项目其他模块的引用不一致。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/competion/cruise-to-kill.lua` at line 1, The directory name "competion" is misspelled; rename the folder to "competition" and update all module import/require references that point to the old path so they match the new directory name (e.g., any requires that load "cruise-to-kill.lua" or reference the module containing local intent = {} must be updated), then run a quick search for "competion" across the repo to fix any remaining references and verify the module (intent) still loads correctly.src/lua/task/switch-mode.lua-6-6 (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win移除冗余的类型验证
第 6 行的类型断言
assert(type(mode) == "string", ...)与第 3 行的类型注解---@parammode string重复。根据编码规范，不应为类型注解已经记录的函数参数添加冗余验证检查，应信任调用者传递匹配文档类型的参数。🔧 建议的修复
--- `@param` mode string --- `@return` boolean is_success return function(mode) - assert(type(mode) == "string", "mode should be a string") action:update_chassis_mode(mode) return true endAs per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations - trust that callers pass parameters matching the documented types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/switch-mode.lua` at line 6, 移除冗余的运行时类型断言：删除那行 assert(type(mode) == "string", "mode should be a string")，因为函数已经用注释类型标注了参数（--- `@param` mode string），应信任调用者并保留静态/文档类型注释；只保留或补充必要的逻辑校验（如果有语义性约束而非类型约束，则在函数体内检查），确保无需其他更改的情况下删除该 assert 语句即可通过现有调用点。src/lua/blackboard.lua-186-188 (1)
186-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win拼写错误：
oupost_survival应为outpost_survival函数名中少了字母
t，若外部调用方按照正确拼写（outpost_survival）查找该条件谓词，将导致nil调用错误。🐛 建议修复
- oupost_survival = function() + outpost_survival = function()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/blackboard.lua` around lines 186 - 188, Rename the misspelled predicate function oupost_survival to outpost_survival so external callers using the correct spelling won't get a nil-call; update the function definition (oupost_survival -> outpost_survival) and search & replace any internal references to oupost_survival (calls, table keys, registrations) to the new outpost_survival identifier to keep behavior identical.src/lua/rmuc_map.lua-28-28 (1)
28-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
RegionName拼写错误："thems_road_begin"应为"them_road_begin"所有其他对应条目均使用
them_前缀（them_home、them_fluctuant、them_trapezoidal_highland、them_road_waypoint_1等），此处多了一个s，导致命名不一致，且下游任何依赖此字符串做匹配的代码都会静默失效。🐛 建议修改
- [Region.THEM_ROAD_BEGIN] = "thems_road_begin", + [Region.THEM_ROAD_BEGIN] = "them_road_begin",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/rmuc_map.lua` at line 28, Fix the typo in the Region.THEM_ROAD_BEGIN mapping: change the value string from "thems_road_begin" to "them_road_begin" so it matches the other them_* entries and downstream matchers; locate the entry keyed by Region.THEM_ROAD_BEGIN in rmuc_map.lua and update the string literal accordingly.
🧹 Nitpick comments (19)
src/lua/intent/competion/cruise-to-kill.lua (1)
4-7: 💤 Low value占位字段
a = 1/b = 2缺乏语义。当前字段名无法表达"巡航至击杀"意图的具体含义。建议在合并前替换为具有实际业务语义的字段（例如巡航速度、攻击范围等），避免将无意义的存根代码合入主干。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/competion/cruise-to-kill.lua` around lines 4 - 7, The table assigned to variable details currently uses meaningless placeholders a and b; replace them with semantic fields that describe the "cruise-to-kill" intent (for example cruise_speed, attack_range, engagement_timeout, target_priority) and initialize them to sensible defaults or nil as appropriate so the intent data is self-describing; update any code that reads details.a/details.b to use the new names (e.g., details.cruise_speed, details.attack_range) and ensure types/units are documented in comments near the details declaration.godot-mock/models/rmuc.glb.import (1)
40-46: 确认generate/physics: true对复杂网格的性能影响。在
PATH:RMUC2026上启用自动物理生成后，Godot 会直接从模型几何体生成碰撞形状。若rmuc.glb为高精度场地模型（面数较多），自动生成的 trimesh 碰撞体代价较高，可能影响仿真帧率。建议评估场地模型的实际面数，必要时在 Blender 中手动简化碰撞网格或使用低精度代理碰撞体（collision proxy），以在物理精度与性能之间取得平衡。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@godot-mock/models/rmuc.glb.import` around lines 40 - 46, 在 PATH:RMUC2026 的子资源中发现了 generate/physics: true，会让 Godot 从模型几何体直接生成高精度 trimesh 碰撞体，若 rmuc.glb 为高面数场景会严重影响性能；为修复，先评估该模型面数并在 Blender 中为场景生成一个低面数的碰撞代理（或手动简化/烘焙碰撞网格），然后在 rmuc.glb 的 PATH:RMUC2026 子资源中将 generate/physics 设为 false（或移除该键）并替换为引用简化后的碰撞子网格/代理，从而在保留物理行为同时显著降低仿真开销。src/lua/task/supply/supply-ammunition.lua (1)
10-13: ⚡ Quick win将校验逻辑移至模块初始化阶段
当前
assert位于任务函数内部，每次任务执行时均重复校验，违反了编码规范中"在初始化阶段进行全面验证而非运行时检查"的要求。由于blackboard.singleton()已在模块加载时调用（第 1 行），可将捕获和断言直接提至模块顶层，使其在模块加载时一次性触发。♻️ 建议重构
local blackboard = require("blackboard").singleton() local request = require("util.scheduler").request local action = require("action") local POLL_INTERVAL = 0.2 +local condition = blackboard.condition +local bullet_ready = blackboard.rule.bullet_ready +assert(type(condition.bullet_ready) == "function", "blackboard.condition.bullet_ready should be a function") +assert(type(bullet_ready) == "number", "blackboard.rule.bullet_ready should be a number") + --- 在补给点等待，直到弹量达到 ready 阈值。 --- `@return` boolean is_success return function() - local condition = blackboard.condition - local bullet_ready = blackboard.rule.bullet_ready - assert(type(condition.bullet_ready) == "function", "blackboard.condition.bullet_ready should be a function") - assert(type(bullet_ready) == "number", "blackboard.rule.bullet_ready should be a number") - if condition.bullet_ready() thenAs per coding guidelines: "Perform comprehensive validation during initialization rather than runtime checks - validate all configuration parameters and constructor arguments upfront."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/supply/supply-ammunition.lua` around lines 10 - 13, Move the two runtime asserts out of the task function into module initialization immediately after calling blackboard.singleton(): capture local condition = blackboard.condition and local bullet_ready = blackboard.rule.bullet_ready at the top-level and perform assert(type(condition.bullet_ready) == "function", ...) and assert(type(bullet_ready) == "number", ...) there so validation runs once during module load; keep the same local names so the task function can use condition and bullet_ready without re-checking.src/lua/task/cross-road/cross-road-zone.lua (1)
16-17: ⚡ Quick win违反编码规范：
ours_zone和forward_center的类型断言与类型注解重复两个参数均已通过
@param注解声明为boolean，根据编码规范无需添加额外的类型校验。♻️ 建议修改
return function(ours_zone, forward_center) - assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") - assert(type(forward_center) == "boolean", "forward_center should be a boolean") action:info("开始cross-road-zone")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations — trust that callers pass parameters matching the documented types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/cross-road/cross-road-zone.lua` around lines 16 - 17, Remove the redundant runtime type assertions for the parameters named ours_zone and forward_center (the two assert(...) lines) since their types are already declared via `@param` annotations; delete those assert calls from the function that uses ours_zone and forward_center, ensure no subsequent logic depends on assertion side-effects, and run unit/static checks to confirm no regressions.src/lua/task/cross-fluctuant/cross-fluctuant-road.lua (1)
9-10: ⚡ Quick win违反编码规范：
ours_zone和forward_center的类型断言与类型注解重复两个参数均已通过
@param注解声明为boolean，根据编码规范无需添加额外的类型校验。♻️ 建议修改
return function(ours_zone, forward_center) - assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") - assert(type(forward_center) == "boolean", "forward_center should be a boolean") action:info("开始cross-fluctuant-road")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations — trust that callers pass parameters matching the documented types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/cross-fluctuant/cross-fluctuant-road.lua` around lines 9 - 10, Remove the redundant runtime type assertions for the boolean parameters by deleting the assert calls that check type(ours_zone) and type(forward_center); trust the `@param` annotations and keep the function signature and any other logic intact, ensuring no other code depends on those assert side-effects.src/lua/task/cross-fluctuant/navigate-to-fluctuant-begin.lua (1)
16-20: ⚡ Quick win违反编码规范：
use_begin的类型断言恒为真，且两处断言均与类型注解重复
- 第 16 行：
ours_zone已通过@param ours_zone boolean注解声明，类型断言多余。- 第 20 行：在执行到此处时，
use_begin必然是boolean——若其为nil则已在第 17–19 行被替换为true，因此该assert恒真，永远不会触发，既无意义也违反编码规范。♻️ 建议修改
return function(ours_zone, use_begin) - assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") if use_begin == nil then use_begin = true end - assert(type(use_begin) == "boolean", "use_begin should be a boolean")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations — trust that callers pass parameters matching the documented types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/cross-fluctuant/navigate-to-fluctuant-begin.lua` around lines 16 - 20, 删除冗余的类型断言：移除对参数 `ours_zone` 的 assert(type(ours_zone) == "boolean", ...) 和在默认化 `use_begin` 之后的第二个 assert(type(use_begin) == "boolean", ...)，保留现有的默认赋值逻辑 (if use_begin == nil then use_begin = true end) 以简化代码并遵循注解信任原则；定位符号为 `ours_zone` 和 `use_begin` 可用于找到并删除相应断言。src/lua/task/navigate-to-point.lua (1)
11-11: ⚡ Quick win违反编码规范：对有类型注解的参数添加了冗余的类型断言
以下类型校验与已有的类型注解重复，根据编码规范应予以删除，仅保留边界条件检查（第 15、20、25 行的非负校验）：
- 第 11 行：执行
options = options or {}之后，options必定是 table，后续的assert(type(options) == "table", ...)对nil入参而言恒真；而options的注解已声明为NavigateToPointOptions?，无需再做类型校验。- 第 18–19 行：
tolerance.x/tolerance.y的类型已通过@field tolerance? number|{x: number, y: number}覆盖，断言多余。- 第 24 行：
timeout注解为number，assert(type(timeout) == "number", ...)与注解重复。- 第 35–37 行：
point注解为{x: number, y: number}，三条类型断言均为冗余。♻️ 建议修改
local function normalize_options(options) options = options or {} - assert(type(options) == "table", "options should be a table") local tolerance = options.tolerance or 0.1 if type(tolerance) == "number" then assert(tolerance >= 0, "tolerance should be non-negative") else - assert(type(tolerance) == "table", "tolerance should be number or {x, y}") - assert(type(tolerance.x) == "number", "tolerance.x should be a number") - assert(type(tolerance.y) == "number", "tolerance.y should be a number") assert(tolerance.x >= 0 and tolerance.y >= 0, "tolerance.{x,y} should be non-negative") end local timeout = options.timeout or 10 - assert(type(timeout) == "number", "timeout should be a number") assert(timeout >= 0, "timeout should be non-negative") return tolerance, timeout end return function(point, options) - assert(type(point) == "table", "point should be a table") - assert(type(point.x) == "number", "point.x should be a number") - assert(type(point.y) == "number", "point.y should be a number") action:info("开始navigate-to-point")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations — trust that callers pass parameters matching the documented types. Only add additional checks for boundary conditions that annotations cannot cover."
Also applies to: 18-19, 24-24, 35-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/navigate-to-point.lua` at line 11, Remove the redundant runtime type assertions that duplicate the function's type annotations in navigate-to-point.lua: delete the assert checking options is a table (the options = options or {} line and the NavigateToPointOptions? annotation make this redundant), remove the asserts on tolerance.x / tolerance.y and the assert on timeout being a number, and remove the three asserts validating point.x / point.y; retain only the boundary checks (non-negative checks) already present (the existing tolerance/timeout/point non-negative validations) so callers are trusted to follow the documented types while still enforcing value boundaries.src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua (2)
18-19: ⚡ Quick win移除冗余的类型断言
ours_zone（@param ours_zone boolean，第 15 行）和switch_interval（@param switch_interval number，第 16 行）均已在注解中声明类型，对应的type()检查与注解重复。第 20 行的> 0边界检查保留即可。♻️ 建议修改
- assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") - assert(type(switch_interval) == "number", "switch_interval should be a number") assert(switch_interval > 0, "switch_interval should be positive")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua` around lines 18 - 19, 移除对函数参数的冗余类型断言：删除对变量 ours_zone 的 assert(type(ours_zone) == "boolean", ...) 与对 switch_interval 的 assert(type(switch_interval) == "number", ...)，保留后续对 switch_interval 边界的检查（> 0）；定位时请查找代码中包含变量名 ours_zone 和 switch_interval 的 assert 调用并删除这些两行断言即可。
78-80: ⚡ Quick win
return true为不可达死代码与
forward-press-in-two-step.lua相同的模式：while true循环体内只能通过return false（第 67 行）退出，第 80 行的return true永远不会被执行。♻️ 建议修改
end - - return true end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua` around lines 78 - 80, 当前在 cruise-in-central-highlands.lua 存在一个 while true 循环，循环内通过 return false （与 forward-press-in-two-step.lua 相同的模式）退出，导致末尾的 return true 永远不可达；请在包含 return false 的 while true 循环中改用 break（或设置一个结果变量）以退出循环，然后在循环外放置 return true，或直接删除该不可达的 return true；定位依据：查找文件内的 while true 循环、内部的 return false（第 ~67 行）和尾部的 return true（第 ~80 行），将逻辑改为在需要结束循环时 break 并在循环外返回 true。src/lua/task/guard-home/cruise-in-front-of-base.lua (2)
19-19: ⚡ Quick win移除冗余的类型断言
ours_zone已通过@param ours_zone boolean（第 16 行）注解声明类型，type(ours_zone) == "boolean"的检查与注解重复。♻️ 建议修改
- assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") action:info("开始cruise-in-front-of-base")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/guard-home/cruise-in-front-of-base.lua` at line 19, Remove the redundant runtime type assertion for the parameter by deleting the assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") check; rely on the existing `@param` ours_zone boolean annotation for documentation and static typing, and ensure no other duplicate runtime checks for ours_zone remain in the function (look for occurrences of "ours_zone" and the assert call).
76-78: ⚡ Quick win
return true为不可达死代码
while true循环体只能通过return false（第 66 行）退出，第 78 行的return true永远不会被执行。♻️ 建议修改
end - - return true end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/guard-home/cruise-in-front-of-base.lua` around lines 76 - 78, 文件中的 while true 循环仅通过内部的 return false（当前为循环退出路径）返回，所以尾部的 return true（当前不可达）永远不会执行；请删除该不可达的 return true，或者如果语义上需要在循环结束后返回成功，则改为在循环内用 break 退出（替换 return false 的非错误路径）并在循环外保留 return true；参考代码中的 while true、内部的 return false 和末尾的 return true 来定位并修正。src/lua/task/forward-press/forward-press-in-two-step.lua (2)
11-11: ⚡ Quick win移除冗余的类型断言
switch_interval已通过@param switch_interval number注解声明类型，type(switch_interval) == "number"与注解重复。正下方第 12 行的边界检查（> 0）是类型注解无法覆盖的，保留即可。♻️ 建议修改
- assert(type(switch_interval) == "number", "switch_interval should be a number") assert(switch_interval > 0, "switch_interval should be positive")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations - trust that callers pass parameters matching the documented types. Only add additional checks for boundary conditions (e.g., index out of range) that annotations cannot cover."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/forward-press/forward-press-in-two-step.lua` at line 11, Remove the redundant type assertion that checks type(switch_interval) == "number" (the assert call referencing switch_interval) and keep the boundary check that enforces switch_interval > 0; simply delete the assert line so only the semantic/boundary validation remains.
58-60: ⚡ Quick win
return true为不可达死代码
while true循环体内仅通过return false（第 48 行）退出，循环本身永不结束，因此第 60 行的return true永远不会被执行。♻️ 建议修改
end - - return true end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/forward-press/forward-press-in-two-step.lua` around lines 58 - 60, The final "return true" is unreachable because the function uses an infinite "while true" that only exits via "return false" inside the loop; either remove the unreachable "return true" or modify the loop so it can break to reach it. Locate the infinite loop (the "while true" block) and the internal "return false" exit, then either (A) delete the trailing "return true" statement, or (B) change the loop to use a conditional break (e.g., set a flag or use "break" when done) so execution can fall through to and return true; update the function accordingly (the loop construct and the final return) to keep logic consistent.src/lua/task/one-step/go-down-onestep.lua (1)
11-11: ⚡ Quick win移除冗余的类型断言
ours_zone已通过@param ours_zone boolean（第 8 行）注解声明类型，type(ours_zone) == "boolean"的检查与注解重复。♻️ 建议修改
- assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") action:info("开始go-down-onestep")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/task/one-step/go-down-onestep.lua` at line 11, 移除冗余的运行时类型断言：删除在 go-down-onestep.lua 中对变量 ours_zone 的断言语句 (assert(type(ours_zone) == "boolean", "ours_zone should be a boolean"))，因为该参数已由 `@param` ours_zone boolean 注解约束；保留其余逻辑不变，确保没有其他代码依赖该断言的副作用。src/lua/intent/train/cross-road.lua (1)
21-24: ⚡ Quick win为
forward_center设置默认值违反了"校验失败即报错"原则，且会修改调用方的args表存在两个问题：
- 当
args.forward_center == nil时，当前代码写入args.forward_center = true并继续执行，相当于使用默认值而非报错，这会掩盖调用方漏传该参数的 bug。- 直接写入
args.forward_center会改变调用方传入的表，产生不预期的副作用。♻️ 建议修改
- if args.forward_center == nil then - args.forward_center = true - end assert(type(args.forward_center) == "boolean", "args.forward_center should be a boolean")As per coding guidelines: "If validation fails, raise errors immediately instead of using default values, as using defaults can obscure bugs during debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/train/cross-road.lua` around lines 21 - 24, The code should not mutate args or silently supply a default; remove the assignment args.forward_center = true and instead validate presence and type without modifying args: assert that args.forward_center is not nil (e.g. assert(args.forward_center ~= nil, "args.forward_center is required")), then assert its type is "boolean" (e.g. assert(type(args.forward_center) == "boolean", "args.forward_center should be a boolean")); alternatively capture into a local variable local forward_center = args.forward_center for subsequent use so the original args table is not changed.src/lua/blackboard_sync.lua (1)
66-69: ⚡ Quick win
M.apply校验失败时应立即抛出错误，而非静默返回当
snapshot不是 table 时，当前实现静默返回，这会掩盖调用方传入错误类型的 bug。♻️ 建议修改
function M.apply(snapshot) - if type(snapshot) ~= "table" then - return - end + assert(type(snapshot) == "table", "snapshot should be a table") for _, section in ipairs(sections) doAs per coding guidelines: "If validation fails, raise errors immediately instead of using default values, as using defaults can obscure bugs during debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/blackboard_sync.lua` around lines 66 - 69, 当前 M.apply 中对 snapshot 的类型校验静默返回会掩盖调用方错误；在 M.apply 函数开头（检查表达式 type(snapshot) ~= "table" 的位置）改为抛出错误而不是 return：当 snapshot 不是 table 时调用 error(...) 抛出包含明确上下文的错误信息（例如标注函数名和期望类型），这样调用方能立即发现并定位传参问题。src/lua/intent/train/start-cruise-train.lua (1)
10-10: ⚡ Quick win移除冗余的类型断言
ours_zone已通过@param ours_zone boolean注解声明类型，type(ours_zone) == "boolean"的检查与注解重复。布尔类型没有需要额外边界检查的场景。♻️ 建议修改
- assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") action:info("开始start-cruise-train")As per coding guidelines: "Do not add redundant validation checks for function arguments that are already documented in type annotations - trust that callers pass parameters matching the documented types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/train/start-cruise-train.lua` at line 10, Remove the redundant runtime type assertion for ours_zone in start-cruise-train.lua: delete the assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") line and rely on the existing `@param` ours_zone boolean annotation instead; ensure no other logic depends on the assert (e.g., error handling or early returns) inside the function that would be affected by its removal.src/lua/intent/train/escape-to-home.lua (1)
26-43: 💤 Low value考虑对必需参数强制验证而非使用默认值。
根据编码规范，初始化时应优先进行全面验证并在失败时抛出错误，而非使用默认值，因为默认值可能在调试时掩盖问题。如果
route是调用方必须明确指定的参数，建议移除默认值并强制要求传入。♻️ 可选的修改建议
function M.new(args) args = args or {} assert(type(args) == "table", "args should be a table") + assert(type(args.route) == "string", "args.route should be a string") if args.ours_zone == nil then args.ours_zone = true end assert(type(args.ours_zone) == "boolean", "args.ours_zone should be a boolean") return setmetatable({ ours_zone = args.ours_zone, - route = args.route or "ours_home", + route = args.route, tolerance = args.tolerance or 0.15, timeout = args.timeout or 10.0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/intent/train/escape-to-home.lua` around lines 26 - 43, The constructor M.new currently silently defaults required parameters (notably route) which can hide caller errors; change M.new to validate required fields instead of providing defaults: remove the fallback route = args.route or "ours_home" and add an assertion that args.route exists and is a string (e.g., assert(type(args.route) == "string", "args.route must be a string")), keeping optional defaults only for truly optional fields like tolerance and timeout; update the returned table to use args.route directly and ensure the function references (M.new, route, args) match the diff.src/lua/endpoint/train-decision.lua (1)
147-150: ⚖️ Poor tradeoff队列清理使用
table.remove(queue, 1)效率较低。
table.remove(queue, 1)在移除首元素时需要移动所有后续元素，时间复杂度为 O(n)。虽然max_history = 64较小，影响有限，但如果频繁触发清理，可以考虑使用环形缓冲区或反向填充来优化。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/endpoint/train-decision.lua` around lines 147 - 150, 当前使用 table.remove(queue, 1) 在循环中移除队首效率低（O(n)）；请改为不搬移元素的实现，例如将 queue 设计为环形缓冲或使用 head/tail 索引来标记有效范围（保留变量 like max_history, queue），在入队时写入 tail 并在必要时移动 head 而不是调用 table.remove，或改为从尾部反向填充并用 pop 移除末尾；调整所有对 queue 的读取/遍历逻辑以使用 head/tail 索引以保持语义一致（涉及的符号：queue、max_history、table.remove）。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e9ac2fa-7f77-4251-b87c-20b630d7c33d
⛔ Files ignored due to path filters (8)
godot-mock/material/20260422-192911.pngis excluded by!**/*.pnggodot-mock/material/white_wood.pngis excluded by!**/*.pnggodot-mock/models/icon.svgis excluded by!**/*.svgmaps/map.pngis excluded by!**/*.pngmaps/rmuc.pngis excluded by!**/*.pngmaps/rmuc.region.pngis excluded by!**/*.pngmaps/train_map.pngis excluded by!**/*.pngmaps/train_map.region.pngis excluded by!**/*.png
📒 Files selected for processing (94)
CMakeLists.txtcmake/export.cmakeconfig/motion.yamlgodot-mock/.editorconfiggodot-mock/.gitattributesgodot-mock/.gitignoregodot-mock/material/20260422-192911.png.importgodot-mock/material/Cn_Mat_U_LMSuit_21_body_BIANSE.tresgodot-mock/material/white_wood.png.importgodot-mock/models/Cn_Mat_U_LMSuit_21_body_BIANSE.tresgodot-mock/models/cube.glbgodot-mock/models/cube.glb.importgodot-mock/models/cube.meshgodot-mock/models/cube_material.tresgodot-mock/models/d7a3f20de4e4486182806c40699d26d4.glbgodot-mock/models/d7a3f20de4e4486182806c40699d26d4.glb.importgodot-mock/models/icon.svg.importgodot-mock/models/rmuc.glbgodot-mock/models/rmuc.glb.importgodot-mock/models/white_cube_material.tresgodot-mock/project.godotgodot-mock/scene/base_cube.tscngodot-mock/scene/battlefiled.tscngodot-mock/scene/character_body_3d.gdgodot-mock/scene/character_body_3d.gd.uidgodot-mock/scene/enemy.gdgodot-mock/scene/enemy.gd.uidgodot-mock/scene/outpost_cube.tscngodot-mock/scene/sim_armor.gdgodot-mock/scene/sim_armor.gd.uidgodot-mock/scene/sim_bullet.gdgodot-mock/scene/sim_bullet.gd.uidgodot-mock/scene/sim_sidecar_client.gdgodot-mock/scene/sim_sidecar_client.gd.uidgodot-mock/scene/sim_structure_target.gdgodot-mock/scene/sim_structure_target.gd.uidlaunch/motion.launch.yamllaunch/static.launch.yamlmaps/map.yamlmaps/rmuc.luamaps/rmuc.region.jsonmaps/rmuc.yamlmaps/train_map.luamaps/train_map.region.jsonmaps/train_map.yamlsrc/cxx/component.ccsrc/cxx/context.ccsrc/cxx/context.hhsrc/cxx/sim_sidecar.ccsrc/cxx/util/localization/engine.ccsrc/cxx/util/localization/engine.hhsrc/cxx/util/navigation/navigation.ccsrc/cxx/util/navigation/navigation.hhsrc/lua/AGENTS.mdsrc/lua/api.luasrc/lua/blackboard.luasrc/lua/blackboard_sync.luasrc/lua/endpoint/competition-test.luasrc/lua/endpoint/main.luasrc/lua/endpoint/mock.luasrc/lua/endpoint/test.luasrc/lua/endpoint/train-decision.luasrc/lua/endpoint/train-test.luasrc/lua/intent/competion/cruise-to-kill.luasrc/lua/intent/competion/escape-to-home.luasrc/lua/intent/competion/forward-press.luasrc/lua/intent/competion/guard-home.luasrc/lua/intent/competion/keep-cruise.luasrc/lua/intent/competion/start-cruise.luasrc/lua/intent/escape-to-home.luasrc/lua/intent/train/cross-road.luasrc/lua/intent/train/escape-to-home.luasrc/lua/intent/train/keep-cruise.luasrc/lua/intent/train/start-cruise-train.luasrc/lua/region.luasrc/lua/rmuc_map.luasrc/lua/task/cross-fluctuant/cross-fluctuant-road.luasrc/lua/task/cross-fluctuant/navigate-to-fluctuant-begin.luasrc/lua/task/cross-road/cross-road-zone.luasrc/lua/task/crossing-road-zone.luasrc/lua/task/cruise-in-central-highland/cruise-in-central-highlands.luasrc/lua/task/forward-press/forward-press-in-one-step.luasrc/lua/task/forward-press/forward-press-in-two-step.luasrc/lua/task/guard-home/cruise-in-front-of-base.luasrc/lua/task/guard-home/occupy-fortress.luasrc/lua/task/navigate-to-point.luasrc/lua/task/one-step/go-down-onestep.luasrc/lua/task/supply/supply-ammunition.luasrc/lua/task/supply/supply-health.luasrc/lua/task/switch-mode.luasrc/lua/train_map.luasrc/lua/util/blackboard_logger.luasrc/lua/util/native.luasrc/lua/util/return-stage.lua
💤 Files with no reviewable changes (1)
- src/lua/task/crossing-road-zone.lua
| std::shared_ptr<Cloud> map; | ||
| std::shared_ptr<Cloud> collected = std::make_shared<Cloud>(); | ||
|
|
||
| explicit Context(Config& config) { | ||
| const auto& filename = config.map_filename; | ||
| if (pcl::io::loadPCDFile<Point>(filename, *map) != 0) | ||
| throw std::runtime_error{"Couldn't read " + filename}; |
There was a problem hiding this comment.
严重错误：map 未初始化就被解引用，会导致崩溃。
map 声明为 std::shared_ptr<Cloud> 但未初始化（默认为 nullptr），第 31 行 *map 解引用空指针会导致未定义行为。
🐛 修复建议
- std::shared_ptr<Cloud> map;
+ std::shared_ptr<Cloud> map = std::make_shared<Cloud>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cxx/util/localization/engine.cc` around lines 26 - 32, The Context
constructor is dereferencing the uninitialized shared_ptr map (causing UB);
before calling pcl::io::loadPCDFile in Context(Config&), initialize map (e.g.,
set map = std::make_shared<Cloud>() or reuse the already-initialized collected)
so that *map is valid, then call pcl::io::loadPCDFile<Point>(filename, *map) and
keep the existing error throw behavior if load fails; update references to map
in the Context class accordingly.
| explicit Impl(Localization::Config config) | ||
| : config{std::move(config)} | ||
| , logging{config.rclcpp} | ||
| , context{config} { |
There was a problem hiding this comment.
Impl 构造函数中 config 已被移动，后续访问可能是未定义行为。
第 51 行 config{std::move(config)} 将参数移动到成员变量，但第 52 行 logging{config.rclcpp} 和第 53 行 context{config} 仍在使用原参数 config，此时参数已处于 moved-from 状态。
🐛 修复建议：调整初始化顺序
explicit Impl(Localization::Config config)
- : config{std::move(config)}
- , logging{config.rclcpp}
- , context{config} {
+ : config{config}
+ , logging{this->config.rclcpp}
+ , context{this->config} {或者先初始化 logging 和 context，最后再移动 config。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| explicit Impl(Localization::Config config) | |
| : config{std::move(config)} | |
| , logging{config.rclcpp} | |
| , context{config} { | |
| explicit Impl(Localization::Config config) | |
| : config{config} | |
| , logging{this->config.rclcpp} | |
| , context{this->config} { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cxx/util/localization/engine.cc` around lines 50 - 53, The Impl
constructor moves the parameter into the member config before initializing
logging and context, leaving the parameter in a moved-from state when
logging{config.rclcpp} and context{config} run; fix by reordering member
initializers so logging and context are initialized from the incoming parameter
first (e.g., use logging{config.rclcpp} and context{config}), and only then move
the parameter into the member (config{std::move(config)}), or alternatively
accept/construct from a const ref and copy as appropriate; target the
Impl(Localization::Config) initializer list and the members config, logging, and
context.
| auto start_localizing(const Eigen::Isometry3d& initial_solution) | ||
| -> std::future<std::expected<Eigen::Isometry3d, std::string>> { | ||
| std::ignore = this; | ||
| std::ignore = initial_solution; | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
start_localizing 返回默认构造的 std::future 会导致未定义行为。
返回 {} 会创建一个没有共享状态的 std::future，调用方对其调用 .get() 或 .wait() 是未定义行为。即使是 stub 实现，也应返回有效的 future。
🐛 修复建议
auto start_localizing(const Eigen::Isometry3d& initial_solution)
-> std::future<std::expected<Eigen::Isometry3d, std::string>> {
std::ignore = this;
std::ignore = initial_solution;
- return {};
+ std::promise<std::expected<Eigen::Isometry3d, std::string>> promise;
+ promise.set_value(std::unexpected{"not implemented"});
+ return promise.get_future();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto start_localizing(const Eigen::Isometry3d& initial_solution) | |
| -> std::future<std::expected<Eigen::Isometry3d, std::string>> { | |
| std::ignore = this; | |
| std::ignore = initial_solution; | |
| return {}; | |
| } | |
| auto start_localizing(const Eigen::Isometry3d& initial_solution) | |
| -> std::future<std::expected<Eigen::Isometry3d, std::string>> { | |
| std::ignore = this; | |
| std::ignore = initial_solution; | |
| std::promise<std::expected<Eigen::Isometry3d, std::string>> promise; | |
| promise.set_value(std::unexpected{"not implemented"}); | |
| return promise.get_future(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cxx/util/localization/engine.cc` around lines 67 - 72, start_localizing
currently returns a default-constructed std::future (via "{}"), which has no
shared state and causes UB when callers call .get()/.wait(); replace that with a
valid ready future that carries a meaningful std::expected error result.
Concretely, inside start_localizing create a promise or use std::async to
produce a ready std::future<std::expected<Eigen::Isometry3d, std::string>> and
set/return an std::unexpected<std::string> (e.g., "not implemented" or similar)
so callers always receive a valid future; update references to the function
signature and ensure the error string is descriptive.
| gold_coin = 0, -- 队伍剩余金币数 | ||
| exchangeable_ammunition_quantity = 0, -- 队伍 17mm 允许发弹量的剩余可兑换数 | ||
|
|
||
| our_dart_nmber_of_hits = false, -- 己方飞镖击中次数 |
There was a problem hiding this comment.
dart_hit_first_time 永远无法返回 true——our_dart_nmber_of_hits 初始化类型错误
game.our_dart_nmber_of_hits 在第 51 行被初始化为布尔值 false，但 dart_hit_first_time 使用整数相等性来检测 0 → 1 的转变：
local triggered = last_our_dart_nmber_of_hits == 0 and current == 1在 Lua 中，如果两个值类型不同，它们会被视为不相等。因此 false == 0 的结果恒为 false，完整的调用链如下：
- 首次调用：
last_our_dart_nmber_of_hits = nil→ 将last置为false（取自初始值），返回false。 - 后续所有调用：
triggered = false == 0 and current == 1→false and X→ 始终为false，即使 C++ 同步后current已变为1，飞镖首次击中事件也永远无法被检测到。
将初始值改为 0 即可修复：
🐛 建议修复
- our_dart_nmber_of_hits = false, -- 己方飞镖击中次数
+ our_dart_nmber_of_hits = 0, -- 己方飞镖击中次数Also applies to: 190-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lua/blackboard.lua` at line 51, game.our_dart_nmber_of_hits is
initialized to a boolean false but dart_hit_first_time compares integer values
(e.g., last_our_dart_nmber_of_hits == 0 and current == 1), so change the
initializers from false to numeric 0 for game.our_dart_nmber_of_hits (and any
similar fields referenced in the dart_hit_first_time logic in the same file,
e.g., the variables touched around lines 190-201) so the equality checks work as
intended; update the initial assignment(s) where game.our_dart_nmber_of_hits
(and the analogous opponent or related hit counters) are set to ensure they are
integers (0) rather than booleans.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/cxx/sim_sidecar.cc (1)
1079-1100: ⚡ Quick win这一段新增变量的构造风格建议统一成仓库约定。
这里混用了前置类型声明和
=初始化；既然仓库要求新增 C++ 代码使用 brace initialization + postfix types，建议把这一段一起统一，避免同文件继续混用两套风格。As per coding guidelines, "Use brace initialization and postfix types for variable construction in C++:
auto var = T { };".🤖 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 `@src/cxx/sim_sidecar.cc` around lines 1079 - 1100, The create_server_socket function mixes pre-declaration and = initialization styles (e.g., "auto hints = addrinfo{};" vs "auto service = std::to_string(port);" vs "addrinfo* result = nullptr;"), violating the repo rule to use brace initialization with postfix types; update all local variable constructions in this function (hints, service, result, cleanup, fd and any other newly added locals) to the mandated style (auto var = T { ... }; or auto var = T{...}; with postfix types like ScopedFd) so the file uses a single consistent brace-initialization + postfix-type pattern throughout.godot-mock/scene/sim_structure_target.gd (1)
35-37: 💤 Low value建议缓存
SimSidecarClient引用，避免每帧场景树查找
_process每帧都会调用_refresh_health_display()→_current_health()→_get_sim_sidecar_client()，每次都做一次get_tree().current_scene.get_node_or_null("SimSidecarClient")。当场景中存在多个sim_structure_target（基地 + 前哨站 + 潜在的多个目标）时，会产生不必要的逐帧字符串名称查找开销。可在
_ready()中解析一次并缓存；若需应对场景切换或客户端节点动态创建，可在缓存失效时再重新解析。♻️ 建议重构
var hp_label: Label3D = null +var _cached_sidecar_client: Node = null @@ func _get_sim_sidecar_client() -> Node: + if _cached_sidecar_client != null and is_instance_valid(_cached_sidecar_client): + return _cached_sidecar_client var scene_root := get_tree().current_scene if scene_root == null: return null - return scene_root.get_node_or_null("SimSidecarClient") + _cached_sidecar_client = scene_root.get_node_or_null("SimSidecarClient") + return _cached_sidecar_clientAlso applies to: 60-71
🤖 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 `@godot-mock/scene/sim_structure_target.gd` around lines 35 - 37, The code currently calls get_tree().current_scene.get_node_or_null("SimSidecarClient") every frame via _process -> _refresh_health_display -> _current_health -> _get_sim_sidecar_client; cache the SimSidecarClient node reference once (e.g. in _ready()) and use that cached reference in _get_sim_sidecar_client to avoid repeated string lookups, and add a null-check that re-resolves the cache if the cached reference is invalid (to handle scene switches or dynamic creation). Update _ready(), _get_sim_sidecar_client(), and any callers (_current_health, _refresh_health_display, _process) to use the cached variable (e.g. sim_sidecar_client) instead of fetching by name each time.
🤖 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 `@godot-mock/scene/sim_structure_hitbox.gd`:
- Around line 30-36: The on_bullet_hit function is passing a hardcoded 100 to
owner_target.call("apply_structure_damage") instead of the incoming damage
parameter; update the call in on_bullet_hit(damage: int, from_team: String) so
it forwards the damage argument (cast to the expected type if necessary) to
owner_target.call("apply_structure_damage", damage) and keep returning the bool
conversion of the call result; locate this in the function on_bullet_hit and
replace the literal 100 with the damage parameter.
In `@godot-mock/scene/sim_structure_target.gd`:
- Around line 182-190: In _face_health_display_to_camera(), avoid calling
health_bar_anchor.look_at when the camera is almost directly above/below the
anchor by computing dir := camera.global_position -
health_bar_anchor.global_position, then compute horizontal_dir := dir -
dir.project(Vector3.UP) and check if horizontal_dir.length() is below a small
epsilon (e.g. 0.001); if it is, skip the look_at update (or perform a Y-only yaw
by using health_bar_anchor.look_at(health_bar_anchor.global_position +
horizontal_dir, Vector3.UP, true) when horizontal_dir is valid). Add the epsilon
constant and use health_bar_anchor and camera.global_position as the referenced
symbols to locate the change.
In `@src/cxx/sim_sidecar.cc`:
- Around line 286-297: send_line currently blocks on ::send until all bytes are
written; change it to respect send timeouts and retry on EINTR by either setting
a per-socket SO_SNDTIMEO (via setsockopt) before the loop or by waiting for
POLLOUT with ::poll/ppoll and a configured timeout before calling ::send; treat
written == -1 with errno == EINTR as retry (continue) and treat
EAGAIN/EWOULDBLOCK or poll timeout as a send timeout error that throws a
descriptive std::runtime_error using client fd context; update the send_line(int
fd, std::string_view payload) implementation to perform these checks and to
clear/reset any socket option if you set it locally.
- Around line 1515-1528: In handle_client(), the poll loop only handles POLLIN
and ignores POLLHUP/POLLERR which causes busy-looping when the peer disconnects
before sending data; modify the poll result handling after checking fd.revents
to also detect ((fd.revents & (POLLHUP|POLLERR)) != 0) and immediately
return/close the client (same behavior as when recv returns <= 0), ensuring you
reference the existing variables fd and client_fd and keep the existing
recv-based exit path.
- Around line 1530-1538: The code currently appends incoming chunks into
input_buffer without any size limit; add a hard cap (e.g., MAX_INPUT_BUFFER and
MAX_LINE_LENGTH constants) and enforce them around the append/parse logic in the
receive loop: before input_buffer.append(...) check if input_buffer.size() +
recv_size would exceed MAX_INPUT_BUFFER and if so either drop the connection,
log and clear the buffer, or truncate the incoming chunk; when extracting a line
(using input_buffer.substr/... and trim) also enforce MAX_LINE_LENGTH and if a
single frame exceeds that limit handle it similarly (reject/truncate/log) to
avoid unbounded memory growth; update the loop that uses input_buffer,
trim(...), and recv_size to apply these checks.
---
Nitpick comments:
In `@godot-mock/scene/sim_structure_target.gd`:
- Around line 35-37: The code currently calls
get_tree().current_scene.get_node_or_null("SimSidecarClient") every frame via
_process -> _refresh_health_display -> _current_health ->
_get_sim_sidecar_client; cache the SimSidecarClient node reference once (e.g. in
_ready()) and use that cached reference in _get_sim_sidecar_client to avoid
repeated string lookups, and add a null-check that re-resolves the cache if the
cached reference is invalid (to handle scene switches or dynamic creation).
Update _ready(), _get_sim_sidecar_client(), and any callers (_current_health,
_refresh_health_display, _process) to use the cached variable (e.g.
sim_sidecar_client) instead of fetching by name each time.
In `@src/cxx/sim_sidecar.cc`:
- Around line 1079-1100: The create_server_socket function mixes pre-declaration
and = initialization styles (e.g., "auto hints = addrinfo{};" vs "auto service =
std::to_string(port);" vs "addrinfo* result = nullptr;"), violating the repo
rule to use brace initialization with postfix types; update all local variable
constructions in this function (hints, service, result, cleanup, fd and any
other newly added locals) to the mandated style (auto var = T { ... }; or auto
var = T{...}; with postfix types like ScopedFd) so the file uses a single
consistent brace-initialization + postfix-type pattern throughout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffc0b3a7-8234-44a9-a266-e988e8727bee
📒 Files selected for processing (8)
godot-mock/scene/battlefiled.tscngodot-mock/scene/sim_sidecar_client.gdgodot-mock/scene/sim_structure_hitbox.gdgodot-mock/scene/sim_structure_hitbox.gd.uidgodot-mock/scene/sim_structure_target.gdmaps/rmuc.luasrc/cxx/sim_sidecar.ccsrc/lua/endpoint/competition-test.lua
✅ Files skipped from review due to trivial changes (1)
- godot-mock/scene/sim_structure_hitbox.gd.uid
🚧 Files skipped from review as they are similar to previous changes (3)
- godot-mock/scene/battlefiled.tscn
- godot-mock/scene/sim_sidecar_client.gd
- src/lua/endpoint/competition-test.lua
| func on_bullet_hit(damage: int, from_team: String) -> bool: | ||
| if from_team == team: | ||
| return false | ||
|
|
||
| if owner_target != null and owner_target.has_method("apply_structure_damage"): | ||
| return bool(owner_target.call("apply_structure_damage", 100)) | ||
| return false |
There was a problem hiding this comment.
严重 Bug：硬编码伤害值 100，丢失了入参 damage
第 35 行直接传入字面量 100 给 apply_structure_damage，完全忽略了函数参数 damage。这会导致：
- 子弹的实际伤害值（来自
sim_bullet.gd的on_bullet_hit(damage, team)调用）被丢弃； - 所有命中都按
100点伤害结算，建筑血量曲线与子弹设定脱钩； - 若有不同口径子弹/不同伤害类型，全部失效。
🐛 建议修复
if owner_target != null and owner_target.has_method("apply_structure_damage"):
- return bool(owner_target.call("apply_structure_damage", 100))
+ return bool(owner_target.call("apply_structure_damage", damage))
return false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func on_bullet_hit(damage: int, from_team: String) -> bool: | |
| if from_team == team: | |
| return false | |
| if owner_target != null and owner_target.has_method("apply_structure_damage"): | |
| return bool(owner_target.call("apply_structure_damage", 100)) | |
| return false | |
| func on_bullet_hit(damage: int, from_team: String) -> bool: | |
| if from_team == team: | |
| return false | |
| if owner_target != null and owner_target.has_method("apply_structure_damage"): | |
| return bool(owner_target.call("apply_structure_damage", damage)) | |
| return false |
🤖 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 `@godot-mock/scene/sim_structure_hitbox.gd` around lines 30 - 36, The
on_bullet_hit function is passing a hardcoded 100 to
owner_target.call("apply_structure_damage") instead of the incoming damage
parameter; update the call in on_bullet_hit(damage: int, from_team: String) so
it forwards the damage argument (cast to the expected type if necessary) to
owner_target.call("apply_structure_damage", damage) and keep returning the bool
conversion of the call result; locate this in the function on_bullet_hit and
replace the literal 100 with the damage parameter.
| func _face_health_display_to_camera() -> void: | ||
| if health_bar_anchor == null: | ||
| return | ||
|
|
||
| var camera := get_viewport().get_camera_3d() | ||
| if camera == null: | ||
| return | ||
|
|
||
| health_bar_anchor.look_at(camera.global_position, Vector3.UP, true) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Godot 4 Node3D look_at error when direction aligned with up vector behavior
💡 Result:
In Godot 4, Node3D.look_at/look_at_from_position will error (and/or fail to update) when the look direction becomes parallel to the provided up vector. This happens exactly in the “looking straight up/down” case, because there’s no unique rotation around the up axis when forward and up are aligned. What triggers the failure 1) Node3D.look_at docs explicitly state that the direction from the node position to the target “cannot be parallel to the up vector” [1]. 2) Node3D.look_at_from_position has the same constraint: it fails when p_up.cross(p_target - p_pos) is zero/parallel (and also when origin==target and when up==ZERO) [2]. 3) The engine’s math method Basis::looking_at builds an orthonormal basis where the forward axis points to the target and the up axis points “as close as possible” to your up while staying perpendicular; it cannot construct a valid perpendicular direction when target direction and up are parallel [3]. How to fix/handle the “up-aligned” case A) Detect the singularity yourself and choose a different up (or a different orientation strategy). - A common check is whether forward.cross(up) is ~zero (parallel). If so, switch to an alternative up vector that is not parallel to forward. Example (GDScript): - Let forward_dir be the normalized direction from your node to the target. - If forward_dir.cross(up).is_zero_approx: pick up2 that’s not parallel (e.g. use node’s basis axes or a fixed axis like Vector3.RIGHT depending on which one isn’t parallel). Pseudo-code: - var dir = (target - global_position).normalized - var up = desired_up - if dir.cross(up).is_zero_approx: - up = global_transform.basis.y (or some other axis) OR choose a fallback like Vector3.RIGHT when dir is close to UP/DOWN - look_at(target, up) Why “keeping the last yaw” is needed Even if you avoid the error by picking a fallback up, there is still an inherent ambiguity: when looking straight up/down, rotation around the forward/up axis cannot be determined uniquely from the inputs. Community discussion notes that you must handle this yourself (e.g., preserve previous yaw/rotation state) because look_at has no concept of “keep yaw” [4][5]. B) Use your node/bone’s existing frame as the up reference If your goal is a stable orientation relative to the actor (not world up), compute an “up” from the node’s current basis (or a parent’s basis) so it is unlikely to become perfectly parallel to the target direction in ordinary gameplay. C) Avoid calling look_at in physics/animation paths where transform state isn’t what you think Separate from the up-parallel singularity, note that look_at requires the node to be in the scene tree to work correctly; otherwise it fails and you should use look_at_from_position instead [1]. If you share your exact code snippet (especially how you compute direction and what up you pass), I can suggest a robust up-fallback and/or “preserve yaw” strategy tailored to your coordinate conventions.
Citations:
- 1: https://docs.godotengine.org/cs/4.x/classes/class_node3d.html
- 2: Change behavior of
look_at()andlook_at_from_position()godotengine/godot-proposals#11041 - 3: https://docs.godotengine.org/en/4.4/classes/class%5Fbasis.html
- 4: look_at_from_position: Up vector and direction between node origin and target are aligned, look_at() failed. godotengine/godot#53793
- 5: look_at() can't look up godotengine/godot#57457
🏁 Script executed:
fd -t f "sim_structure_target.gd" -x cat -n {}Repository: Alliance-Algorithm/rmcs-navigation-mock
Length of output: 6849
look_at 在相机与锚点近似垂直对齐时会报错
当相机几乎位于 health_bar_anchor 正上方或正下方时（即相机方向与 Vector3.UP 平行），Godot 4 的 Node3D.look_at 会触发"look_at() failed: The up vector and direction between node origin and target are aligned"错误并跳过更新。由于建筑通常较高，俯视视角或调试镜头容易触发这个退化情况。
建议在调用 look_at 前，将方向向量投影到水平平面，如果水平投影接近零（相机几乎正上下方），则跳过更新，避免调用失败。这样做可以让健康条退化为仅绕 Y 轴朝向相机，同时避免触发 look_at 的奇异点。
🛡️ 建议防御性处理
func _face_health_display_to_camera() -> void:
if health_bar_anchor == null:
return
var camera := get_viewport().get_camera_3d()
if camera == null:
return
- health_bar_anchor.look_at(camera.global_position, Vector3.UP, true)
+ var to_cam := camera.global_position - health_bar_anchor.global_position
+ # 将方向向量投影到水平面，若水平投影近似为零则跳过，避免 look_at 报错
+ to_cam.y = 0.0
+ if to_cam.length_squared() < 0.0001:
+ return
+ health_bar_anchor.look_at(
+ health_bar_anchor.global_position + to_cam.normalized(),
+ Vector3.UP,
+ true
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func _face_health_display_to_camera() -> void: | |
| if health_bar_anchor == null: | |
| return | |
| var camera := get_viewport().get_camera_3d() | |
| if camera == null: | |
| return | |
| health_bar_anchor.look_at(camera.global_position, Vector3.UP, true) | |
| func _face_health_display_to_camera() -> void: | |
| if health_bar_anchor == null: | |
| return | |
| var camera := get_viewport().get_camera_3d() | |
| if camera == null: | |
| return | |
| var to_cam := camera.global_position - health_bar_anchor.global_position | |
| # 将方向向量投影到水平面，若水平投影近似为零则跳过，避免 look_at 报错 | |
| to_cam.y = 0.0 | |
| if to_cam.length_squared() < 0.0001: | |
| return | |
| health_bar_anchor.look_at( | |
| health_bar_anchor.global_position + to_cam.normalized(), | |
| Vector3.UP, | |
| true | |
| ) |
🤖 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 `@godot-mock/scene/sim_structure_target.gd` around lines 182 - 190, In
_face_health_display_to_camera(), avoid calling health_bar_anchor.look_at when
the camera is almost directly above/below the anchor by computing dir :=
camera.global_position - health_bar_anchor.global_position, then compute
horizontal_dir := dir - dir.project(Vector3.UP) and check if
horizontal_dir.length() is below a small epsilon (e.g. 0.001); if it is, skip
the look_at update (or perform a Y-only yaw by using
health_bar_anchor.look_at(health_bar_anchor.global_position + horizontal_dir,
Vector3.UP, true) when horizontal_dir is valid). Add the epsilon constant and
use health_bar_anchor and camera.global_position as the referenced symbols to
locate the change.
| auto send_line(int fd, std::string_view payload) -> void { | ||
| auto packed = std::string{payload}; | ||
| packed.push_back('\n'); | ||
|
|
||
| std::size_t offset = 0; | ||
| while (offset < packed.size()) { | ||
| auto written = ::send(fd, packed.data() + offset, packed.size() - offset, MSG_NOSIGNAL); | ||
| if (written <= 0) { | ||
| throw std::runtime_error( | ||
| std::format("socket send failed: {}", std::strerror(errno))); | ||
| } | ||
| offset += static_cast<std::size_t>(written); |
There was a problem hiding this comment.
给发送路径加超时控制。
这里是阻塞式 send() 循环，但没有任何发送超时或 POLLOUT 控制。客户端一旦停止读取，sidecar 会直接卡在这里，后面的收包、tick 和回到 accept() 都不会再推进。至少要给 client_fd 配发送超时，并顺手把 EINTR 视为可重试。
🤖 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 `@src/cxx/sim_sidecar.cc` around lines 286 - 297, send_line currently blocks on
::send until all bytes are written; change it to respect send timeouts and retry
on EINTR by either setting a per-socket SO_SNDTIMEO (via setsockopt) before the
loop or by waiting for POLLOUT with ::poll/ppoll and a configured timeout before
calling ::send; treat written == -1 with errno == EINTR as retry (continue) and
treat EAGAIN/EWOULDBLOCK or poll timeout as a send timeout error that throws a
descriptive std::runtime_error using client fd context; update the send_line(int
fd, std::string_view payload) implementation to perform these checks and to
clear/reset any socket option if you set it locally.
| auto result = ::poll(&fd, 1, timeout); | ||
| if (result < 0) { | ||
| if (errno == EINTR) { | ||
| continue; | ||
| } | ||
| throw std::runtime_error(std::format("poll failed: {}", std::strerror(errno))); | ||
| } | ||
|
|
||
| if ((fd.revents & POLLIN) != 0) { | ||
| auto chunk = std::array<char, 4096>{}; | ||
| auto recv_size = ::recv(client_fd, chunk.data(), chunk.size(), 0); | ||
| if (recv_size <= 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
断链时要直接退出 handle_client()。
现在只处理了 POLLIN。如果客户端在还没发任何输入前就断开，poll() 会持续立即返回 POLLHUP/POLLERR，这个循环不会退出，还会一直占住唯一的 client 处理路径。
可参考的最小修正
auto result = ::poll(&fd, 1, timeout);
if (result < 0) {
if (errno == EINTR) {
continue;
}
throw std::runtime_error(std::format("poll failed: {}", std::strerror(errno)));
}
+ if ((fd.revents & (POLLERR | POLLHUP | POLLNVAL)) != 0) {
+ return;
+ }
+
if ((fd.revents & POLLIN) != 0) {
auto chunk = std::array<char, 4096>{};
auto recv_size = ::recv(client_fd, chunk.data(), chunk.size(), 0);
if (recv_size <= 0) {
return;🤖 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 `@src/cxx/sim_sidecar.cc` around lines 1515 - 1528, In handle_client(), the
poll loop only handles POLLIN and ignores POLLHUP/POLLERR which causes
busy-looping when the peer disconnects before sending data; modify the poll
result handling after checking fd.revents to also detect ((fd.revents &
(POLLHUP|POLLERR)) != 0) and immediately return/close the client (same behavior
as when recv returns <= 0), ensuring you reference the existing variables fd and
client_fd and keep the existing recv-based exit path.
| input_buffer.append(chunk.data(), static_cast<std::size_t>(recv_size)); | ||
| while (true) { | ||
| auto pos = input_buffer.find('\n'); | ||
| if (pos == std::string::npos) { | ||
| break; | ||
| } | ||
|
|
||
| auto line = trim(input_buffer.substr(0, pos)); | ||
| input_buffer.erase(0, pos + 1); |
There was a problem hiding this comment.
需要限制单条报文/缓冲区大小。
这里按换行分帧，但在收到超长未终止消息时会无限追加到 input_buffer。服务默认监听 0.0.0.0，任意客户端都能用一条不带 \n 的大消息把进程内存吃满。
可参考的最小修正
input_buffer.append(chunk.data(), static_cast<std::size_t>(recv_size));
+ constexpr auto kMaxFrameBytes = std::size_t {1 << 20};
+ if (input_buffer.size() > kMaxFrameBytes) {
+ throw std::runtime_error("incoming frame too large");
+ }
+
while (true) {
auto pos = input_buffer.find('\n');
if (pos == std::string::npos) {
break;
}🤖 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 `@src/cxx/sim_sidecar.cc` around lines 1530 - 1538, The code currently appends
incoming chunks into input_buffer without any size limit; add a hard cap (e.g.,
MAX_INPUT_BUFFER and MAX_LINE_LENGTH constants) and enforce them around the
append/parse logic in the receive loop: before input_buffer.append(...) check if
input_buffer.size() + recv_size would exceed MAX_INPUT_BUFFER and if so either
drop the connection, log and clear the buffer, or truncate the incoming chunk;
when extracting a line (using input_buffer.substr/... and trim) also enforce
MAX_LINE_LENGTH and if a single frame exceeds that limit handle it similarly
(reject/truncate/log) to avoid unbounded memory growth; update the loop that
uses input_buffer, trim(...), and recv_size to apply these checks.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua (1)
18-19: ⚡ Quick win移除与类型注解重复的参数类型断言
Line 18 和 Line 19 的
type(...)断言与---@param已声明类型重复，建议删除这两条防御式类型检查，仅保留 Line 20 的范围校验（switch_interval > 0）。建议修改
return function(ours_zone, switch_interval) - assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") - assert(type(switch_interval) == "number", "switch_interval should be a number") assert(switch_interval > 0, "switch_interval should be positive") action:info("开始cruise-in-central-highlands")As per coding guidelines,
src/lua/**/*.lua: "Do not add defensive checks for function parameters that are already documented with type annotations."🤖 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 `@src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua` around lines 18 - 19, Remove the redundant defensive type assertions assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") and assert(type(switch_interval) == "number", "switch_interval should be a number") since their types are already declared with ---@param annotations; keep the existing range check assert(switch_interval > 0, "switch_interval must be > 0") (or equivalent) to validate value bounds for switch_interval. Ensure only the two assert(type(...)) lines referencing ours_zone and switch_interval are deleted and no other validations are removed.
🤖 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 `@godot-mock/scene/sim_sidecar_client.gd`:
- Around line 1209-1213: The code trusts blackboard.get(...) for nested sections
and may receive non-Dictionary values; replace direct calls to
blackboard.get("user"/"game"/"play") in the UI update path with the defensive
accessor _get_dict_value to ensure each of user, game, and play is a Dictionary
(or a safe default) before using them; update the block that sets ui_updating
and the three vars (ui_updating, user, game, play) to call
_get_dict_value(blackboard, "user") / _get_dict_value(blackboard, "game") /
_get_dict_value(blackboard, "play") so downstream .get(...) calls are safe.
- Around line 1242-1250: _begin_competition_session currently always resets
sim_remaining_time to initial_remaining_time, which causes the clock to restart
if the function is called again; change it so the reset happens only when the
session is starting for the first time (e.g., guard the assignment with a check
like if not competition_started or detect a transition into "STARTED"), then set
competition_started = true and call _apply_local_patch as before; reference
sim_remaining_time, initial_remaining_time, competition_started, and
_apply_local_patch to locate the code to update.
In `@src/lua/task/cross-fluctuant/cross-fluctuant-road.lua`:
- Around line 9-10: Remove the defensive type assertions for the
already-annotated parameters by deleting the assert(type(ours_zone) ==
"boolean", ...) and assert(type(forward_center) == "boolean", ...) statements so
the function relies on the declared type annotations (referencing the parameters
ours_zone and forward_center) instead of redundant runtime checks; ensure no
other logic depends on those asserts and run tests to confirm behavior remains
correct.
In `@src/lua/task/forward-press/forward-press-in-two-step.lua`:
- Line 11: Remove the redundant type assertion assert(type(switch_interval) ==
"number", ...) for the parameter switch_interval (it duplicates the `@param`
switch_interval number annotation); keep the existing positive-bound check that
validates the value (the assert that ensures switch_interval > 0 or similar) so
only the runtime boundary validation remains while the type annotation handles
the type contract.
---
Nitpick comments:
In `@src/lua/task/cruise-in-central-highland/cruise-in-central-highlands.lua`:
- Around line 18-19: Remove the redundant defensive type assertions
assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") and
assert(type(switch_interval) == "number", "switch_interval should be a number")
since their types are already declared with ---@param annotations; keep the
existing range check assert(switch_interval > 0, "switch_interval must be > 0")
(or equivalent) to validate value bounds for switch_interval. Ensure only the
two assert(type(...)) lines referencing ours_zone and switch_interval are
deleted and no other validations are removed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 763b10df-8d6e-4fb8-8f8c-8fe6b47be266
📒 Files selected for processing (11)
godot-mock/scene/sim_sidecar_client.gdsrc/lua/endpoint/competition-test.luasrc/lua/intent/competion/escape-to-home.luasrc/lua/task/cross-fluctuant/cross-fluctuant-road.luasrc/lua/task/cross-fluctuant/navigate-to-fluctuant-begin.luasrc/lua/task/cross-road/cross-road-zone.luasrc/lua/task/cruise-in-central-highland/cruise-in-central-highlands.luasrc/lua/task/forward-press/forward-press-in-one-step.luasrc/lua/task/forward-press/forward-press-in-two-step.luasrc/lua/task/guard-home/cruise-in-front-of-base.luasrc/lua/task/guard-home/occupy-fortress.lua
✅ Files skipped from review due to trivial changes (1)
- src/lua/task/cross-fluctuant/navigate-to-fluctuant-begin.lua
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lua/task/forward-press/forward-press-in-one-step.lua
- src/lua/task/guard-home/cruise-in-front-of-base.lua
- src/lua/task/cross-road/cross-road-zone.lua
- src/lua/task/guard-home/occupy-fortress.lua
- src/lua/endpoint/competition-test.lua
- src/lua/intent/competion/escape-to-home.lua
| ui_updating = true | ||
| var user: Dictionary = blackboard.get("user", {}) | ||
| var game: Dictionary = blackboard.get("game", {}) | ||
| var play: Dictionary = blackboard.get("play", {}) | ||
|
|
There was a problem hiding this comment.
这里不要直接信任 blackboard 的嵌套分节类型。
blackboard 来自 TCP 外部输入，但这里直接用 blackboard.get("user" / "game" / "play") 取值，少了对分节类型的防御。一旦 sidecar 发来 null、字符串，或版本不一致的结构，下面这些 .get(...) 读取就会在 UI 刷新路径里出错。既然本文件已经有 _get_dict_value()，这里也应统一走它。
💡 可参考的修正方式
ui_updating = true
- var user: Dictionary = blackboard.get("user", {})
- var game: Dictionary = blackboard.get("game", {})
- var play: Dictionary = blackboard.get("play", {})
+ var user := _get_dict_value(blackboard, "user")
+ var game := _get_dict_value(blackboard, "game")
+ var play := _get_dict_value(blackboard, "play")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ui_updating = true | |
| var user: Dictionary = blackboard.get("user", {}) | |
| var game: Dictionary = blackboard.get("game", {}) | |
| var play: Dictionary = blackboard.get("play", {}) | |
| ui_updating = true | |
| var user := _get_dict_value(blackboard, "user") | |
| var game := _get_dict_value(blackboard, "game") | |
| var play := _get_dict_value(blackboard, "play") |
🤖 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 `@godot-mock/scene/sim_sidecar_client.gd` around lines 1209 - 1213, The code
trusts blackboard.get(...) for nested sections and may receive non-Dictionary
values; replace direct calls to blackboard.get("user"/"game"/"play") in the UI
update path with the defensive accessor _get_dict_value to ensure each of user,
game, and play is a Dictionary (or a safe default) before using them; update the
block that sets ui_updating and the three vars (ui_updating, user, game, play)
to call _get_dict_value(blackboard, "user") / _get_dict_value(blackboard,
"game") / _get_dict_value(blackboard, "play") so downstream .get(...) calls are
safe.
| func _begin_competition_session() -> void: | ||
| sim_remaining_time = initial_remaining_time | ||
| competition_started = true | ||
| _apply_local_patch({ | ||
| "game": { | ||
| "stage": "STARTED", | ||
| "remaining_time": sim_remaining_time, | ||
| } | ||
| }) |
There was a problem hiding this comment.
避免重复启动时把比赛时钟重置回初始值。
_begin_competition_session() 每次都会把 sim_remaining_time 重新设成 initial_remaining_time。现在它既会被 Start Decision 按钮调用，也会被 Stage 切到 STARTED 时调用；如果比赛进行中再次触发，下一帧 sim.input 就会把 420 秒重新推给 sidecar，覆盖正在运行的倒计时（见 src/cxx/sim_sidecar.cc:1346-1356）。这里应该只在“首次进入 STARTED”时初始化时钟。
💡 可参考的修正方式
func _begin_competition_session() -> void:
- sim_remaining_time = initial_remaining_time
+ if competition_started:
+ return
+
competition_started = true
+ if sim_remaining_time <= 0.0 or sim_remaining_time > initial_remaining_time:
+ sim_remaining_time = initial_remaining_time
_apply_local_patch({
"game": {
"stage": "STARTED",
"remaining_time": sim_remaining_time,
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func _begin_competition_session() -> void: | |
| sim_remaining_time = initial_remaining_time | |
| competition_started = true | |
| _apply_local_patch({ | |
| "game": { | |
| "stage": "STARTED", | |
| "remaining_time": sim_remaining_time, | |
| } | |
| }) | |
| func _begin_competition_session() -> void: | |
| if competition_started: | |
| return | |
| competition_started = true | |
| if sim_remaining_time <= 0.0 or sim_remaining_time > initial_remaining_time: | |
| sim_remaining_time = initial_remaining_time | |
| _apply_local_patch({ | |
| "game": { | |
| "stage": "STARTED", | |
| "remaining_time": sim_remaining_time, | |
| } | |
| }) |
🤖 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 `@godot-mock/scene/sim_sidecar_client.gd` around lines 1242 - 1250,
_begin_competition_session currently always resets sim_remaining_time to
initial_remaining_time, which causes the clock to restart if the function is
called again; change it so the reset happens only when the session is starting
for the first time (e.g., guard the assignment with a check like if not
competition_started or detect a transition into "STARTED"), then set
competition_started = true and call _apply_local_patch as before; reference
sim_remaining_time, initial_remaining_time, competition_started, and
_apply_local_patch to locate the code to update.
| assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") | ||
| assert(type(forward_center) == "boolean", "forward_center should be a boolean") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
移除对已注解参数的防御式类型断言
ours_zone 与 forward_center 已有类型注解，这里的 assert(type(...)) 与仓库 Lua 规范冲突，建议删除并让类型错误自然暴露。
建议修改
return function(ours_zone, forward_center)
- assert(type(ours_zone) == "boolean", "ours_zone should be a boolean")
- assert(type(forward_center) == "boolean", "forward_center should be a boolean")
action:info("开始cross-fluctuant-road")As per coding guidelines "Do not add defensive checks for function parameters that are already documented with type annotations. Trust that parameters passed to functions match their documented types."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(type(ours_zone) == "boolean", "ours_zone should be a boolean") | |
| assert(type(forward_center) == "boolean", "forward_center should be a boolean") | |
| return function(ours_zone, forward_center) | |
| action:info("开始cross-fluctuant-road") |
🤖 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 `@src/lua/task/cross-fluctuant/cross-fluctuant-road.lua` around lines 9 - 10,
Remove the defensive type assertions for the already-annotated parameters by
deleting the assert(type(ours_zone) == "boolean", ...) and
assert(type(forward_center) == "boolean", ...) statements so the function relies
on the declared type annotations (referencing the parameters ours_zone and
forward_center) instead of redundant runtime checks; ensure no other logic
depends on those asserts and run tests to confirm behavior remains correct.
| --- @param switch_interval number 切换周期（秒） | ||
| --- @return boolean is_success | ||
| return function(switch_interval) | ||
| assert(type(switch_interval) == "number", "switch_interval should be a number") |
There was a problem hiding this comment.
移除重复的参数类型防御校验
switch_interval 已通过 ---@param switch_interval number 声明类型，这里的 type(...) 断言与仓库 Lua 规范冲突。建议仅保留 Line 12 的正数边界校验。
建议修改
return function(switch_interval)
- assert(type(switch_interval) == "number", "switch_interval should be a number")
assert(switch_interval > 0, "switch_interval should be positive")
action:info("开始forward-press-in-two-step")As per coding guidelines, “Do not add defensive checks for function parameters that are already documented with type annotations.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(type(switch_interval) == "number", "switch_interval should be a number") | |
| return function(switch_interval) | |
| assert(switch_interval > 0, "switch_interval should be positive") | |
| action:info("开始forward-press-in-two-step") |
🤖 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 `@src/lua/task/forward-press/forward-press-in-two-step.lua` at line 11, Remove
the redundant type assertion assert(type(switch_interval) == "number", ...) for
the parameter switch_interval (it duplicates the `@param` switch_interval number
annotation); keep the existing positive-bound check that validates the value
(the assert that ensures switch_interval > 0 or similar) so only the runtime
boundary validation remains while the type annotation handles the type contract.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
godot-mock/scene/sim_sidecar_client.gd (2)
1392-1400:⚠️ Potential issue | 🟠 Major | ⚡ Quick win不要在重复进入 STARTED 时重置比赛时钟。
_begin_competition_session()现在每次都会把sim_remaining_time设回初始值。它既会被 “Start Decision” 调用，也会被 Stage 切到STARTED调用；比赛进行中再次触发时，会把正在运行的倒计时重置回去。🤖 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 `@godot-mock/scene/sim_sidecar_client.gd` around lines 1392 - 1400, The _begin_competition_session function resets sim_remaining_time unconditionally which causes the running countdown to restart when STARTED is re-entered; change it so sim_remaining_time is only set to initial_remaining_time if the competition is not already started (check competition_started or current game stage), then set competition_started = true and call _apply_local_patch as before; reference symbols: _begin_competition_session, sim_remaining_time, initial_remaining_time, competition_started, and _apply_local_patch.
1334-1337:⚠️ Potential issue | 🟠 Major | ⚡ Quick win继续用
blackboard.get()直取分节会把脏数据带进 UI。
blackboard来自 TCP 输入，这里仍然直接把user/game/play当成Dictionary。只要 sidecar 发来null、字符串或旧版结构，下面这些.get(...)就会在刷新路径里出错。这里已经有_get_dict_value()，建议统一走它。🤖 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 `@godot-mock/scene/sim_sidecar_client.gd` around lines 1334 - 1337, The code pulls user/game/play directly from blackboard using blackboard.get(...) which can bring nulls/strings/old formats into the UI; replace those direct calls with the existing sanitizer helper _get_dict_value so each of user, game, play is fetched via _get_dict_value(blackboard, "user") / _get_dict_value(blackboard, "game") / _get_dict_value(blackboard, "play") (or the actual helper signature if it takes only the key), ensuring a safe Dictionary (defaulting to {}) before any .get(...) usages while keeping ui_updating = true.src/cxx/sim_sidecar.cc (3)
286-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick win给发送路径加超时，并把
EINTR视为可重试。当前
::send()循环会在对端停止读取时一直阻塞，而且errno == EINTR也会被直接当成失败抛出。这个 sidecar 是单连接串行处理的，一旦卡在发送路径，后续收包、tick 和重新accept()都会一起停住。🤖 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 `@src/cxx/sim_sidecar.cc` around lines 286 - 297, In send_line, make the blocking send loop resilient by adding a per-send timeout and treating EINTR as retryable: before each ::send(fd, ...) call use poll()/ppoll() (or select) to wait for the socket to be writable with a configured timeout (e.g. few seconds); if poll times out, throw a timeout error; if poll reports writable then call ::send and if it returns -1 and errno==EINTR simply continue the loop (do not throw), and if errno==EAGAIN/EWOULDBLOCK go back to polling; keep updating offset until all bytes are sent and include descriptive errors on real failures—modify the send_line function to implement this logic.
1573-1581:⚠️ Potential issue | 🟠 Major | ⚡ Quick win给
input_buffer和单帧长度都加硬上限。这里会持续把未换行的数据追加进
input_buffer。服务默认监听0.0.0.0，而且每次只处理一个 client；任意客户端都可以用超长未终止报文把进程内存持续吃大。🤖 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 `@src/cxx/sim_sidecar.cc` around lines 1573 - 1581, input_buffer currently grows unbounded (appended from chunk via recv_size) and a malicious client can exhaust memory; add two hard limits: a MAX_FRAME_BYTES to refuse/trim any single recv_size/frame larger than the limit before append, and a MAX_INPUT_BUFFER_BYTES to cap input_buffer growth—if append would exceed that limit then drop the connection or clear/bail with an error/log. Implement these checks around the append call that uses input_buffer.append(chunk.data(), static_cast<std::size_t>(recv_size)) and in the while loop that processes lines (where trim and substr are used) to ensure any extracted line length is also limited to MAX_FRAME_BYTES before processing.
1558-1566:⚠️ Potential issue | 🟠 Major | ⚡ Quick win收到
POLLHUP/POLLERR/POLLNVAL时应立即退出 client 循环。这里现在只处理了
POLLIN。如果客户端在还没发任何数据前就断开，poll()会持续立即返回挂断/错误事件，这个循环不会退出，还会一直占住唯一的 client 处理路径。🤖 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 `@src/cxx/sim_sidecar.cc` around lines 1558 - 1566, The loop after calling ::poll currently only checks for POLLIN and ignores hangup/error events; update the post-poll handling around the fd.revents check (where POLLIN is tested) to detect if (fd.revents & (POLLHUP | POLLERR | POLLNVAL)) != 0 and immediately break/return from the client loop (or close the client and stop processing) so the client handler does not spin forever when the peer disconnects or an error occurs; make this change in the same block that currently inspects fd.revents and ensure cleanup/closure code runs before exiting.
🤖 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 `@godot-mock/scene/sim_sidecar_client.gd`:
- Around line 1026-1027: The SpinBox for remaining time currently hardcodes its
max to 420.0; change it to use the configurable initial_remaining_time so the UI
max tracks the match total. Locate the call creating remaining_time_spin
(function/variable names: remaining_time_spin and _add_spin_row) and replace the
literal 420.0 with the runtime value initial_remaining_time (or a computed max
based on initial_remaining_time if units differ), and ensure any code that
updates initial_remaining_time also updates remaining_time_spin.max so the
control stays in sync (handlers: where initial_remaining_time is set and
_on_remaining_time_changed).
---
Duplicate comments:
In `@godot-mock/scene/sim_sidecar_client.gd`:
- Around line 1392-1400: The _begin_competition_session function resets
sim_remaining_time unconditionally which causes the running countdown to restart
when STARTED is re-entered; change it so sim_remaining_time is only set to
initial_remaining_time if the competition is not already started (check
competition_started or current game stage), then set competition_started = true
and call _apply_local_patch as before; reference symbols:
_begin_competition_session, sim_remaining_time, initial_remaining_time,
competition_started, and _apply_local_patch.
- Around line 1334-1337: The code pulls user/game/play directly from blackboard
using blackboard.get(...) which can bring nulls/strings/old formats into the UI;
replace those direct calls with the existing sanitizer helper _get_dict_value so
each of user, game, play is fetched via _get_dict_value(blackboard, "user") /
_get_dict_value(blackboard, "game") / _get_dict_value(blackboard, "play") (or
the actual helper signature if it takes only the key), ensuring a safe
Dictionary (defaulting to {}) before any .get(...) usages while keeping
ui_updating = true.
In `@src/cxx/sim_sidecar.cc`:
- Around line 286-297: In send_line, make the blocking send loop resilient by
adding a per-send timeout and treating EINTR as retryable: before each
::send(fd, ...) call use poll()/ppoll() (or select) to wait for the socket to be
writable with a configured timeout (e.g. few seconds); if poll times out, throw
a timeout error; if poll reports writable then call ::send and if it returns -1
and errno==EINTR simply continue the loop (do not throw), and if
errno==EAGAIN/EWOULDBLOCK go back to polling; keep updating offset until all
bytes are sent and include descriptive errors on real failures—modify the
send_line function to implement this logic.
- Around line 1573-1581: input_buffer currently grows unbounded (appended from
chunk via recv_size) and a malicious client can exhaust memory; add two hard
limits: a MAX_FRAME_BYTES to refuse/trim any single recv_size/frame larger than
the limit before append, and a MAX_INPUT_BUFFER_BYTES to cap input_buffer
growth—if append would exceed that limit then drop the connection or clear/bail
with an error/log. Implement these checks around the append call that uses
input_buffer.append(chunk.data(), static_cast<std::size_t>(recv_size)) and in
the while loop that processes lines (where trim and substr are used) to ensure
any extracted line length is also limited to MAX_FRAME_BYTES before processing.
- Around line 1558-1566: The loop after calling ::poll currently only checks for
POLLIN and ignores hangup/error events; update the post-poll handling around the
fd.revents check (where POLLIN is tested) to detect if (fd.revents & (POLLHUP |
POLLERR | POLLNVAL)) != 0 and immediately break/return from the client loop (or
close the client and stop processing) so the client handler does not spin
forever when the peer disconnects or an error occurs; make this change in the
same block that currently inspects fd.revents and ensure cleanup/closure code
runs before exiting.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d887cb19-a1d9-41dd-8603-ae591f154bc4
📒 Files selected for processing (3)
godot-mock/scene/sim_sidecar_client.gdsrc/cxx/sim_sidecar.ccsrc/lua/endpoint/competition-test.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lua/endpoint/competition-test.lua
| remaining_time_spin = _add_spin_row(edit_box, "Remain Time", 0.0, 420.0, 1.0) | ||
| remaining_time_spin.value_changed.connect(_on_remaining_time_changed) |
There was a problem hiding this comment.
让 Remaining Time 的上限跟随 initial_remaining_time。
这里把 SpinBox 上限写死成 420.0，但比赛总时长本身是可配置的。把时长调大后，UI 将无法输入完整范围；调小时，UI 又会允许输入超出实际配置的值。
💡 可参考的最小修正
- remaining_time_spin = _add_spin_row(edit_box, "Remain Time", 0.0, 420.0, 1.0)
+ remaining_time_spin = _add_spin_row(edit_box, "Remain Time", 0.0, initial_remaining_time, 1.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remaining_time_spin = _add_spin_row(edit_box, "Remain Time", 0.0, 420.0, 1.0) | |
| remaining_time_spin.value_changed.connect(_on_remaining_time_changed) | |
| remaining_time_spin = _add_spin_row(edit_box, "Remain Time", 0.0, initial_remaining_time, 1.0) | |
| remaining_time_spin.value_changed.connect(_on_remaining_time_changed) |
🤖 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 `@godot-mock/scene/sim_sidecar_client.gd` around lines 1026 - 1027, The SpinBox
for remaining time currently hardcodes its max to 420.0; change it to use the
configurable initial_remaining_time so the UI max tracks the match total. Locate
the call creating remaining_time_spin (function/variable names:
remaining_time_spin and _add_spin_row) and replace the literal 420.0 with the
runtime value initial_remaining_time (or a computed max based on
initial_remaining_time if units differ), and ensure any code that updates
initial_remaining_time also updates remaining_time_spin.max so the control stays
in sync (handlers: where initial_remaining_time is set and
_on_remaining_time_changed).
3 participants
拉取请求摘要
构建与安装
${RMCS_CORE_SOURCE_DIR}），收窄/重塑了rmcs-navigation共享库的链接依赖（保留 Lua/sol2，新增 sensor_msgs 与 pcl_conversions 等），新增可执行目标${PROJECT_NAME}-sim-sidecar（设置 C++23、添加 RMCS_NAVIGATION_SOURCE_DIR 编译定义、关联 Lua/sol2/yaml-cpp include 与链接）。${PROJECT_NAME}与${PROJECT_NAME}-sim-sidecar。ROS 2 / 启动与地图
static_local_map并将其传入 controller_server 的 local_costmap 配置。static_local_map: true。C++ 后端与工具
Godot 模拟（资源、场景与脚本）
Lua 决策框架扩展
影响与注意事项
该 PR 包含大量新增文件与显著改动，审查工作量高；建议按功能子集（构建/CMake、C++ sim-sidecar、Godot 客户端、Lua 黑板与单个 endpoint、整体端到端）分批进行代码审查与集成测试。