675 simulation data#676
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:
📝 WalkthroughWalkthroughThis PR adds bounded retry and telemetry controls to the CLI runtime, extends simulation with injected data and mapping-key reporting, migrates Usercentrics and session flows to official lifecycle events, and adds collector, BigQuery, and Express delivery timeout and acknowledgement changes. ChangesCLI runtime, logging, and simulation
Usercentrics and session lifecycle
Collector, BigQuery, and Express delivery behavior
Sequence Diagram(s)sequenceDiagram
participant runPipeline
participant fetchWithRetry
participant RemoteAPI
runPipeline->>fetchWithRetry: fetch bundle/config/secrets
loop attempts within budget
fetchWithRetry->>RemoteAPI: request
alt transient failure
RemoteAPI-->>fetchWithRetry: timeout / 5xx / network error
fetchWithRetry->>fetchWithRetry: backoff and retry
else success
RemoteAPI-->>fetchWithRetry: response
end
end
fetchWithRetry-->>runPipeline: response or exhaustion error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/push/index.ts (1)
1376-1421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
mappingKeyfor destination error resultsLine 1386 captures
mappingKeyfromFlowState, but whencollector.push(...)throws, Line 1415 returns through the error callback withoutmappingKey. That drops the matched rule key for exactly the failing destination path.Suggested fix
export async function simulateDestination( configOrPath: string | Flow.Json, event: WalkerOS.DeepPartialEvent, options: SimulateDestinationOptions, ): Promise<Simulation.Result> { const startTime = Date.now(); + let mappingKey: string | undefined; @@ - let mappingKey: string | undefined; const targetStepId = stepId('destination', options.destinationId); @@ - if (collector.observers instanceof Set) { - collector.observers.add(captureMappingKey); - } + const hasObservers = collector.observers instanceof Set; + if (hasObservers) collector.observers.add(captureMappingKey); // Full pipeline: consent, mapping, enrichment, before chains // include filter ensures only the target destination receives the event - await collector.push(event, { - include: [options.destinationId], - }); + try { + await collector.push(event, { + include: [options.destinationId], + }); + } finally { + if (hasObservers) collector.observers.delete(captureMappingKey); + } @@ (error) => buildSimulationResult({ step: 'destination', name: options.destinationId, startTime, + mappingKey, error, }),🤖 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/push/index.ts` around lines 1376 - 1421, The mappingKey variable is captured from FlowState events during the collector.push() execution, but when push throws an error, the error callback handler does not pass the captured mappingKey to buildSimulationResult. Fix this by adding the mappingKey property (with the same value captured by captureMappingKey) to the buildSimulationResult call in the error callback handler that starts around line 1413, ensuring the matched rule key is preserved even when the destination step fails.packages/web/sources/cmps/usercentrics/src/schemas/settings.ts (1)
7-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SettingsSchemano longer exposesv3EventName, breaking the V3 adapter config contract.
packages/web/sources/cmps/usercentrics/src/lib/v3.tsreadssettings.v3EventName, andpackages/web/sources/cmps/usercentrics/src/types/index.tsdeclares it, but the schema object here omits it. If this schema is used to parse settings,v3EventNamewill be stripped and ignored at runtime.Suggested fix
export const SettingsSchema = z .object({ @@ explicitOnly: z .boolean() .describe( 'Only publish when the user has actively decided (V3: consent.type EXPLICIT; V2: an EXPLICIT entry in service consent history). Implicit/default page-load states are suppressed. Set false to publish any snapshot including implicit. Default: true.', ) .optional(), + + v3EventName: z + .string() + .describe( + "V3 window event name to listen for consent changes. Default: 'UC_UI_CMP_EVENT'.", + ) + .optional(), })🤖 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/web/sources/cmps/usercentrics/src/schemas/settings.ts` around lines 7 - 21, The SettingsSchema object is missing the v3EventName field definition, which causes it to be stripped during runtime parsing even though it is declared in the types and used by the V3 adapter code. Add a new field definition for v3EventName to the schema object alongside categoryMap and explicitOnly. Use z.string() to define it as a string type, chain .optional() to make it optional, and include a .describe() call with an appropriate description explaining its purpose for the V3 event configuration.
🧹 Nitpick comments (3)
packages/web/sources/cmps/usercentrics/README.md (1)
56-64: ⚡ Quick winExplicit decision detection guidance is accurate across all three docs; reinforce the dual-API pattern in decision matrix.
All three documentation files correctly state V3's
consent.type === EXPLICITand V2's EXPLICIT entry in service consent history. However, the decision matrix in SKILL.md oversimplifies Usercentrics as "events only" (line 142) and "event detail only" (line 151), which obscures the critical dual-path pattern:
packages/web/sources/cmps/usercentrics/README.md#L56-L64: Correctly documents explicit detection and timing pathswebsite/docs/sources/web/cmps/usercentrics/index.mdx#L112-L118: Correctly documents explicit consent subsection and V2/V3 verificationskills/walkeros-create-cmp-source/SKILL.md#L140-L152: Decision matrix should clarify "Already loaded" (static read + UC_UI_INITIALIZED) and "Consent access" (official getters + event-triggered reads), not just "events only"Update the decision matrix rows 142 and 151 to accurately represent the dual static-read + event-listener pattern so new implementers understand the full scope.
🤖 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/web/sources/cmps/usercentrics/README.md` around lines 56 - 64, The anchor documentation at packages/web/sources/cmps/usercentrics/README.md lines 56-64 and the sibling documentation at website/docs/sources/web/cmps/usercentrics/index.mdx lines 112-118 both correctly document the dual-path pattern for explicit decision detection (static getter read or UC_UI_INITIALIZED). However, the decision matrix in skills/walkeros-create-cmp-source/SKILL.md lines 140-152 oversimplifies this pattern by describing Usercentrics as "events only" and "event detail only". Update the decision matrix rows at lines 142 and 151 in SKILL.md to accurately represent both paths: clarify that "Already loaded" encompasses static read plus UC_UI_INITIALIZED timing, and that "Consent access" includes both official getters and event-triggered reads, not just events. This ensures new implementers understand the complete dual static-read and event-listener pattern consistent with the correct documentation in the other two files.skills/walkeros-create-cmp-source/SKILL.md (1)
703-709: 💤 Low value"Previous-choice" note is correct; consider minor wording improvement for implementer clarity.
The guidance correctly identifies that returning visitors' explicit decisions must be detected from official stored consent metadata (V3
consent.type, V2 service history), not from a per-pageload eventtypefield. This aligns with the actual implementation (v2.tshasExplicitDecision()and v3.tsbuildDetailFromV3()).Minor suggestion for clarity: The phrase "not a per-pageload event
typefield" could be misinterpreted as ruling out all event-derived data. Consider:"...not from the real-time
typefield of decision events (which reflects the current user action), but from official stored consent metadata or decision history (which proves an explicit prior decision was active)."This helps implementers understand they're distinguishing between current-page intent vs. stored decision proof, not between events and non-events.
🤖 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 `@skills/walkeros-create-cmp-source/SKILL.md` around lines 703 - 709, Update the wording in the "previous-choice" detection section to clarify the distinction between real-time decision event data and stored consent metadata. The current phrase "not a per-pageload event `type` field" could be misinterpreted as ruling out all event-derived data. Replace it with more explicit language that explains the difference: implementers should not use the real-time `type` field of decision events (which reflects the current user action), but instead rely on official stored consent metadata or decision history (which proves an explicit prior decision was actually active). This clarification helps implementers understand they are distinguishing between current-page intent versus stored decision proof.packages/web/sources/cmps/usercentrics/src/lib/v3.ts (1)
109-112: ⚡ Quick winAvoid silent failure on
UC_UI_INITIALIZEDconsent reads.Line 111 swallows errors without logging. If
getConsentDetails()fails on init, this path fails invisibly and is hard to diagnose.💡 Suggested refactor
const onInitialized = (): void => { const cmp = win.__ucCmp; - if (cmp) publishConsent(cmp).catch(() => {}); + if (cmp) + publishConsent(cmp).catch((err) => { + logger.warn('v3 initialized consent read failed', err); + }); };🤖 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/web/sources/cmps/usercentrics/src/lib/v3.ts` around lines 109 - 112, The onInitialized function silently catches errors from the publishConsent call without logging them, making it difficult to diagnose failures during consent initialization. Replace the empty catch handler with proper error logging that captures and logs the actual error details. This will ensure that if publishConsent fails, the error information is recorded for debugging purposes rather than being swallowed invisibly.
🤖 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 @.github/workflows/test.yml:
- Line 44: The actions/cache action on line 44 is pinned to a version tag (v4)
instead of a full commit SHA, which is inconsistent with other actions in the
workflow and weakens supply-chain integrity. Replace actions/cache@v4 with
actions/cache pinned to its full commit SHA (in the format
actions/cache@<full-commit-sha>) to match the pinning strategy used by
actions/checkout, actions/setup-node, and actions/upload-artifact elsewhere in
the workflow.
In `@packages/cli/src/runtime/fetch-retry.ts`:
- Around line 184-187: The AbortSignal.timeout(attemptTimeoutMs) at line 186 in
fetch-retry.ts remains active after fetch() resolves, causing the timeout to
potentially fire and abort the response body read downstream (in fetchTextToDisk
or extractToDir), resulting in an unretried AbortError. Create an
AbortController to manage the timeout signal, call abort() on it immediately
after the fetch completes to clear the timeout, and ensure the signal is no
longer active during body consumption. Additionally, update the incorrect
comment in resolve-bundle.ts (lines 126-132) that claims the timeout "already
fired and settled" to accurately describe that the timeout signal must be
managed to prevent aborting body reads.
In `@packages/cli/src/types/api.gen.d.ts`:
- Around line 6033-6041: The OpenAPI spec in packages/cli/openapi/spec.json
incorrectly advertises both text/event-stream and application/json media types
for the deployment stream endpoint's 200 response, which can cause generated
clients to use buffered JSON handling instead of streaming. Locate the
deployment stream endpoint in the OpenAPI spec (the endpoint that corresponds to
the generated DeploymentStreamStatusEvent content definition shown in the diff)
and remove the application/json entry from the 200 response content definition,
leaving only text/event-stream. After making this change, run npm run
generate:types to regenerate the TypeScript types file.
In `@packages/core/src/__tests__/cache-types.test-d.ts`:
- Around line 32-34: The test file currently only validates the negative case by
showing that `StoreCacheRule` rejects the `update` property, but doesn't test
the positive case for `EventCacheRule`. Add the `update` property to the
`_eventOk` object assignment to provide a positive assertion that
`EventCacheRule` accepts the `update` field, ensuring this type definition is
preserved in future changes and preventing accidental removal of the `update`
property.
In `@packages/web/sources/cmps/usercentrics/src/lib/v2.ts`:
- Around line 96-99: The early return condition in the read function is
unnecessarily gating V2 consent publishing on the optional isInitialized method.
Remove the isInitialized check from the conditional guard in the read function
so that the function only requires the getServicesBaseInfo method to be
available, allowing V2 consent reads to proceed on compatible deployments that
may not have the isInitialized method.
---
Outside diff comments:
In `@packages/cli/src/commands/push/index.ts`:
- Around line 1376-1421: The mappingKey variable is captured from FlowState
events during the collector.push() execution, but when push throws an error, the
error callback handler does not pass the captured mappingKey to
buildSimulationResult. Fix this by adding the mappingKey property (with the same
value captured by captureMappingKey) to the buildSimulationResult call in the
error callback handler that starts around line 1413, ensuring the matched rule
key is preserved even when the destination step fails.
In `@packages/web/sources/cmps/usercentrics/src/schemas/settings.ts`:
- Around line 7-21: The SettingsSchema object is missing the v3EventName field
definition, which causes it to be stripped during runtime parsing even though it
is declared in the types and used by the V3 adapter code. Add a new field
definition for v3EventName to the schema object alongside categoryMap and
explicitOnly. Use z.string() to define it as a string type, chain .optional() to
make it optional, and include a .describe() call with an appropriate description
explaining its purpose for the V3 event configuration.
---
Nitpick comments:
In `@packages/web/sources/cmps/usercentrics/README.md`:
- Around line 56-64: The anchor documentation at
packages/web/sources/cmps/usercentrics/README.md lines 56-64 and the sibling
documentation at website/docs/sources/web/cmps/usercentrics/index.mdx lines
112-118 both correctly document the dual-path pattern for explicit decision
detection (static getter read or UC_UI_INITIALIZED). However, the decision
matrix in skills/walkeros-create-cmp-source/SKILL.md lines 140-152
oversimplifies this pattern by describing Usercentrics as "events only" and
"event detail only". Update the decision matrix rows at lines 142 and 151 in
SKILL.md to accurately represent both paths: clarify that "Already loaded"
encompasses static read plus UC_UI_INITIALIZED timing, and that "Consent access"
includes both official getters and event-triggered reads, not just events. This
ensures new implementers understand the complete dual static-read and
event-listener pattern consistent with the correct documentation in the other
two files.
In `@packages/web/sources/cmps/usercentrics/src/lib/v3.ts`:
- Around line 109-112: The onInitialized function silently catches errors from
the publishConsent call without logging them, making it difficult to diagnose
failures during consent initialization. Replace the empty catch handler with
proper error logging that captures and logs the actual error details. This will
ensure that if publishConsent fails, the error information is recorded for
debugging purposes rather than being swallowed invisibly.
In `@skills/walkeros-create-cmp-source/SKILL.md`:
- Around line 703-709: Update the wording in the "previous-choice" detection
section to clarify the distinction between real-time decision event data and
stored consent metadata. The current phrase "not a per-pageload event `type`
field" could be misinterpreted as ruling out all event-derived data. Replace it
with more explicit language that explains the difference: implementers should
not use the real-time `type` field of decision events (which reflects the
current user action), but instead rely on official stored consent metadata or
decision history (which proves an explicit prior decision was actually active).
This clarification helps implementers understand they are distinguishing between
current-page intent versus stored decision proof.
🪄 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: c6dbde46-5a76-4abf-a8b6-becab6a400c3
📒 Files selected for processing (63)
.changeset/cli-runner-fetch-resilience.md.changeset/cli-runtime-observe-frozen-env.md.changeset/cli-simulate-data-injection.md.changeset/core-simulation-mapping-key.md.changeset/session-ungated-run-emit.md.changeset/usercentrics-official-consent.md.github/workflows/test.ymlapps/explorer/turbo.jsonpackage.jsonpackages/cli/openapi/spec.jsonpackages/cli/src/__tests__/integration/bundle/cdn-jsdom.test.tspackages/cli/src/__tests__/unit/bundle/bundler-codegen.test.tspackages/cli/src/__tests__/unit/commands/run-pipeline-frozen.test.tspackages/cli/src/__tests__/unit/commands/run-pipeline-telemetry.test.tspackages/cli/src/__tests__/unit/runtime/resolve-bundle.test.tspackages/cli/src/commands/bundle/__tests__/config-classifier.test.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/commands/push/__tests__/simulate-data-injection.test.tspackages/cli/src/commands/push/__tests__/simulate-mapping-key.test.tspackages/cli/src/commands/push/__tests__/simulate-result-shape.test.tspackages/cli/src/commands/push/index.tspackages/cli/src/commands/push/simulation-result.tspackages/cli/src/commands/run/pipeline.tspackages/cli/src/index.tspackages/cli/src/runtime/__tests__/config-fetcher.test.tspackages/cli/src/runtime/__tests__/fetch-retry.test.tspackages/cli/src/runtime/__tests__/secrets-fetcher.test.tspackages/cli/src/runtime/config-fetcher.tspackages/cli/src/runtime/fetch-retry.tspackages/cli/src/runtime/resolve-bundle.tspackages/cli/src/runtime/secrets-fetcher.tspackages/cli/src/types/api.gen.d.tspackages/core/src/__tests__/cache-types.test-d.tspackages/core/src/__tests__/cache-types.test.tspackages/core/src/__tests__/types/contract.test.tspackages/core/src/__tests__/types/observer.test.tspackages/core/src/__tests__/types/store.test.tspackages/core/src/__tests__/types/telemetry.test.tspackages/core/src/types/simulation.tspackages/web/sources/browser/src/__tests__/command-coverage.test.tspackages/web/sources/cmps/usercentrics/README.mdpackages/web/sources/cmps/usercentrics/src/__tests__/index.test.tspackages/web/sources/cmps/usercentrics/src/__tests__/stepExamples.test.tspackages/web/sources/cmps/usercentrics/src/__tests__/test-utils.tspackages/web/sources/cmps/usercentrics/src/__tests__/v2.test.tspackages/web/sources/cmps/usercentrics/src/__tests__/v3.test.tspackages/web/sources/cmps/usercentrics/src/examples/inputs.tspackages/web/sources/cmps/usercentrics/src/examples/outputs.tspackages/web/sources/cmps/usercentrics/src/examples/step.tspackages/web/sources/cmps/usercentrics/src/index.tspackages/web/sources/cmps/usercentrics/src/lib/v2.tspackages/web/sources/cmps/usercentrics/src/lib/v3.tspackages/web/sources/cmps/usercentrics/src/schemas/settings.tspackages/web/sources/cmps/usercentrics/src/types/index.tspackages/web/sources/session/src/__tests__/index.integration.test.tspackages/web/sources/session/src/__tests__/index.test.tspackages/web/sources/session/src/__tests__/stepExamples.test.tspackages/web/sources/session/src/index.tsskills/walkeros-create-cmp-source/SKILL.mdturbo.jsonwebsite/docs/apps/cli.mdxwebsite/docs/apps/runner.mdxwebsite/docs/sources/web/cmps/usercentrics/index.mdx
💤 Files with no reviewable changes (7)
- packages/core/src/tests/types/contract.test.ts
- packages/core/src/tests/cache-types.test.ts
- packages/core/src/tests/types/observer.test.ts
- packages/web/sources/browser/src/tests/command-coverage.test.ts
- packages/core/src/tests/types/store.test.ts
- packages/core/src/tests/types/telemetry.test.ts
- packages/web/sources/cmps/usercentrics/src/index.ts
alexanderkirtzel
temporarily deployed
to
npm-publish
GitHub Actions
Inactive
Contributor
|
📦 Pre-release published (next) Packages
Install: 🐳 Docker images published
Docker: |
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/cli/src/runtime/__tests__/redact.test.ts`:
- Around line 265-283: The performance tests in the redact.test.ts file use
overly tight wall-clock time thresholds of 200ms in the
expect(elapsed).toBeLessThan(200) assertions within both the 60KB and 80KB test
cases. These thresholds are too strict and cause nondeterministic failures on
slower CI runners. Increase the time threshold from 200ms to a looser but still
reasonable bound (such as 500ms or 1000ms) in both test cases to preserve the
anti-ReDoS intent while eliminating test instability and flakiness.
🪄 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: 0c0a8378-5878-4568-bfc1-f07beb366f3d
📒 Files selected for processing (27)
.changeset/cli-runtime-log-rings.md.changeset/collector-consent-init-gate.md.github/workflows/test.ymlpackages/cli/openapi/spec.jsonpackages/cli/src/commands/run/index.tspackages/cli/src/commands/run/pipeline.tspackages/cli/src/core/__tests__/cli-logger.test.tspackages/cli/src/core/cli-logger.tspackages/cli/src/runtime/__tests__/fetch-retry.test.tspackages/cli/src/runtime/__tests__/heartbeat.test.tspackages/cli/src/runtime/__tests__/log-ring.test.tspackages/cli/src/runtime/__tests__/redact.test.tspackages/cli/src/runtime/fetch-retry.tspackages/cli/src/runtime/heartbeat.tspackages/cli/src/runtime/index.tspackages/cli/src/runtime/log-ring.tspackages/cli/src/runtime/redact.tspackages/cli/src/runtime/resolve-bundle.tspackages/cli/src/types/api.gen.d.tspackages/collector/src/__tests__/destination.consent-init-gate.test.tspackages/collector/src/destination.tspackages/core/src/__tests__/cache-types.test-d.tspackages/web/destinations/heap/src/__tests__/stepExamples.test.tspackages/web/destinations/heap/src/examples/step.tspackages/web/sources/cmps/usercentrics/src/__tests__/v2.test.tspackages/web/sources/cmps/usercentrics/src/lib/v2.tswebsite/docs/apps/runner.mdx
✅ Files skipped from review due to trivial changes (4)
- .changeset/cli-runtime-log-rings.md
- website/docs/apps/runner.mdx
- .changeset/collector-consent-init-gate.md
- packages/cli/src/types/api.gen.d.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cli/src/runtime/resolve-bundle.ts
- packages/core/src/tests/cache-types.test-d.ts
- packages/cli/src/runtime/tests/fetch-retry.test.ts
- .github/workflows/test.yml
- packages/cli/src/runtime/fetch-retry.ts
- packages/web/sources/cmps/usercentrics/src/lib/v2.ts
- packages/web/sources/cmps/usercentrics/src/tests/v2.test.ts
alexanderkirtzel
temporarily deployed
to
npm-publish
GitHub Actions
Inactive
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/web/destinations/fullstory/src/__tests__/stepExamples.test.ts`:
- Around line 96-103: The elb call registering the walker destination at line 96
is not awaited, creating a race condition where the subsequent elb(event) call
at line 103 may execute before the destination is fully registered, causing
flaky assertions. Add the await keyword before the elb('walker destination',
...) call to ensure the destination registration completes before the event is
dispatched.
🪄 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: b07c7aae-9631-4c6e-bcd6-dc6503f046fa
📒 Files selected for processing (20)
packages/server/destinations/amplitude/src/__tests__/stepExamples.test.tspackages/server/destinations/amplitude/src/examples/step.tspackages/server/destinations/posthog/src/__tests__/stepExamples.test.tspackages/server/destinations/posthog/src/examples/step.tspackages/web/destinations/amplitude/src/__tests__/stepExamples.test.tspackages/web/destinations/amplitude/src/examples/step.tspackages/web/destinations/clarity/src/__tests__/stepExamples.test.tspackages/web/destinations/clarity/src/examples/step.tspackages/web/destinations/fullstory/src/__tests__/stepExamples.test.tspackages/web/destinations/fullstory/src/examples/step.tspackages/web/destinations/heap/src/__tests__/stepExamples.test.tspackages/web/destinations/heap/src/examples/step.tspackages/web/destinations/mixpanel/src/__tests__/stepExamples.test.tspackages/web/destinations/mixpanel/src/examples/step.tspackages/web/destinations/optimizely/src/__tests__/stepExamples.test.tspackages/web/destinations/optimizely/src/examples/step.tspackages/web/destinations/posthog/src/__tests__/stepExamples.test.tspackages/web/destinations/posthog/src/examples/step.tspackages/web/destinations/tiktok/src/__tests__/stepExamples.test.tspackages/web/destinations/tiktok/src/examples/step.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/destinations/heap/src/tests/stepExamples.test.ts
- packages/web/destinations/heap/src/examples/step.ts
Contributor
|
📦 Pre-release published (next) Packages
Install: 🐳 Docker images published
Docker: |
alexanderkirtzel
had a problem deploying
to
npm-publish
GitHub Actions
Failure
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/cli/src/__tests__/integration/runtime/hot-swap.test.ts`:
- Around line 130-131: The current assertion on the oldCommand variable only
checks that it was not called with the specific argument 'shutdown', but allows
it to be called with other arguments. To strengthen the rollback safety check
and ensure complete no-shutdown behavior on a failed swap, replace the
`.not.toHaveBeenCalledWith('shutdown')` assertion with `.not.toHaveBeenCalled()`
so that oldCommand is never invoked at all during the failed swap scenario.
In `@packages/cli/src/runtime/__tests__/heartbeat.test.ts`:
- Around line 29-30: The mockLogger created using makeSpyLogger() persists
across test cases, causing jest.fn() call history to leak between tests and
weaken test isolation. Move the mockLogger creation into a beforeEach hook
instead of creating it once at the module level, or alternatively add
jest.clearAllMocks() in a beforeEach hook to reset mock call histories before
each test runs. Ensure that the existing afterEach restoration code (which
appears to be around lines 97-100) is preserved or adjusted as needed to work
with the new setup.
🪄 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: 0994833d-4d3a-43f0-9fdd-47e88230c929
📒 Files selected for processing (43)
packages/cli/openapi/spec.jsonpackages/cli/src/__tests__/integration/runtime/hot-swap.test.tspackages/cli/src/__tests__/unit/commands/run-command-guard.test.tspackages/cli/src/__tests__/unit/commands/run-env-file-wiring.test.tspackages/cli/src/__tests__/unit/commands/run-pipeline-frozen.test.tspackages/cli/src/__tests__/unit/runtime/heartbeat-counters.test.tspackages/cli/src/commands/bundle/__tests__/package-manager-resilience.test.tspackages/cli/src/commands/bundle/package-manager.tspackages/cli/src/commands/run/__tests__/pipeline.test.tspackages/cli/src/commands/run/index.tspackages/cli/src/commands/run/pipeline.tspackages/cli/src/core/__tests__/cli-logger-config.test.tspackages/cli/src/core/__tests__/cli-logger-redact.test.tspackages/cli/src/core/__tests__/redact-line.test.tspackages/cli/src/core/cli-logger.tspackages/cli/src/core/redact-line.tspackages/cli/src/runtime/__tests__/heartbeat.test.tspackages/cli/src/runtime/__tests__/poller.test.tspackages/cli/src/runtime/__tests__/redact.test.tspackages/cli/src/runtime/__tests__/runner-logger-tap.test.tspackages/cli/src/runtime/__tests__/runner.test.tspackages/cli/src/runtime/heartbeat.tspackages/cli/src/runtime/poller.tspackages/cli/src/runtime/redact.tspackages/cli/src/runtime/runner.tspackages/cli/src/types/api.gen.d.tspackages/collector/src/__tests__/destination-timeout.test.tspackages/collector/src/destination.tspackages/core/src/types/destination.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__/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/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.tspackages/web/destinations/fullstory/src/__tests__/stepExamples.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/server/destinations/gcp/src/bigquery/tests/bigquery-storage-mock.d.ts
- packages/server/sources/express/src/types.ts
- packages/server/destinations/gcp/src/bigquery/pushBatch.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web/destinations/fullstory/src/tests/stepExamples.test.ts
- packages/cli/src/runtime/tests/redact.test.ts
- packages/cli/src/tests/unit/commands/run-pipeline-frozen.test.ts
- packages/cli/src/types/api.gen.d.ts
alexanderkirtzel
temporarily deployed
to
npm-publish
GitHub Actions
Inactive
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/schemas/destination.ts`:
- Around line 173-178: The timeout schema field in the destination.ts file
currently accepts negative values, but the runtime resolver silently defaults
any non-positive value to 10000 ms, hiding configuration errors. Add the
`.nonnegative()` constraint to the timeout field definition (between the
`.optional()` and `.describe()` calls) to ensure schema validation rejects
negative values and fails fast, aligning with the documented runtime behavior of
the resolveDestinationTimeout resolver.
🪄 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: fd2448f5-67ea-4f25-8681-21b40a020216
📒 Files selected for processing (3)
packages/cli/src/__tests__/integration/runtime/hot-swap.test.tspackages/cli/src/runtime/__tests__/heartbeat.test.tspackages/core/src/schemas/destination.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/tests/integration/runtime/hot-swap.test.ts
- packages/cli/src/runtime/tests/heartbeat.test.ts
Comment on lines
+173
to
+178
| timeout: z | ||
| .number() | ||
| .optional() | ||
| .describe( | ||
| 'Per-destination delivery timeout in ms (default 10000); a delivery that does not settle in this window is routed to the DLQ like a thrown push.', | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout normalization logic and whether negative-timeout behavior is covered by tests.
rg -n -C3 'function resolveDestinationTimeout|timeout > 0|DEFAULT_DESTINATION_TIMEOUT_MS'
rg -n -C3 'timeout' packages/collector/src/__tests__/destination-timeout.test.tsRepository: elbwalker/walkerOS
Length of output: 5607
🏁 Script executed:
cat -n packages/core/src/schemas/destination.ts | sed -n '165,185p'Repository: elbwalker/walkerOS
Length of output: 940
🏁 Script executed:
fd -e json package.json | head -5 | xargs grep -l zodRepository: elbwalker/walkerOS
Length of output: 92
🏁 Script executed:
rg -n 'timeout.*-\d+|timeout.*<.*0' packages/Repository: elbwalker/walkerOS
Length of output: 290
🏁 Script executed:
cat packages/core/package.json | grep -A2 -B2 zodRepository: elbwalker/walkerOS
Length of output: 130
🏁 Script executed:
cat -n packages/web/core/src/__tests__/sendWeb.test.ts | sed -n '180,195p'Repository: elbwalker/walkerOS
Length of output: 882
🏁 Script executed:
rg -n 'timeout.*-|negative' packages/collector/src/__tests__/destination-timeout.test.tsRepository: elbwalker/walkerOS
Length of output: 135
🏁 Script executed:
rg -n 'schema.*parse|validate.*timeout|destination.*schema' packages/core/src/schemas/destination.ts -A3 -B3Repository: elbwalker/walkerOS
Length of output: 44
Add numeric constraint to timeout schema to prevent silent misconfiguration.
The schema accepts negative values, but the runtime resolver (resolveDestinationTimeout) treats any non-positive value as invalid and silently defaults to 10000 ms. This hides configuration errors instead of failing fast. Adding .nonnegative() aligns schema validation with documented runtime behavior.
Suggested fix
timeout: z
.number()
+ .nonnegative()
.optional()
.describe(
- 'Per-destination delivery timeout in ms (default 10000); a delivery that does not settle in this window is routed to the DLQ like a thrown push.',
+ 'Per-destination delivery timeout in ms (default 10000). Use 0 to apply the default; deliveries that do not settle in this window are routed to the DLQ like a thrown push.',
),🤖 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/schemas/destination.ts` around lines 173 - 178, The timeout
schema field in the destination.ts file currently accepts negative values, but
the runtime resolver silently defaults any non-positive value to 10000 ms,
hiding configuration errors. Add the `.nonnegative()` constraint to the timeout
field definition (between the `.optional()` and `.describe()` calls) to ensure
schema validation rejects negative values and fails fast, aligning with the
documented runtime behavior of the resolveDestinationTimeout resolver.
Contributor
|
📦 Pre-release published (next) 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
WALKEROS_OBSERVE_LEVELandWALKEROS_CONFIG_FROZENto control telemetry baseline and frozen-config runs.data(without rebundling) and can reportmappingKeyfor destination mapping matches.run: false.