677 heartbeat reporting#678
Merged
Merged
Conversation
Closed
|
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:
📝 WalkthroughWalkthroughThe PR adds shared step-isolation contracts, collector error routing and telemetry framing, CLI degraded readiness and error persistence, BigQuery writer recovery, Express async response control, observe API schema updates, and secure ID/session length changes. ChangesStep isolation across shared runtime and platform code
Sequence Diagram(s)sequenceDiagram
participant runPipeline
participant registerProcessGuards
participant createOutOfBandErrorTracker
participant HealthServer
participant ErrorRing
runPipeline->>registerProcessGuards: register uncaughtException / unhandledRejection
registerProcessGuards-->>runPipeline: ProcessGuardHandle
runPipeline->>createOutOfBandErrorTracker: configure threshold/window
runPipeline->>registerProcessGuards: setOnOutOfBandError(record)
ErrorRing-->>runPipeline: listener(isNew=true)
runPipeline->>HealthServer: setDegraded(reason)
sequenceDiagram
participant StreamConnection
participant destinationBigQuery
participant settings
participant ensureWriter
participant push
StreamConnection-->>destinationBigQuery: onConnectionError(err)
destinationBigQuery->>settings: writerBroken=true, lastStreamError=err
push->>settings: check writerBroken
push->>ensureWriter: reopenWriter()
ensureWriter->>settings: swap handles, clear broken state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/collector/src/report-error.ts`:
- Around line 159-162: The logger event key uses camelCase format
('reportError') but coding guidelines require space-separated format ('entity
action'). Change the first argument in the logger.error call from 'reportError'
to 'report error' to maintain consistent event naming across the codebase.
In `@packages/server/sources/express/src/types.ts`:
- Around line 31-36: The comment block describing the Ack contract in the
types.ts file contains stale wording about GET tracking pixel behavior. Update
the comment to accurately reflect that GET requests also respect the
config.async flag and will wait for delivery to settle when config.async is set
to false, rather than stating that GET always responds first regardless of the
flag. This should align with the actual implementation behavior found in
index.ts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dd5279f-31cf-4c54-a47d-83a42fc7dc7e
📒 Files selected for processing (61)
.changeset/source-config-async.md.changeset/step-isolation-cli.md.changeset/step-isolation-collector.md.changeset/step-isolation-core.md.changeset/step-isolation-gcp.mdpackages/cli/src/__tests__/unit/commands/run-pipeline-frozen.test.tspackages/cli/src/__tests__/unit/commands/run-pipeline-telemetry.test.tspackages/cli/src/__tests__/unit/commands/run-pipeline.test.tspackages/cli/src/__tests__/unit/runtime/health-server.test.tspackages/cli/src/commands/run/__tests__/error-redaction.test.tspackages/cli/src/commands/run/__tests__/error-sink.test.tspackages/cli/src/commands/run/__tests__/pipeline.test.tspackages/cli/src/commands/run/error-sink.tspackages/cli/src/commands/run/index.tspackages/cli/src/commands/run/pipeline.tspackages/cli/src/runtime/__tests__/heartbeat.test.tspackages/cli/src/runtime/__tests__/log-ring.test.tspackages/cli/src/runtime/__tests__/runner.test.tspackages/cli/src/runtime/health-server.tspackages/cli/src/runtime/heartbeat.tspackages/cli/src/runtime/log-ring.tspackages/collector/src/__tests__/breaker.test.tspackages/collector/src/__tests__/observerEmit.test.tspackages/collector/src/__tests__/queue-bounds.test.tspackages/collector/src/__tests__/report-error.test.tspackages/collector/src/__tests__/store-cache-wrapper.observer.test.tspackages/collector/src/__tests__/store.test.tspackages/collector/src/__tests__/transformer-branch.test.tspackages/collector/src/breaker.tspackages/collector/src/collector.tspackages/collector/src/destination.tspackages/collector/src/index.tspackages/collector/src/on.tspackages/collector/src/report-error.tspackages/collector/src/source.tspackages/collector/src/store.tspackages/collector/src/transformer.tspackages/core/src/__tests__/emitStep.test.tspackages/core/src/schemas/destination.tspackages/core/src/schemas/source.tspackages/core/src/types/collector.tspackages/core/src/types/context.tspackages/core/src/types/destination.tspackages/core/src/types/index.tspackages/core/src/types/source.tspackages/server/destinations/gcp/src/bigquery/__mocks__/@google-cloud/bigquery-storage.tspackages/server/destinations/gcp/src/bigquery/__tests__/bigquery-storage-mock.d.tspackages/server/destinations/gcp/src/bigquery/__tests__/index.test.tspackages/server/destinations/gcp/src/bigquery/__tests__/stepExamples.test.tspackages/server/destinations/gcp/src/bigquery/__tests__/writer.test.tspackages/server/destinations/gcp/src/bigquery/index.tspackages/server/destinations/gcp/src/bigquery/push.tspackages/server/destinations/gcp/src/bigquery/pushBatch.tspackages/server/destinations/gcp/src/bigquery/types/index.tspackages/server/destinations/gcp/src/bigquery/writer.tspackages/server/sources/express/src/__tests__/cache-roundtrip.test.tspackages/server/sources/express/src/__tests__/index.test.tspackages/server/sources/express/src/index.tspackages/server/sources/express/src/schemas/settings.tspackages/server/sources/express/src/types.tsskills/walkeros-understanding-destinations/SKILL.md
💤 Files with no reviewable changes (1)
- packages/server/sources/express/src/schemas/settings.ts
alexanderkirtzel
had a problem deploying
to
npm-publish
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/collector/src/push.ts (1)
2-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent observer mutation from changing inbound collector.push payloads.
Line 238 stores the original inbound event by reference, and
innerPushruns right after. Observer mutation can change what gets processed.Suggested fix
import { + clone, createIngest, emitStep, FatalError, @@ - inState.inEvent = event; + inState.inEvent = clone(event);Also applies to: 238-239
🤖 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/collector/src/push.ts` around lines 2 - 10, At lines 238-239 in the push.ts file, the original inbound event is being stored by reference, which allows observer mutations to modify the stored value before innerPush runs. Create a deep copy of the inbound event when storing it (rather than storing by reference) to ensure that any mutations made by observers do not affect the value that gets processed by the subsequent innerPush call. This will preserve the original event state for proper processing.
🤖 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 `@packages/cli/src/commands/validate/__tests__/flow-routes.test.ts`:
- Around line 458-480: The positive-control test case for validateFlow is
incomplete and can pass even if unrelated validation errors are introduced. In
addition to the existing assertion that checks for platform-related errors using
result.errors.some(), add a new assertion to verify that result.valid is true.
This ensures the entire validation passes, not just that platform-specific
errors are absent, making the test more robust against future changes.
In `@packages/collector/src/destination.ts`:
- Around line 1454-1455: The code passes `processed.event` by reference directly
to `inState.inEvent` before calling `emitStep(collector, inState)`, allowing
observers to mutate the event object and affect downstream behavior. Create a
deep copy or clone of `processed.event` before assigning it to `inState.inEvent`
at line 1454, and apply the same fix to the similar assignments at lines
1485-1488 where event objects are assigned to state objects before being emitted
via `emitStep`.
---
Outside diff comments:
In `@packages/collector/src/push.ts`:
- Around line 2-10: At lines 238-239 in the push.ts file, the original inbound
event is being stored by reference, which allows observer mutations to modify
the stored value before innerPush runs. Create a deep copy of the inbound event
when storing it (rather than storing by reference) to ensure that any mutations
made by observers do not affect the value that gets processed by the subsequent
innerPush call. This will preserve the original event state for proper
processing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b37919-ca8e-4769-a708-697962843bc2
📒 Files selected for processing (10)
.changeset/collector-inevent-telemetry.mdpackages/cli/openapi/spec.jsonpackages/cli/src/commands/validate/__tests__/flow-routes.test.tspackages/cli/src/types/api.gen.d.tspackages/collector/src/__tests__/destination.observer.test.tspackages/collector/src/__tests__/push.observer.test.tspackages/collector/src/__tests__/transformer.observer.test.tspackages/collector/src/destination.tspackages/collector/src/push.tspackages/collector/src/transformer.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/collector-inevent-telemetry.md
- packages/cli/openapi/spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/collector/src/transformer.ts
Comment on lines
+458
to
+480
| it('accepts a flow whose config declares a valid platform', () => { | ||
| // Positive control: an otherwise identical flow with `config.platform` | ||
| // present produces no platform-related schema error. | ||
| const flow = { | ||
| version: 4, | ||
| flows: { | ||
| default: { | ||
| config: { platform: 'web' as const, url: 'https://example.com' }, | ||
| sources: { | ||
| browser: { | ||
| package: '@walkeros/web-source-browser', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const result = validateFlow(flow); | ||
|
|
||
| expect( | ||
| result.errors.some((e) => /platform/i.test(e.message + ' ' + e.path)), | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Strengthen the positive-control assertion in this test.
At Line 458, this case should also assert result.valid is true; otherwise it can still pass if unrelated validation errors are introduced later.
Suggested patch
const result = validateFlow(flow);
+ expect(result.valid).toBe(true);
expect(
result.errors.some((e) => /platform/i.test(e.message + ' ' + e.path)),
).toBe(false);📝 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
| it('accepts a flow whose config declares a valid platform', () => { | |
| // Positive control: an otherwise identical flow with `config.platform` | |
| // present produces no platform-related schema error. | |
| const flow = { | |
| version: 4, | |
| flows: { | |
| default: { | |
| config: { platform: 'web' as const, url: 'https://example.com' }, | |
| sources: { | |
| browser: { | |
| package: '@walkeros/web-source-browser', | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; | |
| const result = validateFlow(flow); | |
| expect( | |
| result.errors.some((e) => /platform/i.test(e.message + ' ' + e.path)), | |
| ).toBe(false); | |
| }); | |
| it('accepts a flow whose config declares a valid platform', () => { | |
| // Positive control: an otherwise identical flow with `config.platform` | |
| // present produces no platform-related schema error. | |
| const flow = { | |
| version: 4, | |
| flows: { | |
| default: { | |
| config: { platform: 'web' as const, url: 'https://example.com' }, | |
| sources: { | |
| browser: { | |
| package: '`@walkeros/web-source-browser`', | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; | |
| const result = validateFlow(flow); | |
| expect(result.valid).toBe(true); | |
| expect( | |
| result.errors.some((e) => /platform/i.test(e.message + ' ' + e.path)), | |
| ).toBe(false); | |
| }); |
🤖 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/cli/src/commands/validate/__tests__/flow-routes.test.ts` around
lines 458 - 480, The positive-control test case for validateFlow is incomplete
and can pass even if unrelated validation errors are introduced. In addition to
the existing assertion that checks for platform-related errors using
result.errors.some(), add a new assertion to verify that result.valid is true.
This ensures the entire validation passes, not just that platform-specific
errors are absent, making the test more robust against future changes.
Comment on lines
+1454
to
1455
| inState.inEvent = processed.event; | ||
| emitStep(collector, inState); |
There was a problem hiding this comment.
Avoid exposing mutable event references in emitted FlowState.
Line 1454 and Lines 1485-1488 pass processed.event by reference into observer frames. An observer can mutate that object and affect downstream behavior (especially before push on the in frame).
Suggested fix
- inState.inEvent = processed.event;
+ inState.inEvent = clone(processed.event);
@@
- outState.outEvent = processed.event;
+ outState.outEvent = clone(processed.event);Also applies to: 1485-1488
🤖 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/collector/src/destination.ts` around lines 1454 - 1455, The code
passes `processed.event` by reference directly to `inState.inEvent` before
calling `emitStep(collector, inState)`, allowing observers to mutate the event
object and affect downstream behavior. Create a deep copy or clone of
`processed.event` before assigning it to `inState.inEvent` at line 1454, and
apply the same fix to the similar assignments at lines 1485-1488 where event
objects are assigned to state objects before being emitted via `emitStep`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/getId.ts`:
- Around line 26-27: The buffer allocation in the getId function at the line
with `new Uint8Array(Math.ceil(need * 1.3) + 4)` can exceed the Web Crypto API's
per-call limit of 65,536 bytes, causing crypto.getRandomValues() to throw a
QuotaExceededError that is silently caught by secureBytes, resulting in a
fallback to Math.random. Cap the buffer size calculation using Math.min() to
ensure it never exceeds 65,536 bytes, preventing the silent RNG downgrade while
maintaining the current usage patterns.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71cf67dd-a164-47bd-a6fe-aa77dc3b808d
📒 Files selected for processing (7)
.changeset/getid-secure-random.mdpackages/core/src/__tests__/getId.test.tspackages/core/src/getId.tspackages/web/sources/session/src/__tests__/sessionStorage.test.tspackages/web/sources/session/src/__tests__/stepExamples.test.tspackages/web/sources/session/src/lib/sessionStorage.tspackages/web/sources/session/src/lib/sessionWindow.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/getid-secure-random.md
Comment on lines
+26
to
+27
| const buffer = new Uint8Array(Math.ceil(need * 1.3) + 4); | ||
| if (!secureBytes(buffer)) break; // No secure source, drop to fallback below |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum byte length allowed per call to Web Crypto crypto.getRandomValues, and what exception is thrown when that limit is exceeded?
💡 Result:
The maximum byte length allowed per call to the Web Crypto API's crypto.getRandomValues method is 65,536 bytes [1][2][3]. When a TypedArray with a byte length exceeding this limit is passed to the method, it throws a QuotaExceededError DOMException [1][4][3].
Citations:
- 1: https://www.w3.org/TR/webcrypto/
- 2: https://w3c.github.io/webcrypto/
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues
- 4: https://docs.w3cub.com/dom/crypto/getrandomvalues.html
🏁 Script executed:
#!/bin/bash
# Find obvious large constant callsites that could trigger oversized secure buffers.
rg -nP --type=ts --type=tsx '\bgetId\s*\(\s*([5-9]\d{4,}|\d{6,})\b'Repository: elbwalker/walkerOS
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Locate and read the getId.ts file
find . -type f -name "getId.ts" 2>/dev/nullRepository: elbwalker/walkerOS
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for the getId function and surrounding context in TypeScript files
rg -n "export function getId" --type ts -A 35Repository: elbwalker/walkerOS
Length of output: 2396
🏁 Script executed:
#!/bin/bash
# Find the secureBytes function to understand exception handling
rg -n "secureBytes" --type ts -B 3 -A 10Repository: elbwalker/walkerOS
Length of output: 1758
🏁 Script executed:
#!/bin/bash
# Search for calls to getId with potentially large arguments
rg -n "getId\s*\(" --type ts --type js -B 2 -A 2Repository: elbwalker/walkerOS
Length of output: 10443
Cap getRandomValues request size to prevent silent RNG downgrade.
At Line 26, the buffer allocation can exceed the Web Crypto API's per-call limit of 65,536 bytes. When that happens, crypto.getRandomValues() throws a QuotaExceededError, which is caught by secureBytes at Line 27. This silent exception handling causes a fallback to Math.random, degrading entropy despite CSPRNG availability.
While current usage in the codebase calls getId with small lengths (max 16 bytes), this is a latent vulnerability that should be fixed proactively to prevent future issues.
Suggested fix
const defaultCharset = '0123456789abcdefghijklmnopqrstuvwxyz';
+const MAX_RANDOM_VALUES_BYTES = 65536;
export function getId(length = 6, charset: string = defaultCharset): string {
@@
while (str.length < length) {
const need = length - str.length;
- const buffer = new Uint8Array(Math.ceil(need * 1.3) + 4);
+ const bufferSize = Math.min(
+ MAX_RANDOM_VALUES_BYTES,
+ Math.ceil(need * 1.3) + 4,
+ );
+ const buffer = new Uint8Array(bufferSize);
if (!secureBytes(buffer)) break; // No secure source, drop to fallback below
for (let i = 0; i < buffer.length && str.length < length; i++) {
if (buffer[i] < limit) str += charset[buffer[i] % n];
}
}📝 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
| const buffer = new Uint8Array(Math.ceil(need * 1.3) + 4); | |
| if (!secureBytes(buffer)) break; // No secure source, drop to fallback below | |
| const defaultCharset = '0123456789abcdefghijklmnopqrstuvwxyz'; | |
| const MAX_RANDOM_VALUES_BYTES = 65536; | |
| export function getId(length = 6, charset: string = defaultCharset): string { | |
| let str = ''; | |
| while (str.length < length) { | |
| const need = length - str.length; | |
| const bufferSize = Math.min( | |
| MAX_RANDOM_VALUES_BYTES, | |
| Math.ceil(need * 1.3) + 4, | |
| ); | |
| const buffer = new Uint8Array(bufferSize); | |
| if (!secureBytes(buffer)) break; // No secure source, drop to fallback below | |
| for (let i = 0; i < buffer.length && str.length < length; i++) { | |
| if (buffer[i] < limit) str += charset[buffer[i] % n]; | |
| } | |
| } | |
| // ... rest of 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 `@packages/core/src/getId.ts` around lines 26 - 27, The buffer allocation in
the getId function at the line with `new Uint8Array(Math.ceil(need * 1.3) + 4)`
can exceed the Web Crypto API's per-call limit of 65,536 bytes, causing
crypto.getRandomValues() to throw a QuotaExceededError that is silently caught
by secureBytes, resulting in a fallback to Math.random. Cap the buffer size
calculation using Math.min() to ensure it never exceeds 65,536 bytes, preventing
the silent RNG downgrade while maintaining the current usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/collector/src/__tests__/handle.test.ts`:
- Around line 370-382: The deep equality assertion comparing viaEnrich and
viaCompose is flaky because both objects independently read the current time,
causing timestamp and timing fields to drift between them. In addition to
normalizing source.count to 0 (which is already done), you must also normalize
the time-derived fields (timestamp and timing) in both viaEnrich and viaCompose
to fixed values before performing the deep equality comparison to ensure
deterministic test behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d701361c-bbca-4777-8dea-28d69b473801
📒 Files selected for processing (30)
.changeset/run-scoped-trace.mdpackages/cli/openapi/spec.jsonpackages/cli/src/types/api.gen.d.tspackages/collector/src/__tests__/boundary-error.test.tspackages/collector/src/__tests__/handle-createEvent.test.tspackages/collector/src/__tests__/handle.test.tspackages/collector/src/__tests__/inline-code.test.tspackages/collector/src/__tests__/observerEmit.test.tspackages/collector/src/__tests__/queue-bounds.test.tspackages/collector/src/__tests__/run-trace.test.tspackages/collector/src/__tests__/store-cache-wrapper.observer.test.tspackages/collector/src/__tests__/store.test.tspackages/collector/src/__tests__/transformer-branch.test.tspackages/collector/src/__tests__/transformer-init-error.test.tspackages/collector/src/__tests__/transformer.test.tspackages/collector/src/collector.tspackages/collector/src/handle.tspackages/core/src/__tests__/emitStep.test.tspackages/core/src/__tests__/getSpanId.test.tspackages/core/src/__tests__/getTraceId.test.tspackages/core/src/getSpanId.tspackages/core/src/getTraceId.tspackages/core/src/hexId.tspackages/core/src/index.tspackages/core/src/schemas/walkeros.tspackages/core/src/types/collector.tspackages/core/src/types/walkeros.tspackages/server/destinations/gcp/src/pubsub/examples/step.tsskills/walkeros-understanding-events/SKILL.mdwebsite/docs/getting-started/event-model.mdx
✅ Files skipped from review due to trivial changes (11)
- packages/core/src/hexId.ts
- packages/core/src/getSpanId.ts
- packages/collector/src/tests/boundary-error.test.ts
- packages/collector/src/tests/transformer-init-error.test.ts
- packages/collector/src/tests/transformer.test.ts
- packages/core/src/tests/emitStep.test.ts
- .changeset/run-scoped-trace.md
- packages/core/src/schemas/walkeros.ts
- website/docs/getting-started/event-model.mdx
- packages/core/src/types/walkeros.ts
- packages/cli/src/types/api.gen.d.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/collector/src/collector.ts
- packages/collector/src/tests/store.test.ts
- packages/collector/src/tests/observerEmit.test.ts
- packages/collector/src/tests/store-cache-wrapper.observer.test.ts
- packages/collector/src/tests/queue-bounds.test.ts
- packages/core/src/types/collector.ts
Comment on lines
+370
to
382
| const viaEnrich = enrichEvent(c, partial); | ||
| const viaCompose = createEvent(c, prepareEvent(c, partial)); | ||
| // Both paths produce the same event. `source.count` is a per-run sequence | ||
| // that legitimately advances on each createEvent call, so compare it | ||
| // independently and assert deep equality on everything else. | ||
| expect(viaEnrich.source.count).toBe(1); | ||
| expect(viaCompose.source.count).toBe(2); | ||
| expect({ ...viaEnrich, source: { ...viaEnrich.source, count: 0 } }).toEqual( | ||
| { | ||
| ...viaCompose, | ||
| source: { ...viaCompose.source, count: 0 }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Stabilize this equality test by fixing time-derived fields.
viaEnrich and viaCompose each read current time independently, so timestamp/timing can drift and make this deep-equality assertion flaky even after normalizing source.count.
Proposed fix
const partial: WalkerOS.DeepPartialEvent = {
name: 'page view',
id: 'fixed',
+ timestamp: 1_700_000_000_000,
+ timing: 12.34,
};🤖 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/collector/src/__tests__/handle.test.ts` around lines 370 - 382, The
deep equality assertion comparing viaEnrich and viaCompose is flaky because both
objects independently read the current time, causing timestamp and timing fields
to drift between them. In addition to normalizing source.count to 0 (which is
already done), you must also normalize the time-derived fields (timestamp and
timing) in both viaEnrich and viaCompose to fixed values before performing the
deep equality comparison to ensure deterministic test behavior.
Contributor
|
📦 Pre-release published (next) Packages
Install: 🐳 Docker images published
Docker: |
Contributor
|
🚀 Stable release published Packages
Install: 🐳 Docker images published
Docker: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
async/respond-first configuration for response-producing server sources (2xx now reflects acceptance when enabled).reportErrorhandling with/readydegrading to 503 when error thresholds are exceeded.source.trace/source.count.