fix(ai): raise task error/warning history cap from 50 to a named constant (#1414)#1568
fix(ai): raise task error/warning history cap from 50 to a named constant (#1414)#1568meetp06 wants to merge 2 commits into
Conversation
…tant (rocketride-org#1414) Live task status kept only the most recent 50 error and 50 warning messages (`task_engine.py`, hardcoded `[-50:]`), so on a run with many diagnostics the earlier errors were dropped before anyone could read them — the exact symptom called out in rocketride-org#1414 ("error/warning arrays capped at 50"). Replace the two magic `50`s with a single named constant, `CONST_STATUS_HISTORY_LIMIT` (1000), alongside the other `CONST_*` limits, and update the `TASK_STATUS` field descriptions in the Python SDK to match. This retains far more history in memory while keeping a bound so a pathological run cannot grow the buffers without limit. Scope: this addresses only the in-memory cap. Durable run-history persistence (the other half of rocketride-org#1414 — JSONL sink, downloadable trace, survive disconnect) is a larger change and is intentionally left as separate follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
module:client-python
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
📝 WalkthroughWalkthroughThe task status history limit increases from 50 to 1000 retained error and warning messages. The task engine uses the shared constant, client model descriptions reflect the updated limit, and trimming tests use the shared value. ChangesTask status history
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 944-953: Update the status-history error-buffer test to import and
use CONST_STATUS_HISTORY_LIMIT, seed exactly limit existing entries, then append
the new error and assert the buffer is trimmed to the configured limit rather
than a hard-coded 50.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1f2e775f-703e-4ca6-bcd8-4ea8b922f780
📒 Files selected for processing (3)
packages/ai/src/ai/constants.pypackages/ai/src/ai/modules/task/task_engine.pypackages/client-python/src/rocketride/types/task.py
| if len(self._status.errors) > CONST_STATUS_HISTORY_LIMIT: | ||
| self._status.errors = self._status.errors[-CONST_STATUS_HISTORY_LIMIT:] | ||
|
|
||
| # Handle warning messages with buffer management | ||
| elif event_type == 'apaevt_status_warning': | ||
| warning_message = body.get('message', '') | ||
| self._status.warnings.append(warning_message) | ||
|
|
||
| if len(self._status.warnings) > 50: | ||
| self._status.warnings = self._status.warnings[-50:] | ||
| if len(self._status.warnings) > CONST_STATUS_HISTORY_LIMIT: | ||
| self._status.warnings = self._status.warnings[-CONST_STATUS_HISTORY_LIMIT:] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the status-history tests for the new limit.
The existing error-buffer test still initializes 50 entries and asserts a final length of 50. With this implementation, it will contain 51 entries, so the test suite will fail. Update the test to use CONST_STATUS_HISTORY_LIMIT and seed limit entries before appending the new message.
🤖 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 `@packages/ai/src/ai/modules/task/task_engine.py` around lines 944 - 953,
Update the status-history error-buffer test to import and use
CONST_STATUS_HISTORY_LIMIT, seed exactly limit existing entries, then append the
new error and assert the buffer is trimmed to the configured limit rather than a
hard-coded 50.
test_update_status_errors_buffer_trims_to_50 hardcoded the old 50 cap and broke when the limit was raised. Seed CONST_STATUS_HISTORY_LIMIT entries and assert the buffer trims to that constant, so the test tracks the configured limit instead of a magic number. Rename accordingly and drop the stale "50" wording from the warning test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/tests/ai/modules/task/test_task_engine.py`:
- Around line 490-491: Rename test_update_status_warning_event_appends_and_trims
and its docstring to describe only appending a warning, matching the existing
implementation and analogous error tests. Add a separate warning-buffer trimming
test around Task._update_status that fills warnings to
CONST_STATUS_HISTORY_LIMIT, appends a new warning, and verifies the limit is
preserved and the oldest entry is evicted.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 678e9ae5-97b6-4875-b5d6-4687b08b2305
📒 Files selected for processing (1)
packages/ai/tests/ai/modules/task/test_task_engine.py
| def test_update_status_warning_event_appends_and_trims(): | ||
| """An ``apaevt_status_warning`` event appends to warnings with the same 50-cap.""" | ||
| """An ``apaevt_status_warning`` event appends to warnings with the same history cap.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Align the test name and docstring with its implementation, or add trimming assertions.
The test is named test_update_status_warning_event_appends_and_trims and its docstring mentions the history cap, but the implementation only asserts that a single warning is appended. It does not actually test the trimming behavior.
Consider renaming the test to match the error tests (e.g., test_update_status_warning_event_appends_to_warnings) and adding a dedicated trimming test for warnings to ensure complete coverage.
♻️ Proposed refactor
-def test_update_status_warning_event_appends_and_trims():
- """An ``apaevt_status_warning`` event appends to warnings with the same history cap."""
+def test_update_status_warning_event_appends_to_warnings():
+ """An ``apaevt_status_warning`` event appends to ``status.warnings``."""To fully cover the trimming behavior for warnings, you can add this new test alongside it:
def test_update_status_warnings_buffer_trims_to_limit():
"""Warning buffer keeps only the most recent CONST_STATUS_HISTORY_LIMIT entries."""
t = _task(status=_make_status_for_update())
t._status.warnings = [f'warn-{i}' for i in range(CONST_STATUS_HISTORY_LIMIT)]
Task._update_status(
t,
{
'event': 'apaevt_status_warning',
'body': {'message': 'warn-new'},
},
)
assert len(t._status.warnings) == CONST_STATUS_HISTORY_LIMIT
assert t._status.warnings[-1] == 'warn-new'
assert 'warn-0' not in t._status.warnings # oldest evicted📝 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.
| def test_update_status_warning_event_appends_and_trims(): | |
| """An ``apaevt_status_warning`` event appends to warnings with the same 50-cap.""" | |
| """An ``apaevt_status_warning`` event appends to warnings with the same history cap.""" | |
| def test_update_status_warning_event_appends_to_warnings(): | |
| """An ``apaevt_status_warning`` event appends to ``status.warnings``.""" |
🤖 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 `@packages/ai/tests/ai/modules/task/test_task_engine.py` around lines 490 -
491, Rename test_update_status_warning_event_appends_and_trims and its docstring
to describe only appending a warning, matching the existing implementation and
analogous error tests. Add a separate warning-buffer trimming test around
Task._update_status that fills warnings to CONST_STATUS_HISTORY_LIMIT, appends a
new warning, and verifies the limit is preserved and the oldest entry is
evicted.
1 participant
Addresses #1414 (the error/warning cap portion — see Scope below).
Problem
Live task status kept only the 50 most recent error and 50 most recent warning messages. In
packages/ai/src/ai/modules/task/task_engine.pythe buffers were trimmed with a hardcoded magic number:On a run that emits many diagnostics, the earliest errors are discarded before anyone can read them — the exact symptom reported in #1414 ("error/warning arrays capped at 50").
Fix
50s with a single named constantCONST_STATUS_HISTORY_LIMIT = 1000, added next to the otherCONST_*limits inai/constants.py.TASK_STATUS.errors/.warningsfield descriptions in the Python SDK (packages/client-python/src/rocketride/types/task.py) to match.This retains far more history in memory (1000 vs 50) while keeping a bound, so a pathological run can't grow the buffers without limit. Using a named constant means the value lives in one place if it ever needs tuning.
Scope
This intentionally covers only the in-memory cap. The other half of #1414 — durable run-history persistence (JSONL sink, downloadable trace, surviving disconnect) — is a substantially larger change and is left as a separate follow-up. Filing this as a focused, reviewable first step rather than bundling both.
Verification
ruff check— All checks passedruff format --check— clean (all 3 files)🤖 Generated with Claude Code
Summary by CodeRabbit