fix(ai): getNodeConfig drops top-level keys and delivers arrays as IJson when profile is set - #2039
Open
nishant-k02 wants to merge 4 commits into
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesProfile configuration resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR fixes configuration values being dropped or returned in unusable types; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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).
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
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
1 participant
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