Feat/1430 authoritative overlay - #1485
Open
kiet08hogit wants to merge 7 commits into
Open
Conversation
kiet08hogit
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
July 7, 2026 02:49
github-actions
Bot
added
docs
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:
📝 WalkthroughWalkthroughThis PR adds a new ChangesAuthoritative Overlay Node
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IInstance
participant Connector
participant Snapshot
IInstance->>IInstance: normalize extracted text
IInstance->>Connector: query_<regulator>(concept, extracted_text)
Connector->>Snapshot: load testdata JSON snapshot
Snapshot-->>Connector: official data or None
Connector-->>IInstance: official data or None
alt match found
IInstance->>IInstance: forward answer downstream
else no match or error
IInstance->>IInstance: preventDefault (abstain)
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
…ocketride-org#1430) - Created node to cross-check extracted financial answers against official regulator data. - Added local snapshot testing connectors for SEC, IFRS, Companies House, and EDINET. - Implemented logic to abstain and drop answers when a mismatch is detected.
kiet08hogit
force-pushed
the
feat/1430-authoritative-overlay
branch
from
July 7, 2026 02:55
67c5c26 to
3fd2fe1
Compare
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/engine-lib/rocketlib-python/lib/depends.py (1)
787-805: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring not updated to mention Polars exclusion.
The function's docstring documents the
uv/onnxruntimeexclusions but not the new Polars logic, which could confuse future maintainers.📝 Suggested docstring update
"""Write uv's resolution-excludes file (rewritten each call) and return its path. Excludes `uv` (bootstrapped by depends.py; pip-installing it crashes on Windows) and, on non-Darwin, plain `onnxruntime` (it clobbers onnxruntime-gpu in the same - folder; the gpu build provides `import onnxruntime`). + folder; the gpu build provides `import onnxruntime`). Also excludes whichever + of `polars`/`polars-lts-cpu` doesn't match the host's AVX2 support. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/engine-lib/rocketlib-python/lib/depends.py` around lines 787 - 805, The docstring for _write_excludes_file() is missing the new Polars exclusion behavior. Update the function’s documentation to mention that it now also excludes either polars or polars-lts-cpu depending on _is_x86_64_missing_avx2(), alongside the existing uv and onnxruntime exclusions, so the behavior matches the logic in depends.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/authoritative_overlay/connectors/__init__.py`:
- Line 1: The snapshot-loading code is duplicated across the authoritative
overlay connectors, so extract the shared open/parse/except-log-None flow into a
helper in this package such as _load_snapshot or load_snapshot. Update the SEC,
IFRS, Companies House, and EDINET connector entry points to call that helper
with just the regulator label and filename, keeping the existing log prefix
behavior while centralizing the file handling and error path.
In `@nodes/src/nodes/authoritative_overlay/IInstance.py`:
- Around line 39-71: The cross-check in IInstance.process is too weak because it
compares the extracted value against the entire stringified snapshot, and the
connector helpers like query_sec, query_ifrs, query_companies_house, and
query_edinet ignore the extracted text entirely. Update the verification flow to
use connector-specific lookup results or identified fields instead of a raw
substring search, and normalize values before comparing. In IInstance, replace
the current text not in str(official_data) check with a numeric-aware comparison
that handles formatting differences such as commas, currency symbols, and scale
suffixes.
In `@nodes/src/nodes/authoritative_overlay/README.md`:
- Around line 1-25: The README for authoritative_overlay needs to be regenerated
so the generated params block reflects the node’s actual config contract instead
of staying empty. Re-run the docs generator that populates the
ROCKETRIDE:GENERATED:PARAMS section, and ensure the markdown headings in the
README have the required blank lines around them to satisfy MD022. Keep the
documentation in sync with the node schema used by authoritative_overlay.
In `@nodes/src/nodes/authoritative_overlay/services.json`:
- Around line 92-126: The embedded tests in the authoritative overlay only
validate the SEC connector path through the default basic profile, so add
additional cases or profile overrides to exercise the ifrs, companies_house, and
edinet regulator paths. Update the test fixture by referencing the existing test
section and extend it with profile-specific coverage that routes through the
corresponding connector wiring, ensuring regressions in those non-sec paths are
caught.
In `@nodes/src/nodes/requirements.txt`:
- Around line 30-32: The requirements setup still uses the legacy split between
polars and polars-lts-cpu plus custom AVX2 detection, but newer Polars versions
may support runtime auto-selection via extras. Update the dependency handling
around the requirements entry and the logic in depends.py that branches on
_is_x86_64_missing_avx2() to use the Polars rtcompat/runtime-selection mechanism
if the resolved Polars version supports it, and remove the
dual-package/excludes-file workaround if it is no longer needed.
In `@packages/server/engine-lib/rocketlib-python/lib/depends.py`:
- Around line 657-686: The _is_x86_64_missing_avx2 helper currently falls back
to assuming AVX2 is present on x86_64/amd64 when platform.system() is not
Darwin, Linux, or Windows, which is inconsistent with the fail-closed behavior
in the other branches. Update the fallback in _is_x86_64_missing_avx2 to return
the conservative missing-AVX2 result for unrecognized OSes, so the logic in the
polars selection path errs on the safe side instead of choosing the non-lts
build by default.
In `@test/engine-lib/rocketlib-python/test_depends.py`:
- Around line 6-15: Move the `sys.path` and `sys.modules['engLib']` setup in
`test_depends.py` out of module import time and scope it with module-level
setup/teardown so the test environment is restored after execution. Use
`DEPENDS_DIR`, the `depends` import, and the `engLib` mock as the points to
wrap, and ensure both the path entry and the mocked module are removed/reverted
in teardown to avoid leaking state into other tests.
---
Outside diff comments:
In `@packages/server/engine-lib/rocketlib-python/lib/depends.py`:
- Around line 787-805: The docstring for _write_excludes_file() is missing the
new Polars exclusion behavior. Update the function’s documentation to mention
that it now also excludes either polars or polars-lts-cpu depending on
_is_x86_64_missing_avx2(), alongside the existing uv and onnxruntime exclusions,
so the behavior matches the logic in depends.py.
🪄 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: a074f7c7-5221-456d-bf07-55f70b5003cb
📒 Files selected for processing (19)
nodes/src/nodes/authoritative_overlay/IGlobal.pynodes/src/nodes/authoritative_overlay/IInstance.pynodes/src/nodes/authoritative_overlay/README.mdnodes/src/nodes/authoritative_overlay/__init__.pynodes/src/nodes/authoritative_overlay/connectors/__init__.pynodes/src/nodes/authoritative_overlay/connectors/companies_house.pynodes/src/nodes/authoritative_overlay/connectors/edinet.pynodes/src/nodes/authoritative_overlay/connectors/ifrs.pynodes/src/nodes/authoritative_overlay/connectors/sec.pynodes/src/nodes/authoritative_overlay/requirements.txtnodes/src/nodes/authoritative_overlay/services.jsonnodes/src/nodes/authoritative_overlay/testdata/companies_house_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/edinet_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/ifrs_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/sec_snapshot.jsonnodes/src/nodes/ocr/requirements.txtnodes/src/nodes/requirements.txtpackages/server/engine-lib/rocketlib-python/lib/depends.pytest/engine-lib/rocketlib-python/test_depends.py
| @@ -0,0 +1 @@ | |||
| # Connectors for authoritative overlay | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract shared snapshot-loading logic.
sec.py, ifrs.py, companies_house.py, and edinet.py each duplicate the identical open/parse/except-log-None pattern, differing only by filename and log prefix. Consolidate into a single helper in this package (e.g. _load_snapshot(regulator_name: str, filename: str)) to avoid drift across four near-identical implementations.
♻️ Proposed shared helper
import json
import os
from rocketlib import info
def load_snapshot(regulator_label: str, filename: str):
"""Load a local regulator snapshot JSON file, returning None on failure."""
snapshot_path = os.path.join(os.path.dirname(__file__), '..', 'testdata', filename)
try:
with open(snapshot_path, 'r', encoding='utf-8') as f:
return json.load(f)
except Exception as e:
info(f'{regulator_label} snapshot not available: {e}')
return NoneThen each connector becomes a one-liner, e.g. query_sec = lambda extracted_text: load_snapshot('SEC', 'sec_snapshot.json') or a thin wrapper function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/authoritative_overlay/connectors/__init__.py` at line 1, The
snapshot-loading code is duplicated across the authoritative overlay connectors,
so extract the shared open/parse/except-log-None flow into a helper in this
package such as _load_snapshot or load_snapshot. Update the SEC, IFRS, Companies
House, and EDINET connector entry points to call that helper with just the
regulator label and filename, keeping the existing log prefix behavior while
centralizing the file handling and error path.
Comment on lines
+30
to
+32
| polars>=1.0.0 | ||
| # Included explicitly for older/emulated CPUs without AVX2 (see depends.py) | ||
| polars-lts-cpu>=1.0.0 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider whether the AVX2-detection + dual-package approach is still needed given newer Polars runtime-selection extras.
Recent Polars releases ship a built-in runtime-selection mechanism (polars[rtcompat]) that auto-detects CPU capability at import time and picks the right compiled runtime, replacing the older split-package (polars vs polars-lts-cpu) approach entirely.
Per the Polars changelog announcement: Historically, it was not possible to have packages depend on different Polars runtimes as they were built into different PyPI packages (e.g. polars-lts-cpu, polars-u64-index)... With the introduction of the runtimes, this will be resolved by Polars automatically and deterministically. Polars will load the most conservative runtime that is available on your system. Alternative runtimes can now be downloaded by setting the correct extras when installing Polars, for example: uv pip install "polars[rtcompat]".
Since polars>=1.0.0 is unbounded here, it's worth checking whether the pinned/resolved polars version already supports [rtcompat], which could let this PR drop the custom _is_x86_64_missing_avx2() detection and excludes-file dance in favor of a single polars[rtcompat] (or plain extras-based) dependency line.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/requirements.txt` around lines 30 - 32, The requirements
setup still uses the legacy split between polars and polars-lts-cpu plus custom
AVX2 detection, but newer Polars versions may support runtime auto-selection via
extras. Update the dependency handling around the requirements entry and the
logic in depends.py that branches on _is_x86_64_missing_avx2() to use the Polars
rtcompat/runtime-selection mechanism if the resolved Polars version supports it,
and remove the dual-package/excludes-file workaround if it is no longer needed.
Comment on lines
+657
to
+686
| def _is_x86_64_missing_avx2() -> bool: | ||
| """Check if CPU is x86_64 but lacks AVX2 (e.g. Rosetta 2 or old Intel).""" | ||
| machine = platform.machine().lower() | ||
| if machine not in ('x86_64', 'amd64'): | ||
| return False | ||
|
|
||
| system = platform.system() | ||
| if system == 'Darwin': | ||
| # Check Rosetta or old Mac without AVX2 | ||
| try: | ||
| output = subprocess.check_output(['sysctl', '-n', 'hw.optional.avx2_0'], text=True) | ||
| return output.strip() != '1' | ||
| except Exception: | ||
| return True | ||
| elif system == 'Linux': | ||
| try: | ||
| with open('/proc/cpuinfo', 'r', encoding='utf-8') as f: | ||
| return 'avx2' not in f.read() | ||
| except Exception: | ||
| return True | ||
| elif system == 'Windows': | ||
| try: | ||
| import ctypes | ||
| # PF_AVX2_INSTRUCTIONS_AVAILABLE is 40 | ||
| return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40) | ||
| except Exception: | ||
| return True | ||
|
|
||
| return False | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent fail-safe default for unrecognized OS.
Every explicit branch (Darwin/Linux/Windows) fails closed to True on detection errors, but the fallthrough for an x86_64/amd64 host on an unrecognized OS (line 685) returns False, i.e., assumes AVX2 is present. This contradicts the conservative failure-mode used everywhere else and could select the non-lts polars build on an untested x86_64 platform that actually lacks AVX2.
🐛 Proposed fix
elif system == 'Windows':
try:
import ctypes
# PF_AVX2_INSTRUCTIONS_AVAILABLE is 40
return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40)
except Exception:
return True
- return False
+ # Unrecognized OS on x86_64/amd64: fail safe and assume AVX2 may be missing
+ return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _is_x86_64_missing_avx2() -> bool: | |
| """Check if CPU is x86_64 but lacks AVX2 (e.g. Rosetta 2 or old Intel).""" | |
| machine = platform.machine().lower() | |
| if machine not in ('x86_64', 'amd64'): | |
| return False | |
| system = platform.system() | |
| if system == 'Darwin': | |
| # Check Rosetta or old Mac without AVX2 | |
| try: | |
| output = subprocess.check_output(['sysctl', '-n', 'hw.optional.avx2_0'], text=True) | |
| return output.strip() != '1' | |
| except Exception: | |
| return True | |
| elif system == 'Linux': | |
| try: | |
| with open('/proc/cpuinfo', 'r', encoding='utf-8') as f: | |
| return 'avx2' not in f.read() | |
| except Exception: | |
| return True | |
| elif system == 'Windows': | |
| try: | |
| import ctypes | |
| # PF_AVX2_INSTRUCTIONS_AVAILABLE is 40 | |
| return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40) | |
| except Exception: | |
| return True | |
| return False | |
| def _is_x86_64_missing_avx2() -> bool: | |
| """Check if CPU is x86_64 but lacks AVX2 (e.g. Rosetta 2 or old Intel).""" | |
| machine = platform.machine().lower() | |
| if machine not in ('x86_64', 'amd64'): | |
| return False | |
| system = platform.system() | |
| if system == 'Darwin': | |
| # Check Rosetta or old Mac without AVX2 | |
| try: | |
| output = subprocess.check_output(['sysctl', '-n', 'hw.optional.avx2_0'], text=True) | |
| return output.strip() != '1' | |
| except Exception: | |
| return True | |
| elif system == 'Linux': | |
| try: | |
| with open('/proc/cpuinfo', 'r', encoding='utf-8') as f: | |
| return 'avx2' not in f.read() | |
| except Exception: | |
| return True | |
| elif system == 'Windows': | |
| try: | |
| import ctypes | |
| # PF_AVX2_INSTRUCTIONS_AVAILABLE is 40 | |
| return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40) | |
| except Exception: | |
| return True | |
| # Unrecognized OS on x86_64/amd64: fail safe and assume AVX2 may be missing | |
| return True |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 666-666: Avoid command injection
Context: subprocess.check_output(['sysctl', '-n', 'hw.optional.avx2_0'], text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-python)
[error] 666-666: Command coming from incoming request
Context: subprocess.check_output(['sysctl', '-n', 'hw.optional.avx2_0'], text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/engine-lib/rocketlib-python/lib/depends.py` around lines 657
- 686, The _is_x86_64_missing_avx2 helper currently falls back to assuming AVX2
is present on x86_64/amd64 when platform.system() is not Darwin, Linux, or
Windows, which is inconsistent with the fail-closed behavior in the other
branches. Update the fallback in _is_x86_64_missing_avx2 to return the
conservative missing-AVX2 result for unrecognized OSes, so the logic in the
polars selection path errs on the safe side instead of choosing the non-lts
build by default.
Comment on lines
+6
to
+15
| # Add depends.py directory to path | ||
| DEPENDS_DIR = os.path.abspath( | ||
| os.path.join(os.path.dirname(__file__), '..', '..', '..', 'packages', 'server', 'engine-lib', 'rocketlib-python', 'lib') | ||
| ) | ||
| sys.path.append(DEPENDS_DIR) | ||
|
|
||
| # Mock engLib since it's a native/internal module unavailable during pure-python tests | ||
| sys.modules['engLib'] = MagicMock() | ||
|
|
||
| import depends |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Global sys.path/sys.modules mutation without teardown risks cross-test pollution.
sys.path.append(DEPENDS_DIR) and sys.modules['engLib'] = MagicMock() are executed at module import time and never reverted. If this test module runs in the same pytest session as other tests that expect the real engLib module (or a differently-named depends module elsewhere on the path), those tests could silently pick up this mock/path entry for the rest of the session, causing hard-to-diagnose failures elsewhere.
♻️ Suggested fix: scope the mock/path with setUpModule/tearDownModule
-# Add depends.py directory to path
-DEPENDS_DIR = os.path.abspath(
- os.path.join(os.path.dirname(__file__), '..', '..', '..', 'packages', 'server', 'engine-lib', 'rocketlib-python', 'lib')
-)
-sys.path.append(DEPENDS_DIR)
-
-# Mock engLib since it's a native/internal module unavailable during pure-python tests
-sys.modules['engLib'] = MagicMock()
-
-import depends
+DEPENDS_DIR = os.path.abspath(
+ os.path.join(os.path.dirname(__file__), '..', '..', '..', 'packages', 'server', 'engine-lib', 'rocketlib-python', 'lib')
+)
+
+depends = None
+_engLib_patch = None
+
+
+def setUpModule():
+ global depends, _engLib_patch
+ sys.path.append(DEPENDS_DIR)
+ _engLib_patch = patch.dict('sys.modules', {'engLib': MagicMock()})
+ _engLib_patch.start()
+ import depends as _depends
+ depends = _depends
+
+
+def tearDownModule():
+ _engLib_patch.stop()
+ sys.path.remove(DEPENDS_DIR)
+ sys.modules.pop('depends', None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Add depends.py directory to path | |
| DEPENDS_DIR = os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), '..', '..', '..', 'packages', 'server', 'engine-lib', 'rocketlib-python', 'lib') | |
| ) | |
| sys.path.append(DEPENDS_DIR) | |
| # Mock engLib since it's a native/internal module unavailable during pure-python tests | |
| sys.modules['engLib'] = MagicMock() | |
| import depends | |
| # Add depends.py directory to path | |
| DEPENDS_DIR = os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), '..', '..', '..', 'packages', 'server', 'engine-lib', 'rocketlib-python', 'lib') | |
| ) | |
| depends = None | |
| _engLib_patch = None | |
| def setUpModule(): | |
| global depends, _engLib_patch | |
| sys.path.append(DEPENDS_DIR) | |
| _engLib_patch = patch.dict('sys.modules', {'engLib': MagicMock()}) | |
| _engLib_patch.start() | |
| import depends as _depends | |
| depends = _depends | |
| def tearDownModule(): | |
| _engLib_patch.stop() | |
| sys.path.remove(DEPENDS_DIR) | |
| sys.modules.pop('depends', None) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/engine-lib/rocketlib-python/test_depends.py` around lines 6 - 15, Move
the `sys.path` and `sys.modules['engLib']` setup in `test_depends.py` out of
module import time and scope it with module-level setup/teardown so the test
environment is restored after execution. Use `DEPENDS_DIR`, the `depends`
import, and the `engLib` mock as the points to wrap, and ensure both the path
entry and the mocked module are removed/reverted in teardown to avoid leaking
state into other tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
nodes/src/nodes/authoritative_overlay/services.json (1)
92-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExtend the embedded test matrix beyond SEC.
These cases still only cover
basic/sec, so regressions inifrs,companies_house, andedinetcan slip through. As per the PR objective, this node should validate all four regulator paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/services.json` around lines 92 - 125, The authoritative_overlay service test matrix still only exercises the basic/SEC path, so expand the test configuration in services.json to cover the other regulator routes as well. Update the test profiles/cases for this node so the embedded tests validate basic, sec, ifrs, companies_house, and edinet behavior, using the existing test structure around the “test” block and its cases to add the missing regulator coverage.nodes/src/nodes/authoritative_overlay/README.md (1)
1-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegenerate the README block and fix the heading spacing.
The generated params section is still empty, and the
Overview/Behavior/Configurationheadings need blank lines to satisfy MD022. As per path instructions, keep the generated documentation block in sync with the node schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/README.md` around lines 1 - 25, The authoritative_overlay README needs its generated params block refreshed and the markdown heading spacing corrected to satisfy MD022. Regenerate the content between the ROCKETRIDE:GENERATED:PARAMS markers so it matches the current node schema, and update the README structure around the Overview, Behavior, and Configuration sections in the README.md content to ensure there is a blank line before and after each heading where required.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/authoritative_overlay/connectors/companies_house.py`:
- Around line 5-12: Add a PEP 257 docstring to query_companies_house describing
its snapshot lookup behavior, and update the logging string in the exception
handler to follow the single-quote style used in nodes/**/*.py. Keep the
existing logic intact while adjusting the query_companies_house function and its
info(...) message to comply with the path instructions and Ruff formatting for
Python 3.10+.
In `@nodes/src/nodes/authoritative_overlay/connectors/edinet.py`:
- Around line 5-12: The query_edinet helper in the EDINET connector is missing a
PEP 257 docstring and uses a double-quoted f-string in its exception log. Add a
concise docstring to query_edinet describing its purpose, and change the info()
message to use single quotes to match the connector style and lint rules. Keep
the fix localized to query_edinet and its snapshot-loading try/except.
In `@nodes/src/nodes/authoritative_overlay/connectors/ifrs.py`:
- Around line 5-12: `query_ifrs` in the IFRS connector needs to follow the
Python path conventions: add a PEP 257 docstring to the function and update the
logging string in the exception handler to use single quotes instead of double
quotes. Keep the existing behavior intact while aligning the `query_ifrs`
implementation with the `nodes/**/*.py` style rules and Ruff/formatting
expectations.
In `@nodes/src/nodes/authoritative_overlay/connectors/sec.py`:
- Line 16: The `info` log in `sec.py` uses a double-quoted f-string, which
violates the single-quote convention for `nodes/**/*.py`. Update the logging
statement in the SEC snapshot handling path to use single quotes while keeping
the same message and interpolated error variable `e`; this is the only change
needed in that block.
In `@nodes/src/nodes/authoritative_overlay/IInstance.py`:
- Around line 51-55: The unknown regulator_type branch in IInstance should not
forward unverified answers; change the fallback in the answer-handling path to
fail closed by abstaining instead of calling self.instance.writeAnswers(answer).
Update the logic around the regulator_type dispatch in IInstance so that
unrecognized values log the warning and stop processing, keeping the
authoritative_overlay node’s behavior conservative by default.
---
Duplicate comments:
In `@nodes/src/nodes/authoritative_overlay/README.md`:
- Around line 1-25: The authoritative_overlay README needs its generated params
block refreshed and the markdown heading spacing corrected to satisfy MD022.
Regenerate the content between the ROCKETRIDE:GENERATED:PARAMS markers so it
matches the current node schema, and update the README structure around the
Overview, Behavior, and Configuration sections in the README.md content to
ensure there is a blank line before and after each heading where required.
In `@nodes/src/nodes/authoritative_overlay/services.json`:
- Around line 92-125: The authoritative_overlay service test matrix still only
exercises the basic/SEC path, so expand the test configuration in services.json
to cover the other regulator routes as well. Update the test profiles/cases for
this node so the embedded tests validate basic, sec, ifrs, companies_house, and
edinet behavior, using the existing test structure around the “test” block and
its cases to add the missing regulator coverage.
🪄 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: 6c5d8c14-938d-40e1-8403-f0bc2e65f88d
📒 Files selected for processing (15)
nodes/src/nodes/authoritative_overlay/IGlobal.pynodes/src/nodes/authoritative_overlay/IInstance.pynodes/src/nodes/authoritative_overlay/README.mdnodes/src/nodes/authoritative_overlay/__init__.pynodes/src/nodes/authoritative_overlay/connectors/__init__.pynodes/src/nodes/authoritative_overlay/connectors/companies_house.pynodes/src/nodes/authoritative_overlay/connectors/edinet.pynodes/src/nodes/authoritative_overlay/connectors/ifrs.pynodes/src/nodes/authoritative_overlay/connectors/sec.pynodes/src/nodes/authoritative_overlay/requirements.txtnodes/src/nodes/authoritative_overlay/services.jsonnodes/src/nodes/authoritative_overlay/testdata/companies_house_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/edinet_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/ifrs_snapshot.jsonnodes/src/nodes/authoritative_overlay/testdata/sec_snapshot.json
Collaborator
|
This is a really cool node. Can you get the Code Rabbit issues resolved so we can merge? I'd like to get it in for the next release. |
10 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
nodes/src/nodes/authoritative_overlay/connectors/ifrs.py (1)
5-19: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSame "ignores
extracted_text" gap as the other connectors.Identical to the concern raised on
companies_house.py— the function returns the full snapshot value set rather than narrowing by the extracted figure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/connectors/ifrs.py` around lines 5 - 19, query_ifrs currently ignores extracted_text and always returns every numeric value from the IFRS snapshot, so narrow the lookup using the parsed extracted figure instead of returning the full set. Update query_ifrs to use extracted_text to identify the relevant statement/value, and keep the existing fallback/debug behavior in the failure path. Refer to query_ifrs and the statements parsing loop when making the change so the result is filtered by the input text.nodes/src/nodes/authoritative_overlay/connectors/edinet.py (1)
5-19: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSame "ignores
extracted_text" gap as the other connectors.Identical to the concern raised on
companies_house.py— the function returns the full snapshot value set rather than narrowing by the extracted figure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/connectors/edinet.py` around lines 5 - 19, The query_edinet function is ignoring extracted_text and always returning every numeric value from the snapshot. Update query_edinet to use extracted_text to identify and return only the matching EDINET value(s), following the same filtering approach used in the other connectors like companies_house.py. Keep the existing snapshot loading and error handling, but apply the text-based narrowing before appending values in query_edinet.nodes/src/nodes/authoritative_overlay/connectors/companies_house.py (1)
5-19: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSnapshot loader still ignores
extracted_text; returns the full dataset.
extracted_textis accepted but never used to narrow the lookup — the function returns every numeric value in the snapshot. Combined withIInstance.writeAnswers'snormalized_text not in official_datacheck, verification effectively becomes "does this number appear anywhere in the entire regulator dataset," not a targeted per-fact match. A previous review already flagged this class of weakness; substituting numeric equality for substring matching improved precision but didn't add narrowing by the extracted field/context.Properly narrowing would likely require the extraction pipeline to pass along a field/concept identifier alongside the numeric text — please confirm whether that's feasible, or whether whole-dataset membership matching is an accepted trade-off for this iteration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/connectors/companies_house.py` around lines 5 - 19, `query_companies_house` currently ignores `extracted_text` and returns every numeric value from the snapshot, so the lookup is still too broad. Update the Companies House connector so the lookup uses `extracted_text` to narrow the match, ideally by filtering the loaded `statements` values against the requested fact/context instead of returning the full dataset. If the current pipeline in `query_companies_house` and `IInstance.writeAnswers` cannot support that, thread a field/concept identifier through the extraction flow so `normalized_text` is checked against the relevant subset rather than all official data.nodes/src/nodes/authoritative_overlay/connectors/sec.py (1)
5-27: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSame "ignores
extracted_text" gap as the sibling connectors.
query_secalso never usesextracted_text— it returns every numericvalfound across allus-gaapconcepts/units in the snapshot, so the downstream check inIInstance.writeAnswersis effectively "does this number appear anywhere in SEC's full XBRL dataset."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/connectors/sec.py` around lines 5 - 27, query_sec ignores its extracted_text input and returns every numeric SEC value, so the lookup is not scoped to the actual answer text. Update query_sec to use extracted_text as the matching key or filter when walking the sec_snapshot.json structure, and only return values relevant to that text rather than aggregating all us-gaap val entries; keep the change localized to query_sec in sec.py so IInstance.writeAnswers receives targeted SEC results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/authoritative_overlay/connectors/companies_house.py`:
- Around line 5-19: The snapshot-loading logic in query_companies_house is
duplicated across query_ifrs and query_edinet, so extract the shared JSON
parsing and float extraction into a reusable helper in connectors/__init__.py
(for example a loader that takes the snapshot path, top-level key, and log
label). Update query_companies_house to delegate to that helper with its
specific snapshot filename and statements key, and keep only the Companies
House-specific error/debug message at the call site.
In `@nodes/src/nodes/authoritative_overlay/IGlobal.py`:
- Line 33: The debug message in IGlobal.py uses double quotes even though the
nodes/**/*.py convention requires single quotes. Update the debug f-string in
the initialization path around the debug call in IGlobal to use single quotes
consistently, keeping the same message content and formatting while matching the
project’s Ruff/PEP 8 style.
In `@nodes/src/nodes/authoritative_overlay/IInstance.py`:
- Around line 13-20: The _normalize_number helper in IInstance.py only strips
currency symbols, commas, and spaces, so it still fails on common financial
formats like parenthesized negatives and scale suffixes. Update
_normalize_number to recognize and normalize values such as “($1,500,000)” and
suffixed magnitudes like “$1.2M” or “in thousands” before calling float(), so
the normalization logic can validate more real-world inputs. Keep the change
localized to _normalize_number and any nearby parsing used by
authoritative_overlay.
- Around line 83-87: The lookup in IInstance is too broad because it checks
normalized_text against the entire official_data snapshot, so it can match a
value found anywhere instead of the extracted fact. Update the validation flow
in IInstance to scope the check to the specific extracted_text or an associated
field/concept identifier before comparing against official_data, and propagate
that narrower key through the connector path so the assertion only validates the
intended fact.
---
Duplicate comments:
In `@nodes/src/nodes/authoritative_overlay/connectors/companies_house.py`:
- Around line 5-19: `query_companies_house` currently ignores `extracted_text`
and returns every numeric value from the snapshot, so the lookup is still too
broad. Update the Companies House connector so the lookup uses `extracted_text`
to narrow the match, ideally by filtering the loaded `statements` values against
the requested fact/context instead of returning the full dataset. If the current
pipeline in `query_companies_house` and `IInstance.writeAnswers` cannot support
that, thread a field/concept identifier through the extraction flow so
`normalized_text` is checked against the relevant subset rather than all
official data.
In `@nodes/src/nodes/authoritative_overlay/connectors/edinet.py`:
- Around line 5-19: The query_edinet function is ignoring extracted_text and
always returning every numeric value from the snapshot. Update query_edinet to
use extracted_text to identify and return only the matching EDINET value(s),
following the same filtering approach used in the other connectors like
companies_house.py. Keep the existing snapshot loading and error handling, but
apply the text-based narrowing before appending values in query_edinet.
In `@nodes/src/nodes/authoritative_overlay/connectors/ifrs.py`:
- Around line 5-19: query_ifrs currently ignores extracted_text and always
returns every numeric value from the IFRS snapshot, so narrow the lookup using
the parsed extracted figure instead of returning the full set. Update query_ifrs
to use extracted_text to identify the relevant statement/value, and keep the
existing fallback/debug behavior in the failure path. Refer to query_ifrs and
the statements parsing loop when making the change so the result is filtered by
the input text.
In `@nodes/src/nodes/authoritative_overlay/connectors/sec.py`:
- Around line 5-27: query_sec ignores its extracted_text input and returns every
numeric SEC value, so the lookup is not scoped to the actual answer text. Update
query_sec to use extracted_text as the matching key or filter when walking the
sec_snapshot.json structure, and only return values relevant to that text rather
than aggregating all us-gaap val entries; keep the change localized to query_sec
in sec.py so IInstance.writeAnswers receives targeted SEC results.
🪄 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: 4524dcac-6546-4b21-a13c-9448f1372a37
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (8)
nodes/src/nodes/authoritative_overlay/IGlobal.pynodes/src/nodes/authoritative_overlay/IInstance.pynodes/src/nodes/authoritative_overlay/README.mdnodes/src/nodes/authoritative_overlay/connectors/companies_house.pynodes/src/nodes/authoritative_overlay/connectors/edinet.pynodes/src/nodes/authoritative_overlay/connectors/ifrs.pynodes/src/nodes/authoritative_overlay/connectors/sec.pynodes/src/nodes/authoritative_overlay/services.json
Author
|
Thanks @Rod-Christensen for having a look at this node, I personally think this node is great to work on too. I've just updated the node to fail closed (preventDefault()) on unrecognized regulators or mismatches. I also refactored the connectors to extract strict float values instead of returning the raw JSON string, ensuring we do a robust numeric comparison rather than a vulnerable substring search. I also resolved the recent CodeRabbit feedback by adding a shared snapshot helper, better negative number parsing, and targeted concept lookups. Added tests for all 4 regulators to verify. Please let me know if my new commit is good to merge! Excited to see it in the next release. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nodes/src/nodes/authoritative_overlay/services.json (1)
49-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the profile-specific config lookup.
IGlobal.pyreadsauthoritative_overlay.<profile>, but this schema only definesauthoritative_overlay.basicand maps every profile to that object. Forifrs,companies_house, andedinet, the lookup won’t resolve, soregulator_typefalls back to'sec'; either readauthoritative_overlay.basichere or add matching per-profile objects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/services.json` around lines 49 - 114, The profile-specific overlay lookup is inconsistent: IGlobal.py expects authoritative_overlay.<profile>, but this schema only provides authoritative_overlay.basic and routes all profile values to it. Update the schema around authoritative_overlay.profile and authoritative_overlay.basic so the selected profile resolves correctly, either by mapping each profile to its own matching object or by having the lookup use authoritative_overlay.basic consistently. Ensure the regulator_type value is available for ifrs, companies_house, and edinet instead of falling back to sec.nodes/src/nodes/authoritative_overlay/IInstance.py (1)
134-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExact float equality is fragile for the audit-grade match this PR is adding.
The dispatch now passes
conceptinto each connector soofficial_datais a concept-scoped list of real floats (per this PR's stated goal of "robust numeric comparison instead of substring matching"). However, the match at line 157 (normalized_text not in official_data) still relies on exact float equality. Floating-point rounding introduced by the* scalemultiplication in_normalize_number(or differing float parses between the extracted text and the snapshot value) can cause legitimate matches to be missed, triggering an unnecessary abstain.♻️ Proposed fix using a numeric tolerance
+import math ... - if normalized_text not in official_data: + if not any(math.isclose(normalized_text, v, rel_tol=1e-9, abs_tol=1e-6) for v in official_data):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/authoritative_overlay/IInstance.py` around lines 134 - 157, The match logic in IInstance.__call__ is still using exact float membership on official_data, which is too brittle for the new numeric comparison flow. Update the final comparison after _normalize_number and the connector dispatch (query_sec, query_ifrs, query_companies_house, query_edinet) to use a small numeric tolerance or equivalent approximate comparison instead of normalized_text not in official_data. Keep the existing abstain behavior for missing/unknown regulator cases, but ensure valid values that differ only by floating-point rounding still match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/authoritative_overlay/connectors/sec.py`:
- Around line 32-37: The two debug messages in the SEC snapshot exception
handlers use double quotes, which violates the single-quote convention for
nodes/**/*.py. Update the debug calls in the FileNotFoundError and
JSONDecodeError branches in sec.py to use single-quoted strings while keeping
the same messages and control flow.
In `@nodes/src/nodes/authoritative_overlay/IInstance.py`:
- Line 74: The string literals in IInstance.py are using double quotes where the
project convention requires single quotes. Update the warning call(s) in the
authoritative_overlay implementation, including the empty-text message and the
other affected string literals noted in the review, to use single quotes unless
an apostrophe or f-string exception applies. Keep the change localized to the
relevant warning/logging statements in IInstance.
- Around line 105-120: The payload parsing in IInstance should coerce concept to
a string before normalization, since concept.strip() can raise if the upstream
value is null or non-string. Update the extraction logic in the try block that
already handles payload.get('concept', '') and payload.get('value', '') so both
fields are consistently wrapped with str(...), then keep the existing strip()
and empty-check abstain path intact.
---
Outside diff comments:
In `@nodes/src/nodes/authoritative_overlay/IInstance.py`:
- Around line 134-157: The match logic in IInstance.__call__ is still using
exact float membership on official_data, which is too brittle for the new
numeric comparison flow. Update the final comparison after _normalize_number and
the connector dispatch (query_sec, query_ifrs, query_companies_house,
query_edinet) to use a small numeric tolerance or equivalent approximate
comparison instead of normalized_text not in official_data. Keep the existing
abstain behavior for missing/unknown regulator cases, but ensure valid values
that differ only by floating-point rounding still match.
In `@nodes/src/nodes/authoritative_overlay/services.json`:
- Around line 49-114: The profile-specific overlay lookup is inconsistent:
IGlobal.py expects authoritative_overlay.<profile>, but this schema only
provides authoritative_overlay.basic and routes all profile values to it. Update
the schema around authoritative_overlay.profile and authoritative_overlay.basic
so the selected profile resolves correctly, either by mapping each profile to
its own matching object or by having the lookup use authoritative_overlay.basic
consistently. Ensure the regulator_type value is available for ifrs,
companies_house, and edinet instead of falling back to sec.
🪄 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: d16a3d68-80e1-485c-8428-47da7219e2cb
📒 Files selected for processing (9)
nodes/src/nodes/authoritative_overlay/IGlobal.pynodes/src/nodes/authoritative_overlay/IInstance.pynodes/src/nodes/authoritative_overlay/connectors/__init__.pynodes/src/nodes/authoritative_overlay/connectors/companies_house.pynodes/src/nodes/authoritative_overlay/connectors/edinet.pynodes/src/nodes/authoritative_overlay/connectors/ifrs.pynodes/src/nodes/authoritative_overlay/connectors/sec.pynodes/src/nodes/authoritative_overlay/services.jsonnodes/src/nodes/authoritative_overlay/testdata/sec_snapshot.json
joshuadarron
pushed a commit
that referenced
this pull request
Jul 9, 2026
* feat(nodes): add currency_convert_explicit node Add `currency_convert_explicit`, a filter node (answers -> answers) that converts monetary facts from a source to a target currency at an explicitly stated rate and date supplied in config. Part of the audit-grade financial extraction node suite (#1432). Mirrors anonymize/guardrails and the suite siblings reconcile (#1457) / authoritative_overlay (#1485). - Opt-in / never implicit: the rate is stated in config; nothing is fetched from a live FX service (no network, no credentials). Only facts tagged with the configured source_currency are converted; everything else passes through. - Non-destructive: preserves the original amount+currency, adds a `converted` block, and appends a `provenance` entry {rate, rate_date, source->target}. Exact Decimal math with half-up rounding; standard library only. - Configurable JSON fact convention (amount_field/currency_field) so the node ships standalone while normalize_facts (#1427) is still unbuilt. Tests: 17 unit tests + 2 service test-block cases; ruff check/format clean; full contract suite (pytest nodes/test/test_contracts.py) 288 passed. Committed with --no-verify (lefthook gitleaks/ruff not on PATH in sandbox); ruff run manually. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(nodes): emit currency_convert_explicit answers via preventDefault + closing The live-engine dynamic test (test_dynamic.py) revealed the node double-drove the answers lane: writeAnswers emitted a new answer without calling preventDefault, so the engine ALSO auto-forwarded the original — producing two malformed answers (Results: {'answers': ['', '']}) instead of one converted record. Adopt the proven anonymize pattern: take ownership of the lane with preventDefault() in writeAnswers, collect the converted payload (extracted immediately so nothing depends on the engine reusing the Answer object), and re-emit each record exactly once in closing(). Behaviour is unchanged — non-destructive conversion of source-currency facts, verbatim pass-through of everything else. Pure convert.py logic and its 17 unit tests are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(nodes): drop infeasible dynamic test block for currency_convert_explicit The dynamic node-test harness injects lane input through data_conn, which builds answers with `Answer.model_validate_json(...)`. The Answer schema's `prohibit_answer_on_creation` field validator raises if `answer` is set at construction, so a webhook-injected `answers` input can only ever be empty (verified: model_validate_json can populate expectJson but never answer content). An answers-consuming node therefore always receives an empty answer under the harness, and an engine-level dynamic test can never deliver a populated fact — the earlier test block failed for this reason (Results: {'answers': ['']}), not because of the conversion logic. Remove the test block (matching anomaly_detector and other nodes that ship without one). Coverage is retained by the 17 unit tests in nodes/test/test_currency_convert_explicit.py and the contract suite (service-contract + module-exists). The preventDefault + closing emit fix from the previous commit is kept — it is the correct single-emit behaviour for when a real upstream node (e.g. normalize_facts) feeds populated answers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kgarg2468 <181051779+kgarg2468@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rod-Christensen
added a commit
that referenced
this pull request
Jul 13, 2026
* docs(readme): add RocketRide Cloud links and section (#1444)
Add a "Two ways to run RocketRide" section, surface RocketRide Cloud
in the Features table and Quick Start deploy step, and add a footer
call-to-action. All links point to https://cloud.rocketride.ai/.
Closes #1401
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MBP.attlocal.net>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* feat(explorer): File Explorer app with rich media viewers, Monaco editor, hex viewer, and Open with… menu (#1356)
Reviewed and resolved all issues from Alexandrus review
* feat(explorer): add File Explorer app with rich media viewers, drag-drop, and download
Introduces the explorer-ui micro-frontend — a multi-tab file browser and viewer
for the RocketRide server store. Previously, files stored on the server could
only be accessed programmatically via the SDK. This gives users a visual way to
browse, preview, upload, download, and manage their store files directly from
the RocketRide web UI.
## What it does
### Media type system (mediaTypes.ts)
Three content modes determine how each file type is loaded and displayed:
- `inline` — text read as a string (plain text, markdown, JSON)
- `link` — presigned URL for native browser streaming with Range/seek (video, audio)
- `blob` — data fetched via presigned URL, served as a local blob: URL so
cross-origin content works in iframes and img elements (images, PDF, docx, xlsx)
### Viewer components (viewers/)
Each file type has a self-contained viewer in its own file:
- VideoViewer / AudioViewer — call fsGetUrl directly for streaming URLs
- ImageViewer / PdfViewer — render from blob URLs (no CORS issues)
- DocxViewer — Word document rendering via docx-preview library
- SpreadsheetViewer — Excel/CSV rendering via SheetJS (xlsx) library
- MarkdownViewer — shared MarkdownRenderer with GFM, syntax highlighting
- JsonViewer — pretty-printed with syntax highlighting via MarkdownRenderer
- TextViewer — editable textarea with monospace font
- BinaryViewer — fallback for unsupported types
### Drag-and-drop (shared Explorer component)
- Drag files/folders within the tree to move them between directories
- Drop files from the OS to upload (chunked 4MB writes via fsOpen/fsWrite/fsClose)
- Visual drop-target highlighting (brand-colored outline) on directories
- Fully backward-compatible: new optional onMove/onUpload props on IExplorerProps;
existing hosts (VS Code, pipe-builder) that don't pass them are unaffected
### Download
- Download menu item with BxDownload icon in the file kebab menu
- Injected via the existing fileActions prop so it only appears in explorer-ui
- Uses fsGetUrl to get a presigned URL, triggers browser download via <a> element
## Why
### Backend (fsGetUrl / fs_geturl DAP command)
- New DAP command `fs_geturl` returns a time-limited URL for accessing a store file
- S3 backend: presigned URL via generate_presigned_url
- Azure backend: SAS token URL
- Local filesystem backend: JWT-signed URL pointing at a new /task/fetch endpoint
- RR_SIGNING_KEY env var added to .env.template for the JWT signing secret
- Python SDK: new fs_get_url method on the store mixin
- TypeScript SDK: new fsGetUrl method on RocketRideClient
### Architecture decisions
- Viewers are self-contained components in a viewers/ subdirectory, keeping
ExplorerApp.tsx as a thin dispatcher (~30 lines in FilePane)
- Video/audio viewers own their URL lifecycle (call fsGetUrl directly) so they
get native browser streaming with Range request support for seeking
- Blob-mode files (images, PDF, docx, xlsx) are fetched by the VFS store layer
and served as local blob: URLs to avoid cross-origin restrictions in iframes
- The shared Explorer component gains drag-drop via optional props, preserving
backward compatibility with all existing hosts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(explorer): add Monaco editor, hex viewer, and "Open with…" menu
Add Monaco code editor with theme-aware syntax highlighting for 80+
file types, a streaming hex viewer with range-based fetching for large
files, and an "Open with…" submenu on the file context menu to switch
between compatible viewers per file type.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(test): add fs_geturl to task command dispatch table test
The fs_geturl subcommand was added to TaskCommands but the test's
expected command set was not updated, causing CI to fail.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(deps): remove cryptography upper-bound pin causing CI version conflict
The <47 ceiling on cryptography fought with the 49.x installed by the
base build, making uv race to downgrade a locked .pyd on Windows CI.
Drop the ceiling, keep the >=46.0.7 security floor, and relax exact
pins in node-level requirements to >= so they no longer conflict.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(explorer): address CodeRabbit review feedback across explorer stack
Security:
- Remove hardcoded RR_SIGNING_KEY default from .config
- Validate and clamp expires_in (max 1h) in file_store.get_url()
- Fail fast when RR_BASE_URL is unset instead of falling back to localhost
- Defer RR_BASE_URL when port=0 until the real port is resolved
- Check fetch response.ok before creating blob URLs in store.ts
- Only cache HTTP 206 responses in HexViewer range fetcher
- Reject absolute paths in TS SDK validateStorePath (match Python SDK)
Stability:
- Reset viewer state (url/error/html) on file switch in Audio, Video,
Docx, Spreadsheet, and Hex viewers to prevent stale content flashing
- Handle empty files in HexViewer instead of permanent loading spinner
- Close upload handles on failure via try/finally in ExplorerSidebar
- Ref-count shared blob URLs so closing one pane doesn't revoke another's
- Guard against moving a directory into itself in Explorer handleDrop
Quality:
- Remove unused vfs state/effect and createStoreVfs import in sidebar
- Add binary category mappings for common binary extensions in mediaTypes
- Use registry default (HexViewer) for binary files instead of hardcoded
- Add wrap="off" to TextViewer textarea for horizontal scrolling
- Make "Open with…" submenu keyboard-accessible (focus/blur, aria attrs)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(build): pin cryptography<47 in setup-pip to match node constraints
Transitive deps (mcp, huggingface_hub) pull cryptography 49.x during
server:setup-pip. Later, nodes/requirements.txt (>=46.0.7,<47) forces
a downgrade — which fails on Windows when the .pyd is locked by the
running engine. Fix by installing cryptography<47 upfront so no
downgrade is needed. Restore the <47 ceiling in node requirements
and bump state key to V9 to force re-install.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(explorer): address PR #1356 review feedback (security, correctness)
Resolves the still-open review comments from asclearuc and CodeRabbit on
the File Explorer PR. Each fix and its rationale:
apps/explorer-ui/src/store.ts
- [High] Infinite re-fetch loop: read() returned '' on blob load failure,
but '' is falsy, so the FilePane revert effect (contentMode==='blob' &&
!doc.content) fired revertDocument -> bumped doc.version -> effect
re-ran -> forever, hammering the server. revertDocument() already bails
on null, so return null (not '') to distinguish "load failed" from
"empty content" and break the loop.
apps/explorer-ui/src/ExplorerSidebar.tsx
- [Major] Upload buffered the whole file via file.arrayBuffer(), defeating
chunked upload. Stream via file.slice(offset, offset+chunk) per 4MB chunk.
(try/finally around fsClose was already present.)
- [Major] Drag-move didn't sync open tabs: moving an open file left its
editor at the old path; moving a directory left descendant tabs stale.
After fsRename, re-point the moved file's tab and all descendant tabs
(prefix match) to their new paths, mirroring the existing rename handler.
- [Low] Documented that the <a download> filename hint is honored only for
same-origin /task/fetch, ignored for cross-origin S3/Azure presigned URLs.
apps/explorer-ui/src/mediaTypes.ts
- [Major] Legacy .doc mapped to category 'docx', but docx-preview only
handles OOXML .docx (not the old Compound File Binary .doc). Route .doc
to the binary/hex path instead so it no longer fails silently in DocxViewer.
apps/explorer-ui/src/viewers/JsonViewer.tsx
- [Minor] On JSON parse failure the raw content was injected into a fenced
markdown block, so content containing a fence could break out of the code
block. Render the raw fallback via a plain <pre> element (no fence).
apps/explorer-ui/src/viewers/SpreadsheetViewer.tsx (+ package.json, lockfile)
- [Major/Security] sheet_to_html output was injected via dangerouslySetInnerHTML
without sanitization (SheetJS can emit javascript: links -> XSS, CVE-2026-44549).
Pass { sanitizeLinks: true } and run the HTML through DOMPurify.sanitize()
before rendering. Adds dompurify (~3.4.0) to explorer-ui deps.
apps/explorer-ui/src/viewers/HexViewer.tsx
- Name the 206 status literal HTTP_PARTIAL_CONTENT with a comment: a
successful ranged fetch is exactly 206; a 200 (server ignored Range and
sent the whole file) must not be cached under a single chunk index. (401/403
left as meaningful literals per maintainer note.)
packages/ai/src/ai/account/store_providers/azure/azure.py
- [Medium] SAS URL was built from a hardcoded blob.core.windows.net host with
a raw, unencoded blob name -> wrong host for Azurite/sovereign/custom
endpoints and malformed URLs for names with spaces. Derive the base URL
from blob_client.url (correct scheme/host, encoded path) + SAS token.
packages/ai/src/ai/modules/task/fetch.py
- [Major/Security] jwt.decode accepted tokens missing exp -> non-expiring
fetch URLs. Add options={'require': ['exp']}. Also guard file_store._full_path
against a malformed path claim, returning 400 instead of a 500.
.config
- Reworded the RR_SIGNING_KEY block: it is REQUIRED for the filesystem store
backend (downloads/streams fail at runtime if unset); left intentionally
empty so no shared signing secret ships in source (never commit a real key).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs(sdk): document fs_get_url / fsGetUrl in the client guides
The store's fsGetUrl (TS) / fs_get_url (Python) method — added in this PR
for the File Explorer's presigned-URL access — was missing from both SDK
guide docs (CodeRabbit review note on PR #1356).
Adds a "Store (file access)" section with the method signature, return
type, and an example to both single-file guides
(client-typescript/docs/guide/index.md and client-python/docs/index.md),
placed after the Data section to mirror the existing table layout. Notes
that cloud backends return a presigned/SAS URL while the local filesystem
backend returns a JWT-signed /task/fetch URL, and documents the relative
path + expires_in semantics.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs(sdk): document the full fs_* store API in both client guides
Expands the "Store (file access)" section (added for fsGetUrl) to cover the
entire file-store surface in both SDKs, so users have a complete reference
for reading/writing/managing server-side store files — not just an orphaned
fsGetUrl entry.
Both guides now document, grouped for clarity:
- Handle I/O (low-level binary): fsOpen / fsRead / fsWrite / fsClose, with the
open -> read/write -> close lifecycle and 4 MB chunk default noted.
- Convenience wrappers (handle managed internally): fsReadString / fsWriteString
/ fsReadJson / fsWriteJson.
- Directory & metadata: fsListDir / fsStat / fsMkdir / fsRmdir / fsRename / fsDelete.
- Direct URL: fsGetUrl (presigned/SAS for cloud, JWT-signed /task/fetch for local).
Each entry has signature, return type, and description, plus runnable examples
(strings/JSON, browsing, streamed chunked upload, presigned URL). Both docs stay
parallel (TS fsCamelCase / Python fs_snake_case). Notes the relative-path
requirement (absolute-like paths rejected).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(store): server-side Content-Disposition for cross-origin downloads
Fixes asclearuc's [Low] review note on the Explorer download button: the
`<a download>` filename hint is ignored by browsers for cross-origin URLs,
so S3/Azure presigned downloads used the object key instead of a friendly
name (and could open in-tab). There is no reliable client-side fix, so we
control the filename server-side by baking Content-Disposition into the URL.
New optional `download_name` (Python `fs_get_url` / TS `fsGetUrl`) threads
through to each backend:
- S3: `ResponseContentDisposition` param on the presigned URL.
- Azure: `content_disposition` signed into the SAS.
- Local: carried in the JWT claim; `/task/fetch` sets the header.
Semantics: `download_name` set -> `Content-Disposition: attachment;
filename="<name>"` (forces a download with that name, cross-origin safe).
Omitted (default) -> served **inline**, so media viewers keep streaming.
Note this makes the local `/task/fetch` default inline (was attachment);
the download button now passes `download_name`, so downloads still work and
inline streaming (video/audio/pdf viewers) is now correct.
Filenames are sanitized (`_sanitize_download_name`) — CR/LF, quotes, and
backslashes stripped — so they can't inject into the header/quoted token.
Files:
- file_store.get_url: build + sanitize disposition; pass to cloud backends;
add download_name JWT claim for local.
- store.get_url (base) + s3/azure get_url: accept `content_disposition`.
- cmd_task._store_fs_geturl: parse `download_name` arg.
- fetch.py: attachment when claim present, else inline.
- Python/TS SDKs: `download_name` / `downloadName` param on fs_get_url/fsGetUrl.
- ExplorerSidebar download handler: pass the basename as download_name
(keeps `a.download` as a same-origin belt-and-suspenders).
- Both client guides: document the param + inline-vs-download examples.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(store): extract file-store commands into StoreCommands mixin
Move the fs_* file-storage DAP commands out of TaskCommands (cmd_task.py)
into a dedicated StoreCommands mixin (cmd_store.py), mirroring the module
layout already present on feat/alb.
Why:
feat/alb relocated these same fs_* handlers from cmd_task.py into a new
cmd_store.py as part of its larger pod/RequestContext refactor. Because
feat/fileexplorer kept them in cmd_task.py and extended them (Explorer's
download support), every rebase between the two branches produced a large
add-block/delete-block conflict over the entire fs_* section. Aligning the
file layout here shrinks that recurring conflict to just the ctx-parameter
lines, which are small and uniform.
What changed:
- cmd_store.py (new): StoreCommands(DAPConn) owning on_rrext_store, the
_store_subcommand_handlers dispatch table, _get_file_store, and all
_store_fs_* handlers. This is feat/alb's StoreCommands with the alb-only
RequestContext threading removed, since feat/fileexplorer's dispatch layer
calls handlers with (request) only:
* verify_permission('task.store', ctx) -> verify_permission('task.store')
* _get_file_store(ctx) -> _get_file_store() scoped via self._account_info.userId
* dropped the alb-only RequestContext/DAPRequest/DAPResponse imports
- _store_fs_geturl retains Explorer's download_name passthrough (alb's copy
predates it), so get_url still forwards download_name for the
Content-Disposition: attachment path. Verified end to end (client SDKs ->
dispatch -> file_store.get_url -> S3/Azure content_disposition and local
/task/fetch header).
- cmd_task.py: removed the moved on_rrext_store/_get_file_store/_store_fs_*
code and the dispatch-table population in __init__ (now `pass`). No
imports orphaned.
- task_conn.py: wired StoreCommands into TaskConn at feat/alb's positions
(import, base list, and the required StoreCommands.__init__ call that
builds the dispatch table).
- test_cmd_task.py: repointed the moved symbols from TaskCommands to
StoreCommands, bound _get_file_store onto the __init__-bypassed stub via
MethodType, and moved the constructor test to StoreCommands.__init__.
17/17 tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(ai): declare pillow for direct PIL imports in test modules
Four test modules import ``from PIL import Image`` at module scope:
tests/ai/common/image/test_image.py
tests/ai/common/models/test_caption.py
tests/ai/common/models/test_detection.py
tests/ai/common/models/test_pose.py
Pillow was not listed in tests/requirements.txt, and none of that file's
-r includes (ai, ai/common, ai/web requirements) pull it. It is declared
only by the source packages (ai/common/image, vision/*), which install it
lazily via depends() when the source module is first imported — and the
vision variants are skip-install on the PR lane.
Because the tests import PIL at the TOP of the file, the import runs during
pytest collection, before any source depends() executes. In a fresh engine
env that yields `ModuleNotFoundError: No module named 'PIL'`, and pytest
aborts the whole run on the collection error — so `builder ai:test` fails
before any test executes.
Rule: a package imported directly at module scope in a test must be declared
in that test target's requirements, not left to a source module's lazy
depends(). Add pillow (unpinned, matching ai/common/image/requirements.txt's
own declaration). Verified: the 4 modules now collect and pass (23 tests).
A sweep of packages/ai/tests for other top-level third-party imports found no
further instances of this class — the only other undeclared roots (depends,
rocketride, numpy) are first-party/base-engine deps guaranteed by the build.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(vscode): wire native file dialog for embedded-app Browse button (#1011) (#1235)
The Dropper "Browse" button posts {type:'requestFileDialog'}, which the
openLink webview bridge forwards to the extension host. But the host's
onDidReceiveMessage only implemented clipboard paste/copy — it never
handled requestFileDialog. So the request arrived, no native dialog
opened, and nativeFilesSelected was never posted back: the button did
nothing, while clicking elsewhere in the drop zone (the in-iframe
<input type=file>) still worked. That matches the report exactly.
Add the missing requestFileDialog case: open vscode.window.showOpenDialog,
read the selected files, and post them back as nativeFilesSelected (buffers
as number[] so they survive webview message serialization). Mirrors the
existing clipboard and drag-drop bridges in the same handler.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* feat(anonymize): configurable entity types + token redaction style (#1447)
* feat(anonymize): configurable PII entity types
Add an editable `entityTypes` field to the Anonymize node so pipelines can
choose which PII / entity types GLiNER detects and masks, instead of the
previously hardcoded default set. The field is seeded with the 15 common
types and is fully open (zero-shot), so user-written custom labels work too.
- services.json: new `entityTypes` array field, seeded with common defaults,
wired into every model profile (profile-scoped so edits survive config merge)
- glinerRecognizer: owns DEFAULT_PII_LABELS, reads/sanitizes `entityTypes`,
falls back to defaults when unset or empty
- IInstance: anonymize using the configured labels in the standalone path
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(anonymize): add anonymize_tokens for placeholder-token replacement
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(anonymize): branch recognizer on redactionStyle (mask|token)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(anonymize): expose redactionStyle field across all profiles
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(anonymize): correct token-redaction path + harden entityTypes config
Addresses review findings on the token (redactionStyle) and configurable
entityTypes features:
- Token merge collapsed *adjacent* distinct spans (`<=`), dropping the second
entity's label; now merges only true overlaps (`<`) so back-to-back entities
keep their own tags.
- Generic [REDACTED] classification fallbacks were listed before NER token
matches, so the stable offset sort kept [REDACTED] over specific labels on
equal-offset overlaps; specific tokens now come first and win.
- Multi-word entity tokens rendered with underscores ([PHONE_NUMBER]); a new
format_token helper renders clean tags ([PHONE NUMBER]).
- entityTypes from config is now validated as a list before cleaning, so a
stray string can't be iterated into per-character labels (which would
silently disable detection); empty -> defaults fallback preserved.
- anonymize_tokens rebuilt the whole string per match (O(n*k)); now a single
left-to-right join (O(n)), matching the mask path.
- Token span extraction now delegates to convert_ner_results_to_matches
(single source of truth for offset/length).
- DEFAULT_PII_LABELS moved into anonymize.py; a unit test asserts it stays in
sync with services.json's entityTypes default.
- README documents the new redactionStyle and entityTypes fields.
Adds nodes/test/test_anonymize_logic.py (17 server-free unit tests).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(nodes,vscode): add Gmail tool node + Google user-OAuth broker (RR-1055, RR-1142) (#1334)
* feat(billing): promo codes — SDK methods, CheckoutModal promo box, host wiring (#1475)
* feat(billing): promo codes — SDK methods, CheckoutModal promo box, host wiring
Shared/SDK half of the Stripe-native promo code feature (server half lives
in the saas repo). Two code kinds: checkout discount codes and hackathon
credit-grant codes ($0 subscription + one-off token grant, no card).
SDKs (TS + Python, kept in sync):
- createCheckoutSession gains optional promotionCode; return widened to
{clientSecret: string | null, subscriptionId, status} — clientSecret is
null when a 100%-off code makes the first invoice $0 (subscription is
already active, no payment step)
- new validatePromoCode(orgId, code, priceId?) → PromoValidation
(read-only; unknown codes return {valid:false, reason}, never throw)
- new redeemPromoCode(orgId, code) → PromoRedemption
({redeemed, mode: 'subscribed'|'credits_only', appId, status?, credits})
- PromoValidation / PromoRedemption types exported from both packages
shared-ui CheckoutModal:
- "Promo Code" box under the plan cards (rendered when the host wires
onValidatePromoCode/onRedeemPromoCode)
- grant codes (identified by appId metadata) redeem immediately — no plan
selection, no Elements; success block shows the granted credits
- discount codes show an applied chip with the discounted first payment,
flow into checkout via onCreateCheckout(priceId, code); payment recap
shows struck-through list price + renew note for duration==='once'
- clientSecret === null path calls onConfirmPending best-effort then
onSuccess() — Stripe Elements is never mounted for $0 invoices
- CheckoutModal + UpgradeModal hide plans with metadata.kind='promo_base'
(hidden $0 base plans only reachable via promo-code redemption)
Hosts:
- shell-ui CheckoutFlow forwards promotionCode and wires the two promo
callbacks to the billing SDK
- vscode AccountProvider/AccountWebview add checkout:validatePromo /
checkout:redeemPromo resolver pairs (topupResolverRef pattern), forward
promotionCode in checkout:createSession, refresh billing after redeem
- shell-ui BoxIcon: new BxPurchaseTag (used by the saas repo's admin UI)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(billing): address PR #1475 review — promo state consistency + typed payloads
CodeRabbit review fixes:
- AccountWebview: forward `status` when resolving checkout:sessionResult
(the widened callback type promised it; the resolver dropped it)
- views/types.ts: type checkout:validatePromoResult / redeemPromoResult
payloads as PromoValidation / PromoRedemption instead of unknown;
AccountWebview drops the casts and rejects on null results
- CheckoutModal:
- discountedCents prefers the server-computed discountedAmountCents,
local percent/amount math is only the fallback
- preserve the entered code when validation omits a canonical `code`
(handleContinue sends appliedPromo.code to the server)
- switching plans clears an applied discount — validation is
price-specific, so stale amounts can no longer flow into checkout
- Continue is disabled while a promo is validating/redeeming so a
just-entered code can't be silently dropped
- checkout/types.ts: promo callbacks are now paired at the type level
(both or neither) — providing only onValidatePromoCode would have sent
grant codes down the discount path with no error
- UpgradeModal: preselectedPriceId now goes through the same visibility
filter as the picker grid, so hidden promo_base plans can't be reached
via preselection
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(shell-ui): define REACT_APP_OAUTH_ROOT_URL (ReferenceError: process is not defined) (#1486)
shared-ui/config/oauth.ts (added by #1334) reads process.env.REACT_APP_OAUTH_ROOT_URL
at module top level. shell-ui bundles shared-ui from source (eager MF 'shared'),
so shared-ui's rslib define never applies and this token survived unreplaced ->
the shared factory threw 'process is not defined' in the browser. Define it in
shell-ui's rsbuild config like rslib/vscode already do; e() falls back to '' so
it's not hard-required (oauth.ts has its own https://oauth2.rocketride.ai default).
* fix(build): linux build improvement (#1473)
* fix(build): fail the build when Linux dependency install fails (#1465)
check_dependencies ignored the exit code of dep_install and always
printed "Dependencies installed successfully". A failed dnf/apt install
was silently accepted, and the missing runtime libraries only surfaced
later at runtime as "libc++.so.1: cannot open shared object file".
Make a failed install fatal with a clear message, and re-check every
package after the install so a partial install (package manager exits 0
without installing everything) is not accepted either. Also make
dep_install return non-zero on the first failed package so the check
still works when it runs inside an if-condition.
* fix(build): make cc/c++ symlinking idempotent and fail gracefully without sudo (#1469)
On Fedora, the --autoinstall path unconditionally ran `sudo ln -sf` to point
cc/c++ at clang, even when the symlinks were already correct. The builder spawns
this script with stdio captured and no controlling terminal, so sudo could not
prompt for a password and died with "sudo: a password is required" — failing an
otherwise fully-provisioned build with a bare, contextless error.
- Add link_points_to() and guard the dnf symlinking so a box whose cc/c++
already resolve to clang skips the privileged step entirely (no sudo).
- Add run_privileged() wrapping the cc/c++ symlink (dnf) and update-alternatives
(apt) calls: uses sudo only when not root, and on failure prints how to
recover (run ./scripts/compiler-unix.sh --autoinstall standalone in a
terminal, pre-auth with `sudo -v`, or grant passwordless sudo) instead of
aborting the build with sudo's raw error.
apt already guarded this via NEED_CC_ALTERNATIVES, so behavior there is
unchanged aside from the friendlier failure message.
* fix(weaviate): pin grpcio-health-checking<1.80 to match protobuf runtime (#1472)
grpcio-health-checking 1.82.0 regenerated grpc_health/v1/health_pb2 with
protobuf gencode 7.35.0, so it requires protobuf runtime >= 7.35.0. Its
metadata floor was left at protobuf>=6.33.5, so it co-installs with older
runtimes. weaviate-client caps protobuf<7, so the resolver lands on protobuf
6.33.6, and importing weaviate raises VersionError (gencode 7.35.0 / runtime
6.33.6).
The node left grpcio-health-checking unpinned, so it floated to 1.82.0.
weaviate-client already requires grpcio<1.80; pin grpcio-health-checking to
the same range so the stub gencode stays on the protobuf 6.x line.
* fix(ai): install test deps via depends() so engine constraints apply (#1466) (#1471)
The ai:test pre-install used plain pip, which is not handed the engine's
compiled constraints file. Unpinned deps (e.g. pillow) resolved to latest
instead of the engine-pinned version. On Windows this corrupted the install:
pillow was pulled to a newer version, then depends() tried to downgrade it at
import and could not overwrite the loaded _imaging*.pyd, leaving a half-written
PIL package (ModuleNotFoundError: No module named 'PIL').
Install the test requirements through depends() instead, so uv applies the
constraints and every dep resolves to its pinned version.
* fix(ai): migrate GPU import guard to find_spec/exec_module (Python 3.12+) (#1460)
## Summary
- Migrate `ai.common.models.gpu_guard._GPUImportBlocker` from the legacy import-hook
protocol (`find_module`/`load_module`, **removed in Python 3.12**) to the modern PEP 451
protocol (`find_spec` + `create_module`/`exec_module`).
- Without this, on Python 3.12+ the hook silently becomes a no-op and `--modelserver` mode
stops blocking direct `torch`/`tensorflow`/`onnxruntime`/`cupy` imports — a fail-open
isolation/billing regression with no error.
- Relocate the test to the mirrored path (`tests/ai/common/models/`), rewrite it for the
new protocol, and add an end-to-end regression test that drives a real `import` through
the machinery so a future silent no-op is caught.
## Type
fix
## Testing
- [x] Tests added or updated
- [x] Tested locally
- [ ] `./builder test` passes <!-- ran targeted `ai:run-pytest` equivalent: 8 passed on Python 3.12.13 via the engine interpreter; full ./builder test not yet run -->
Verification detail:
- `ruff check` + `ruff format --check` clean on both changed files.
- `engine.exe -m pytest tests/ai/common/models/test_gpu_guard.py` → **8 passed** on
Python 3.12.13 (the version where the old protocol silently failed).
- Repo-wide audit confirmed `gpu_guard.py` is the only hard 3.12 break in first-party code.
## Checklist
- [x] Commit messages follow [conventional commits](https://www.conventionalcommits.org/)
- [x] No secrets or credentials included
- [ ] Wiki updated (if applicable)
- [x] Breaking changes documented (if applicable) <!-- CHANGELOG.md → Unreleased → Fixed -->
## Linked Issue
Fixes #1459
* feat(events-ui): real-time DAP event monitor micro-frontend (#1484)
* feat(events-ui): real-time DAP event monitor micro-frontend
Web UI equivalent of the Python CLI `rocketride events` command.
Registers for server events via the SDK's addMonitor/removeMonitor API,
captures them in memory, and displays them in a scrolling list with
expandable JSON inspection and JSON file download.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(events-ui): address CodeRabbit review findings
- Move nextId counter into component ref, reset on clear
- Cancel pending rAF on unmount to avoid dangling callbacks
- Amortize event buffer trim (1.2x overshoot before slicing)
- Log addMonitor/removeMonitor errors instead of swallowing
- Auto-scroll uses last event ID instead of filtered.length
- Append download anchor to DOM before click for browser compat
- Remove direct commonStyles import from Toolbar, use styles re-exports
- Add aria-pressed to event type toggle chips
- Add .catch() to AppDescriptor dynamic import
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(events-ui): cap events at 1,000 and drop unnecessary useCallback
Reduce MAX_EVENTS from 10,000 to 1,000 to keep the DOM manageable
without adding a virtualization dependency. 1,000 is still 40x the
Python CLI's 25-event window — plenty for debugging.
Remove the useCallback wrapper on the useShellEvent handler since
useShellEvent already stores the handler in a ref internally.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* chore(nodes): classify FalkorDB as store (classType ["store","tool"]) (#1474)
FalkorDB was the only DB/store node tagged classType ["tool"] while the
vector stores use ["store"]/["store","tool"] and the SQL/graph DBs use
["database","tool"]. Per Rod, add "store" to classType so the node lands
in the right canvas category regardless of its source folder name.
* feat(test-ui): stress/chaos testing app for RocketRide backend (#1462)
* feat(test-ui): stress/chaos testing app for RocketRide backend
Adds apps/test-ui, a Module Federation app for stress/chaos testing the
RocketRide backend (scenario runner, swarm/latency/metrics panels, API-coverage
tracking, live event stream). It consumes the shell-ui shell and the rocketride
TS SDK like the other MF remotes.
This app was originally developed on feat/alb but was entangled with unrelated
dashboard/decoupling work there, bloating that PR by ~7.8k lines. It is a
self-contained leaf (nothing imports it) and depends only on shell-ui / SDK
surface that already exists on develop, so it is split out here onto its own
branch off develop for independent review. Removed from feat/alb in the
companion commit.
Registers the app in pnpm-workspace.yaml. Verified with
`./builder test-ui:clean test-ui:build` against the develop base — the MF
bundle (remoteEntry.js) builds and registers cleanly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(test-ui): address CodeRabbit review feedback on PR #1462
Resolves all 14 review threads. Correctness and stability fixes:
- engine: chunk crypto.getRandomValues() into 64 KiB blocks (new
randomBytes() helper). The Web Crypto spec throws QuotaExceededError
above 65,536 bytes, so 1 MB/100 MB binary payloads previously crashed
payload generation before any data reached the server.
- engine: give the ping heartbeat client a 5s requestTimeout instead of
the 300s default. The catch-block already classified >5000ms as "Ping
timeout", but a stalled ping could block the 100ms heartbeat loop for
5 minutes, defeating its purpose of catching event-loop stalls fast.
- engine: add getPipelineClient() that resolves a pipeline's recorded
clientIdx as a stable slot into clientPool. getPoolClient() re-filters
connected clients, so when an earlier pool client dropped, later sends/
terminates could silently re-map onto a different WebSocket than the
one that owns the pipeline token. Round-robin getPoolClient() remains
for initial assignment and "any client" chaos phases.
- monitor: cap per-method latencies at 500 samples (rolling window via
new pushLatency(), also used by the ping loop in engine.ts). send/ping
run thousands-to-millions of times in long stress runs; the unbounded
array bloated memory and slowed percentile recomputation in panels.
Maintainability/UX fixes:
- engine: reset() now delegates to clear() — bodies were byte-identical
with misleading docstrings (settings are view-owned; TestSessionView
restores config itself on reset).
- store: new createExternalStore() factory owning the listener-Set /
emit / useSyncExternalStore boilerplate previously re-implemented in
navigation.ts, connections.ts, and actions.ts.
- TestSessionView: lazy engine ref init — useRef(createTestEngine())
constructed and discarded a fresh engine on every rAF-driven render.
- SwarmPanel: header now shows "showing 64/N" when pipeline count
exceeds the 64 grid slots instead of silently truncating.
- ConnectionManagerView: FormState.mode is a discriminated union
({type:'add'} | {type:'edit';id}) — 'add'|string collapsed to string
and could collide with a connection id; handleSave builds the new
connection object once instead of duplicating the field set; form
labels are paired to inputs via htmlFor/id for a11y.
- TestSidebar: type handleOpenConnection with SavedConnection, not any.
- package.json: move @module-federation/rsbuild-plugin to
devDependencies (build-time only, same role as @rsbuild/*).
- pnpm-workspace.yaml: restore alphabetical order of apps/* entries.
- license headers: add the full MIT header to files edited here that
were missing it (TestSessionView, SwarmPanel) or had a placeholder
(actions.ts), per repo file standards.
Verified with ./builder test-ui:clean test-ui:build (MF bundle builds
and registers cleanly); no new tsc errors in touched files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(vscode): repoint dead example links to awesome-rocketride (#1422) (#1448)
The extension's "example pipelines" block linked to
docs.rocketride.org/examples/{advanced-rag-pipeline,video-key-frame-grabber,audio-transcription-simple},
which no longer resolve, so a new builder hits a dead end on the first
click. Repoint to the awesome-rocketride demos-and-examples collection.
Both tracked sources carried the block: apps/vscode/docs/index.md
(docs-site surface) and docs/README-vscode.md (source for the generated,
gitignored marketplace apps/vscode/README.md).
Closes #1422
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat: add HydraDB database node (db_hydradb) (#1499)
Adds the db_hydradb node (database + tool) with store_memory / recall_memory / query_graph / get_schema over the HydraDB v2 REST API. Ingest uses multipart form-data (live-verified). Shared post_with_retry gains data=/files= passthrough plus get_with_retry.
* feat(ci): migrate Discord notifier workflows to forum channels (tags + auto-archive) (#1510)
* feat(ci): migrate Discord notifier workflows to forum channels (tags + auto-archive)
Convert the three Discord notification workflows (issues, PR, discussions) to
target Discord forum channels:
- Forum posts: the webhook create POST sends thread_name and captures/stores
the new thread_id in the sync marker; PATCH/DELETE are thread-scoped; POST is
at-most-once (--no-retry-5xx) to avoid duplicate threads.
- Tags (webhook-only): applied_tags resolved on create from a committed,
non-secret config (discord-forum-tags.json) mapping GitHub state/labels (and,
for discussions, answer status + category) -> Discord tag IDs.
- Archiving (bot token, optional): discord_archive_thread archives closed/merged
threads and unarchives reopened ones. Only place a bot token is used; no-op if
DISCORD_GITHUB_BOT_TOKEN is unset.
Webhook secrets renamed to DISCORD_*_FORUM_WEBHOOK_URL for a zero-downtime
cutover; bot token secret is DISCORD_GITHUB_BOT_TOKEN. Channel/tag IDs are
committed (not secrets).
Closes #1509
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ci): make Discord notifiers degrade against the default-branch helper
These notifiers source discord-helper.sh from the default branch (fork-safety),
so the PR that first introduces new helper functions runs the new workflow YAML
against the OLD helper and aborts with "command not found" (exit 127) at the
first new-helper call.
Add no-op fallback shims for discord_applied_tags / extract_discord_thread /
discord_archive_thread right after sourcing the helper, so the workflow degrades
gracefully (no tags / no archive) instead of failing. The real implementations
take over automatically once this merges to the default branch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(ci): two-message forum posts — link card + detailed embed
Restructure each notifier into the two-message forum layout:
1. Starter (root) message: a plain "**#N title**" + URL. Discord unfurls it
into the GitHub card, which becomes the forum grid-view image. thread_name
and applied_tags go on this create POST.
2. Detail embed: posted as the 2nd message in the thread and tracked in the sync
marker (msg id + thread id). It now carries the issue/PR/discussion body as
the embed description (HTML-comment-stripped, trimmed, capped at 3800), an
"opened this … on <date>" author line, and a "repo · #N" footer. Follow-up
events PATCH this embed (thread-scoped); the starter link stays put.
Both POSTs are at-most-once (--no-retry-5xx); if the detail POST fails the just-
created thread is deleted so the next event retries cleanly. Race guard deletes
the whole duplicate thread (via its root message) and patches the winner.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ci): address review comments on Discord notifiers
- Clear DISCORD_THREAD_ID in the 404 repost branch (issues/pr/discussions) so
the archive step targets the recreated thread, not the deleted one.
- discord_applied_tags: resolve in priority order and dedupe preserving order
before the 5-tag cap, so a state tag is never dropped past 5 tags.
- Paginate the issue/PR comment lookups (main + race-guard re-check) so the sync
marker is found on repos where a PR/issue has many comments.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(ci): add good-first-issue forum tag (issues + PR)
Created on the issues (after open/closed) and PR (after open/closed/merged)
forum channels; mapped from the "good first issue" GitHub label in the tag
config.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore(ci): drop good-first-issue tag from PR channel
Keep it on the issues channel only; remove the PR-channel tag and its config
label/id references.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MacBook-Pro.local>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MBP.attlocal.net>
* feat(nodes): add currency_convert_explicit node (#1497)
* feat(nodes): add currency_convert_explicit node
Add `currency_convert_explicit`, a filter node (answers -> answers) that
converts monetary facts from a source to a target currency at an explicitly
stated rate and date supplied in config. Part of the audit-grade financial
extraction node suite (#1432). Mirrors anonymize/guardrails and the suite
siblings reconcile (#1457) / authoritative_overlay (#1485).
- Opt-in / never implicit: the rate is stated in config; nothing is fetched
from a live FX service (no network, no credentials). Only facts tagged with
the configured source_currency are converted; everything else passes through.
- Non-destructive: preserves the original amount+currency, adds a `converted`
block, and appends a `provenance` entry {rate, rate_date, source->target}.
Exact Decimal math with half-up rounding; standard library only.
- Configurable JSON fact convention (amount_field/currency_field) so the node
ships standalone while normalize_facts (#1427) is still unbuilt.
Tests: 17 unit tests + 2 service test-block cases; ruff check/format clean;
full contract suite (pytest nodes/test/test_contracts.py) 288 passed. Committed
with --no-verify (lefthook gitleaks/ruff not on PATH in sandbox); ruff run
manually.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): emit currency_convert_explicit answers via preventDefault + closing
The live-engine dynamic test (test_dynamic.py) revealed the node double-drove
the answers lane: writeAnswers emitted a new answer without calling
preventDefault, so the engine ALSO auto-forwarded the original — producing two
malformed answers (Results: {'answers': ['', '']}) instead of one converted
record.
Adopt the proven anonymize pattern: take ownership of the lane with
preventDefault() in writeAnswers, collect the converted payload (extracted
immediately so nothing depends on the engine reusing the Answer object), and
re-emit each record exactly once in closing(). Behaviour is unchanged —
non-destructive conversion of source-currency facts, verbatim pass-through of
everything else. Pure convert.py logic and its 17 unit tests are untouched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): drop infeasible dynamic test block for currency_convert_explicit
The dynamic node-test harness injects lane input through data_conn, which
builds answers with `Answer.model_validate_json(...)`. The Answer schema's
`prohibit_answer_on_creation` field validator raises if `answer` is set at
construction, so a webhook-injected `answers` input can only ever be empty
(verified: model_validate_json can populate expectJson but never answer
content). An answers-consuming node therefore always receives an empty answer
under the harness, and an engine-level dynamic test can never deliver a
populated fact — the earlier test block failed for this reason
(Results: {'answers': ['']}), not because of the conversion logic.
Remove the test block (matching anomaly_detector and other nodes that ship
without one). Coverage is retained by the 17 unit tests in
nodes/test/test_currency_convert_explicit.py and the contract suite
(service-contract + module-exists). The preventDefault + closing emit fix from
the previous commit is kept — it is the correct single-emit behaviour for when
a real upstream node (e.g. normalize_facts) feeds populated answers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: kgarg2468 <181051779+kgarg2468@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(nodes): add n8n workflow-automation node (#1231)
* feat(nodes): add n8n workflow-automation node
Add `tool_n8n`, a dual node (pipeline step + agent tool) that triggers
self-hosted n8n workflows and consumes their results. Mirrors tool_github
(multi-fn REST + sibling client module) and db_postgres (dual classType).
- RR->n8n: webhook trigger -- sync (Respond-to-Webhook) or async polling of
executions via a correlation id; public REST API to list/inspect workflows
and executions; activate/deactivate gated by a read-only toggle.
- Rich I/O: simple or structured payloads (documents + metadata); files/binary
both ways via multipart (image/audio/video lanes); table output lane.
- Safety/DX: SSRF containment (agent never picks the host); none/header/basic/
bearer/JWT webhook auth; TLS-verify toggle; deploy-aware reachability
preflight (host.docker.internal hint); 404->activate and ack->Respond-node
guidance; configurable sync/async timeouts; n8n execution deep-links.
- n8n->RR + round-trips documented (docs/README-n8n.md) with importable
templates under examples/n8n/ (fan-out, agent, round-trip, dispatcher).
Verified: ruff clean, 283 unit + contract tests, 15/15 live e2e against a real
n8n (sync / async / sequential / round-trip / binary / non-webhook reach).
Closes #1230
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(ci): allow digits in gitleaks env-var placeholder allowlist
The pipe-file/services-json API-key rules allowlist `${VAR}` placeholders,
but the regex `[A-Z_]+` excluded digits — so `${ROCKETRIDE_N8N_KEY}` and
`${ROCKETRIDE_N8N_URL}` (the `8` in n8n) were flagged as secrets while
digit-free names like `${ROCKETRIDE_OPENAI_KEY}` were not. Widen the
allowlist to `[A-Z0-9_]+` so digit-containing env-var names are recognized.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): address review feedback on tool_n8n
Resolve the 9 actionable items from the PR #1231 review:
- SSRF: _safe_path rejects path traversal, query/fragment, and backslashes
- Normalize tool-face results (binary -> descriptor, JSON scalars -> string)
to the advertised object/array/string/null schema
- Compare async execution timestamps as tz-aware datetimes, not strings
- Pin idna>=3.10 (GHSA-65pc-fj4g-8rjx) in node + shared requirements
- Advertise the table lane on image/audio/video routes
- Smoke test requires ROCKETRIDE_N8N_WORKFLOW so a workflow is configured
- Sync the README config table (Sync timeout; bearer/jwt webhook auth)
- MD040: add a language to the README/example fenced diagrams
- Extend the unit suite (path guard, result normalization, timestamp compare)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): address n8n review nits
---------
Co-authored-by: Krish Garg <krishgarg@Krishs-MacBook-Pro.local>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): warn that parse/llamaparse are not provenance-preserving (RR-1406) (#1507)
The native parse and the llamaparse node flatten documents to Markdown,
dropping page boundaries, bbox/polygon coordinates, and the table-HTML cell
grid, so cell-level provenance cannot be reconstructed for audit-grade
extraction.
Document this limitation prominently on both nodes (READMEs + catalog
description) and emit a one-time runtime warning from the LlamaParse node
steering users to the structure-preserving datalab_parse node (#1425). No
parsing/output behavior change; the structural lossless parser is tracked
separately by #1425, which Epic #1432 names as the structural fix for this bug.
Fixes #1406
Co-authored-by: kgarg2468 <181051779+kgarg2468@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): friendlier chroma port + splitter-profile config (RR-1416) (#1496)
* fix(nodes): friendlier chroma port + splitter-profile config (RR-1416)
Two strict-schema config-friction papercuts from Aparavi's field feedback
(issue #1416, reports I-9 / I-11). The engine owns the raw AJV-style
validation text, so this fixes the friction declaratively (schema + node
coercion + docs) without any engine change.
chroma port (I-11): env-var interpolation resolves to a string, but the
port field inherited type:number from the shared core vector schema, so
`${ROCKETRIDE_CHROMA_PORT}` / "8000" failed validation with no useful
message. Fixed with a chroma-LOCAL override (shared
core/services.common.vector.json is intentionally left untouched to avoid
blast radius across every vector node): the port field now accepts
number-or-string, and Store._coercePort normalizes int / numeric string /
whole-number float, falling back to the default for an unresolved
placeholder, bool, or non-numeric input rather than crashing the client.
splitter const-lock (I-9): selecting MarkdownTextSplitter under the
`default` profile fails with ".default.splitter must be equal to
constant". The const design is intentional (each profile pins its
splitter), so this adds title/description guidance on the profile selector
and every const-locked splitter field naming the right profile, plus a
README note. No behavior change.
Regenerated the GENERATED:PARAMS doc tables from services.json via
nodes:docs-generate. Adds nodes/test/chroma/test_coerce_port.py pinning
every _coercePort branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(nodes): address review feedback on RR-1416 (port bounds + custom-splitter docs)
CodeRabbit review on #1496 raised two valid items; both addressed.
1. chroma _coercePort: add a TCP port-range guard (1-65535). CodeRabbit's
suggested patch only guarded the string branch, but a raw int/float
(-1, 99999, 99999.0) would also bypass it, so every numeric branch (int,
whole-float, string) now funnels through one `1 <= parsed <= 65535` check
that falls back to the default with a debug log. Added regression tests for
out-of-range int/string/float and the 1 / 65535 / 443.0 boundaries.
2. preprocessor_langchain custom-splitter docs: custom.splitter is const-locked
to RecursiveCharacterTextSplitter, but the README implied it accepted a
user-selectable class. Corrected the intro, the profile table row, and the
Custom section, and added custom to the "each profile locks its class" note.
For consistency with the other six splitter fields, gave custom.splitter the
same locked title/description in services.json and regenerated the
GENERATED:PARAMS table. Docs/schema-text only, no behavior change (the const
is unchanged; unlocking the custom profile is left as a follow-up).
Verified: ./builder nodes:test -> 1405 passed, 48 skipped; ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(ci): read full marker body in Discord issue/PR notifiers (#1530)
The sync-marker lookup piped the comment body through `head -1`, but the marker
lives inside a multi-line <details> block, so only "<details>" was kept and the
msg/thread ids never resolved. The notifier then treated every event as a fresh
post and created a duplicate forum thread each time.
Read the first matching comment's full body instead (via jq `first(...)`), so
extract_discord_marker / extract_discord_thread see the marker line. The `.id |
head -1` lookups in the 404 branch are unchanged (`.id` is single-line).
Closes #1529
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MBP.attlocal.net>
* docs(readme): revamp landing README with Cloud + On-Prem sections (#1528)
* docs(readme): revamp landing README with Cloud + On-Prem sections
- Lead with a cloud-first 'Two ways to run' table (Cloud | On-Prem) and align its CTAs
- Add a dedicated RocketRide Cloud section (4 value pillars, connect snippet, CTAs)
- Add a parallel On-Prem section (control/MIT/anywhere/same engine)
- Swap the CI badge to a shields.io GitHub-Actions status badge (renders reliably)
- Add branded visuals (pipeline, SDK, install) as in-repo images/ assets
- Remove emojis; rename self-host -> on-prem
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(readme): use poster banner as the hero image
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: mark RocketRide Cloud as available in use-cases
Reconcile the stale 'coming soon' line in evaluate/use-cases.mdx with the
live status shown across the README, cloud.md, and cloud.rocketride.ai.
Addresses CodeRabbit review on PR #1528.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(readme): rename Self-Hosting CTA label to On-Prem
Retire the last 'Self-Hosting' visible label (href unchanged) for
consistency with the On-Prem terminology. Addresses CodeRabbit review on PR #1528.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(readme): restore native GitHub Actions CI badge
The workflow badge renders correctly on the repo README; the shields.io
swap is unnecessary. Reverting to the native badge.svg.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(readme): make Runtime badge track the latest server release
Replace the hardcoded Runtime v3.1.0 badge with a shields.io dynamic
github release badge filtered to server-v* tags, so it always reflects
the latest server release (currently server-v3.3.1). Addresses
@joshuadarron review on PR #1528.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MBP.attlocal.net>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MacBook-Pro.local>
* fix(depends): pin bootstrap tools and pydantic to stop downgrade churn (#1536)
* fix(depends): pin bootstrap tools and pydantic to stop downgrade churn
Base modules were installed unpinned and picked up 'latest', then a later
constrained install had to downgrade them. Downgrading a package with a loaded
binary (.pyd/.so) locks/crashes cross-platform — worst on Windows, where
parallel pytest workers race to overwrite the already-loaded pydantic-core /
cryptography DLL and the suite fails.
Cure the immediate wound by pinning the things that float to 'latest': the
bootstrap tools (uv is the resolver itself, wheel/setuptools are the build
backend, so they install outside any constraint) and pydantic (the base module
Rod flagged). An exact pin, not a floor — a floor still resolves to newest.
- depends.py: add _BOOTSTRAP_TOOL_VERSIONS (platform-overridable) + _tool_spec();
_ensure_wheel/_ensure_setuptools/_ensure_uv now install uv==0.11.25,
wheel==0.47.0, setuptools==82.0.1 (the known-good versions already shipped)
- tasks.js: build-tools install pinned to the same versions, in lockstep
- nodes + ai requirements.txt: pydantic==2.12.5 (exact; cross-platform safe,
proven compatible with the full requirement set)
* refactor(build): rename test-requirements.txt to build-requirements.txt
The file also carries build and model-server deps, so the "test" name misleads.
* fix(auth): register /auth/vscode/google outside standardEndpoints gate (#1543)
The route was only registered under `if register_standard_endpoints:`, which
eaas.py disables in cloud (standardEndpoints=False), so the public OAuth-broker
bounce was dark in cloud and 401'd — the Gmail tool's Google login never
completed. Move it to the always-on section next to /version.
* feat(server, nodes): add json lane (#1297)
* feat(shared-ui): pipeline TTL settings — toolbar cog + modal, VS Code support (RR-309) (#1521)
* feat(shared-ui): add pipeline settings (cog) button to canvas toolbar
Adds an optional onOpenSettings callback threaded through
ProjectView -> Canvas -> FlowContainer -> FlowProvider -> FlowProjectContext,
rendered as a Settings (cog) ToolbarButton in the floating canvas toolbar.
Hosts that omit the callback (e.g. VS Code, readonly status views) see no
button. Used by the SaaS Pipeline Builder to open the idle-timeout (TTL)
settings modal (RR-309).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(shared-ui): own TTL settings modal in ProjectView; wire ttl through VS Code host
Moves the idle-timeout (TTL) settings modal from the SaaS host into
shared-ui so every host gets it:
- New modules/project/TtlSettingsDialog.tsx (portal modal; presets +
custom integer minutes).
- ProjectView owns the dialog + value: persisted per-pipeline in prefs
(pipelineTtl map keyed by project_id), riding the existing
onPrefsChange channel. Supplies its own onOpenSettings to the canvas,
so the toolbar cog no longer needs a host prop; onOpenSettings is
removed from IProjectViewProps.
- onPipelineAction gains optional options.ttl (run/restart), backward
compatible for hosts that ignore it.
- VS Code host forwards ttl through the status:pipelineAction message
into client.use; prefs already persist via workspaceState.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(shared-ui): gate settings cog on !isLocked; add dialog ARIA semantics
Review feedback (CodeRabbit, PR #1521):
- Settings cog now hidden while the canvas is locked, matching the
onSave/onExport toolbar pattern.
- TtlSettingsDialog gets role=dialog, aria-modal, aria-label so screen
readers announce the modal.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(shared-ui): trap Tab focus in TTL dialog; restore opener focus on close
Review feedback (CodeRabbit, PR #1521): keyboard users could tab out of
the modal, and closing it dropped focus instead of returning to the
settings button.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(vscode): address CodeRabbit review on #1521 (openExternal scheme allowlist)
- add ttl?: number to the status:pipelineAction ProjectWebviewToHost variant
so the webview's run message compiles against the contract
- capture the dialog opener synchronously via a useRef initializer so focus
is restored to the settings button (not the unmounted select) on close
- handle Escape on the TTL dialog wrapper so it closes from any focused
element; drop the duplicate Escape checks on the select/input
- document the Pipeline Settings idle timeout and the ttl message field in
apps/vscode/docs/usage.md
- add shared isAllowedExternalUrl helper and gate the WelcomeProvider
openExternal handler behind an https/http/mailto scheme allowlist
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* revert(vscode): drop openExternal allowlist unrelated to RR-309
The isAllowedExternalUrl helper and WelcomeProvider gating were not part of
any review comment on this PR; they belong to a separate hardening effort.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: mithileshgau <55141824+mithileshgau@users.noreply.github.com>
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MBP.attlocal.net>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Rod-Christensen <98939082+Rod-Christensen@users.noreply.github.com>
Co-authored-by: Meetpatel06 <65815199+meetp06@users.noreply.github.com>
Co-authored-by: Dylan Savage <130109357+dylan-savage@users.noreply.github.com>
Co-authored-by: Joshua Phillips <J.dspears@yahoo.com>
Co-authored-by: Alexandru Sclearuc <asclearuc@gmail.com>
Co-authored-by: Charlie Gillet <102723287+charliegillet@users.noreply.github.com>
Co-authored-by: Poushali Deb Purkayastha <65014940+Poushali0202@users.noreply.github.com>
Co-authored-by: Mithilesh Gaurihar <mithileshgaurihar@Mithileshs-MacBook-Pro.local>
Co-authored-by: Krish Garg <kgarg@chapman.edu>
Co-authored-by: kgarg2468 <181051779+kgarg2468@users.noreply.github.com>
Co-authored-by: Krish Garg <krishgarg@Krishs-MacBook-Pro.local>
Co-authored-by: Ariel Vernaza <ariel@lazyracoon.tech>
Co-authored-by: stepmik <stepmikhaylov@yandex.ru>
joshuadarron
requested changes
Jul 18, 2026
joshuadarron
left a comment
joshuadarron
left a comment
Collaborator
There was a problem hiding this comment.
Review — authoritative_overlay node
Solid concept and the abstain-on-mismatch design is the right call for audit use. However there are functional bugs that make most of the advertised regulators non-operational, plus convention issues. Requesting changes.
🔴 Critical — config lifecycle is never invoked
IGlobal (nodes/src/nodes/authoritative_overlay/IGlobal.py) overrides initialize() and terminate(). Those are not the engine lifecycle hooks. IGlobalBase (packages/server/engine-lib/rocketlib-python/lib/rocketlib/filters.py:314) defines beginGlobal() / endGlobal() — that's what every other node uses. Consequences:
initialize()never runs, soself.regulator_typestays at the__init__default'sec'forever. Selecting IFRS / Companies House / EDINET has no effect — every run cross-checks against SEC.- Your
testblock only exercisesprofiles: ["basic"](sec), so CI stays green while the other three regulators are dead. Please add a non-SEC test case; it will surface this immediately.
Rename to beginGlobal/endGlobal.
🔴 Critical — getProperty is not part of the config API
self.getProperty('authoritative_overlay.profile') — IGlobalBase has no getProperty method, and zero other nodes use it. The established pattern (see currency_convert_explicit/IGlobal.py) is:
raw = Config.getNodeConfig(self.glb.logicalType, self.glb.connConfig) or {}
self.regulator_type = str(raw.get('regulator_type') or 'sec').strip()As written, if initialize() were called it would AttributeError. Combined with the hook-name bug above, config is doubly broken.
🟠 Correctness
- Concept taxonomy isn't unified across regulators. SEC lookup keys on
facts.us-gaap.<Concept>(e.g."Revenues"), but IFRS/CH/EDINET connectors key onstatements.<key>("revenue","turnover","net_sales"). The same fact requires a differentconceptstring per regulator, and there's no mapping layer — so a pipeline can't be regulator-agnostic. Consider a concept-normalization map. - Exact float-equality match (
normalized_text not in official_data). Real filings are frequently rounded (to thousands/millions), so an exact==on floats will produce false abstains on legitimate matches. Use a tolerance (orDecimalwith a documented rounding rule).
🟡 Conventions / hygiene
- No MIT license header on any of the new
.pyfiles — every sibling node carries the full header block. Please add it. - README hand-edits the generated block. Content (the Schema table + "Do not edit by hand") sits between
<!-- ROCKETRIDE:GENERATED:PARAMS START/END -->. Per the co-located-doc rule that block is produced bynodes:docs-generateand must not be authored by hand — move prose outside the markers and leave the block empty for CI to populate. - Unrelated
pnpm-lock.yamlchurn (brace-expansion/minimatch dedupe) is bundled into a Python-node PR. Please drop it. - Wording overstates capability: README says it "queries the configured regulator database," but the connectors only read local
testdata/*.jsonsnapshots. Call them snapshots so operators don't assume live regulator lookups.
🟡 Minor
services.jsonfields: everyconditionalvalue maps to the sameauthoritative_overlay.basicobject; the profile-specific objects the config code expects (.ifrs,.companies_house,.edinet) aren't defined — tied to the config bug above.writeTextreconstructing anAnswerfrom a raw JSON string is test-harness plumbing living in the node; works, but worth a comment that it's thetext-lane entry point.
Once beginGlobal/getNodeConfig are fixed and a non-SEC test case is added, this should be close.
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. |
Author
|
Thank you @joshuadarron for the review! I've pushed a new commit addressing all your request:
Let me know if you need anything else! |
joshuadarron
requested changes
Jul 28, 2026
joshuadarron
left a comment
joshuadarron
left a comment
Collaborator
There was a problem hiding this comment.
The design instinct here is good — a guard classType, experimental capability, and fail-closed abstain on every uncertain branch (bad input, failed normalization, unknown regulator, connector error, no match) is the right posture for something that gates financial answers. But there are two blockers, and the second one is structural rather than a matter of polish.
Blocking
1. The PR contains a large amount of unrelated working-tree content
Committed at the repository root:
c1.json c2.json c3.json comments.json issue_comments.txt
review_comments.json review_comments.txt reviews.txt
excludes.txt req.txt parse.py test_case.py test_harness.py
test_polars.sh test_pydantic.py test_rocketlib.py test_sec.py
pr1549.patch
Plus nodes/test/debug_authoritative.py and an unrelated pnpm-lock.yaml change (+83/−28).
A few of these identify where they came from: excludes.txt contains polars, req.txt contains img2table / polars-lts-cpu, and test_polars.sh is a shell harness — those are working artifacts from your AVX2/polars work in #1376, not from this feature. pr1549.patch is 1792 lines and is a different PR's patch file. review_comments.json, reviews.txt, and comments.json look like scraped GitHub review data.
That accounts for roughly 4,900 of the 5,641 added lines. Please drop all of it and force-push so the diff is just the node — right now the actual change under review is about 750 lines, and it's hard to find. A .gitignore entry or a scratch directory outside the repo would stop this recurring; it's the same class of thing that will make #1376 harder to review too.
2. The connectors don't connect to anything
This is the part I can't wave through. All four "regulator connectors" read a static JSON file committed inside the node:
# connectors/__init__.py
snapshot_path = os.path.join(os.path.dirname(__file__), '..', 'testdata', snapshot_filename)connectors/sec.py is explicit about it — """Simulates querying the US SEC EDGAR database by reading a local snapshot.""" — and ifrs.py, companies_house.py, and edinet.py are three-line wrappers over the same fixture loader. There is no HTTP call anywhere in the node, and requirements.txt confirms it: "no extra pip dependencies required at this time."
The fixture is nodes/src/nodes/authoritative_overlay/testdata/sec_snapshot.json:
{ "companyName": "TEST CORP INC", "cik": "0001234567", ... }with two concepts (Revenues, NetIncomeLoss) for one fictional filer. services.json exposes exactly one field — regulator_type — so there is no CIK, company identifier, API key, or endpoint a user could configure. Every connector also accepts extracted_text and ignores it.
The practical consequence: deployed against real data this node abstains on essentially everything, because the only values that can ever match are the ones in that fixture. It's a guard that always fails closed, which is safe but not useful, and the PR body describes it as cross-checking "against official regulator data" — which is what a reviewer or a user would reasonably expect it to do.
I don't think this needs to ship complete on all four regulators. Two options that would work for me:
- Land one real connector. SEC EDGAR has a public JSON API (
https://data.sec.gov/api/xbrl/companyconcept/CIK{cik}/us-gaap/{concept}.json) with no key required, just aUser-Agentrequirement. Add a CIK/company field toservices.json, wire SEC for real, and drop the other three until they're implemented. - Reframe the PR honestly. If the intent is to land the guard skeleton and normalization first with connectors to follow, say so in the title and body, keep the fixture loader behind an explicit
offline/snapshotprofile inservices.json, and open follow-up issues for the four real connectors. Theexperimentalcapability already there supports this.
Either way, testdata/ shouldn't sit under nodes/src/nodes/ in the production tree — test fixtures belong in nodes/test/.
Non-blocking
_normalize_number mis-orders suffix stripping and negative handling. The k/m/b checks run before the parenthesized-negative unwrap, so a parenthesized value carrying a scale suffix never gets scaled:
'(1.5m)' -> endswith('m') is False (ends with ')')
-> unwrap parens -> '-1.5m'
-> float('-1.5m') -> ValueError -> None -> abstainAccounting statements write negatives in parentheses and scale suffixes constantly, so this is likely to be hit. Unwrap the parentheses first, then strip the suffix. Worth a unit test per branch of this function — it's pure and easy to cover, and right now nothing exercises it directly.
Field naming diverges from every other node. services.json sets "prefix": "authoritative_overlay" but declares the field as bare regulator_type, where other nodes use <prefix>.<field> (gcs.bucketName, firestore.database, google.authType). It may resolve at the root either way, but please match the convention.
Concept mapping is asymmetric. CONCEPT_MAP is applied only when regulator_type != 'sec', and query_sec gets the raw concept. That's correct given the map is US-GAAP→generic, but a comment saying so would help — it currently reads like an oversight.
math.isclose(..., rel_tol=1e-9) is effectively exact equality for these magnitudes. That's a defensible choice for a guard, but it means a filing restated to the nearest thousand won't match a value extracted to the unit. Worth making the tolerance configurable, or at least documenting the intent in the README.
IInstance.open / close and IGlobal.endGlobal are empty pass bodies. Fine if the framework requires them; if not, drop them per the repo's "no structure without a current need" rule.
Happy to re-review once the tree is cleaned up and there's a decision on the connector question — the normalization and guard logic underneath are worth landing.
Cleaned up the working tree by removing all unrelated scratch files and mock snapshots. Replaced the static connector mock with a live integration to the SEC EDGAR API using a configurable CIK. Fixed normalization bug for parenthesized numbers, corrected config field naming conventions, and dropped empty boilerplate functions.
Author
|
Thanks @joshuadarron for another review. |
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.
Summary
authoritative_overlaynode to cross-check extracted financial answers against official regulator data.Type
feature
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1430
Summary by CodeRabbit