Repository navigation
fix(ai): getNodeConfig drops top-level keys and delivers arrays as IJson when profile is set - #2039
Merged
dylan-savage merged 6 commits intoSep 3, 2026
Conversation
When connConfig has an explicit "profile" key, getNodeConfig only read
connConfig[profile], ignoring every key the caller wrote at the top
level (e.g. an "apikey" alongside "profile" instead of nested under
the profile name). The other branch (no "profile" key) already
overlays top-level keys on top of a nested profile-named block; the
two branches disagreed about where a caller's configuration could
live depending on whether "profile" happened to be set.
Extract that overlay into a shared helper (overlay_top_level) and use
it in both branches, so a key written at the top level is honoured
whether or not "profile" is set, and both branches agree on
precedence: profile defaults < nested profile block < real top-level
values. The "profile" selector key itself is now explicitly excluded
so it can't leak into the resolved config.
Confirmed independently against a real pipeline: an llm_anthropic
node configured with {"profile": "claude-sonnet-4-6", "apikey": "..."}
(flat) silently received no API key, while nesting the same apikey
under connConfig["claude-sonnet-4-6"] worked - reproducing exactly
what's described in rocketride-org#1839 defect 1.
Adds TestExplicitProfileBranchTopLevelFields covering the fix, and
renames the test class that pinned the old (dropped) behavior to
TestExplicitProfileBranchNestedShape since it no longer stands alone.
Fixes rocketride-org#1839
nishant-k02
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
August 18, 2026 20:00
Contributor
🤖 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. |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesProfile configuration resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Configuration now consistently applies top-level overrides for explicit profiles and returns native Python collection types. The covered behavior is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The pull request addresses both defects in issue Full details: Out of Scope Changes checkExplanation The changes remain within issue ✨ 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 |
Addresses rocketride-org#1839 defect 2: array/object config values arrive from the engine as IJson, not list/dict, so a node that type-checks its own declared field (isinstance(value, list)) silently falls through to its default. IJson.toDict already does the recursive conversion and is a no-op on values that are already native, so apply it once at the return boundary rather than pushing the conversion onto every node, per the issue's own suggested direction. Adds TestNativeConfigTypes (3 tests) and upgrades the test file's IJson stub from a shallow dict-only shim to one that actually wraps a value and recurses, matching the real rocketlib.types.IJson.toDict closely enough to exercise this. Also adds docstrings to the defect-1 tests to close out the docstring-coverage gap flagged in review. Full suite: 21/21 passed (up from 18; +3 new).
nishant-k02
changed the title
Closes out the docstring-coverage check, which scores the whole file: the pre-existing test methods (written before this PR) had none, relying on their names alone. Every function in the file is now documented; no behavior change.
Covers the four test classes (TestFlatShape, TestNestedShape, TestMixedShapePrecedence, TestExplicitProfileBranchNestedShape) that had per-method docstrings but no class-level summary of which connConfig shape they exercise. File is now fully documented at both the class and function level (35/35). No behavior change.
3 tasks done
rajeev-chaurasia
added a commit
to rajeev-chaurasia/rocketride-server
that referenced
this pull request
Aug 28, 2026
Both are real defects the PR's own additions found in shipped examples. document-processor names provider "anonymize", but that node declares "protocol": "anonymize_text://" and the provider is the protocol minus the scheme, so no such service exists and the component could never instantiate. The new provider check in validate_pipeline flags it; the old validator resolved the definition and silently ignored the miss. exa-search-working carries apikey beside "profile", where getNodeConfig never reads it, so the pipe named "working" resolved its key to the profile's empty default. Moving it inside the profile object is correct under the current resolver and under the one rocketride-org#2039 and rocketride-org#1979 propose. resolve_config reports it. Over every pipeline in the repo, resolve_config now reports no discarded keys.
Merged
4 of 7 tasks
joshuadarron
pushed a commit
that referenced
this pull request
Aug 28, 2026
…in validate_pipeline (#2082) * feat(mcp): add resolve_config, showing what a node actually receives A .pipe rarely says what a node gets. The engine merges profile defaults and, when a profile is set, reads the user layer only from the sub-object named after that profile, so a key written beside it is discarded in silence. The author of #1989 lost a day to exactly that, and the schema cannot express it: a schema describes what a field is, not that this field in this profile will be dropped. resolve_config runs the config through the same Config.getNodeConfig a node calls at load, and returns the resolved result plus the keys the resolver discarded, with a hint naming the object they belong in. Resolution has to happen engine-side. getServiceSchemas copies 15 keys into the catalog and preconfig is not among them (services.cpp:2100-2148), so profile resolution cannot be reproduced from rrext_services or /services. The tool reaches the engine through the existing seam rather than importing Config directly. modules/mcp/engine.py states that tools depend only on the EngineClient protocol so a later revision can swap the WS SDK for in-process calls without touching tool code; a direct import would have made this the first tool to break that. So this adds rrext_resolve_config next to rrext_services and rrext_validate, the matching SDK method, and one line on each side of the seam. Both argument checks default only a genuinely absent config: `or {}` would coerce a falsy non-object such as [] and skip the type check entirely. A test caught that. doc.md gains the tool row and its count moves to 28. * feat(mcp): add scaffold_node, emitting a node that loads on the first try Authoring a node means discovering its contract by failure, and the documented contract is wrong in three places, so reading the docs first does not help: - preconfig is marked optional in README-node-schema.md, but Config.getNodeConfig raises without it and nearly every IGlobal calls that at load. Of 170 services*.json in the tree, the only ones without preconfig are global-field files with no protocol plus a handful whose IGlobal never calls it. - depends() is documented as running in __init__.py, and the same page then contradicts itself. The real nodes call it from IGlobal.beginGlobal, and any third-party import has to follow it. - The "Adding a New Node" example shows a plain class with a process() method, which nothing in the engine calls. The contract is IGlobalBase/IInstanceBase. Two more the docs omit: a services.json without protocol is not registered as a service at all, and register must be filter or endpoint or the node loads but can never be instantiated. The provider name is the protocol, which is the mistake the issue opens with. scaffold_node emits all of that correctly, including the parent local_nodes/__init__.py and the local_nodes.<name> import path. class_type and lane_out are checked against the live catalog rather than a list here, so the allowed sets follow the engine instead of drifting as nodes are added. It returns files rather than writing them. _common.load_pipeline already refuses server-side reads because an MCP caller could otherwise reach the server's disk; writing deserves the same stance. The tests assert the emitted skeleton against the engine's real requirements: every generated Python file parses, the manifest carries the four keys that stop a node loading, depends() is in IGlobal and not __init__.py, and the handler matches the requested lane. Dropping preconfig from the template fails them. doc.md gains the tool row and its count moves to 29. * fix(client-python): let a non-object config reach the engine's type check resolve_config defaulted with `config or {}`, which coerces a falsy non-object such as [] into {} and hides the mistake the engine would have reported. Only an absent config is defaulted now, matching the handler it calls. * fix(mcp): report a config key the profile overwrote, not just an absent one resolve_config decided a key was discarded by checking whether it was missing from the resolved config. Nearly every profile in the catalog declares apikey, so an apikey written beside "profile" comes back present, holding the profile's value rather than the author's, and the tool stayed silent on the exact case it exists to surface. A key now counts as discarded when what the node receives differs from what the author wrote. Found by calling the tool over the MCP transport against a real llm_openai profile; the unit test missed it because its stub returned a resolved config without the key at all. * refactor(mcp): give the catalog lookups one job each _catalog_values branched on whether its key argument was the string 'lanes', so one function carried two different traversals. Splitting them drops the branch and the stringly-typed parameter, and the lane names are now filtered for strings the way the class types already were. * fix(mcp): harden scaffold_node against catalog and argument shapes Three defects found in review: A catalog entry whose classType is a bare string was iterated character by character, so 'text' admitted 't', 'e' and 'x' as class types and rejected the real one. Introduced when the lookup was split in two; the container check that guarded it is back. Dispatch does not apply the tool's inputSchema, so a non-string name reached the regex and raised TypeError instead of returning an actionable answer. Name, both lanes and class_type are now checked for type first. lane_in was validated only against the local handler templates, never against the catalog, so a lane with a template but no engine support produced a manifest with an input lane nothing serves. Also corrects the introspection group heading in doc.md, which still counted 5 tools and did not list scaffold.py as a source. * fix(mcp): report every profile sibling key, not only one whose value changed Comparing the resolved value against the author's missed the sibling that happens to match the profile's own value. The resolver never read that line either, so staying quiet left the author believing it was in effect when the key inside the profile is what applied. With a profile set every sibling is discarded, so the value is not worth comparing and the check is simpler for it. * fix(mcp): keep a supplied falsy argument out of the scaffold defaults lane_in, lane_out and class_type defaulted with `or`, so a supplied 0, '' or [] became the fallback and never reached the type check that would have reported it. Defaulting now applies only to an absent or null argument, matching the handler this PR already corrected on the client side. * feat(mcp): flag a component whose provider the engine cannot serve validate_pipeline passed a pipeline naming a provider that does not exist, so a typed name validated clean and failed later at run time. The engine's provider lookup lives in validate_pipeline.cpp's single-component path, which the whole pipeline path never reaches. The tool now checks each component against the engine's service catalog after the engine's own validation, and reports the component id with the name it could not resolve. The names come from the engine, so no rule moves into the client, and engine findings are added to rather than replaced. Closes the third of the four tools #1989 asked for; it was previously treated as already shipped in #1880, which covers the structural half only. * fix(mcp): report only the config keys a profile actually discarded resolve_config reported every sibling of the selected profile, so it named parameters on all 34 components in the repo's own pipelines that select one, and told the author to move it inside the profile object. parameters is not node configuration: the engine validates and consumes it on its own path (pipeline_config.cpp Rule 5 and Rule 6), and moving it would break the component. name is the same shape, and an editor-saved config keeps a sub-object per profile, so an unselected profile read as a discarded key too. Structural keys and the service's own profile names are now excluded. Over the same pipelines the report goes from 34 components to 1, and that one is a real finding: search_exa carries apikey beside the profile. The existing test passed because its config was hand-written with only apikey beside the profile. The new one is shaped like a real .pipe. Also drops the return in the scaffolded handler's preventDefault call, which raises, matching how the real nodes write it. * fix(examples): repair the two component configs these tools surfaced Both are real defects the PR's own additions found in shipped examples. document-processor names provider "anonymize", but that node declares "protocol": "anonymize_text://" and the provider is the protocol minus the scheme, so no such service exists and the component could never instantiate. The new provider check in validate_pipeline flags it; the old validator resolved the definition and silently ignored the miss. exa-search-working carries apikey beside "profile", where getNodeConfig never reads it, so the pipe named "working" resolved its key to the profile's empty default. Moving it inside the profile object is correct under the current resolver and under the one #2039 and #1979 propose. resolve_config reports it. Over every pipeline in the repo, resolve_config now reports no discarded keys.
4 of 7 tasks
Collaborator
|
Unifying both branches behind Two things before it can land:
Separately, #1806 is adding a placeholder-resolution step at the same return point in |
Resolves the conflict with rocketride-org#2166, which added Config._warnMisnamedKeys() to both getNodeConfig branches after this PR collapsed them into a single path. One call now sits on the unified path, so the warning still fires -- and unlike develop it also sees top-level keys on the explicit-profile branch. Two fixes on top of the merge: - ruff format: unwrap a call that fits on one line, and rephrase a docstring that opened with a quote character. The CI Ruff job was failing on `ruff format --check`. - merge(): require both sides to be dict-like before recursing. A scalar or list written over an object-valued profile default otherwise reaches .items() and raises AttributeError. Develop already crashes this way on the default-profile branch, and unifying the branches would have widened it to the explicit-profile branch. 94 dict-valued profile defaults ship today -- every LLM node's "capabilities", schema_validate's "category_metric_map", normalize_facts' "label_to_metric", autopipe's "remote" -- so this is reachable rather than theoretical. Four regression tests cover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R6gbzJyEdgWhSHPgswzw5L
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
rocketlib's IJson subclasses the engLib IJson the engine actually
constructs, and config.py imports the subclass. Every value the engine
produces is a base-class instance, so isinstance(value, IJson) is False
in production.
overlay_top_level() type-tested the nested profile block on that check,
so it took the else branch and used an empty dict -- discarding the whole
block. The node test harness builds exactly that shape
({"profile": "default", "default": {"apikey": ...}}), which is why every
node with mock credentials failed with "apikey is required", and why the
lane guardrail failed with "Model name was provided but is empty".
Develop carries the same broken check on its default-profile branch, but
harmlessly: when it fails it falls back to using connConfig whole. This
PR made the check the only path in, so failing it lost the config.
Normalize with IJson.toDict() first -- it tests against the correct base
class and is a no-op on already-native values -- then inspect the result.
This also removes the dict(<list>) path that raised
"dictionary update sequence element #0 has length 27".
The unit tests missed all of this because the _FakeIJson stub was a
single flat class that isinstance matched. Split it into _EngineIJson
(the engLib stand-in the engine constructs) and _FakeIJson (the rocketlib
subclass config.py imports) so the trap is reproducible, and add
TestEngineSuppliedConfig. Two of those four fail against the old code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R6gbzJyEdgWhSHPgswzw5L
dylan-savage
approved these changes
Sep 3, 2026
dylan-savage
added a commit
to Aryan-070/rocketride-server
that referenced
this pull request
Sep 4, 2026
…son when profile is set (rocketride-org#2039) * fix(ai): getNodeConfig drops top-level keys when profile is set When connConfig has an explicit "profile" key, getNodeConfig only read connConfig[profile], ignoring every key the caller wrote at the top level (e.g. an "apikey" alongside "profile" instead of nested under the profile name). The other branch (no "profile" key) already overlays top-level keys on top of a nested profile-named block; the two branches disagreed about where a caller's configuration could live depending on whether "profile" happened to be set. Extract that overlay into a shared helper (overlay_top_level) and use it in both branches, so a key written at the top level is honoured whether or not "profile" is set, and both branches agree on precedence: profile defaults < nested profile block < real top-level values. The "profile" selector key itself is now explicitly excluded so it can't leak into the resolved config. Confirmed independently against a real pipeline: an llm_anthropic node configured with {"profile": "claude-sonnet-4-6", "apikey": "..."} (flat) silently received no API key, while nesting the same apikey under connConfig["claude-sonnet-4-6"] worked - reproducing exactly what's described in rocketride-org#1839 defect 1. Adds TestExplicitProfileBranchTopLevelFields covering the fix, and renames the test class that pinned the old (dropped) behavior to TestExplicitProfileBranchNestedShape since it no longer stands alone. Fixes rocketride-org#1839 * fix(ai): normalize getNodeConfig's return value with IJson.toDict Addresses rocketride-org#1839 defect 2: array/object config values arrive from the engine as IJson, not list/dict, so a node that type-checks its own declared field (isinstance(value, list)) silently falls through to its default. IJson.toDict already does the recursive conversion and is a no-op on values that are already native, so apply it once at the return boundary rather than pushing the conversion onto every node, per the issue's own suggested direction. Adds TestNativeConfigTypes (3 tests) and upgrades the test file's IJson stub from a shallow dict-only shim to one that actually wraps a value and recurses, matching the real rocketlib.types.IJson.toDict closely enough to exercise this. Also adds docstrings to the defect-1 tests to close out the docstring-coverage gap flagged in review. Full suite: 21/21 passed (up from 18; +3 new). * docs(ai): add docstrings to remaining test methods in test_config_shapes Closes out the docstring-coverage check, which scores the whole file: the pre-existing test methods (written before this PR) had none, relying on their names alone. Every function in the file is now documented; no behavior change. * docs(ai): add class-level docstrings to remaining test classes Covers the four test classes (TestFlatShape, TestNestedShape, TestMixedShapePrecedence, TestExplicitProfileBranchNestedShape) that had per-method docstrings but no class-level summary of which connConfig shape they exercise. File is now fully documented at both the class and function level (35/35). No behavior change. * fix(ai): read the nested profile block via IJson.toDict, not isinstance rocketlib's IJson subclasses the engLib IJson the engine actually constructs, and config.py imports the subclass. Every value the engine produces is a base-class instance, so isinstance(value, IJson) is False in production. overlay_top_level() type-tested the nested profile block on that check, so it took the else branch and used an empty dict -- discarding the whole block. The node test harness builds exactly that shape ({"profile": "default", "default": {"apikey": ...}}), which is why every node with mock credentials failed with "apikey is required", and why the lane guardrail failed with "Model name was provided but is empty". Develop carries the same broken check on its default-profile branch, but harmlessly: when it fails it falls back to using connConfig whole. This PR made the check the only path in, so failing it lost the config. Normalize with IJson.toDict() first -- it tests against the correct base class and is a no-op on already-native values -- then inspect the result. This also removes the dict(<list>) path that raised "dictionary update sequence element #0 has length 27". The unit tests missed all of this because the _FakeIJson stub was a single flat class that isinstance matched. Split it into _EngineIJson (the engLib stand-in the engine constructs) and _FakeIJson (the rocketlib subclass config.py imports) so the trap is reproducible, and add TestEngineSuppliedConfig. Two of those four fail against the old code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R6gbzJyEdgWhSHPgswzw5L --------- Co-authored-by: Dylan Savage <dylanmsavage@hotmail.com>
erichanwang
pushed a commit
to erichanwang/rocketride-server
that referenced
this pull request
Sep 7, 2026
…in validate_pipeline (rocketride-org#2082) * feat(mcp): add resolve_config, showing what a node actually receives A .pipe rarely says what a node gets. The engine merges profile defaults and, when a profile is set, reads the user layer only from the sub-object named after that profile, so a key written beside it is discarded in silence. The author of rocketride-org#1989 lost a day to exactly that, and the schema cannot express it: a schema describes what a field is, not that this field in this profile will be dropped. resolve_config runs the config through the same Config.getNodeConfig a node calls at load, and returns the resolved result plus the keys the resolver discarded, with a hint naming the object they belong in. Resolution has to happen engine-side. getServiceSchemas copies 15 keys into the catalog and preconfig is not among them (services.cpp:2100-2148), so profile resolution cannot be reproduced from rrext_services or /services. The tool reaches the engine through the existing seam rather than importing Config directly. modules/mcp/engine.py states that tools depend only on the EngineClient protocol so a later revision can swap the WS SDK for in-process calls without touching tool code; a direct import would have made this the first tool to break that. So this adds rrext_resolve_config next to rrext_services and rrext_validate, the matching SDK method, and one line on each side of the seam. Both argument checks default only a genuinely absent config: `or {}` would coerce a falsy non-object such as [] and skip the type check entirely. A test caught that. doc.md gains the tool row and its count moves to 28. * feat(mcp): add scaffold_node, emitting a node that loads on the first try Authoring a node means discovering its contract by failure, and the documented contract is wrong in three places, so reading the docs first does not help: - preconfig is marked optional in README-node-schema.md, but Config.getNodeConfig raises without it and nearly every IGlobal calls that at load. Of 170 services*.json in the tree, the only ones without preconfig are global-field files with no protocol plus a handful whose IGlobal never calls it. - depends() is documented as running in __init__.py, and the same page then contradicts itself. The real nodes call it from IGlobal.beginGlobal, and any third-party import has to follow it. - The "Adding a New Node" example shows a plain class with a process() method, which nothing in the engine calls. The contract is IGlobalBase/IInstanceBase. Two more the docs omit: a services.json without protocol is not registered as a service at all, and register must be filter or endpoint or the node loads but can never be instantiated. The provider name is the protocol, which is the mistake the issue opens with. scaffold_node emits all of that correctly, including the parent local_nodes/__init__.py and the local_nodes.<name> import path. class_type and lane_out are checked against the live catalog rather than a list here, so the allowed sets follow the engine instead of drifting as nodes are added. It returns files rather than writing them. _common.load_pipeline already refuses server-side reads because an MCP caller could otherwise reach the server's disk; writing deserves the same stance. The tests assert the emitted skeleton against the engine's real requirements: every generated Python file parses, the manifest carries the four keys that stop a node loading, depends() is in IGlobal and not __init__.py, and the handler matches the requested lane. Dropping preconfig from the template fails them. doc.md gains the tool row and its count moves to 29. * fix(client-python): let a non-object config reach the engine's type check resolve_config defaulted with `config or {}`, which coerces a falsy non-object such as [] into {} and hides the mistake the engine would have reported. Only an absent config is defaulted now, matching the handler it calls. * fix(mcp): report a config key the profile overwrote, not just an absent one resolve_config decided a key was discarded by checking whether it was missing from the resolved config. Nearly every profile in the catalog declares apikey, so an apikey written beside "profile" comes back present, holding the profile's value rather than the author's, and the tool stayed silent on the exact case it exists to surface. A key now counts as discarded when what the node receives differs from what the author wrote. Found by calling the tool over the MCP transport against a real llm_openai profile; the unit test missed it because its stub returned a resolved config without the key at all. * refactor(mcp): give the catalog lookups one job each _catalog_values branched on whether its key argument was the string 'lanes', so one function carried two different traversals. Splitting them drops the branch and the stringly-typed parameter, and the lane names are now filtered for strings the way the class types already were. * fix(mcp): harden scaffold_node against catalog and argument shapes Three defects found in review: A catalog entry whose classType is a bare string was iterated character by character, so 'text' admitted 't', 'e' and 'x' as class types and rejected the real one. Introduced when the lookup was split in two; the container check that guarded it is back. Dispatch does not apply the tool's inputSchema, so a non-string name reached the regex and raised TypeError instead of returning an actionable answer. Name, both lanes and class_type are now checked for type first. lane_in was validated only against the local handler templates, never against the catalog, so a lane with a template but no engine support produced a manifest with an input lane nothing serves. Also corrects the introspection group heading in doc.md, which still counted 5 tools and did not list scaffold.py as a source. * fix(mcp): report every profile sibling key, not only one whose value changed Comparing the resolved value against the author's missed the sibling that happens to match the profile's own value. The resolver never read that line either, so staying quiet left the author believing it was in effect when the key inside the profile is what applied. With a profile set every sibling is discarded, so the value is not worth comparing and the check is simpler for it. * fix(mcp): keep a supplied falsy argument out of the scaffold defaults lane_in, lane_out and class_type defaulted with `or`, so a supplied 0, '' or [] became the fallback and never reached the type check that would have reported it. Defaulting now applies only to an absent or null argument, matching the handler this PR already corrected on the client side. * feat(mcp): flag a component whose provider the engine cannot serve validate_pipeline passed a pipeline naming a provider that does not exist, so a typed name validated clean and failed later at run time. The engine's provider lookup lives in validate_pipeline.cpp's single-component path, which the whole pipeline path never reaches. The tool now checks each component against the engine's service catalog after the engine's own validation, and reports the component id with the name it could not resolve. The names come from the engine, so no rule moves into the client, and engine findings are added to rather than replaced. Closes the third of the four tools rocketride-org#1989 asked for; it was previously treated as already shipped in rocketride-org#1880, which covers the structural half only. * fix(mcp): report only the config keys a profile actually discarded resolve_config reported every sibling of the selected profile, so it named parameters on all 34 components in the repo's own pipelines that select one, and told the author to move it inside the profile object. parameters is not node configuration: the engine validates and consumes it on its own path (pipeline_config.cpp Rule 5 and Rule 6), and moving it would break the component. name is the same shape, and an editor-saved config keeps a sub-object per profile, so an unselected profile read as a discarded key too. Structural keys and the service's own profile names are now excluded. Over the same pipelines the report goes from 34 components to 1, and that one is a real finding: search_exa carries apikey beside the profile. The existing test passed because its config was hand-written with only apikey beside the profile. The new one is shaped like a real .pipe. Also drops the return in the scaffolded handler's preventDefault call, which raises, matching how the real nodes write it. * fix(examples): repair the two component configs these tools surfaced Both are real defects the PR's own additions found in shipped examples. document-processor names provider "anonymize", but that node declares "protocol": "anonymize_text://" and the provider is the protocol minus the scheme, so no such service exists and the component could never instantiate. The new provider check in validate_pipeline flags it; the old validator resolved the definition and silently ignored the miss. exa-search-working carries apikey beside "profile", where getNodeConfig never reads it, so the pipe named "working" resolved its key to the profile's empty default. Moving it inside the profile object is correct under the current resolver and under the one rocketride-org#2039 and rocketride-org#1979 propose. resolve_config reports it. Over every pipeline in the repo, resolve_config now reports no discarded keys.
erichanwang
pushed a commit
to erichanwang/rocketride-server
that referenced
this pull request
Sep 7, 2026
…son when profile is set (rocketride-org#2039) * fix(ai): getNodeConfig drops top-level keys when profile is set When connConfig has an explicit "profile" key, getNodeConfig only read connConfig[profile], ignoring every key the caller wrote at the top level (e.g. an "apikey" alongside "profile" instead of nested under the profile name). The other branch (no "profile" key) already overlays top-level keys on top of a nested profile-named block; the two branches disagreed about where a caller's configuration could live depending on whether "profile" happened to be set. Extract that overlay into a shared helper (overlay_top_level) and use it in both branches, so a key written at the top level is honoured whether or not "profile" is set, and both branches agree on precedence: profile defaults < nested profile block < real top-level values. The "profile" selector key itself is now explicitly excluded so it can't leak into the resolved config. Confirmed independently against a real pipeline: an llm_anthropic node configured with {"profile": "claude-sonnet-4-6", "apikey": "..."} (flat) silently received no API key, while nesting the same apikey under connConfig["claude-sonnet-4-6"] worked - reproducing exactly what's described in rocketride-org#1839 defect 1. Adds TestExplicitProfileBranchTopLevelFields covering the fix, and renames the test class that pinned the old (dropped) behavior to TestExplicitProfileBranchNestedShape since it no longer stands alone. Fixes rocketride-org#1839 * fix(ai): normalize getNodeConfig's return value with IJson.toDict Addresses rocketride-org#1839 defect 2: array/object config values arrive from the engine as IJson, not list/dict, so a node that type-checks its own declared field (isinstance(value, list)) silently falls through to its default. IJson.toDict already does the recursive conversion and is a no-op on values that are already native, so apply it once at the return boundary rather than pushing the conversion onto every node, per the issue's own suggested direction. Adds TestNativeConfigTypes (3 tests) and upgrades the test file's IJson stub from a shallow dict-only shim to one that actually wraps a value and recurses, matching the real rocketlib.types.IJson.toDict closely enough to exercise this. Also adds docstrings to the defect-1 tests to close out the docstring-coverage gap flagged in review. Full suite: 21/21 passed (up from 18; +3 new). * docs(ai): add docstrings to remaining test methods in test_config_shapes Closes out the docstring-coverage check, which scores the whole file: the pre-existing test methods (written before this PR) had none, relying on their names alone. Every function in the file is now documented; no behavior change. * docs(ai): add class-level docstrings to remaining test classes Covers the four test classes (TestFlatShape, TestNestedShape, TestMixedShapePrecedence, TestExplicitProfileBranchNestedShape) that had per-method docstrings but no class-level summary of which connConfig shape they exercise. File is now fully documented at both the class and function level (35/35). No behavior change. * fix(ai): read the nested profile block via IJson.toDict, not isinstance rocketlib's IJson subclasses the engLib IJson the engine actually constructs, and config.py imports the subclass. Every value the engine produces is a base-class instance, so isinstance(value, IJson) is False in production. overlay_top_level() type-tested the nested profile block on that check, so it took the else branch and used an empty dict -- discarding the whole block. The node test harness builds exactly that shape ({"profile": "default", "default": {"apikey": ...}}), which is why every node with mock credentials failed with "apikey is required", and why the lane guardrail failed with "Model name was provided but is empty". Develop carries the same broken check on its default-profile branch, but harmlessly: when it fails it falls back to using connConfig whole. This PR made the check the only path in, so failing it lost the config. Normalize with IJson.toDict() first -- it tests against the correct base class and is a no-op on already-native values -- then inspect the result. This also removes the dict(<list>) path that raised "dictionary update sequence element #0 has length 27". The unit tests missed all of this because the _FakeIJson stub was a single flat class that isinstance matched. Split it into _EngineIJson (the engLib stand-in the engine constructs) and _FakeIJson (the rocketlib subclass config.py imports) so the trap is reproducible, and add TestEngineSuppliedConfig. Two of those four fail against the old code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R6gbzJyEdgWhSHPgswzw5L --------- Co-authored-by: Dylan Savage <dylanmsavage@hotmail.com>
erichanwang
pushed a commit
to erichanwang/rocketride-server
that referenced
this pull request
Sep 7, 2026
…son when profile is set (rocketride-org#2039) * fix(ai): getNodeConfig drops top-level keys when profile is set When connConfig has an explicit "profile" key, getNodeConfig only read connConfig[profile], ignoring every key the caller wrote at the top level (e.g. an "apikey" alongside "profile" instead of nested under the profile name). The other branch (no "profile" key) already overlays top-level keys on top of a nested profile-named block; the two branches disagreed about where a caller's configuration could live depending on whether "profile" happened to be set. Extract that overlay into a shared helper (overlay_top_level) and use it in both branches, so a key written at the top level is honoured whether or not "profile" is set, and both branches agree on precedence: profile defaults < nested profile block < real top-level values. The "profile" selector key itself is now explicitly excluded so it can't leak into the resolved config. Confirmed independently against a real pipeline: an llm_anthropic node configured with {"profile": "claude-sonnet-4-6", "apikey": "..."} (flat) silently received no API key, while nesting the same apikey under connConfig["claude-sonnet-4-6"] worked - reproducing exactly what's described in rocketride-org#1839 defect 1. Adds TestExplicitProfileBranchTopLevelFields covering the fix, and renames the test class that pinned the old (dropped) behavior to TestExplicitProfileBranchNestedShape since it no longer stands alone. Fixes rocketride-org#1839 * fix(ai): normalize getNodeConfig's return value with IJson.toDict Addresses rocketride-org#1839 defect 2: array/object config values arrive from the engine as IJson, not list/dict, so a node that type-checks its own declared field (isinstance(value, list)) silently falls through to its default. IJson.toDict already does the recursive conversion and is a no-op on values that are already native, so apply it once at the return boundary rather than pushing the conversion onto every node, per the issue's own suggested direction. Adds TestNativeConfigTypes (3 tests) and upgrades the test file's IJson stub from a shallow dict-only shim to one that actually wraps a value and recurses, matching the real rocketlib.types.IJson.toDict closely enough to exercise this. Also adds docstrings to the defect-1 tests to close out the docstring-coverage gap flagged in review. Full suite: 21/21 passed (up from 18; +3 new). * docs(ai): add docstrings to remaining test methods in test_config_shapes Closes out the docstring-coverage check, which scores the whole file: the pre-existing test methods (written before this PR) had none, relying on their names alone. Every function in the file is now documented; no behavior change. * docs(ai): add class-level docstrings to remaining test classes Covers the four test classes (TestFlatShape, TestNestedShape, TestMixedShapePrecedence, TestExplicitProfileBranchNestedShape) that had per-method docstrings but no class-level summary of which connConfig shape they exercise. File is now fully documented at both the class and function level (35/35). No behavior change. * fix(ai): read the nested profile block via IJson.toDict, not isinstance rocketlib's IJson subclasses the engLib IJson the engine actually constructs, and config.py imports the subclass. Every value the engine produces is a base-class instance, so isinstance(value, IJson) is False in production. overlay_top_level() type-tested the nested profile block on that check, so it took the else branch and used an empty dict -- discarding the whole block. The node test harness builds exactly that shape ({"profile": "default", "default": {"apikey": ...}}), which is why every node with mock credentials failed with "apikey is required", and why the lane guardrail failed with "Model name was provided but is empty". Develop carries the same broken check on its default-profile branch, but harmlessly: when it fails it falls back to using connConfig whole. This PR made the check the only path in, so failing it lost the config. Normalize with IJson.toDict() first -- it tests against the correct base class and is a no-op on already-native values -- then inspect the result. This also removes the dict(<list>) path that raised "dictionary update sequence element #0 has length 27". The unit tests missed all of this because the _FakeIJson stub was a single flat class that isinstance matched. Split it into _EngineIJson (the engLib stand-in the engine constructs) and _FakeIJson (the rocketlib subclass config.py imports) so the trap is reproducible, and add TestEngineSuppliedConfig. Two of those four fail against the old code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R6gbzJyEdgWhSHPgswzw5L --------- Co-authored-by: Dylan Savage <dylanmsavage@hotmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 of 7 tasks
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
3 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.
Description
Fixes both defects from #1839.
Defect 1 — with an explicit
"profile"key set,getNodeConfigonly readconnConfig[profile], silently dropping every key the caller wrote at the top level (e.g. an"apikey"sitting alongside"profile"instead of nested under the profile name). The sibling branch (no"profile"key) already overlays top-level keys on top of a nested profile-named block — the two branches disagreed about where configuration could live depending on whether"profile"happened to be set.Defect 2 — array/object config values arrive from the engine as
IJson, notlist/dict, so a node that type-checks its own declared field (isinstance(value, list)) silently falls through to its default and reports success while ignoring what was configured.Fix
overlay_top_levelhelper and used it in both branches, so a key written at the top level is honoured regardless of whether"profile"is set. The"profile"selector key itself is explicitly excluded so it can't leak into the resolved config.getNodeConfignow returnsIJson.toDict(config)instead of the rawconfig. Verified against the realrocketlib.types.IJson.toDict(recursive, no-op on already-native values) that this is safe to apply unconditionally at the return boundary rather than pushing the conversion onto every node.Precedence, both branches now agree:
profile defaults < nested profile block < real top-level values(top-level wins).A note on #1979
I noticed #1979 (open, same files) after starting this PR. It fixes the same reported symptom but takes the opposite precedence — profile-specific (nested) keys win over top-level — and only touches the explicit-
profilebranch, not the default-profile branch. Since the default-profile branch's existing, currently-shipped, tested behavior already has top-level winning over nested (test_top_level_overrides_nested_value,test_real_top_level_beats_empty_nested_defaultintest_config_shapes.py), #1979's change would leave the two branches disagreeing again — just with the opposite branch now being the odd one out, which is the exact meta-problem #1839 raised ("the two branches disagree about where a user's configuration lives"). I chose to match the already-shipped precedent instead of the issue's literal suggested text, so the fix is a pure bug fix rather than a behavior change to already-shipped, tested resolution. Flagging this explicitly since it's a genuine design decision the issue author (Aryan-070) also called out as unresolved — happy to adjust if maintainers prefer #1979's direction, but in that case the default-profile branch would need the same precedence flip for consistency, which is a larger, more disruptive change to existing behavior.Testing performed
TestExplicitProfileBranchTopLevelFields(4 tests, defect 1) — top-level fields resolve with an explicit profile, top-level overrides a nested value, aNonetop-level placeholder doesn't clobber nested, and"profile"itself doesn't leak into the resolved config.TestNativeConfigTypes(3 tests, defect 2) — an IJson-wrapped array resolves to a nativelist, nested IJson-wrapped objects resolve recursively, and an already-native default value is unaffected. Upgraded the test file'sIJsonstub from a shallow dict-only shim to one that actually wraps a value and recurses, so these tests exercise real conversion behavior rather than a no-op.TestExplicitProfileBranchUnaffected→TestExplicitProfileBranchNestedShapesince its docstring/name asserted the branch was unaffected by config-shape handling, which is no longer accurate.How I found this
Building an app against RocketRide Cloud, I configured an
llm_anthropicnode as:{ "profile": "claude-sonnet-4-6", "apikey": "sk-ant-..." }and got
Invalid Anthropic API key formateven though the key was valid — theapikeyvalue was silently discarded. Nesting the same key underconnConfig["claude-sonnet-4-6"]worked, which is exactly the shape described in #1839 defect 1.Breaking changes
None for existing shapes. Flat config (no profile) and nested-under-profile config continue to resolve exactly as before; this adds resolution for a shape (top-level keys + explicit profile) that previously silently failed, and normalizes array/object return values to native types (previously-broken
isinstancechecks in nodes will now correctly see their configured values instead of silently falling back to defaults).Fixes #1839
Summary by CodeRabbit
Bug Fixes
Tests