Repository navigation
feat(nodes): add context window optimizer node for token budget management - #527
Closed
nihalnihalani wants to merge 25 commits into
Closed
nihalnihalani wants to merge 25 commits into
nihalnihalani wants to merge 25 commits into
Conversation
nihalnihalani
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
March 30, 2026 22:43
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Context Optimizer node: global lifecycle loads config and conditionally instantiates a ContextOptimizer; instance wrapper intercepts and deep-copies questions, runs token-budgeted optimization (system prompt, query, documents, history) when available, and forwards optimized questions. Package metadata, dependency, and tests added. Changes
Sequence DiagramsequenceDiagram
participant Startup as "Startup"
participant IGlobal as "IGlobal"
participant Optimizer as "ContextOptimizer"
participant IInstance as "IInstance"
participant Pipeline as "Pipeline"
Startup->>IGlobal: beginGlobal()
alt CONFIG mode
IGlobal->>IGlobal: keep optimizer = None
else NORMAL mode
IGlobal->>IGlobal: import depends, load node config
IGlobal->>Optimizer: instantiate ContextOptimizer(config)
Optimizer->>Optimizer: prepare tokenizer & model limits
end
Pipeline->>IInstance: writeQuestions(question)
alt Optimizer available
IInstance->>IInstance: deep-copy & extract system/query/docs/history
IInstance->>Optimizer: optimize(system_prompt, model, documents, history)
Optimizer->>Optimizer: count tokens -> allocate budgets
Optimizer->>Optimizer: truncate system/query, rank/select docs, summarize history
Optimizer-->>IInstance: optimized components + metadata
IInstance->>IInstance: rebuild Question, log metadata
else No optimizer
IInstance->>IInstance: emit warning & pass-through question
end
IInstance->>Pipeline: writeQuestions(optimized_question)
Startup->>IGlobal: endGlobal()
IGlobal->>IGlobal: clear optimizer & config
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/context_optimizer/IInstance.py`:
- Around line 69-71: Replace the explicit for-loop that builds the history list
in IInstance.py with a single list comprehension: instead of initializing
history = [] and looping over question.history to append {'role': h.role,
'content': h.content}, assign history to a comprehension that iterates over
question.history or [] and constructs the same dicts; update the code around the
variable named history and the use of question.history accordingly.
In `@nodes/src/nodes/context_optimizer/optimizer.py`:
- Around line 299-302: The code builds the final message list via concatenation
into an intermediate variable `result`; replace that with list unpacking to
simplify and improve clarity by returning `[first_msg, summary_placeholder,
*recent]` directly instead of `result = [first_msg, summary_placeholder] +
recent` and `return result`—apply this change in the function where
`summary_placeholder` is set (the block using `first_msg`,
`summary_placeholder`, and `recent`).
In `@nodes/src/nodes/context_optimizer/services.json`:
- Around line 111-118: The "auto" profile currently claims to detect the model
from the pipeline but actually uses the configured default model_name ("gpt-5");
update the "auto" profile definition by either changing the "title" to
accurately reflect current behavior (e.g., "Auto - uses configured default model
(gpt-5)") or add a descriptive note in the profile (e.g., a "description" field)
stating that pipeline model detection is a planned future enhancement; modify
the "auto" object (keys: "title", "model_name") in the JSON to reflect this
clarification.
In `@test/nodes/test_context_optimizer.py`:
- Around line 348-350: Replace the two separate endswith checks with a single
tuple form: update the assertion that currently does "assert
result.endswith('.') or result.endswith('sentence')" to use a single endswith
call with a tuple (e.g., result.endswith(('.', 'sentence'))) so the static
analyzer warning is resolved; this change should be applied where the test calls
optimizer.truncate_to_budget and asserts on the result variable.
🪄 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: 57cc9afb-270e-4b0d-8a41-9510a6dda2e5
📒 Files selected for processing (7)
nodes/src/nodes/context_optimizer/IGlobal.pynodes/src/nodes/context_optimizer/IInstance.pynodes/src/nodes/context_optimizer/__init__.pynodes/src/nodes/context_optimizer/optimizer.pynodes/src/nodes/context_optimizer/requirements.txtnodes/src/nodes/context_optimizer/services.jsontest/nodes/test_context_optimizer.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/context_optimizer/IInstance.py`:
- Around line 97-101: Move the from ai.common.schema import QuestionHistory out
of the method and place it at module top-level so the QuestionHistory symbol is
imported once (improves readability and avoids repeated import overhead); update
the file-level imports and remove the inline import inside the block that
updates question.history (the code section using result['history'] and
constructing QuestionHistory(role=..., content=...)); if a circular-import
prevents a top-level import, use a typing.TYPE_CHECKING guard or keep the local
import but add a short comment explaining the circular dependency.
In `@nodes/src/nodes/context_optimizer/optimizer.py`:
- Around line 409-414: The history token calculation is inconsistent: in
optimizer.optimize (original_history) you add a flat "+ 4" for role overhead
while summarize_history._msg_tokens uses count_tokens(role) + 4; extract a
shared helper (e.g., msg_token_count(message: dict) or _msg_tokens(role,
content)) that returns count_tokens(role) + count_tokens(content) + 4 and use it
both in summarize_history._msg_tokens and in the optimize
original_history/doc/history counting so both places compute identical
per-message tokens and remove the hardcoded "+ 4" discrepancy.
- Around line 209-223: The current logic splits text with _SENTENCE_RE and then
joins selected pieces with ' ', which collapses original inter-sentence
whitespace; change the approach to preserve original separators by using
_SENTENCE_RE with a capturing group (or iterate re.finditer over matches) so you
keep each sentence plus its following separator, measure tokens with
count_tokens(sentence_piece, encoding), append the exact sentence_piece to
result_parts, and return ''.join(result_parts) (instead of ' '.join) when
returning the truncated text; update the code paths that reference
sentences/result_parts and keep count_tokens, encoding and max_tokens usage
unchanged.
- Around line 47-66: The MODEL_LIMITS class variable is mutable and triggers
Ruff RUF012; annotate it as a ClassVar to mark it read-only by importing
ClassVar from typing and changing the declaration to use ClassVar[Dict[str,
int]] for MODEL_LIMITS, or if you prefer to keep the current annotation add a
trailing "# noqa: RUF012" to the MODEL_LIMITS line to suppress the warning;
locate the MODEL_LIMITS symbol in optimizer.py and apply one of these two fixes.
- Around line 282-291: The loop building recent currently uses recent.insert(0,
msg) which causes O(n²) behavior; instead append messages to recent and then
reverse once at the end to preserve original order. Update the block that
iterates over reversed(messages[1:]) (uses _msg_tokens and budget_for_recent) to
call recent.append(msg) when within budget and after the loop call
recent.reverse() so the logic and variables (recent, recent_cost, _msg_tokens,
budget_for_recent, messages) remain the same but with O(n) performance.
In `@nodes/src/nodes/context_optimizer/services.json`:
- Around line 88-101: Extend the "test" cases in services.json to cover token
truncation, document selection under budget, and history compression: add
separate entries in the "cases" array that (1) provide a very long question text
to assert truncation behavior (extend the existing "questions" object and set an
"expect" that checks truncated length or presence of a truncation marker), (2)
supply multiple documents plus a budget constraint to assert the chosen
documents (create an input field representing documents and an "expect"
asserting selected document IDs or count), and (3) include a long conversation
history to verify compression (add a "history" input and an "expect" that
compressed history length or summary is within expected bounds); reference the
existing test structure keys ("test", "cases", "questions", "expect") so new
cases match the same schema and validation style.
In `@test/nodes/test_context_optimizer.py`:
- Around line 764-792: Test currently only checks that 'question' was passed to
optimizer.optimize; tighten it by asserting the actual extracted values: fetch
call_args = mock_opt.optimize.call_args and read the question from
call_args.kwargs.get('question', call_args.args[0] if call_args.args else None)
and assert it equals the expected question text (from the Question instance or
q.get_text()/repr), and similarly assert that 'components' (or the expected
components key) is present and is a list containing the components extracted by
inst.writeQuestions; update test_optimizer_called_with_components to perform
these specific checks using the optimizer.optimize call_args instead of the
loose presence check.
- Around line 234-235: The test test_none_returns_zero calls count_tokens with
None but count_tokens is typed as text: str; update the count_tokens signature
to accept Optional[str] (import typing.Optional) and update its docstring to
state None is allowed, keeping the existing None-check (if not text: return 0)
so the test remains valid; alternatively change the test to pass an empty string
''—pick one consistent approach and update typing and imports accordingly in the
function definition for count_tokens.
🪄 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: 3dcd3061-e08b-4c22-8e64-17cc00d04a83
📒 Files selected for processing (4)
nodes/src/nodes/context_optimizer/IInstance.pynodes/src/nodes/context_optimizer/optimizer.pynodes/src/nodes/context_optimizer/services.jsontest/nodes/test_context_optimizer.py
asclearuc
requested changes
Apr 1, 2026
asclearuc
left a comment
asclearuc
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for this contribution — the token counting, budget allocation, and
sentence-aware truncation are well implemented, and reusing tiktoken is the right
choice. The overall design is solid.
A few issues need to be resolved before merge. Inline comments have been left on
specific lines.
Collaborator
Author
|
@asclearuc Thank you for the thorough and deeply technical review! Your feedback is excellent and touches on real correctness issues. I'm working on addressing all 8 points:
Will push these fixes shortly. Thanks again for the insightful review! |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
nodes/src/nodes/context_optimizer/IInstance.py (1)
98-102: 🧹 Nitpick | 🔵 TrivialMove
QuestionHistoryimport to module level.The
QuestionHistoryimport at line 99 is still inside the method. As you acknowledged in a previous review, since there's no circular import concern, moving it to the top level alongside the existingQuestionimport improves readability.♻️ Move import to top of file
from rocketlib import IInstanceBase, Entry, debug, warning from .IGlobal import IGlobal -from ai.common.schema import Question +from ai.common.schema import Question, QuestionHistory ... # Update history - from ai.common.schema import QuestionHistory - if result['history']:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/context_optimizer/IInstance.py` around lines 98 - 102, The import of QuestionHistory is currently inside a method in IInstance.py; move it to the module level alongside the existing Question import at the top of the file so the module-level imports read both Question and QuestionHistory, then remove the local "from ai.common.schema import QuestionHistory" inside the method where result['history'] is used (the code that sets question.history = [QuestionHistory(...)] should continue to reference QuestionHistory unchanged).nodes/src/nodes/context_optimizer/optimizer.py (1)
318-327: 🧹 Nitpick | 🔵 TrivialUse
append+reverseto avoid O(n²) list insertions.Using
recent.insert(0, msg)inside a loop causes O(n²) behavior due to list shifting. You agreed to refactor this in a previous review.♻️ Suggested optimization
# Greedily add recent messages from the end recent: List[Dict[str, str]] = [] recent_cost = 0 for msg in reversed(messages[1:]): cost = _msg_tokens(msg) if recent_cost + cost <= budget_for_recent: - recent.insert(0, msg) + recent.append(msg) recent_cost += cost else: break + recent.reverse()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/context_optimizer/optimizer.py` around lines 318 - 327, The loop building recent currently uses recent.insert(0, msg) causing O(n²) behavior; change it to append messages in iteration order (e.g., append to recent when iterating reversed(messages[1:])), then call recent.reverse() after the loop so the final order is preserved; update references in this block (variables recent, recent_cost, and the loop over reversed(messages[1:])) to use append + reverse to avoid repeated front-insertions.test/nodes/test_context_optimizer.py (1)
789-793: 🧹 Nitpick | 🔵 TrivialTighten assertions on optimizer call arguments.
The assertion at line 793 is still loose—it only checks that
'question'exists somewhere in the call. You agreed to verify the actual extracted values in a previous review.♻️ More specific assertion
mock_opt.optimize.assert_called_once() call_kwargs = mock_opt.optimize.call_args - assert 'question' in call_kwargs.kwargs or len(call_kwargs.args) > 0 + # Verify extracted components + assert call_kwargs.kwargs.get('question') == 'What is AI?' + assert call_kwargs.kwargs.get('system_prompt') == 'You are helpful.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nodes/test_context_optimizer.py` around lines 789 - 793, The optimizer call assertion is too loose—replace the generic check with a precise comparison to the questions you passed in: after inst.writeQuestions(q) and mock_opt.optimize.assert_called_once(), grab call_kwargs = mock_opt.optimize.call_args and assert that the question was passed exactly (either assert call_kwargs.kwargs.get('question') == q or assert call_kwargs.args and call_kwargs.args[0] == q), and if writeQuestions can pass a list/tuple ensure you compare to the correct element/structure (e.g., q[0] or q) so the test verifies the actual extracted values rather than just the presence of the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nodes/src/nodes/context_optimizer/IInstance.py`:
- Around line 98-102: The import of QuestionHistory is currently inside a method
in IInstance.py; move it to the module level alongside the existing Question
import at the top of the file so the module-level imports read both Question and
QuestionHistory, then remove the local "from ai.common.schema import
QuestionHistory" inside the method where result['history'] is used (the code
that sets question.history = [QuestionHistory(...)] should continue to reference
QuestionHistory unchanged).
In `@nodes/src/nodes/context_optimizer/optimizer.py`:
- Around line 318-327: The loop building recent currently uses recent.insert(0,
msg) causing O(n²) behavior; change it to append messages in iteration order
(e.g., append to recent when iterating reversed(messages[1:])), then call
recent.reverse() after the loop so the final order is preserved; update
references in this block (variables recent, recent_cost, and the loop over
reversed(messages[1:])) to use append + reverse to avoid repeated
front-insertions.
In `@test/nodes/test_context_optimizer.py`:
- Around line 789-793: The optimizer call assertion is too loose—replace the
generic check with a precise comparison to the questions you passed in: after
inst.writeQuestions(q) and mock_opt.optimize.assert_called_once(), grab
call_kwargs = mock_opt.optimize.call_args and assert that the question was
passed exactly (either assert call_kwargs.kwargs.get('question') == q or assert
call_kwargs.args and call_kwargs.args[0] == q), and if writeQuestions can pass a
list/tuple ensure you compare to the correct element/structure (e.g., q[0] or q)
so the test verifies the actual extracted values rather than just the presence
of the key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bbc2114-9f39-4749-abe1-660f0d9e06c9
📒 Files selected for processing (4)
nodes/src/nodes/context_optimizer/IGlobal.pynodes/src/nodes/context_optimizer/IInstance.pynodes/src/nodes/context_optimizer/optimizer.pytest/nodes/test_context_optimizer.py
Collaborator
Author
All review feedback addressed in latest commits@asclearuc review items — all fixed:
CodeRabbit nitpicks — also addressed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/context_optimizer/IInstance.py`:
- Around line 86-92: The current block updates question.questions[0].text but
silently discards result['question'] when question.questions is empty; add a
guard that checks if not question.questions and result['question'] and then emit
a debug log (including the optimized text and context) to surface this edge
case, and optionally append a new Question object with text=result['question']
(or preserve embedding info if available) to question.questions so the optimized
content isn’t lost; reference the question.questions list and result['question']
in your changes and use the module's existing logger to record the event.
🪄 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: b77db817-bf83-4e4f-8c16-177629dd3afd
📒 Files selected for processing (4)
nodes/src/nodes/context_optimizer/IInstance.pynodes/src/nodes/context_optimizer/optimizer.pynodes/src/nodes/context_optimizer/requirements.txttest/nodes/test_context_optimizer.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/context_optimizer/IInstance.py`:
- Around line 63-68: The current conversion in IInstance.py discards fields like
score by only appending {'content': content, '_original': doc}; change the
transformation so you preserve all dumped fields from doc_dict (including
'score') and add/override 'content' and '_original' instead of replacing the
entire dict; update the loop that builds docs (used by rank_documents()) to
merge doc_dict with {'content': content, '_original': doc} so score-first
ordering works correctly.
- Around line 84-103: The code treats empty optimizer outputs as "no update" by
using truthiness checks (e.g., if result['question'], if result['history'], if
question.questions) so empty strings/lists are ignored; change those checks to
explicit presence checks and assign/clear fields even when the optimizer returns
empty values: use "if 'question' in result:" to update
question.questions[0].text = result['question'] (and truncate extras) or set
question.questions = [] when result['question'] is an empty string and no
embeddings should remain; use "if 'documents' in result:" to set
question.documents = [d['_original'] for d in result['documents'] if '_original'
in d] (allowing empty list to clear documents); and use "if 'history' in
result:" to set question.history = [QuestionHistory(... ) for m in
result['history']] (allowing empty history to clear it); also replace the role
assignment to use result.get('system_prompt', '') or assign explicitly when
'system_prompt' in result so an empty string overwrites the previous role.
In `@test/nodes/test_context_optimizer.py`:
- Around line 32-140: The test's _install_stubs() and global _STUBS_INSTALLED
permanently mutate sys.modules (adding 'rocketlib', 'ai.common.schema', etc.) at
import time which leaks fake modules into other tests; change this to apply the
stubs only within a context/fixture (use unittest.mock.patch.dict(sys.modules,
{...}) or a pytest fixture that patches sys.modules) and import the modules
under test inside that scoped patch so the originals are restored after the
test; update where _install_stubs() is called (and the duplicate stubbing at the
other occurrence around the 177-178 area) to use the scoped patch/fixture
instead of setting sys.modules globally.
- Around line 147-175: The mock tiktoken setup is currently inside the except
block so it only runs if find_spec raises—move the mock creation to run whenever
_TIKTOKEN_AVAILABLE is False: keep the try/except that sets _TIKTOKEN_AVAILABLE
(using importlib.util.find_spec('tiktoken')), but remove the mock from the
except and add an if not _TIKTOKEN_AVAILABLE: block that defines tiktoken_mod,
_MockEncoding, _get_encoding and sets sys.modules['tiktoken']; reference
_TIKTOKEN_AVAILABLE, tiktoken_mod, _MockEncoding and _get_encoding so the
optimizer import that follows will see the mocked module when tiktoken is
absent.
🪄 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: 688affea-3221-4a2e-90c6-8528165e9c45
📒 Files selected for processing (2)
nodes/src/nodes/context_optimizer/IInstance.pytest/nodes/test_context_optimizer.py
Collaborator
|
@nihalnihalani, please address CodeRabbit’s concerns as well. |
Collaborator
|
Please address all CodeRabbit comments, not only for this PR but also for others. |
Collaborator
Author
|
All CodeRabbit review comments have been addressed across prior commits — verified with 81/81 tests passing, ruff lint clean, and ruff format clean. I've gone through this PR (and the others) to make sure no CodeRabbit feedback was left unaddressed. Will continue to check back after pushes for any new CodeRabbit comments that may appear, as you suggested. Thanks for the process guidance — applying this as a standard practice going forward. |
Collaborator
Author
|
All CodeRabbit review comments on this PR have been addressed in commit 270498d: Fixes applied:
All 81 tests pass, ruff lint and format checks are clean. I will continue to monitor the PR for any new CodeRabbit comments that may appear after this push. |
nihalnihalani
force-pushed
the
feature/context-optimizer-node
branch
from
April 6, 2026 18:46
270498d to
82f225a
Compare
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Apr 8, 2026
…etride-org#527) Clarify "auto" profile title in services.json to say "Use default model limits" since pipeline detection is not implemented — the implementation uses the configured model_name default to look up limits. All other review feedback items (module-level import, _message_tokens helper, Optional[str] signature, list comprehension, ClassVar annotation, endswith tuple form) were addressed in prior commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Review Feedback AddressedFixes
Ready for re-review. |
Contributor
|
No description provided. |
Collaborator
Author
🚀 Merge RequestAll review feedback addressed:
Ready for re-review and merge. @asclearuc |
Collaborator
Author
|
@asclearuc — All review feedback addressed and pushed (7 fixes, 63/64 tests passing — 1 pre-existing unrelated failure). Ready for re-review. Thanks! 🙏 |
asclearuc
approved these changes
Apr 15, 2026
Collaborator
Author
|
@asclearuc @alexs-aparavi — re-requesting review. All items from both your reviews are addressed:
81 tests pass. Deferred follow-ups (acknowledged behind the experimental flag): budget-redistribution of unused allocation, preservation of vector-DB document ordering when the scored path isn't available. |
asclearuc
requested changes
Apr 21, 2026
asclearuc
left a comment
asclearuc
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the contribution! Before we continue the review, please:
- Rebase against
develop— your branch has diverged and needs to be up to date. - Remove changes to
pnpm-lock.yaml— the diff includes unrelated additions (lucide-react) and removeslibcconstraints from ~20 platform-specific packages. Please restore the lock file fromdevelop. - Make sure all tests pass — ensure CI is green before requesting re-review.
Once those are done, push your updated branch and we'll continue with the code review.
Collaborator
Author
|
@asclearuc — ready for re-review. Branch merged with latest All review items verified in current HEAD:
CodeRabbit items addressed: document |
Collaborator
Author
|
@asclearuc @alexs-aparavi -- thanks for the thorough reviews. Pushed Addressed in the most recent commit (2ad344e)
Already resolved by earlier commits on this branch (carried over from previous review rounds, verified against current code)
Deferred / awaiting guidance
Test status
Ready for another look. Note for CodeRabbit (per @asclearuc's earlier asks): will revisit once it has had time to scan the new push. |
Pure mechanical `ruff format` output. No semantic changes. Required to clear `ruff format --check` in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Could you please review the new commit? |
Collaborator
Author
Review statusAll 5 outstanding @asclearuc threads from the 2026-05-04 review were addressed in 2ad344e (
Branch is up to date with develop; CI green on 6988517. Requesting re-review from @asclearuc, and first-pass from @jmaionchi @Rod-Christensen @stepmikhaylov @kwit75. |
Collaborator
|
Thanks for implementing the context window optimizer node — please address the three change requests above before merging. |
stepmikhaylov
requested changes
May 19, 2026
stepmikhaylov
left a comment
stepmikhaylov
left a comment
Collaborator
There was a problem hiding this comment.
Change Requests
-
[CR-1] Stub for
rocketlibviolates built-in interpreter contract —nodes/test/context_optimizer/test_all.pylines 47–67_build_stub_modules()creates a fakerocketlib(includingIGlobalBase,IInstanceBase,OPEN_MODE,debug,warning).rocketlibis provided by the built-in interpreter and must not be mocked — the real module should be imported directly. Onlydependsrequires a stub in the test environment. -
[CR-2] Stubs for
aipackage (ai.common.config,ai.common.schema) violate built-in interpreter contract —nodes/test/context_optimizer/test_all.pylines 69–143_build_stub_modules()also creates fakes forai,ai.common,ai.common.config, andai.common.schema. Theaipackage is provided by the built-in interpreter and must not be mocked. Replacing the realQuestion,QuestionHistory, andConfigclasses with inline replicas means the tests do not exercise the real schema interfaces. -
[CR-3]
tiktokenfallback mock is defined inline; belongs innodes/test/mocks/—nodes/test/context_optimizer/test_all.pylines 158–178The
_MockEncodingclass and_get_encodingstub are defined in the test file itself. Per project convention, node-specific library mocks should live innodes/test/mocks/(e.g.nodes/test/mocks/tiktoken/__init__.py), matching the pattern used foropenai,anthropic,elasticsearch, and other third-party stubs.
Collaborator
Author
|
@asclearuc @Rod-Christensen @jmaionchi @kwit75 @stepmikhaylov @alexs-aparavi all three outstanding comments are already addressed on the current head (6988517). The empty |
Collaborator
|
@nihalnihalani, I must have been misunderstood. Let me provide some specifics. For example, this code creates a dummy “rocketlib”: whereas the real one should be used: Although our codebase already contains similar examples, the approach to stub our framework is entities generally incorrect. |
… to mocks dir Address stepmikhaylov test-convention review: - Import the real rocketlib (IGlobalBase/IInstanceBase/OPEN_MODE/debug/warning) and real ai.common.schema (Question/QuestionHistory) / ai.common.config (Config) instead of faking them; only the opaque engine bootstrap modules (engLib, depends) are stubbed, mirroring nodes/test/tool_git. - Move the inline tiktoken fallback mock to nodes/test/mocks/tiktoken/ per the third-party mock convention; add a json5 mock so the real ai.common.config imports under a bare pytest run. - Defensively evict leftover fake rocketlib/ai stubs left in sys.modules by other node test modules so collection order does not poison the real imports. Also remove the unrelated empty nodes/test/guardrails/__init__.py that did not belong to this node (asclearuc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors json5.JSONError (ValueError subclass) so callers that catch it by name work when the mock shadows real json5 under ROCKETRIDE_MOCK.
nihalnihalani
force-pushed
the
feature/context-optimizer-node
branch
from
May 21, 2026 00:37
e28a0eb to
2bc7e05
Compare
Collaborator
Author
|
All requested changes are in — requesting re-review. @asclearuc @stepmikhaylov @alexs-aparavi @jmaionchi @Rod-Christensen @kwit75 asclearuc's threads
stepmikhaylov CR-1/2/3 (test convention)
context_optimizer tests pass and ruff is clean locally; the latest commit is building in CI now. |
The dynamic integration test
test_node_cases[context_optimizer:services:test2:ci_tiny] failed in CI
("Expected maximum length 160 but got 480"). nodes/test/mocks/tiktoken/
(a whitespace-splitting stub) was loaded engine-wide via ROCKETRIDE_MOCK
(packages/ai/src/ai/node.py inserts the mocks dir at the front of
sys.path). The stub under-counted tokens, so the live node never hit the
ci_tiny budget and never truncated the long input. nodes/test/mocks/json5/
similarly shadowed real json5 (context_optimizer/services.json is JSON5).
Remove both mock directories so the live dynamic test uses the REAL
tiktoken/json5 that are installed in CI, restoring pre-change behavior.
Keep the unit suite green by injecting test-LOCAL tiktoken/json5 stubs
(defined inline in test_all.py, not under nodes/test/mocks/) into the
pytest process's sys.modules only when the real libs are absent. The
engine runs the dynamic test in a separate subprocess, so the inline
stubs never reach the live run. Unit assertions pass identically with the
real libraries or the stubs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Correction to my earlier note + CI fix. Relocating the tiktoken stub (and a json5 shim) into The engine puts tiktoken/json5 are deterministic compute/parse libraries that are installed in CI (not external services), so the live node should use the real ones. I removed both stub dirs and kept the stand-ins test-local in @stepmikhaylov — this is why CR-3's relocation into |
The build interpreter provides rocketlib, ai.* and depends (plus the native engLib) at runtime, and CI installs tiktoken/json5. Mirror the validated chunker pattern (PR rocketride-org#897): keep only the nodes/src/nodes sys.path insert and import the context_optimizer package by name. Remove the engLib/depends stubs, the rocketlib/ai/client-python sys.path additions, the stale rocketlib/ai sys.modules purge, and the test-local tiktoken/json5 stub injection. No skips or fallbacks: outside the build interpreter, imports error at collection by design. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI showed tiktoken is absent in the unit-test interpreter (it's a node dep installed only at engine runtime via depends), so dropping the fallback broke 10 unit tests with ModuleNotFoundError. Framework stubs (rocketlib/ai/depends/ engLib) stay removed per the build-interpreter convention; only the node- specific tiktoken/json5 stubs return, injected solely when the real lib is absent and never under nodes/test/mocks/ (which would shadow the dynamic run).
Collaborator
Author
|
@stepmikhaylov — followed up on the "don't stub framework entities" point. The context_optimizer test no longer stubs
|
Collaborator
|
This is a genuinely nice piece of engineering for a hackathon PR — the two-pass approach (skip all the truncation machinery entirely when everything already fits) is the right call, and 68 tests covering token counting, budgeting, truncation, history compression and doc ranking is real coverage, not just smoke tests. On the substance: the two CodeRabbit issues that actually mattered are already folded into this diff. IInstance.py now spreads **doc_dict before overriding content/_original, so score survives into rank_documents() (that was the major one — without it the score-first document ordering silently never ran on real Question objects). And the truncation results are now assigned unconditionally (result['question'] or '', always reassigning question.documents/question.history), so an intentionally-emptied component won't get left in place, since Python treats ''/[] as falsy. Good catch-and-fix on both, and the remaining CodeRabbit items (ClassVar on MODEL_LIMITS, the list comprehension for history conversion, the auto profile title) are already addressed too. One thing worth a second look if this work continues: in rank_documents, when any document carries a score, the code just preserves documents in their incoming order rather than sorting by score — the comment assumes the vector DB already returns them score-descending. That's probably true today, but it's an implicit contract with the upstream node rather than something this code enforces; a future retriever handing back unsorted-but-scored docs would silently cause budget selection to pick a suboptimal subset. Worth an explicit sorted(..., key=lambda d: -d['score']) for scored docs, or at least a stronger assert/comment documenting the assumption, whenever this is picked back up. Separately from the code quality: this PR appears to have gone inactive. Given that, I'd suggest closing it rather than leaving it open indefinitely — the work here is solid and the fixes are already in place, so if the author (or someone else) wants to pick it back up, reopening with a fresh review pass (particularly on the score-ordering assumption above) should be quick given how close this already is to mergeable. |
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Jul 7, 2026
…t selection rank_documents previously assumed the vector DB returned documents already sorted score-descending and merely preserved incoming order when any doc carried a score. That is an implicit contract with the upstream node. Make it explicit: when any document has a score, sort score-descending (stable, missing score treated as -inf so unscored docs sink below scored ones while keeping their incoming relative order) before greedy budget selection. Add tests for descending sort, top-scoring subset selection under a tight budget with unsorted input, and missing-score placement. Addresses dsapandora's close-out review note on PR rocketride-org#527. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
6 of 7 tasks
Collaborator
Author
|
@dsapandora Thanks for the thorough close-out review — I'm picking this back up per your note. I tried to reopen this PR but GitHub won't allow it once new commits have been pushed to the head branch, so the continuation lives at #1489 (same branch, review history preserved here). The score-ordering fix you flagged is in — explicit score-descending sort in |
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Sep 4, 2026
RAG pipelines push a system prompt, the user question, retrieved documents and chat history into an LLM node with no accounting of the model's context window. When the total exceeds the limit the provider rejects the request or silently truncates, and no node in the catalog trims content by token budget before the LLM call. context_optimizer is a questions -> questions filter that counts tokens with tiktoken, allocates a per-component budget (system prompt / query / documents / history, configurable percentages), truncates on sentence boundaries, keeps the highest-scoring documents that fit, and compacts the middle of the conversation history. Requests that already fit pass through unchanged and no extra model call is made. Context-window sizes are resolved from the live model catalog first -- the modelTotalTokens values on the preconfig profiles of the sibling llm_* nodes, which tools/sync_models keeps in step with the providers -- so ids such as gpt-5.4, claude-sonnet-4-6 and models/gemini-3.1-pro-preview resolve without this node hard-coding them. MODEL_LIMITS remains as a fallback for deployments without the llm_* nodes and for the abbreviated family aliases no provider publishes; it was refreshed against that catalog. This answers the open MODEL_LIMITS note from rocketride-org#527 without reaching for IInvokeLLM.GetContextLength(), which is only available after beginFilter. classType is "text" so the docs sidebar files the node under Text rather than the unmapped "transform" fallback category (packages/docs/scripts/lib/ gather.js NODE_CATEGORIES). util-text.svg is co-located because the engine resolves icons relative to services.json since 488d076 and otherwise logs "Icon : **** MISSING" and renders the unknown glyph. The unit suite guards that fallback: one test asserts MODEL_LIMITS and the live catalog never state a different window for an id they both carry, so a stale table entry fails loudly instead of changing the budget depending on whether the llm_* nodes are deployed. The catalog discovery test asserts structure -- non-empty mapping, positive integer windows, no discovered provider silently dropped -- rather than naming model ids that tools/ sync_models is free to rename. The services.json test block carries a comment noting that tiktoken downloads its BPE vocabulary on first use and that TIKTOKEN_CACHE_DIR pre-seeds it. Continues rocketride-org#527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Sep 4, 2026
RAG pipelines push a system prompt, the user question, retrieved documents and chat history into an LLM node with no accounting of the model's context window. When the total exceeds the limit the provider rejects the request or silently truncates, and no node in the catalog trims content by token budget before the LLM call. context_optimizer is a questions -> questions filter that counts tokens with tiktoken, allocates a per-component budget (system prompt / query / documents / history, configurable percentages), truncates on sentence boundaries, keeps the highest-scoring documents that fit, and compacts the middle of the conversation history. Requests that already fit pass through unchanged and no extra model call is made. Context-window sizes are resolved from the live model catalog first -- the modelTotalTokens values on the preconfig profiles of the sibling llm_* nodes, which tools/sync_models keeps in step with the providers -- so ids such as gpt-5.4, claude-sonnet-4-6 and models/gemini-3.1-pro-preview resolve without this node hard-coding them. MODEL_LIMITS remains as a fallback for deployments without the llm_* nodes and for the abbreviated family aliases no provider publishes; it was refreshed against that catalog. This answers the open MODEL_LIMITS note from rocketride-org#527 without reaching for IInvokeLLM.GetContextLength(), which is only available after beginFilter. classType is "text" so the docs sidebar files the node under Text rather than the unmapped "transform" fallback category (packages/docs/scripts/lib/ gather.js NODE_CATEGORIES). util-text.svg is co-located because the engine resolves icons relative to services.json since 488d076 and otherwise logs "Icon : **** MISSING" and renders the unknown glyph. The unit suite guards that fallback: one test asserts MODEL_LIMITS and the live catalog never state a different window for an id they both carry, so a stale table entry fails loudly instead of changing the budget depending on whether the llm_* nodes are deployed. The catalog discovery test asserts structure -- non-empty mapping, positive integer windows, no discovered provider silently dropped -- rather than naming model ids that tools/ sync_models is free to rename. The services.json test block carries a comment noting that tiktoken downloads its BPE vocabulary on first use and that TIKTOKEN_CACHE_DIR pre-seeds it. Continues rocketride-org#527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dsapandora
pushed a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Sep 8, 2026
RAG pipelines push a system prompt, the user question, retrieved documents and chat history into an LLM node with no accounting of the model's context window. When the total exceeds the limit the provider rejects the request or silently truncates, and no node in the catalog trims content by token budget before the LLM call. context_optimizer is a questions -> questions filter that counts tokens with tiktoken, allocates a per-component budget (system prompt / query / documents / history, configurable percentages), truncates on sentence boundaries, keeps the highest-scoring documents that fit, and compacts the middle of the conversation history. Requests that already fit pass through unchanged and no extra model call is made. Context-window sizes are resolved from the live model catalog first -- the modelTotalTokens values on the preconfig profiles of the sibling llm_* nodes, which tools/sync_models keeps in step with the providers -- so ids such as gpt-5.4, claude-sonnet-4-6 and models/gemini-3.1-pro-preview resolve without this node hard-coding them. MODEL_LIMITS remains as a fallback for deployments without the llm_* nodes and for the abbreviated family aliases no provider publishes; it was refreshed against that catalog. This answers the open MODEL_LIMITS note from rocketride-org#527 without reaching for IInvokeLLM.GetContextLength(), which is only available after beginFilter. classType is "text" so the docs sidebar files the node under Text rather than the unmapped "transform" fallback category (packages/docs/scripts/lib/ gather.js NODE_CATEGORIES). util-text.svg is co-located because the engine resolves icons relative to services.json since 488d076 and otherwise logs "Icon : **** MISSING" and renders the unknown glyph. The unit suite guards that fallback: one test asserts MODEL_LIMITS and the live catalog never state a different window for an id they both carry, so a stale table entry fails loudly instead of changing the budget depending on whether the llm_* nodes are deployed. The catalog discovery test asserts structure -- non-empty mapping, positive integer windows, no discovered provider silently dropped -- rather than naming model ids that tools/ sync_models is free to rename. The services.json test block carries a comment noting that tiktoken downloads its BPE vocabulary on first use and that TIKTOKEN_CACHE_DIR pre-seeds it. Continues rocketride-org#527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Sep 14, 2026
RAG pipelines push a system prompt, the user question, retrieved documents and chat history into an LLM node with no accounting of the model's context window. When the total exceeds the limit the provider rejects the request or silently truncates, and no node in the catalog trims content by token budget before the LLM call. context_optimizer is a questions -> questions filter that counts tokens with tiktoken, allocates a per-component budget (system prompt / query / documents / history, configurable percentages), truncates on sentence boundaries, keeps the highest-scoring documents that fit, and compacts the middle of the conversation history. Requests that already fit pass through unchanged and no extra model call is made. Context-window sizes are resolved from the live model catalog first -- the modelTotalTokens values on the preconfig profiles of the sibling llm_* nodes, which tools/sync_models keeps in step with the providers -- so ids such as gpt-5.4, claude-sonnet-4-6 and models/gemini-3.1-pro-preview resolve without this node hard-coding them. MODEL_LIMITS remains as a fallback for deployments without the llm_* nodes and for the abbreviated family aliases no provider publishes; it was refreshed against that catalog. This answers the open MODEL_LIMITS note from rocketride-org#527 without reaching for IInvokeLLM.GetContextLength(), which is only available after beginFilter. classType is "text" so the docs sidebar files the node under Text rather than the unmapped "transform" fallback category (packages/docs/scripts/lib/ gather.js NODE_CATEGORIES). util-text.svg is co-located because the engine resolves icons relative to services.json since 488d076 and otherwise logs "Icon : **** MISSING" and renders the unknown glyph. The unit suite guards that fallback: one test asserts MODEL_LIMITS and the live catalog never state a different window for an id they both carry, so a stale table entry fails loudly instead of changing the budget depending on whether the llm_* nodes are deployed. The catalog discovery test asserts structure -- non-empty mapping, positive integer windows, no discovered provider silently dropped -- rather than naming model ids that tools/ sync_models is free to rename. The services.json test block carries a comment noting that tiktoken downloads its BPE vocabulary on first use and that TIKTOKEN_CACHE_DIR pre-seeds it. Continues rocketride-org#527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
asclearuc
pushed a commit
that referenced
this pull request
Sep 15, 2026
…ement (continues #527) (#1489) * feat(nodes): add context_optimizer token-budget node RAG pipelines push a system prompt, the user question, retrieved documents and chat history into an LLM node with no accounting of the model's context window. When the total exceeds the limit the provider rejects the request or silently truncates, and no node in the catalog trims content by token budget before the LLM call. context_optimizer is a questions -> questions filter that counts tokens with tiktoken, allocates a per-component budget (system prompt / query / documents / history, configurable percentages), truncates on sentence boundaries, keeps the highest-scoring documents that fit, and compacts the middle of the conversation history. Requests that already fit pass through unchanged and no extra model call is made. Context-window sizes are resolved from the live model catalog first -- the modelTotalTokens values on the preconfig profiles of the sibling llm_* nodes, which tools/sync_models keeps in step with the providers -- so ids such as gpt-5.4, claude-sonnet-4-6 and models/gemini-3.1-pro-preview resolve without this node hard-coding them. MODEL_LIMITS remains as a fallback for deployments without the llm_* nodes and for the abbreviated family aliases no provider publishes; it was refreshed against that catalog. This answers the open MODEL_LIMITS note from #527 without reaching for IInvokeLLM.GetContextLength(), which is only available after beginFilter. classType is "text" so the docs sidebar files the node under Text rather than the unmapped "transform" fallback category (packages/docs/scripts/lib/ gather.js NODE_CATEGORIES). util-text.svg is co-located because the engine resolves icons relative to services.json since 488d076 and otherwise logs "Icon : **** MISSING" and renders the unknown glyph. The unit suite guards that fallback: one test asserts MODEL_LIMITS and the live catalog never state a different window for an id they both carry, so a stale table entry fails loudly instead of changing the budget depending on whether the llm_* nodes are deployed. The catalog discovery test asserts structure -- non-empty mapping, positive integer windows, no discovered provider silently dropped -- rather than naming model ids that tools/ sync_models is free to rename. The services.json test block carries a comment noting that tiktoken downloads its BPE vocabulary on first use and that TIKTOKEN_CACHE_DIR pre-seeds it. Continues #527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(nodes): list context_optimizer in the node catalog and changelog Every node PR merged since July adds a row to the docs/README-nodes.md catalog and a line under CHANGELOG.md [Unreleased] / Added; do the same for context_optimizer. The hand-maintained "121 node directories -> 151 services" sentence and the lane-ontology produced/consumed counts are left untouched: both are already stale on develop (137 directories / 183 services*.json today), so bumping them by one would propagate a wrong number and add conflict surface with Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(nodes): keep every question entry and hold every context_optimizer budget Addresses the six CodeRabbit threads on the rebased head of #1489. - IInstance.py:102 (Data Integrity, Major) -- FIXED. writeQuestions no longer collapses question.questions to its first entry. Every QuestionText is kept and rewritten in place; when the merged query is truncated the entries share the query allowance, each keeping a slice proportional to its own size via the new ContextOptimizer.truncate_each_to_budget. No other filter node on develop destroys the list (rerank_cohere/IInstance.py:55 and store_elasticsearch/IInstance.py:52 read questions[0] but forward it intact, guardrails/IInstance.py:58 joins every entry and forwards unchanged), while embedding_transformer/sentenceTransformer.py:135 writes an embedding per entry -- so the drop was losing real downstream data. - optimizer.py:334 (Functional Correctness, Minor) -- FIXED. resolve_model_limit builds one candidate list (full id, then the bare name for a scoped id) and uses it for both the catalog and MODEL_LIMITS. With an empty catalog, 'openai/gpt-5' resolved to DEFAULT_MODEL_LIMIT (128000) instead of 400000 and warned; it now resolves through the table, which is what the table is for. - optimizer.py:357 (Stability, Minor) -- FIXED. _parse_pct rejects NaN, which satisfied neither range check and then raised "ValueError: cannot convert float NaN to integer" inside allocate_budget, failing the whole optimize() call. inf/-inf keep clamping as before. - optimizer.py:541 (Functional Correctness, Minor) -- FIXED. truncate_to_budget re-measures the joined sentences before returning them. The per-sentence tally excludes the joining spaces, so the result could exceed max_tokens: measured, 'Alpha beta gamma. 12345 67890.' costs 11 cl100k tokens against a 10-token budget. - optimizer.py:588 (Functional Correctness, Minor) -- FIXED. summarize_history reserves the placeholder's widest wording (the omitted count is substituted only after the budget is fixed, and costs 5 more tokens than the bare marker), so the returned list stays inside max_tokens -- measured, 12 messages against a 25-token budget returned 29. The adjacent single-message path had the same defect (it ignored the role overhead the docstring promises to reserve) and is fixed the same way. - optimizer.py:762 (Refactor, Major) -- FIXED. optimize() derives the encoding from effective_model and threads it through every count, so a model override can no longer apply one model's limit with another model's tokenizer. The encoding is reported in metadata. 26 new tests; each fix was mutation-checked by reverting it and confirming the matching test fails. Suite: 124 passed under real tiktoken 0.14.0 and under the suite's whitespace stub. The three ci_tiny dynamic cases in services.json were replayed and produce identical results, so no expectation changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(nodes): return an empty history when the budget cannot cover role overhead `ContextOptimizer.summarize_history` promises never to exceed `max_tokens`, but two branches could still overshoot. Every returned message costs `count_tokens(role) + 4` before a single content token is spent, so when `max_tokens` is below that floor even an empty-content message breaks the contract: the `len(messages) == 1` branch and the `budget_for_recent <= 0` fallback both computed a negative `content_budget`, got `''` back from `truncate_to_budget`, and returned a ~5-token message anyway. Guard both branches with `if content_budget < 0: return []` and state the empty-list case in the docstring. A budget exactly equal to the overhead still returns one empty-content message, which costs exactly the budget. Adds `TestHistorySummarization::test_budget_below_role_overhead_returns_empty`, covering both branches across every budget below the floor plus the boundary at the floor itself. Mutation-checked by disabling the guard, which fails the new test. Reported by CodeRabbit as an outside-diff comment on 2026-07-08. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(nodes): tokenize provider-scoped model ids with the right encoding resolve_model_limit() already falls back from a provider-scoped id to its bare name, so 'openai/gpt-5' resolves the 400k gpt-5 context window. encoding_name_for_model() did not strip that prefix: the _MODEL_ENCODING_OVERRIDES lookup missed, tiktoken.encoding_for_model() raised KeyError for the scoped name and the count silently fell back to cl100k_base. The gpt-5 window was therefore measured with the gpt-4 tokenizer -- on Japanese text that overcounts by ~21% (510 vs 420 tokens for the same string), so the node truncates content it did not need to. encoding_name_for_model() now builds the same (full, bare) candidate tuple resolve_model_limit() uses and tries the override table and then tiktoken against each. Unscoped ids produce a one-element tuple and are byte-for-byte unaffected. Also validate model_name at the config boundary. Config.getNodeConfig() merges profile defaults into the raw pipeline config without type checking, so a hand-written "model_name": null reached rsplit() and raised AttributeError inside beginGlobal, taking the node down before it served a single record. Non-strings and blank strings now fall back to the documented DEFAULT_MODEL_NAME with a warning, matching how max_context_tokens and the budget percentages are already handled. Adds TestScopedIdEncoding (15 cases) and TestModelNameValidation (17 cases); the suite goes 125 -> 157. Both were mutation-checked by reverting each fix in isolation: 6 and 12 failures respectively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(nodes): context_optimizer reads the model catalog through the engine registry and budgets unknown models conservatively The catalog was discovered by globbing llm_*/services*.json relative to __file__, which only works while the node sits inside the nodes tree: a node installed into a user's store or materialised into --node_path has no siblings, the catalog came back empty, and an unknown model fell to the optimistic 128 000-token default -- four times the real window of a 32k model, so the provider rejected the request. - Read the catalog through the engine's own registry: getServiceDefinitions() for the index, getServiceDefinition() for each raw definition, keeping every definition that publishes preconfig.profiles[*].modelTotalTokens (chat, vision, embedding and search nodes alike). No filesystem access, no local JSONC parser, layout-independent. - Drop the class-level cache: the registry is re-read at every pipeline start and each optimizer keeps its own snapshot, so a reloaded registry is never masked by a stale one. - Replace the 128 000 default with a conservative fallback: the smallest window either the catalog or MODEL_LIMITS publishes (CONSERVATIVE_MODEL_LIMIT_FLOOR only if both are empty). Erring low costs unused context; erring high costs the request. Tests drive the production registry path through substituted rocketlib functions: real source-tree definitions, synthetic registries, unusable indexes, per-type failures, refresh without restart, and the no-filesystem guarantee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(nodes): count the un-budgeted prompt sections in context_optimizer `optimize()` totalled only the four budgeted components (system prompt, question, documents, history), but `Question.getPrompt()` also renders `instructions` (where the `prompt` node appends), `examples`, `context` (where retrieval lands), `goals`, and a skeleton of `###` headers, `Document N) Content:` prefixes and CRLF joins. A question whose four fields fit was therefore waved through pass 1 while the rendered prompt was already over the window -- the exact failure this node exists to prevent. `IInstance` now measures that part from the real rendering -- `getPrompt()` on a copy with the four budgeted fields emptied, counted with the node's own tokenizer -- and hands it to `optimize(overhead_tokens=...)`, which adds it to the pass-1 total and allocates the per-component budgets from `total_limit - overhead_tokens` instead of from the whole window. When the overhead alone fills the window every budget is 0, all four components are emptied and the node warns once, mirroring `summarize_history`'s refusal to overshoot a budget that cannot even cover a message's role overhead. README.md now states which fields are budgeted and which are only counted. Also: - Pin `tiktoken>=0.7.0,<1.0.0`, matching `chunker`. The node relies on `get_encoding()`, on `encoding_for_model()` raising `KeyError`, and on `Encoding.name`, none of which a 1.x major is bound by. - Read `max_context_tokens` with the shared `ai.common.utils.config_int` rather than a node-local try/except with identical semantics. The local parser's warning on a non-numeric value is dropped rather than pushed into `config_int`, which its other adopters rely on being silent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(nodes): recalibrate the context_optimizer ci_tiny profile for the measured overhead The CI-only `ci_tiny` profile was tuned for the old accounting: an 80-token window that an 81-token question just failed to fit, and a history budget that just held the summary placeholder. With the section markup counted (27 tokens for the history case) the placeholder no longer fit, and `test_dynamic.py::TestDynamicNodes::test_node_cases[context_optimizer:services:test2:ci_tiny]` failed on `history[1]`. The profile is now 112 tokens and the over-budget question is 160 words. Every case still exercises its truncation path and passes with margin under real cl100k tokens: the question is truncated to 125 characters (limit 160), the history budget is 29 tokens and keeps the summary placeholder, and the oversized document is still excluded (the cutover is at a 140-token window). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(nodes): re-home the context_optimizer docs onto the new node README schema The documentation revamp (#2139) deleted docs/develop/README-nodes.md and with it the hand-written node catalog table this PR appended a row to. The catalog is now generated at /nodes from each node's own README.md, and docs/development/nodes/readme-schema.md fixes that README's shape: scripts/validate-node-readme.py --all gates the whole corpus in docs:validate, so a node README that predates the schema fails the build. Restructure this node's README to the schema rather than resurrecting the deleted path: - drop the Docusaurus frontmatter and <head> block (the docs build now owns the page's title and position), and lead with an H1 matching the directory name plus a summary paragraph; - lift the lane table out of "What it does" into its own ## Lanes section; - add the required ## Profiles section — auto, manual and the test-only ci_tiny profile, with the declared default named in the introduction; - replace the hand-written field table under ## Configuration, which duplicated the generated ## Schema table, with per-field usage guidance, and move the algorithm detail ("what is budgeted", window resolution, offline tiktoken) under ## Notes where the schema puts it. The budget-percentage guidance is corrected while it moves: the optimizer scales a set summing over 100 back to 100 and only warns below 90, so the earlier "keep the total at or below 100" advice described a constraint the code does not impose. No behaviour change; documentation only. The generated PARAMS region is carried over untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: nihalnihalani <24360630+nihalnihalani@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#Hack-with-bay-2
Contribution Type
Feature — Context window optimizer for LLM token budget management
Problem / Use Case
RocketRide pipelines have no way to manage what fits in an LLM's context window. Users must manually estimate token counts and prune content. When pipelines chain documents + history + system prompts, context overflows cause silent truncation or API errors.
"Context engineering" is the #1 AI trend in 2026 — optimizing what information goes into LLM context.
Proposed Solution
A `context_optimizer` node with:
Why This Feature Fits This Codebase
The node processes Question objects from upstream nodes and optimizes their content before forwarding to LLM nodes. It uses `Config.getNodeConfig()` and the standard IGlobal/IInstance pattern. Token counting via tiktoken complements the existing token-aware features in `rocketlib/types.py` (IInvokeLLM.getTokenCounter).
Validation
68 tests passing. ruff check + format clean.
How This Could Be Extended
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores