feat(nodes): add prompt template node with variable substitution#573
feat(nodes): add prompt template node with variable substitution#573charliegillet wants to merge 8 commits intorocketride-org:developfrom
Conversation
charliegillet
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
github-actions
Bot
added
module:nodes
|
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 new Prompt Template node: lifecycle handlers (IGlobal/IInstance), a pure-Python Jinja2-like template engine, service registration and tests to render templates for text and question lanes using configurable variables. Changes
Sequence DiagramsequenceDiagram
participant Client as External System
participant IGlobal as IGlobal (Lifecycle)
participant IInstance as IInstance (Processor)
participant TemplateEngine as Template Engine
Note over IGlobal: beginGlobal()
IGlobal->>IGlobal: load dependencies & set config
Client->>IInstance: writeText()/writeQuestions()
IInstance->>IInstance: accumulate inputs
Note over IInstance: closing()
IInstance->>IGlobal: read config
IInstance->>IInstance: build context
IInstance->>TemplateEngine: render(template, context)
TemplateEngine->>TemplateEngine: tokenize & evaluate
TemplateEngine-->>IInstance: rendered output
alt Accumulated Questions
IInstance->>IInstance: for each question set context['question']
IInstance->>TemplateEngine: render per question
TemplateEngine-->>IInstance: rendered questions
IInstance->>Client: writeQuestions()
else Only Text
IInstance->>Client: writeText()
end
Note over IGlobal: endGlobal()
IGlobal->>IGlobal: clear config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
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/prompt_template/IInstance.py`:
- Around line 69-84: The closing method currently catches all exceptions and
only calls debug(), which hides failures; update IInstance.closing to log the
exception (keeping debug or use processLogger if available) and then re-raise
the exception so callers see the failure; specifically, after the try block
around config = self.IGlobal.config / template/render(context) /
self.instance.writeQuestions / self.instance.writeText, ensure you re-raise the
caught exception (or raise a new descriptive exception) instead of just
returning, so errors from render(self._get_template_context(), template) or
missing config propagate to the caller.
In `@nodes/src/nodes/prompt_template/services.json`:
- Around line 120-134: The test case under the "test" service only asserts
non-empty output for "questions"; add a second case that validates actual
template substitution by providing a specific template input (e.g., a question
template with a placeholder) and corresponding variables and asserting the
expected rendered string; update the "cases" array to include a case where
"questions": {"questions": [{"text": "Hello, {{name}}"}], "vars":
{"name":"Alice"}} (or equivalent keys used by the prompt template processor) and
an "expect" that checks the output equals "Hello, Alice" so the prompt_template
logic (the "test" service, its "cases" and "questions" entries) verifies real
substitution rather than only notEmpty.
- Around line 24-26: Remove the unnecessary "gpu" capability from the
prompt_template node configuration: open the services.json entry for the
prompt_template node and delete "gpu" from the "capabilities" array so it
matches other non-ML nodes (e.g., dictionary) and only ML/transformer nodes
(ner, embedding_transformer, ocr, anonymize) declare GPU capability.
In `@nodes/src/nodes/prompt_template/template_engine.py`:
- Around line 67-82: The resolution currently prefers built-ins by checking
_BUILTINS before the provided context in _resolve, preventing users from
overriding names like "now", "uuid", or "random"; change the lookup order in
_resolve so it first tries to resolve the dotted name against the provided
context (using dict.get or getattr with _MISSING), and only if that yields
_MISSING should it fall back to calling the corresponding factory in _BUILTINS;
ensure you still return '' when resolution results in _MISSING and keep the
dotted traversal logic intact for nested context lookups.
🪄 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: 59a4dd88-832d-484d-878a-c2d9ade495de
📒 Files selected for processing (8)
apps/vscode/src/providers/views/PageStatus/PageStatus.tsxapps/vscode/src/providers/views/PageStatus/styles.cssnodes/src/nodes/prompt_template/IGlobal.pynodes/src/nodes/prompt_template/IInstance.pynodes/src/nodes/prompt_template/__init__.pynodes/src/nodes/prompt_template/requirements.txtnodes/src/nodes/prompt_template/services.jsonnodes/src/nodes/prompt_template/template_engine.py
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thank you for this contribution — the template engine is well thought out. The tokenizer, recursive block evaluator, and nested depth tracking are all solid, and avoiding a Jinja2 dependency is the right call for this scope.
A few things to address before merging:
Remove "gpu" from capabilities
This node does pure string rendering — no ML, no tensors, no GPU operations. Declaring "gpu" will cause the pipeline scheduler to require a GPU-equipped machine for this node, wasting resources. Remove it.
_get_template_context mutates the shared config dict
variables is a static dict set once in node configuration and stored in self.IGlobal.config, shared across all instances for the entire pipeline run. _get_template_context returns a direct reference to it rather than a copy:
variables = config.get('variables', {}) # live reference, not a copy
variables.setdefault('input', '...') # mutates shared config
return variables # caller mutates it further
Please pay attention also to CodeRabbit's comments
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/prompt_template/IInstance.py`:
- Around line 43-54: The _get_template_context method can raise AttributeError
when self.IGlobal.config is None; update _get_template_context to first check if
self.IGlobal is truthy and self.IGlobal.config is a dict-like (not None) and if
not return an empty variables dict (still adding collected_text as 'input' if
present), or safely obtain config via something like config =
getattr(self.IGlobal, "config", None) and fall back to {} before calling get;
ensure any callers like writeQuestions that rely on this method will receive a
safe empty context instead of raising.
🪄 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: 3b019647-9ee4-4155-98ed-058f8313d26c
📒 Files selected for processing (3)
nodes/src/nodes/prompt_template/IInstance.pynodes/src/nodes/prompt_template/services.jsonnodes/src/nodes/prompt_template/template_engine.py
|
Addressed all review feedback:
Thanks for the thorough review! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/prompt_template/IInstance.py`:
- Around line 48-50: The code calls dict(config.get('variables', {})) which can
raise if config['variables'] is None or non-mapping; change to first retrieve
raw = config.get('variables') (or use config.get('variables', {})), validate
that raw is a dict via isinstance(raw, dict), and only then set variables =
dict(raw); otherwise set variables = {} so the fallback runs safely—update the
logic in IInstance.py where variables is initialized to use this guarded
pattern.
- Around line 58-66: writeQuestions currently renders each question immediately
using _get_template_context(), which makes output order-dependent because later
writeText() calls can modify the context (e.g., {{input}}); instead, change
writeQuestions to store incoming question texts (e.g., push q.text into a
questions buffer on the instance or into self.question as raw entries) without
calling render or question.addQuestion, and move the rendering logic into
closing() where you call _get_template_context() once with the final context and
then render each buffered question with render(template, final_context) before
calling question.addQuestion; update symbols affected: writeQuestions, closing,
_get_template_context, question.addQuestion, render, and IGlobal.config/template
handling.
🪄 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: 318de843-acad-4c44-aaab-7fa5ec7e8db5
📒 Files selected for processing (1)
nodes/src/nodes/prompt_template/IInstance.py
|
@asclearuc Addressed all review feedback: Fixed in this push:
Previously addressed:
CodeRabbit comments reviewed:
|
Senior Review — PR #573: feat(nodes): add prompt template nodeNice work here, @charliegillet. You've been thorough in addressing asclearuc's feedback across both rounds, and the CodeRabbit items have been handled as well. The implementation looks solid. Status: All previously requested changes appear to be addressed. This looks ready for a re-review to verify and move toward approval. One item to double-check: In the except Exception as e:
logger.error(...)it should be: except Exception as e:
logger.error(...)
raiseSwallowing exceptions during cleanup can mask resource leaks or downstream failures. If this is already handled, disregard. Overall, this is in great shape. Requesting a re-review from the maintainers to get this across the finish line. |
🚀 Merge RequestAll feedback from asclearuc has been addressed across 2 rounds. Author has been thorough and responsive. Status: All review items resolved. Ready for re-review and merge. @asclearuc |
|
No description provided. |
|
@asclearuc Thanks for the review! All feedback has been addressed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/prompt_template/services.json`:
- Around line 120-145: Add a new test case to services.json that exercises
per-question variable substitution for IInstance.closing(): create a case with
"config" specifying a template that uses {{question}} (e.g., "Answer:
{{question}}"), provide no global "input" but set "questions":
{"questions":[{"text":"What is 2+2?"}]} and add an "expect" asserting the output
"questions" contains the substituted string (e.g., contains "Answer: What is
2+2?"); reference the existing "config"/"template", "questions" array, and
IInstance.closing() behavior so the test explicitly verifies question-level
interpolation.
In `@nodes/src/nodes/prompt_template/template_engine.py`:
- Line 67: Add explicit return type annotations: change the signature of
_resolve to return Any and the three block handlers _at_top_level, _handle_if,
and _handle_for to return int. Update their function signatures accordingly
(e.g., def _resolve(...) -> Any: and def _at_top_level(...) -> int: etc.), and
import Any from typing if it isn’t already imported so the annotations satisfy
Ruff ANN202.
🪄 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: 5a79d197-46da-4227-ae16-632c82383754
📒 Files selected for processing (3)
nodes/src/nodes/prompt_template/IInstance.pynodes/src/nodes/prompt_template/services.jsonnodes/src/nodes/prompt_template/template_engine.py
|
Thanks for the work here. One blocker before this can merge: there are no tests for At minimum, the tests should cover all the functions from |
| // Capabilities are flags that change the behavior of the underlying | ||
| // engine | ||
| // | ||
| "capabilities": [], |
There was a problem hiding this comment.
Let mark this node as an experimental one.
| "capabilities": [], | |
| "capabilities": ["experimental"], |
There was a problem hiding this comment.
Fixed — marked the node as "experimental" in services.json. Commit 6538ad4.
Adds 57 unit tests covering the custom template engine (stdlib-only, no Jinja2) in nodes/test/test_prompt_template_engine.py — variable substitution, dotted paths, missing variables, builtins, context-over- builtin overrides, if/elif/else, nested blocks, for loops, escape sequences, malformed/unclosed tags, HTML pass-through, and ReDoS / recursion safety. The test module loads template_engine.py directly via importlib so it does not trigger the depends-based package init. Also addresses outstanding review feedback on PR rocketride-org#573: - Mark the node as experimental in services.json (per asclearuc) - Add an explicit {{question}} substitution test case in services.json (per CodeRabbit) - Add missing return type annotations on _resolve (Any), _at_top_level (bool), _handle_if (int) and _handle_for (int) to satisfy Ruff ANN202 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@asclearuc Added comprehensive unit tests for Also addressed the remaining feedback in the same commit (6538ad4):
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/prompt_template/services.json`:
- Around line 118-161: Add a new test case in
nodes/src/nodes/prompt_template/services.json that exercises the text-only
render path so IInstance.closing()'s elif self.collected_text branch is covered;
create a case with no "questions" field and instead include a "text" lane (e.g.,
"text": {"text": "some rendered string"} or the test harness equivalent used by
other tests) and set expect to assert that the returned "text" is not empty
and/or contains the expected rendered content from the template/variables;
ensure the case uses the same "config" style as other tests if needed so the
prompt/template rendering produces the text-only output to trigger closing()'s
collected_text path.
In `@nodes/test/test_prompt_template_engine.py`:
- Around line 339-350: The test test_large_literal_input_runs_quickly currently
enforces a hard wall-clock threshold (assert elapsed < 2.0) which flakes on CI;
remove that strict timing assertion and either (a) drop the elapsed check
entirely and only assert correctness of render(template, {'v':'MID'}) contains
'MID', or (b) convert the test to use a test-timeout mechanism (e.g., add a
pytest.mark.timeout decorator with a generous timeout) so the test only fails on
extreme hangs but won't flake due to transient slowness; update the test
function test_large_literal_input_runs_quickly accordingly and keep references
to the template construction and render(...) call.
🪄 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: 6a8e7f98-7c88-472f-81d2-ee95a91235e8
📒 Files selected for processing (3)
nodes/src/nodes/prompt_template/services.jsonnodes/src/nodes/prompt_template/template_engine.pynodes/test/test_prompt_template_engine.py
| "test": { | ||
| "profiles": ["default"], | ||
| "cases": [ | ||
| { | ||
| "questions": { | ||
| "questions": [{"text": "Hello world"}] | ||
| }, | ||
| "expect": { | ||
| "questions": { | ||
| "notEmpty": true | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "config": { | ||
| "template": "Summarize the following: {{input}}", | ||
| "variables": {"input": "The quick brown fox jumps over the lazy dog."} | ||
| }, | ||
| "questions": { | ||
| "questions": [{"text": "test prompt"}] | ||
| }, | ||
| "expect": { | ||
| "questions": { | ||
| "notEmpty": true, | ||
| "contains": "Summarize the following:" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "config": { | ||
| "template": "Q={{question}} | I={{input}}", | ||
| "variables": {"input": "ctx"} | ||
| }, | ||
| "questions": { | ||
| "questions": [{"text": "what is this?"}] | ||
| }, | ||
| "expect": { | ||
| "questions": { | ||
| "contains": "Q=what is this?" | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one text-lane test case to cover the text-only render path.
Current cases validate only questions output. The nodes/src/nodes/prompt_template/IInstance.py closing() logic has a separate text branch (elif self.collected_text), so this branch should also be exercised in services.json test cases to prevent lane-specific regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/src/nodes/prompt_template/services.json` around lines 118 - 161, Add a
new test case in nodes/src/nodes/prompt_template/services.json that exercises
the text-only render path so IInstance.closing()'s elif self.collected_text
branch is covered; create a case with no "questions" field and instead include a
"text" lane (e.g., "text": {"text": "some rendered string"} or the test harness
equivalent used by other tests) and set expect to assert that the returned
"text" is not empty and/or contains the expected rendered content from the
template/variables; ensure the case uses the same "config" style as other tests
if needed so the prompt/template rendering produces the text-only output to
trigger closing()'s collected_text path.
Latest fixesCommit
Commit
|
|
@asclearuc Thanks for the thorough reviews. Everything you raised is addressed on the current HEAD (28a9e9e):
The tests load Ready for re-review whenever you have a moment. |
|
@nihalnihalani Thanks for the senior review. To confirm your one double-check: yes, except Exception as e:
debug(f'Error in prompt_template node: {e}')
raiseFixed in 6c5f7f5 per CodeRabbit's earlier finding. Exceptions propagate to the pipeline engine as expected. |
Add a new prompt_template node that renders prompt templates with dynamic
variable substitution. Features include:
- {{variable}} placeholder replacement from pipeline context
- Built-in helpers: {{now}}, {{uuid}}, {{random}}
- Conditional sections: {% if var %}...{% endif %}
- Loop support: {% for item in list %}...{% endfor %}
- Escape sequences for literal braces
- Zero external dependencies (stdlib only)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unnecessary "gpu" capability from services.json - Fix _get_template_context to copy variables dict instead of mutating shared config - Re-raise exceptions in closing() instead of silently swallowing them - Fix _resolve to check context before falling back to built-in helpers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents AttributeError when config is None during CONFIG mode or after endGlobal(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix variables dict copy: validate type before calling dict() to avoid TypeError when config['variables'] is None - Defer question rendering to closing() so templates have access to all text collected via writeText(), fixing order-dependent rendering - Remove unrelated PageStatus.tsx and styles.css changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Restructure _resolve() so context is checked before builtins with clear separation and comments documenting the priority order - Add comprehensive test case validating actual template substitution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds 57 unit tests covering the custom template engine (stdlib-only, no Jinja2) in nodes/test/test_prompt_template_engine.py — variable substitution, dotted paths, missing variables, builtins, context-over- builtin overrides, if/elif/else, nested blocks, for loops, escape sequences, malformed/unclosed tags, HTML pass-through, and ReDoS / recursion safety. The test module loads template_engine.py directly via importlib so it does not trigger the depends-based package init. Also addresses outstanding review feedback on PR rocketride-org#573: - Mark the node as experimental in services.json (per asclearuc) - Add an explicit {{question}} substitution test case in services.json (per CodeRabbit) - Add missing return type annotations on _resolve (Any), _at_top_level (bool), _handle_if (int) and _handle_for (int) to satisfy Ruff ANN202 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stepmikhaylov
force-pushed
the
feature/prompt-template-node
branch
from
28a9e9e to
8d0c8dd
Compare
|
The branch rebased to force ci/cd workflow to run all nodes unit tests. |
|
CI/CD checks failed |
4 participants
Summary
prompt_templatenode that renders prompt templates with dynamic variable substitution from pipeline context{{variable}}placeholders, built-in helpers ({{now}},{{uuid}},{{random}}), conditional sections ({% if %}/{% elif %}/{% else %}/{% endif %}), and loop support ({% for item in list %})Implementation
services.jsonIGlobal.pyConfig.getNodeConfigIInstance.pytemplate_engine.pyrequirements.txtTest plan
{{variable}}substitution renders correctly from configured variables{{now}},{{uuid}},{{random}}) produce valid output{% if %}/{% else %}/{% endif %}conditional rendering{% for item in list %}loop rendering\{\{render as literal{{ruff formatandruff check— all passing#Hack-with-bay-2
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests