fix: @Bartok9 contributions#824
Conversation
…821) Signed-off-by: Bartok9 <danielrpike9@gmail.com>
Signed-off-by: Bartok9 <danielrpike9@gmail.com>
|
@Bartok9 once again thanks for the contributions. I'm planning to merge this first batch of fixes and then proceed with the other PRs. Would you mind taking a look at CI errors? |
|
Thanks @maciejmajek — happy to help, and glad these are going in! 🙏 I dug into the two CI failures. Both are small and here are the exact fixes: 1. # Copyright (C) 2025 Robotec.AI
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.2. def test_get_future_result_zero_timeout():
- """Test with zero timeout."""
+ """Test that a zero timeout is rejected as non-positive."""
future = Future()
- result = get_future_result(future, timeout_sec=0.0)
- assert result is None
+ with pytest.raises(ValueError, match="timeout_sec must be positive"):
+ get_future_result(future, timeout_sec=0.0)Since the salvage branch lives on |
|
@Bartok9 Please create a PR for these fixes. Thanks! |
|
Done! Opened #826 with both CI fixes (license header + zero-timeout test alignment). I based it on this |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe change adds validation for invalid configuration and timeout values, rejects empty agent collections, normalizes ROS2 entity names, fixes artifact database path usage, improves aggregator edge-case handling, and adds corresponding tests. Package metadata and spelling are also corrected. ChangesValidation and edge-case corrections
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/rai_core/rai/tools/ros2/cli.py (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the annotation with the accepted timeout values.
The validation accepts floats, and the happy-path test passes
1.0, buttimeout_secis annotated asint. Change the annotation toint | floator reject floats so static and runtime contracts agree.🤖 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/rai_core/rai/tools/ros2/cli.py` around lines 25 - 30, Update the timeout_sec annotation in the affected ROS2 CLI function to int | float, matching its existing runtime validation and float-based happy path. Preserve the current rejection of booleans and other non-numeric values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rai_core/rai/agents/langchain/agent.py`:
- Around line 118-119: Update the max_size validation in the surrounding agent
initialization logic to require a positive int that is not a bool, rejecting
values such as True and 1.5 while preserving the existing error for non-positive
values. Add regression tests covering both invalid non-integer cases.
In `@src/rai_core/rai/agents/langchain/state_based_agent.py`:
- Around line 45-50: Update the _time_interval_positive validator to reject any
non-finite value by checking math.isfinite(v) alongside the existing
positive-value condition, while preserving the current error behavior for
invalid intervals. Add a regression test covering NaN input.
In `@src/rai_core/rai/aggregators/base.py`:
- Around line 31-32: Use one non-boolean positive-integer validation contract
for buffer sizes: update the constructor validation in
src/rai_core/rai/aggregators/base.py lines 31-32 to reject booleans and
non-integer max_size values while preserving None as unbounded; apply the same
validation to the bounded buffer-size check in
src/rai_core/rai/agents/langchain/agent.py lines 118-119, explicitly preserving
its existing None sentinel behavior.
In `@src/rai_core/rai/communication/ros2/api/action.py`:
- Around line 155-165: Update timeout validation in action.py lines 155-165 and
ros_async.py lines 29-30 to reject non-finite values at both API entry points,
while preserving rejection of non-positive values and booleans. Use the existing
validation paths and ensure valid positive finite numbers remain accepted.
In `@src/rai_core/rai/communication/ros2/waiters.py`:
- Around line 33-34: Update the timeout-reporting calculation in the waiter flow
to compare normalized entity names consistently, matching the normalized
requested_set and available_set built from ensure_slash. Reuse the normalized
available set when determining missing entities so names available without a
leading slash are not reported as missing.
In `@src/rai_core/rai/messages/artifacts.py`:
- Around line 29-33: Update store_artifacts in
src/rai_core/rai/messages/artifacts.py to create path.parent with parents=True
and exist_ok=True before opening the database file. In
tests/messages/test_artifacts_db_path.py, remove the test’s db.parent.mkdir call
so the test exercises production directory creation.
In `@src/rai_core/rai/tools/ros2/cli.py`:
- Around line 25-30: Update the timeout_sec validation in the ROS2 CLI timeout
handling to reject non-finite numeric values by checking
math.isfinite(timeout_sec) alongside the existing type and positive-value
checks. Preserve the current TypeError behavior for invalid types and raise the
appropriate validation error before creating the Timer for NaN or infinity.
---
Nitpick comments:
In `@src/rai_core/rai/tools/ros2/cli.py`:
- Around line 25-30: Update the timeout_sec annotation in the affected ROS2 CLI
function to int | float, matching its existing runtime validation and
float-based happy path. Preserve the current rejection of booleans and other
non-numeric values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d07b9e9-30eb-4499-a3d4-f494f6cbb52e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
src/rai_bench/rai_bench/test_models.pysrc/rai_core/pyproject.tomlsrc/rai_core/rai/agents/langchain/agent.pysrc/rai_core/rai/agents/langchain/callback.pysrc/rai_core/rai/agents/langchain/state_based_agent.pysrc/rai_core/rai/agents/runner.pysrc/rai_core/rai/aggregators/base.pysrc/rai_core/rai/aggregators/ros2/aggregators.pysrc/rai_core/rai/communication/base_connector.pysrc/rai_core/rai/communication/ros2/api/action.pysrc/rai_core/rai/communication/ros2/ros_async.pysrc/rai_core/rai/communication/ros2/waiters.pysrc/rai_core/rai/messages/artifacts.pysrc/rai_core/rai/tools/ros2/cli.pytests/agents/langchain/test_hri_callback_handler.pytests/agents/langchain/test_langchain_agent_max_size.pytests/agents/langchain/test_state_based_config.pytests/agents/test_agent_runner.pytests/aggregators/test_base_aggregator.pytests/aggregators/test_ros2_logs_aggregator.pytests/communication/__init__.pytests/communication/ros2/test_get_future_result_timeout.pytests/communication/ros2/test_ros2_async.pytests/communication/ros2/test_waiters.pytests/communication/test_base_connector.pytests/messages/test_artifacts_db_path.pytests/rai_bench/manipulation_o3de/tasks/test_build_tower_task.pytests/tools/test_ros2_cli_timeout.py
| if max_size <= 0: | ||
| raise ValueError(f"max_size must be positive, got {max_size!r}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'File map:\n'
git ls-files 'src/rai_core/rai/agents/langchain/agent.py'
printf '\nRelevant lines:\n'
sed -n '1,220p' src/rai_core/rai/agents/langchain/agent.py
printf '\nSearch for max_size usage:\n'
rg -n "max_size" src/rai_core/rai/agents/langchain/agent.py src/rai_core/rai -g '*.py'
printf '\nPython probe for the current guard semantics:\n'
python3 - <<'PY'
def guard(max_size):
if max_size <= 0:
return "raise"
return "ok"
cases = [None, 0, 1, 1.5, True, False, -1]
for case in cases:
try:
print(repr(case), "=>", guard(case))
except Exception as e:
print(repr(case), "=>", type(e).__name__, str(e))
PYRepository: RobotecAI/rai
Length of output: 11112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for LangChainAgent tests and callers:\n'
rg -n "LangChainAgent\(" src tests -g '*.py' || true
printf '\nSearch for max_size=None with LangChainAgent or its subclasses:\n'
rg -n "max_size\s*=\s*None|max_size:\s*int\s*\|\s*None" src tests -g '*.py' || true
printf '\nInspect nearby class doc/contract for state_based_agent:\n'
sed -n '1,160p' src/rai_core/rai/agents/langchain/state_based_agent.py
printf '\nInspect aggregator base contract for comparison:\n'
sed -n '1,120p' src/rai_core/rai/aggregators/base.pyRepository: RobotecAI/rai
Length of output: 9010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' tests/agents/langchain/test_langchain_agent_max_size.pyRepository: RobotecAI/rai
Length of output: 1310
Reject non-integer max_size values. max_size is documented and typed as an int, but True and 1.5 still pass this guard. Require a non-bool positive int and add regression tests for those cases.
🤖 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/rai_core/rai/agents/langchain/agent.py` around lines 118 - 119, Update
the max_size validation in the surrounding agent initialization logic to require
a positive int that is not a bool, rejecting values such as True and 1.5 while
preserving the existing error for non-positive values. Add regression tests
covering both invalid non-integer cases.
| @field_validator("time_interval") | ||
| @classmethod | ||
| def _time_interval_positive(cls, v: float) -> float: | ||
| if v <= 0: | ||
| raise ValueError(f"time_interval must be positive, got {v!r}") | ||
| return v |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite time_interval values.
float("nan") passes v <= 0, although NaN is not a valid positive interval. Reject non-finite values with math.isfinite and add a NaN regression test.
Proposed validation
+import math
+
`@field_validator`("time_interval")
`@classmethod`
def _time_interval_positive(cls, v: float) -> float:
- if v <= 0:
+ if not math.isfinite(v) or v <= 0:
raise ValueError(f"time_interval must be positive, got {v!r}")📝 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.
| @field_validator("time_interval") | |
| @classmethod | |
| def _time_interval_positive(cls, v: float) -> float: | |
| if v <= 0: | |
| raise ValueError(f"time_interval must be positive, got {v!r}") | |
| return v | |
| import math | |
| `@field_validator`("time_interval") | |
| `@classmethod` | |
| def _time_interval_positive(cls, v: float) -> float: | |
| if not math.isfinite(v) or v <= 0: | |
| raise ValueError(f"time_interval must be positive, got {v!r}") | |
| return v |
🤖 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/rai_core/rai/agents/langchain/state_based_agent.py` around lines 45 - 50,
Update the _time_interval_positive validator to reject any non-finite value by
checking math.isfinite(v) alongside the existing positive-value condition, while
preserving the current error behavior for invalid intervals. Add a regression
test covering NaN input.
| if max_size is not None and max_size <= 0: | ||
| raise ValueError(f"max_size must be positive or None, got {max_size}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one consistent integer-size validation contract.
Both constructors accept positive floats and booleans as buffer sizes, causing inaccurate queue capacities. Apply the same non-boolean positive-integer validation in both locations, while preserving None as an unbounded sentinel where supported.
src/rai_core/rai/aggregators/base.py#L31-L32: reject boolean and non-integermax_sizevalues while retainingNone.src/rai_core/rai/agents/langchain/agent.py#L118-L119: reject boolean and non-integer bounded values, and either retain or explicitly remove the existingNonesentinel behavior.
📍 Affects 2 files
src/rai_core/rai/aggregators/base.py#L31-L32(this comment)src/rai_core/rai/agents/langchain/agent.py#L118-L119
🤖 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/rai_core/rai/aggregators/base.py` around lines 31 - 32, Use one
non-boolean positive-integer validation contract for buffer sizes: update the
constructor validation in src/rai_core/rai/aggregators/base.py lines 31-32 to
reject booleans and non-integer max_size values while preserving None as
unbounded; apply the same validation to the bounded buffer-size check in
src/rai_core/rai/agents/langchain/agent.py lines 118-119, explicitly preserving
its existing None sentinel behavior.
| TypeError: If result_timeout is not a positive number (bool rejected). | ||
| ValueError: If the action server cannot be created or result_timeout <= 0. | ||
| """ | ||
| if isinstance(result_timeout, bool) or not isinstance( | ||
| result_timeout, (int, float) | ||
| ): | ||
| raise TypeError( | ||
| f"result_timeout must be a positive number, got {type(result_timeout).__name__}" | ||
| ) | ||
| if result_timeout <= 0: | ||
| raise ValueError(f"result_timeout must be positive, got {result_timeout!r}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep timeout validation consistent across both APIs.
src/rai_core/rai/communication/ros2/api/action.py#L155-L165 still accepts non-finite timeouts, and src/rai_core/rai/communication/ros2/ros_async.py#L29-L30 also accepts True because it only checks <= 0. Reject non-finite values at both entry points.
📍 Affects 2 files
src/rai_core/rai/communication/ros2/api/action.py#L155-L165(this comment)src/rai_core/rai/communication/ros2/ros_async.py#L29-L30
🤖 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/rai_core/rai/communication/ros2/api/action.py` around lines 155 - 165,
Update timeout validation in action.py lines 155-165 and ros_async.py lines
29-30 to reject non-finite values at both API entry points, while preserving
rejection of non-positive values and booleans. Use the existing validation paths
and ensure valid positive finite numbers remain accepted.
| requested_set = {ensure_slash(name) for name in requested_entities} | ||
| available_set = {ensure_slash(name) for name in callable_get_entities()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the timeout-reporting path too.
Polling uses normalized sets, but the final timeout calculation still compares normalized requested names with raw available_entities. If only some entities are available without /, the timeout error incorrectly includes those entities as missing.
Proposed fix
- available_entities = get_entities()
-
- missing = set(requested) - set(available_entities)
+ missing = get_missing_entities(get_entities, requested)🤖 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/rai_core/rai/communication/ros2/waiters.py` around lines 33 - 34, Update
the timeout-reporting calculation in the waiter flow to compare normalized
entity names consistently, matching the normalized requested_set and
available_set built from ensure_slash. Reuse the normalized available set when
determining missing entities so names available without a leading slash are not
reported as missing.
| path = Path(db_path) | ||
| if not path.is_file(): | ||
| artifact_database: dict = {} | ||
| with path.open("wb") as file: | ||
| pickle.dump(artifact_database, file) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the missing-parent contract in both implementation and test.
store_artifacts only creates the file, while the test creates the parent directory first, so the nested-path failure is hidden.
src/rai_core/rai/messages/artifacts.py#L29-L33: callpath.parent.mkdir(parents=True, exist_ok=True)before opening the file.tests/messages/test_artifacts_db_path.py#L30-L34: removedb.parent.mkdir(...)so the test verifies that production creates the missing directory.
📍 Affects 2 files
src/rai_core/rai/messages/artifacts.py#L29-L33(this comment)tests/messages/test_artifacts_db_path.py#L30-L34
🤖 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/rai_core/rai/messages/artifacts.py` around lines 29 - 33, Update
store_artifacts in src/rai_core/rai/messages/artifacts.py to create path.parent
with parents=True and exist_ok=True before opening the database file. In
tests/messages/test_artifacts_db_path.py, remove the test’s db.parent.mkdir call
so the test exercises production directory creation.
| if isinstance(timeout_sec, bool) or not isinstance(timeout_sec, (int, float)): | ||
| raise TypeError( | ||
| f"timeout_sec must be a positive number, got {type(timeout_sec).__name__}" | ||
| ) | ||
| if timeout_sec <= 0: | ||
| raise ValueError(f"timeout_sec must be positive, got {timeout_sec!r}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/rai_core/rai/tools/ros2/cli.pyRepository: RobotecAI/rai
Length of output: 6071
🏁 Script executed:
python3 - <<'PY'
import math
from threading import Timer
vals = [float('nan'), float('inf'), float('-inf'), 0.1, -1.0, 1]
for v in vals:
try:
print(type(v).__name__, v, 'finite=', math.isfinite(v), '<=0=', (v <= 0))
except Exception as e:
print(type(v).__name__, v, 'error comparing:', e)
# Probe Timer construction/start behavior with NaN/inf without executing callbacks long-term.
def cb():
print("callback fired")
for v in [float('nan'), float('inf'), 0.001]:
t = Timer(v, cb)
print("Timer", v, "interval attr:", t.interval)
PYRepository: RobotecAI/rai
Length of output: 445
🏁 Script executed:
python3 - <<'PY'
import math
from threading import Timer
vals = [float('nan'), float('inf'), float('-inf'), 0.1, -1.0, 1]
for v in vals:
print(type(v).__name__, v, 'finite=', math.isfinite(v), '<=0=', (v <= 0))
for v in [float('nan'), float('inf'), 0.001]:
t = Timer(v, lambda: None)
print("Timer", v, "interval attr:", t.interval)
PYRepository: RobotecAI/rai
Length of output: 445
🏁 Script executed:
python3 - <<'PY'
import time
from threading import Timer
results = []
def mark(label):
results.append((label, time.monotonic()))
start = time.monotonic()
for label, interval in [("nan", float("nan")), ("inf", float("inf")), ("short", 0.05)]:
t = Timer(interval, mark, args=(label,))
t.daemon = True
t.start()
time.sleep(0.2)
print("elapsed", round(time.monotonic() - start, 3))
print("results", results)
PYRepository: RobotecAI/rai
Length of output: 826
Reject non-finite timeout values. float("nan") bypasses the <= 0 check and reaches Timer, where it can fire immediately; float("inf") raises an OverflowError in the timer thread. Add math.isfinite(timeout_sec) here to reject both.
🤖 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/rai_core/rai/tools/ros2/cli.py` around lines 25 - 30, Update the
timeout_sec validation in the ROS2 CLI timeout handling to reject non-finite
numeric values by checking math.isfinite(timeout_sec) alongside the existing
type and positive-value checks. Preserve the current TypeError behavior for
invalid types and raise the appropriate validation error before creating the
Timer for NaN or infinity.
|
Thanks @maciejmajek — CI is now fully green here (the license header + zero-timeout alignment landed via #826, which is merged). On the CodeRabbit review, both flagged items are small and optional:
Since this branch lives on |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #824 +/- ##
==========================================
+ Coverage 73.19% 74.50% +1.31%
==========================================
Files 82 82
Lines 3582 3621 +39
==========================================
+ Hits 2622 2698 +76
+ Misses 960 923 -37 ☔ View full report in Codecov by Harness. |
2 participants
Purpose
Salvaging all the fixes into one single PR to simplify CI process.
Proposed Changes
What does this PR add, remove or fix?
Issues
Testing
Summary by CodeRabbit
Bug Fixes
Validation
Tests
Chores