feat(nodes): add anomaly detection node for pipeline output monitoring#569
feat(nodes): add anomaly detection node for pipeline output monitoring#569charliegillet wants to merge 7 commits intorocketride-org:developfrom
Conversation
…screen Handle TASK_STATE.STOPPING in the control button to show "Stopping..." with a disabled state and distinct orange styling, preventing duplicate clicks and giving immediate visual feedback during pipeline shutdown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charliegillet
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
github-actions
bot
added
module:nodes
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR introduces a new Anomaly Detector pipeline node for statistical anomaly detection (Z-Score, IQR, rolling average methods), updates the VSCode UI to display "Stopping" feedback, and enhances the TypeScript client library with improved error handling via ConnectionException and a new PROTOCOL_CAPS capability enum. Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant IGlobal
participant IInstance
participant AnomalyDetector
Pipeline->>IGlobal: beginGlobal()
activate IGlobal
IGlobal->>IGlobal: Load configuration
IGlobal->>AnomalyDetector: instantiate(config)
deactivate IGlobal
Pipeline->>IInstance: writeText(text)
activate IInstance
IInstance->>IInstance: Check detector available
IInstance->>AnomalyDetector: evaluate_text(text)
AnomalyDetector->>AnomalyDetector: detect(value)
AnomalyDetector-->>IInstance: annotated text
IInstance-->>Pipeline: enriched output
deactivate IInstance
Pipeline->>IInstance: writeDocuments(documents)
activate IInstance
IInstance->>IInstance: Clone document
IInstance->>AnomalyDetector: evaluate_document(metadata)
AnomalyDetector-->>IInstance: anomaly result
IInstance->>IInstance: Add anomaly fields to metadata
IInstance-->>Pipeline: enriched documents
deactivate IInstance
Pipeline->>IGlobal: endGlobal()
activate IGlobal
IGlobal->>IGlobal: Reset detector to None
deactivate IGlobal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thank you for this contribution — the node structure is clean, the services.json follows the required format, and using only stdlib is a nice touch. A few things to address before we merge:
Unrelated VSCode changes
PageStatus.tsx and styles.css are not related to this node. Please remove them from this PR.
Z-Score and Rolling Average are identical
_detect_z_score and _detect_rolling_avg compute exactly the same value — abs(value - mean) / std_dev — and differ only in the details label. This is misleading: a user choosing "Rolling Average" would expect behaviour distinct from Z-Score (e.g., deviation from a moving average rather than from the global window mean). Please either implement genuinely different logic for rolling average, or remove it as a separate option and document that the two are equivalent.
evaluate_text silently skips multi-value text
float(text.strip()) requires the entire string to be a single bare number. For any text that is not — including common formats like "1 1 2 3 5 8", "score: 0.95", or "3.14 2.71" — float() raises ValueError, the except branch returns the text unchanged, and no detection happens at all. This is likely not the intended behaviour. Please clarify what input format this method is designed for and either document the constraint clearly or implement extraction of numeric values from the text.
Crash in CONFIG mode
beginGlobal exits early with pass in CONFIG mode, leaving self.detector = None. If the engine calls writeText or writeDocuments during configuration, both methods will raise AttributeError on self.IGlobal.detector. Please add a guard:
if self.IGlobal.detector is None:
self.instance.writeText(text)
return- Remove unrelated PageStatus.tsx and styles.css changes - Fix rolling average to use a sliding sub-window for local mean instead of duplicating the z-score full-window approach - Fix evaluate_text to extract numeric values via regex when full text parse fails, and log a debug message when no number is found - Add detector None guards in writeText/writeDocuments to prevent crash in CONFIG mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
module:client-typescript
and removed
module:vscode
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/anomaly_detector/IGlobal.py`:
- Around line 47-49: Change all regular string literals to use single quotes in
this block: update the f-string argument to debug(f' Loading anomaly
detector: method={config.get(\'method\', \'z_score\')}') so the inner config.get
uses single quotes instead of double quotes, and ensure any other literal
strings passed to debug or used when instantiating AnomalyDetector (e.g., the
debug call ' Anomaly detector initialized' and the AnomalyDetector(config)
instantiation) follow the single-quote style consistently.
In `@nodes/src/nodes/anomaly_detector/IInstance.py`:
- Around line 70-83: The code is mutating a Pydantic DocMetadata via dict-style
assignment and using a shallow copy; change the copy and assignment to use
Pydantic-friendly patterns: call doc.model_copy(deep=True) when creating
enriched_doc to avoid mutating the original, ensure enriched_doc.metadata is a
DocMetadata instance (not a raw dict) before setting fields, and replace
bracket-style assignments (enriched_doc.metadata['anomaly_score'] etc.) with
attribute assignments (enriched_doc.metadata.anomaly_score = result['score'],
enriched_doc.metadata.anomaly_severity = result['severity'],
enriched_doc.metadata.anomaly_is_anomalous = result['is_anomalous'], and set
enriched_doc.metadata.anomaly_details if result.get('details')). Use the same
symbols shown (documents, enriched_doc, doc.model_copy,
self.IGlobal.detector.evaluate_document) to locate and update the code.
🪄 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: 7aa040a5-2db1-419e-9a56-fc237019ac67
📒 Files selected for processing (7)
nodes/src/nodes/anomaly_detector/IGlobal.pynodes/src/nodes/anomaly_detector/IInstance.pynodes/src/nodes/anomaly_detector/__init__.pynodes/src/nodes/anomaly_detector/detector.pynodes/src/nodes/anomaly_detector/requirements.txtnodes/src/nodes/anomaly_detector/services.jsonpackages/client-typescript/src/cli/rocketride.ts
| for doc in documents: | ||
| enriched_doc = doc.model_copy() | ||
| if enriched_doc.metadata is None: | ||
| enriched_doc.metadata = {} | ||
|
|
||
| result = self.IGlobal.detector.evaluate_document(enriched_doc.metadata) | ||
|
|
||
| enriched_doc.metadata['anomaly_score'] = result['score'] | ||
| enriched_doc.metadata['anomaly_severity'] = result['severity'] | ||
| enriched_doc.metadata['anomaly_is_anomalous'] = result['is_anomalous'] | ||
| if result.get('details'): | ||
| enriched_doc.metadata['anomaly_details'] = result['details'] | ||
|
|
||
| enriched_docs.append(enriched_doc) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Pydantic v2 BaseModel with extra='allow' support dict-style item assignment like model['key'] = value?
💡 Result:
No, Pydantic v2 BaseModel with extra='allow' does not support dict-style item assignment like model['key'] = value. BaseModel instances are not subscriptable by default; they do not implement setitem or getitem. They support attribute access via dot notation (model.key) for defined fields and extra fields (stored in pydantic_extra and accessible via model_extra property or getattr), but not dictionary-style bracket notation model['key']. Users must subclass and implement getitem and setitem manually for dict-like access, as shown in discussions where this is not native behavior. Official docs confirm access via attributes or model_extra dict, not direct subscripting on the model instance.
Citations:
- 1: https://docs.pydantic.dev/latest/api/base_model/
- 2: https://docs.pydantic.dev/latest/api/config/
- 3: Unexpected change in the behavior of BaseModel.__dict__? pydantic/pydantic#6914
- 4: https://stackoverflow.com/questions/69617489/can-i-get-incoming-extra-fields-from-pydantic
- 5: https://docs.pydantic.dev/2.0/usage/model_config/
- 6: https://docs.pydantic.dev/2.12/api/base_model/
- 7: How to index a pydantic model using a string object, returning a pydantic model? pydantic/pydantic#3463
- 8: Cannot override __items__ with RootModel without typechecker error. pydantic/pydantic#12887
Dict-style assignment on Pydantic DocMetadata will fail at runtime.
Lines 77-81 use bracket notation (metadata['anomaly_score'] = value) on a DocMetadata Pydantic model, but Pydantic v2 BaseModel does not support __setitem__. This will raise TypeError.
Use attribute access instead: enriched_doc.metadata.anomaly_score = result['score'].
Additionally, model_copy() without deep=True performs a shallow copy, so modifications to enriched_doc.metadata will also mutate the original doc.metadata.
Proposed fix
for doc in documents:
- enriched_doc = doc.model_copy()
+ enriched_doc = doc.model_copy(deep=True)
if enriched_doc.metadata is None:
enriched_doc.metadata = {}
result = self.IGlobal.detector.evaluate_document(enriched_doc.metadata)
- enriched_doc.metadata['anomaly_score'] = result['score']
- enriched_doc.metadata['anomaly_severity'] = result['severity']
- enriched_doc.metadata['anomaly_is_anomalous'] = result['is_anomalous']
+ enriched_doc.metadata.anomaly_score = result['score']
+ enriched_doc.metadata.anomaly_severity = result['severity']
+ enriched_doc.metadata.anomaly_is_anomalous = result['is_anomalous']
if result.get('details'):
- enriched_doc.metadata['anomaly_details'] = result['details']
+ enriched_doc.metadata.anomaly_details = result['details']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/src/nodes/anomaly_detector/IInstance.py` around lines 70 - 83, The code
is mutating a Pydantic DocMetadata via dict-style assignment and using a shallow
copy; change the copy and assignment to use Pydantic-friendly patterns: call
doc.model_copy(deep=True) when creating enriched_doc to avoid mutating the
original, ensure enriched_doc.metadata is a DocMetadata instance (not a raw
dict) before setting fields, and replace bracket-style assignments
(enriched_doc.metadata['anomaly_score'] etc.) with attribute assignments
(enriched_doc.metadata.anomaly_score = result['score'],
enriched_doc.metadata.anomaly_severity = result['severity'],
enriched_doc.metadata.anomaly_is_anomalous = result['is_anomalous'], and set
enriched_doc.metadata.anomaly_details if result.get('details')). Use the same
symbols shown (documents, enriched_doc, doc.model_copy,
self.IGlobal.detector.evaluate_document) to locate and update the code.
There was a problem hiding this comment.
Fixed: replaced all dict-style item assignment (model["key"] = value) on DocMetadata with attribute access (model.key = value). Since DocMetadata uses extra="allow", Pydantic v2 allows setting arbitrary attributes. Also removed the metadata = {} fallback — if metadata is None, we now skip the document and pass it through unchanged, since creating a bare DocMetadata would fail validation (required fields objectId, chunkId). The evaluate_document call now receives metadata.model_dump() to ensure we pass a plain dict to the detector.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 36 seconds before sending another message.
The previous removal commit left these files behind develop, which would revert the stopping-button feature on merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unrelated formatting and refactoring changes were accidentally included in the anomaly detector feedback commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all review feedback:
Thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-typescript/jest.config.js`:
- Around line 53-56: The Jest config currently sets forceExit: true which masks
cleanup failures; remove or set forceExit to false in jest.config.js and rely on
the existing disconnect() cleanup plus its Promise.race() timeout (lines
referencing disconnect() and Promise.race()) so tests will surface lingering
WebSocket handles/timers instead of silently exiting. Ensure CI/local runs still
complete by keeping or adjusting the disconnect() timeout as needed, but do not
re-enable forceExit.
In `@packages/client-typescript/src/client/client.ts`:
- Around line 398-409: The host-check currently uses
url.hostname.includes('rocketride.ai') which false-positively matches things
like staging-rocketride.ai.internal; change it to test for exact domain or its
subdomains (e.g. url.hostname === 'rocketride.ai' ||
url.hostname.endsWith('.rocketride.ai')) so the block that sets url.port (using
CONST_DEFAULT_WEB_PORT) only skips when the hostname is the exact cloud domain
or a direct subdomain; apply this replacement where hasExplicitPort and url.port
are evaluated.
In `@packages/client-typescript/src/client/types/client.ts`:
- Line 32: Restore the public ConnectErrorCallback type to accept the wider
union (error: Error | string) => void | Promise<void> instead of narrowing it to
(error: ConnectionException) => void; keep ConnectionException only for internal
runtime values but export the callback type as the broader union so existing
consumers (e.g., ConnectErrorCallback usages in apps/vscode, apps/chat-ui,
apps/dropper-ui) remain compatible and typings compile against the previous
contract.
In `@packages/client-typescript/src/client/types/service.ts`:
- Around line 90-94: The client-side PROTOCOL_CAPS enum includes DEPRECATED and
EXPERIMENTAL flags that are not present in the engine's authoritative enum
(stops at FOCUS = BIT(14)); remove the DEPRECATED and EXPERIMENTAL entries from
the PROTOCOL_CAPS enum (i.e., delete the DEPRECATED and EXPERIMENTAL symbols) so
the client only exposes flags that the server actually emits, or alternatively
update the enum to exactly match the engine's bit assignments up through FOCUS
(BIT(14)) to keep the client aligned with the engine source of truth.
In `@packages/client-typescript/tests/RocketRideClient.test.ts`:
- Around line 1825-1840: The current retry inside runPipeline around client.send
can cause duplicate writes because send() is not idempotent; remove the blind
retry and either (a) let the send failure surface so the test fails, or (b)
implement a safe retry by recreating the pipeline/connection or supplying an
idempotency key before retrying. Locate the retry block in runPipeline (the
try/catch around client.send called with token and text) and replace the catch
behavior: do not call client.send(token, ...) a second time without a new
idempotency boundary—either re-establish the pipeline/connection (get a fresh
token) before retrying or simply rethrow the error so the test records the
failure.
- Around line 79-83: The Promise.race teardown uses an un-cleared setTimeout so
the timer handle stays alive and teardown can silently pass; change the pattern
around client.disconnect() (and the pipeline-termination race) to create the
timeout with a handle, race it against the real teardown promise, and on
whichever promise resolves first clear the timeout handle (clearTimeout) and if
the timeout resolved, cause the test to fail (reject or throw) so teardown
cannot silently pass while resources remain; apply this same fix to the pipeline
termination race to clear its timeout handle and fail when the timeout wins.
🪄 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: 905c927f-0893-4bf8-97f3-0de5d50c333d
📒 Files selected for processing (8)
apps/vscode/src/providers/views/PageStatus/PageStatus.tsxapps/vscode/src/providers/views/PageStatus/styles.csspackages/client-typescript/jest.config.jspackages/client-typescript/src/client/client.tspackages/client-typescript/src/client/types/client.tspackages/client-typescript/src/client/types/index.tspackages/client-typescript/src/client/types/service.tspackages/client-typescript/tests/RocketRideClient.test.ts
| // The URL API silently strips ports that are default-for-scheme | ||
| // (e.g. :443 on https, :80 on http), so url.port alone cannot | ||
| // distinguish "no port given" from "scheme-default port given". | ||
| // Check the raw input for an explicit `:digits` after the scheme. | ||
| const withoutScheme = normalized.replace(/^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//, ''); | ||
| const authority = withoutScheme.split(/[/?#]/, 1)[0] ?? ''; | ||
| const hasExplicitPort = authority.startsWith('[') | ||
| ? /\]:\d+$/.test(authority) // IPv6 literal with explicit port | ||
| : /:\d+$/.test(authority); // hostname/IPv4 with explicit port | ||
|
|
||
| if (!url.port && !hasExplicitPort && !url.hostname.includes('rocketride.ai')) { | ||
| url.port = CONST_DEFAULT_WEB_PORT; |
There was a problem hiding this comment.
Use exact cloud-host matching here.
The explicit-port detection looks good, but url.hostname.includes('rocketride.ai') will also match unrelated hosts like staging-rocketride.ai.internal, so those URIs will skip the default :5565 port and normalize incorrectly. Compare against the exact domain or its subdomains instead.
🛠️ Suggested fix
- if (!url.port && !hasExplicitPort && !url.hostname.includes('rocketride.ai')) {
+ const isRocketRideCloudHost =
+ url.hostname === 'rocketride.ai' || url.hostname.endsWith('.rocketride.ai');
+ if (!url.port && !hasExplicitPort && !isRocketRideCloudHost) {
url.port = CONST_DEFAULT_WEB_PORT;
}📝 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.
| // The URL API silently strips ports that are default-for-scheme | |
| // (e.g. :443 on https, :80 on http), so url.port alone cannot | |
| // distinguish "no port given" from "scheme-default port given". | |
| // Check the raw input for an explicit `:digits` after the scheme. | |
| const withoutScheme = normalized.replace(/^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//, ''); | |
| const authority = withoutScheme.split(/[/?#]/, 1)[0] ?? ''; | |
| const hasExplicitPort = authority.startsWith('[') | |
| ? /\]:\d+$/.test(authority) // IPv6 literal with explicit port | |
| : /:\d+$/.test(authority); // hostname/IPv4 with explicit port | |
| if (!url.port && !hasExplicitPort && !url.hostname.includes('rocketride.ai')) { | |
| url.port = CONST_DEFAULT_WEB_PORT; | |
| // The URL API silently strips ports that are default-for-scheme | |
| // (e.g. :443 on https, :80 on http), so url.port alone cannot | |
| // distinguish "no port given" from "scheme-default port given". | |
| // Check the raw input for an explicit `:digits` after the scheme. | |
| const withoutScheme = normalized.replace(/^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//, ''); | |
| const authority = withoutScheme.split(/[/?#]/, 1)[0] ?? ''; | |
| const hasExplicitPort = authority.startsWith('[') | |
| ? /\]:\d+$/.test(authority) // IPv6 literal with explicit port | |
| : /:\d+$/.test(authority); // hostname/IPv4 with explicit port | |
| const isRocketRideCloudHost = | |
| url.hostname === 'rocketride.ai' || url.hostname.endsWith('.rocketride.ai'); | |
| if (!url.port && !hasExplicitPort && !isRocketRideCloudHost) { | |
| url.port = CONST_DEFAULT_WEB_PORT; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-typescript/src/client/client.ts` around lines 398 - 409, The
host-check currently uses url.hostname.includes('rocketride.ai') which
false-positively matches things like staging-rocketride.ai.internal; change it
to test for exact domain or its subdomains (e.g. url.hostname ===
'rocketride.ai' || url.hostname.endsWith('.rocketride.ai')) so the block that
sets url.port (using CONST_DEFAULT_WEB_PORT) only skips when the hostname is the
exact cloud domain or a direct subdomain; apply this replacement where
hasExplicitPort and url.port are evaluated.
There was a problem hiding this comment.
Reverted: all packages/client-typescript/ changes have been restored to match develop. These were unrelated to the anomaly detector node.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 31 seconds before sending another message.
| * including DAP messages, callbacks, configuration options, and transport interfaces. | ||
| */ | ||
|
|
||
| import { ConnectionException } from '../exceptions/index.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/client-typescript/src/client/types/client.ts | sed -n '30,45p'Repository: rocketride-org/rocketride-server
Length of output: 485
🏁 Script executed:
cat -n packages/client-typescript/src/client/types/client.ts | sed -n '135,145p'Repository: rocketride-org/rocketride-server
Length of output: 614
🏁 Script executed:
rg "ConnectErrorCallback|connectErrorCallback" --type ts --type tsx -A 3 -B 1Repository: rocketride-org/rocketride-server
Length of output: 103
🏁 Script executed:
rg "connectErrorCallback\s*=" --type ts --type tsx -A 5Repository: rocketride-org/rocketride-server
Length of output: 103
🏁 Script executed:
rg "ConnectErrorCallback|connectErrorCallback" -A 3 -B 1Repository: rocketride-org/rocketride-server
Length of output: 6927
🏁 Script executed:
rg "onConnectError\s*=" -A 5Repository: rocketride-org/rocketride-server
Length of output: 58
🏁 Script executed:
rg "_callerOnConnectError" -A 2Repository: rocketride-org/rocketride-server
Length of output: 1204
🏁 Script executed:
rg "this\._callerOnConnectError\(" -B 2 -A 2Repository: rocketride-org/rocketride-server
Length of output: 593
🏁 Script executed:
rg "onConnectError|connectErrorCallback" apps/ -A 3Repository: rocketride-org/rocketride-server
Length of output: 2203
Keep ConnectErrorCallback on the wider public contract.
This narrows a public callback from the previously widened (error: Error | string) => void | Promise<void> shape down to (error: ConnectionException) => void, which is a breaking change for existing consumers. Downstream handlers in apps/vscode/src/debugger/session.ts, apps/chat-ui/src/hooks/clientSingleton.ts, and apps/dropper-ui/src/hooks/clientSingleton.ts are all typed to accept string parameters, not ConnectionException objects. The runtime can pass a ConnectionException instance without requiring that specific type in the exported API.
♻️ Suggested fix
-import { ConnectionException } from '../exceptions/index.js';
-
...
-export type ConnectErrorCallback = (error: ConnectionException) => void | Promise<void>;
+export type ConnectErrorCallback = (error: Error | string) => void | Promise<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-typescript/src/client/types/client.ts` at line 32, Restore
the public ConnectErrorCallback type to accept the wider union (error: Error |
string) => void | Promise<void> instead of narrowing it to (error:
ConnectionException) => void; keep ConnectionException only for internal runtime
values but export the callback type as the broader union so existing consumers
(e.g., ConnectErrorCallback usages in apps/vscode, apps/chat-ui,
apps/dropper-ui) remain compatible and typings compile against the previous
contract.
There was a problem hiding this comment.
Reverted: packages/client-typescript/ files restored to develop. Unrelated changes removed from this PR.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 30 seconds before sending another message.
| /** Driver is deprecated */ | ||
| DEPRECATED = 1 << 15, | ||
|
|
||
| /** Driver is experimental */ | ||
| EXPERIMENTAL = 1 << 16, |
There was a problem hiding this comment.
Keep PROTOCOL_CAPS aligned with the engine source of truth.
The authoritative enum in packages/server/engine-core/apLib/url/Url.hpp:41-67 stops at FOCUS = BIT(14). Adding DEPRECATED and EXPERIMENTAL here invents client-visible flags the server does not emit today and risks colliding with future engine assignments.
🧭 Suggested fix
- /** Driver is deprecated */
- DEPRECATED = 1 << 15,
-
- /** Driver is experimental */
- EXPERIMENTAL = 1 << 16,📝 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.
| /** Driver is deprecated */ | |
| DEPRECATED = 1 << 15, | |
| /** Driver is experimental */ | |
| EXPERIMENTAL = 1 << 16, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-typescript/src/client/types/service.ts` around lines 90 - 94,
The client-side PROTOCOL_CAPS enum includes DEPRECATED and EXPERIMENTAL flags
that are not present in the engine's authoritative enum (stops at FOCUS =
BIT(14)); remove the DEPRECATED and EXPERIMENTAL entries from the PROTOCOL_CAPS
enum (i.e., delete the DEPRECATED and EXPERIMENTAL symbols) so the client only
exposes flags that the server actually emits, or alternatively update the enum
to exactly match the engine's bit assignments up through FOCUS (BIT(14)) to keep
the client aligned with the engine source of truth.
There was a problem hiding this comment.
Reverted: packages/client-typescript/ files restored to develop. Unrelated changes removed from this PR.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 29 seconds before sending another message.
| // Use a bounded timeout so teardown never hangs the suite | ||
| await Promise.race([ | ||
| client.disconnect(), | ||
| new Promise<void>(resolve => setTimeout(resolve, 10000)), | ||
| ]); |
There was a problem hiding this comment.
Clear these timeout handles and fail if teardown stalls.
Both Promise.race blocks leave their setTimeout handle alive when cleanup finishes first, so even successful teardowns keep an open timer around for 10-15 seconds. If the timeout wins, teardown also passes silently and the next test can start with live clients/pipelines.
🧹 Suggested pattern
- await Promise.race([
- client.disconnect(),
- new Promise<void>(resolve => setTimeout(resolve, 10000)),
- ]);
+ let timeoutId: ReturnType<typeof setTimeout> | undefined;
+ try {
+ await Promise.race([
+ client.disconnect(),
+ new Promise<never>((_, reject) => {
+ timeoutId = setTimeout(
+ () => reject(new Error('client.disconnect() timed out during test teardown')),
+ 10000
+ );
+ }),
+ ]);
+ } finally {
+ if (timeoutId !== undefined) clearTimeout(timeoutId);
+ }Apply the same helper/pattern to the pipeline-termination race below.
Also applies to: 1569-1581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-typescript/tests/RocketRideClient.test.ts` around lines 79 -
83, The Promise.race teardown uses an un-cleared setTimeout so the timer handle
stays alive and teardown can silently pass; change the pattern around
client.disconnect() (and the pipeline-termination race) to create the timeout
with a handle, race it against the real teardown promise, and on whichever
promise resolves first clear the timeout handle (clearTimeout) and if the
timeout resolved, cause the test to fail (reject or throw) so teardown cannot
silently pass while resources remain; apply this same fix to the pipeline
termination race to clear its timeout handle and fail when the timeout wins.
There was a problem hiding this comment.
Reverted: packages/client-typescript/ files restored to develop. Unrelated changes removed from this PR.
There was a problem hiding this comment.
Rate Limit Exceeded
@charliegillet have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 26 seconds before sending another message.
These changes were accidentally included in the anomaly detector PR and are not related to the feature. Restoring them to match develop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
removed
module:vscode
|
@asclearuc Addressed all review feedback: Removed unrelated files:
Previously addressed (in earlier commits):
The PR now only contains the anomaly detector node files. |
Senior Review — PR #569: feat(nodes): add anomaly detection node@charliegillet, the Z-Score and IQR implementations are well done — clean structure and good test coverage for those methods. However, there is a blocking issue that needs to be resolved before this can move forward: Rolling Average computes the same values as Z-ScoreThe Rolling Average method currently computes anomalies using the global window mean and standard deviation, which produces identical results to the Z-Score method. This defeats the purpose of having it as a separate detection strategy. Suggestion: Implement genuinely different logic for Rolling Average — for example, flag anomalies based on deviation from an exponential moving average (EMA) or a sliding-window mean with a different sensitivity characteristic. The key is that a user choosing "Rolling Average" should get meaningfully different detection behavior than "Z-Score." Additional items
The foundation here is strong. Fixing the Rolling Average logic is the main thing standing between this and approval. |
🚀 Merge RequestSolid Z-Score and IQR implementation. Before merge:
The algorithmic correctness issue is the main blocker. |
- Revert unrelated PageStatus and client-typescript changes to match develop - Use single quotes per coding guidelines in IGlobal.py debug logging - Fix Pydantic v2 dict-style assignment on DocMetadata BaseModel by using attribute access (model.key = value) instead of model['key'] = value - Differentiate rolling average from z-score: use percentage deviation from rolling mean instead of std_dev-normalized deviation - Skip documents with no metadata instead of assigning empty dict Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the review @asclearuc! All feedback has been addressed in commit c426edb: Unrelated changes reverted:
Anomaly detector fixes:
All files pass |
|
No description provided. |
Latest fixes
|
3 participants
Summary
anomaly_detectorpipeline node that monitors numeric output values for statistical anomaliesDetection Methods
Node Structure
Configuration
method: Detection algorithm (z_score,iqr,rolling_avg)sensitivity: Detection sensitivity thresholdwindowSize: Sliding window size for statistical calculationsmetric: Metadata field name containing the numeric value to monitorwarningThreshold/criticalThreshold: Severity classification boundariesType
Feature — new pipeline node
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests