565 ai readiness #566
565 ai readiness #566
Conversation
WalkthroughAdds many documentation/skill files; implements a built‑in "code" destination and related types; threads scoped loggers and richer Destination.Context through collector and destinations; introduces AWS Lambda and Fetch server sources; refactors CLI bundler to programmatic generation; and updates tests, tooling, and demos. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (client)
participant Collector as Collector (startFlow / elb)
participant Destination as Destination.Code
participant Logger as Logger (scoped)
Browser->>Collector: push(event)
Note over Collector,Logger: Collector resolves destination code and scopes logger
Collector->>Logger: logger.scope(destination.type) -> scopedLogger
Collector->>Destination: init(context) [if first time]
Destination->>Logger: scopedLogger.debug("init")
Destination-->>Collector: init done
Collector->>Logger: scopedLogger.debug("push" with event)
Collector->>Destination: push(event, context)
Destination->>Logger: scopedLogger.debug("push done")
Destination-->>Collector: push result
Collector->>Logger: scopedLogger.debug("push batch done")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Demo flows
![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 (1)
packages/collector/src/destination.ts (1)
105-125: Skipped events are not actually re-queued (concatresult ignored)
destination.queue.concat(skippedEvents);does not mutatedestination.queue; it returns a new array that is discarded, so events filtered out by consent are never re-added to the destination queue despite the comment.Consider fixing this while you’re in this module:
- // Add skipped events back to the queue - destination.queue.concat(skippedEvents); + // Add skipped events back to the queue + destination.queue = (destination.queue || []).concat(skippedEvents);This issue appears pre-existing but can lead to silently dropped, consent-denied events that you may intend to preserve for later processing.
🧹 Nitpick comments (11)
turbo.json (1)
4-4: Concurrency reduction impacts overall build/test throughputSetting
"concurrency": "1"effectively serializes Turbo tasks. This is fine if motivated by CI/devcontainer resource limits, but it will noticeably slow builds/tests. If you expect higher local parallelism, consider documenting this choice or overriding via env/CLI for power users.packages/config/jest/index.mjs (1)
100-104: Jest parallelism and shutdown behavior changes look goodReducing
maxWorkersto2and explicitly documenting thatforceExitis disabled aligns with best practices (let Jest surface open-handle issues instead of forcing exit) and helps with devcontainer memory limits. If test runtimes become long on more powerful machines, you could later exposemaxWorkersvia an env var so local runs can opt into higher parallelism.skills/understanding-flow/SKILL.md (1)
21-29: Add a language to the ASCII flow diagram fence to satisfy markdownlintThe fenced block showing the Source → Collector → Destinations diagram lacks a language, which triggers MD040. You can keep the rendering identical and appease tooling by labeling it, e.g.:
-``` +```text Sources → Collector → Destinations (Data Capture) (Processing) (Delivery) ...</blockquote></details> <details> <summary>skills/create-destination/SKILL.md (1)</summary><blockquote> `25-33`: **Label non-code fences (`text`) to satisfy markdownlint MD040** A few fenced blocks are prose/ASCII (process steps, mapping trace, directory tree) and currently have no language, which trips MD040: - Process overview (Phase 2) block around line 25 - Mapping trace example around line 270 - Directory structure tree around line 294 You can keep them exactly as-is visually and fix lint by labeling them as text: ```diff -``` +```text 1. Research → Find SDK, understand vendor API ...Repeat similarly for the other two blocks. Also applies to: 270-277, 294-315 </blockquote></details> <details> <summary>skills/understanding-mapping/SKILL.md (1)</summary><blockquote> `38-55`: **Tighten mapping examples (duplicate key & minor TS syntax nit)** Two small doc nits that would make these examples more copy/paste-friendly: 1. **Duplicate `product` key in the mapping example** In the `const mapping = { ... }` block, `product` is declared twice; in JS/TS the latter wins, so the `view`/`add` rules would be shadowed. Consider merging into a single `product` object: ```typescript const mapping = { product: { // Exact matches view: { name: 'view_item' }, add: { name: 'add_to_cart' }, // Wildcard: any action '*': { name: 'product_interaction' }, }, // Wildcard: any entity '*': { click: { name: 'generic_click' }, }, };
Array loop example trailing
;
The array loop example ends the object with];, which is invalid TS if copied directly. Dropping the semicolon keeps it valid:{ loop: [ 'nested', { map: { /* ... */ } }, ]
- ];
- }
These are purely documentation cleanups; the conceptual mapping guidance itself looks solid.
Also applies to: 135-145
skills/create-source/SKILL.md (2)
509-513: Avoidas anyin example tests to reflect “no any” guideline.The test example for invalid input casts with
as any. Since AGENT explicitly calls out “Noanyin production code”, it would be clearer if the example also avoided it—for instance by widening theInputtype for test-only cases, or constructing an explicitly typed invalid object. That keeps the skill aligned with the repository’s TypeScript guidance.
375-382: Either wiremappingintotransformInputor clarify that the example is name‑mapping only.The earlier section defines a
Mapping.Rulesconfiguration and importsgetMappingValue, but thetransformInputexample doesn’t useconfig.mappingorgetMappingValueand just forwardsinput.propertiesintodata. To reduce confusion for readers:
- Either show a minimal example of applying
config.mappingwithgetMappingValuewhen buildingdata, or- Remove the unused
getMappingValueimport and add a short note that this transform focuses only on event naming and basic field passthrough.Right now it reads as if mapping should be applied, but the sample implementation never does.
Also applies to: 399-427, 268-307
packages/server/destinations/meta/src/push.ts (1)
13-16: Logger integration and endpoint refactor look solid.The updated
PushFnsignature, extractedendpointvariable, and scopedlogger.debug/logger.throwusage align with the destination patterns elsewhere and improve observability without changing behavior. Usingenv?.sendServer || sendServerkeeps it testable.If you want richer debugging later, you could extend the response log to include status/body when available, but that’s not required for this change.
Also applies to: 72-92
packages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts (1)
2-4: Tests correctly adopt logger/context pattern; minor setup cleanup possible.Using
import type { Collector }and passinglogger: createMockLogger()into bothinitandpushmatches the new destination push context and keeps types clean.You can optionally tidy the test setup by:
- Removing the unused top-level
mockCollectordeclaration (Line 18) and relying on the local one in thepushtest, and- Dropping the unused
callsarray if it’s no longer inspected.These are non-functional, just to reduce noise in the spec.
Also applies to: 20-28, 82-91
skills/using-logger/SKILL.md (1)
1-334: Update “Needs Improvement” list and keep logger interface in sync with core typesThe overall guidance here (scoped logger, avoiding duplicate init/push logs, focusing on external calls/validation) matches the patterns in AGENT.md and the new destination changes in this PR. Based on learnings, this is the right direction.
Two adjustments to keep it from drifting:
- Outdated “Needs Improvement” entries
This PR adds structured logging to both Meta and AWS Firehose destinations, so the list under “Needs Improvement” becomes stale immediately:
packages/server/destinations/meta/src/push.tspackages/server/destinations/aws/src/firehose/push.tsConsider trimming that section to only entries that truly still lack logging, e.g.:
-**Needs Improvement:** - -- [packages/server/sources/fetch/src/index.ts](../../packages/server/sources/fetch/src/index.ts) - - Missing all logging -- [packages/server/destinations/meta/src/push.ts](../../packages/server/destinations/meta/src/push.ts) - - Missing push logging -- [packages/server/destinations/aws/src/firehose/push.ts](../../packages/server/destinations/aws/src/firehose/push.ts) - - Missing push logging +**Needs Improvement:** + +- [packages/server/sources/fetch/src/index.ts](../../packages/server/sources/fetch/src/index.ts) - + Missing all logging
- Keep the
Logger.Instanceinterface snippet authoritativeTo avoid future doc drift, please double‑check that the documented
Logger.Instanceshape (methods, argument types, presence ofthrowandscope) exactly matchespackages/core/src/types/logger.ts. If the core type evolves (e.g., new methods), this snippet should be updated in lockstep so it remains a reliable reference.packages/server/destinations/aws/src/firehose/push.ts (1)
4-9: Consider awaitingpushFirehoseto propagate Firehose errors correctly
pushis declared as anasyncPushFn, but the call topushFirehose([{ event }], firehose, context)is not awaited. That means the destination push can resolve before the Firehose client I/O finishes, and any error frompushFirehosewould surface as an unhandled Promise rather than flowing back through the collector.Given other destinations typically await their outbound API calls, I’d recommend:
-export const push: PushFn = async function (event, context) { - const { firehose } = context.config.settings || {}; - - if (firehose) pushFirehose([{ event }], firehose, context); - - return; -}; +export const push: PushFn = async function (event, context) { + const { firehose } = context.config.settings || {}; + + if (firehose) { + await pushFirehose([{ event }], firehose, context); + } +};This keeps the
PushFnreturn type asPromise<void>but ensures Firehose errors are correctly surfaced and that the lifecycle is consistent with other destinations. Please double‑check that this matches how the collector expects destinationpushto behave.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.claude/skills/create-destination/SKILL.md(1 hunks).claude/skills/create-source/SKILL.md(1 hunks).claude/skills/debugging/SKILL.md(1 hunks).claude/skills/mapping-configuration/SKILL.md(1 hunks).claude/skills/testing-strategy/SKILL.md(1 hunks).claude/skills/understanding-destinations/SKILL.md(1 hunks).claude/skills/understanding-development/SKILL.md(1 hunks).claude/skills/understanding-events/SKILL.md(1 hunks).claude/skills/understanding-flow/SKILL.md(1 hunks).claude/skills/understanding-mapping/SKILL.md(1 hunks).claude/skills/understanding-sources/SKILL.md(1 hunks).claude/skills/using-logger/SKILL.md(1 hunks).devcontainer/devcontainer.json(2 hunks).gitignore(1 hunks)AGENT.md(1 hunks)packages/collector/src/__tests__/destination.test.ts(2 hunks)packages/collector/src/destination.ts(3 hunks)packages/config/jest/index.mjs(1 hunks)packages/core/src/dev.ts(1 hunks)packages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts(1 hunks)packages/server/destinations/aws/src/firehose/lib/firehose.ts(4 hunks)packages/server/destinations/aws/src/firehose/push.ts(1 hunks)packages/server/destinations/datamanager/src/index.ts(1 hunks)packages/server/destinations/datamanager/src/push.ts(0 hunks)packages/server/destinations/gcp/src/bigquery/push.ts(2 hunks)packages/server/destinations/meta/src/push.ts(2 hunks)skills/README.md(1 hunks)skills/create-destination/SKILL.md(1 hunks)skills/create-source/SKILL.md(1 hunks)skills/debugging/SKILL.md(1 hunks)skills/mapping-configuration/SKILL.md(1 hunks)skills/testing-strategy/SKILL.md(1 hunks)skills/understanding-destinations/SKILL.md(1 hunks)skills/understanding-development/SKILL.md(1 hunks)skills/understanding-events/SKILL.md(1 hunks)skills/understanding-flow/SKILL.md(1 hunks)skills/understanding-mapping/SKILL.md(1 hunks)skills/understanding-sources/SKILL.md(1 hunks)skills/using-logger/SKILL.md(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/server/destinations/datamanager/src/push.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/server/destinations/meta/src/push.tspackages/collector/src/__tests__/destination.test.tspackages/server/destinations/aws/src/firehose/push.tspackages/server/destinations/gcp/src/bigquery/push.tspackages/core/src/dev.tspackages/server/destinations/aws/src/firehose/__tests__/firehose.test.tspackages/server/destinations/datamanager/src/index.tspackages/server/destinations/aws/src/firehose/lib/firehose.tspackages/collector/src/destination.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENT.md)
packages/*/src/**/*.ts: Use Higher-Order Functions (HOF) and smart abstractions to avoid redundancy and keep implementations lean and performant
Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Files:
packages/collector/src/__tests__/destination.test.tspackages/core/src/dev.tspackages/collector/src/destination.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/collector/src/__tests__/destination.test.tspackages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T16:24:48.032Z
Learning: Refer to AGENT.md for all relevant instructions and guidelines when working with the walkerOS codebase
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
📚 Learning: 2025-11-25T16:24:48.032Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T16:24:48.032Z
Learning: Refer to AGENT.md for all relevant instructions and guidelines when working with the walkerOS codebase
Applied to files:
skills/README.md.claude/skills/understanding-development/SKILL.mdskills/debugging/SKILL.mdAGENT.md.claude/skills/create-source/SKILL.mdskills/using-logger/SKILL.md.claude/skills/testing-strategy/SKILL.mdskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts.claude/skills/understanding-mapping/SKILL.mdskills/understanding-development/SKILL.mdskills/create-destination/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Use the Source → Collector → Destination(s) architecture pattern for event processing: Sources capture events from various sources (browser DOM, dataLayer, server), Collector handles central event processing with consent management, Destinations transform and send events to third-party tools
Applied to files:
skills/understanding-sources/SKILL.mdskills/understanding-flow/SKILL.md.claude/skills/understanding-flow/SKILL.mdskills/debugging/SKILL.mdAGENT.mdskills/using-logger/SKILL.mdskills/understanding-destinations/SKILL.mdskills/understanding-mapping/SKILL.md.claude/skills/understanding-sources/SKILL.mdskills/create-source/SKILL.mdpackages/collector/src/destination.tsskills/create-destination/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: All walkerOS components communicate via push functions: sources use source.push(), collector uses collector.push(), destinations use destination.push(), and ELB uses elb() as the alias for collector.push()
Applied to files:
skills/understanding-sources/SKILL.mdskills/understanding-flow/SKILL.mdpackages/server/destinations/meta/src/push.tspackages/collector/src/__tests__/destination.test.tsskills/using-logger/SKILL.mdpackages/server/destinations/aws/src/firehose/push.tspackages/server/destinations/gcp/src/bigquery/push.tspackages/server/destinations/aws/src/firehose/lib/firehose.ts.claude/skills/understanding-sources/SKILL.mdpackages/collector/src/destination.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
skills/understanding-flow/SKILL.mdpackages/server/destinations/meta/src/push.tsAGENT.mdpackages/collector/src/__tests__/destination.test.tsskills/using-logger/SKILL.mdpackages/server/destinations/aws/src/firehose/push.tspackages/server/destinations/gcp/src/bigquery/push.tsskills/understanding-destinations/SKILL.mdskills/understanding-mapping/SKILL.md.claude/skills/understanding-destinations/SKILL.mdskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.tspackages/server/destinations/datamanager/src/index.tspackages/server/destinations/aws/src/firehose/lib/firehose.tsskills/create-source/SKILL.mdpackages/collector/src/destination.tsskills/create-destination/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Applied to files:
.claude/skills/mapping-configuration/SKILL.mdAGENT.mdskills/understanding-events/SKILL.mdskills/understanding-mapping/SKILL.mdskills/mapping-configuration/SKILL.mdskills/create-source/SKILL.md.claude/skills/understanding-mapping/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
.claude/skills/understanding-development/SKILL.mdAGENT.mdpackages/collector/src/__tests__/destination.test.tsskills/using-logger/SKILL.md.claude/skills/testing-strategy/SKILL.mdskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.tsskills/create-source/SKILL.mdskills/understanding-development/SKILL.mdskills/create-destination/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
AGENT.mdpackages/collector/src/__tests__/destination.test.tsskills/using-logger/SKILL.mdskills/understanding-destinations/SKILL.mdskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/collector/src/__tests__/destination.test.tsskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Applied to files:
skills/understanding-events/SKILL.md.claude/skills/understanding-events/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Always use space-separated 'ENTITY ACTION' format for event names (e.g., 'page view', 'product add', 'button click'). Event names are parsed as: const [entity, action] = event.split(' ')
Applied to files:
skills/understanding-events/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
skills/understanding-events/SKILL.mdskills/understanding-mapping/SKILL.mdskills/mapping-configuration/SKILL.mdskills/testing-strategy/SKILL.mdpackages/server/destinations/aws/src/firehose/__tests__/firehose.test.tspackages/server/destinations/aws/src/firehose/lib/firehose.tsskills/understanding-development/SKILL.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Applied to files:
skills/understanding-development/SKILL.md
🧬 Code graph analysis (3)
packages/collector/src/__tests__/destination.test.ts (2)
packages/core/src/mockLogger.ts (1)
createMockLogger(41-65)packages/collector/src/destination.ts (1)
pushToDestinations(63-202)
packages/server/destinations/aws/src/firehose/push.ts (4)
packages/server/destinations/aws/src/firehose/index.ts (1)
push(23-25)packages/core/src/types/collector.ts (1)
PushFn(93-98)packages/core/src/types/destination.ts (1)
PushFn(141-144)packages/server/destinations/aws/src/firehose/types/index.ts (1)
PushFn(38-38)
packages/collector/src/destination.ts (3)
packages/core/src/useHooks.ts (1)
useHooks(12-39)packages/core/src/types/destination.ts (1)
Instance(59-69)packages/core/src/types/logger.ts (1)
Instance(70-98)
🪛 markdownlint-cli2 (0.18.1)
skills/README.md
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
skills/understanding-flow/SKILL.md
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
AGENT.md
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
skills/create-source/SKILL.md
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
314-314: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
342-342: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
skills/understanding-development/SKILL.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
skills/create-destination/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
270-270: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (30)
.devcontainer/devcontainer.json (2)
10-13: CLAUDE_CONFIG_DIR path is correctly configured.The
.claude/data/directory is intentionally gitignored to exclude runtime data from version control while tracking source files (settings and skills in.claude/). The directory structure exists and has appropriate permissions; thedata/subdirectory will be created by Claude Code on first use or is available at container runtime. No action needed.
44-44: No action required. The@anthropic-ai/claude-codepackage is publicly available on npm as Anthropic's official Claude Code CLI tool and is compatible with this project. The project's Node.js engine requirement (≥20.0) exceeds the package requirement (≥18.0), and no dependency conflicts exist since the package is installed globally..gitignore (1)
95-97: Ignoring Claude Code data is appropriateIgnoring
.claude/data/keeps transient conversations/cache out of git, which is the right default for tool-generated artifacts..claude/skills/understanding-development/SKILL.md (1)
1-16: Skill pointer pattern is clear and consistentYAML metadata plus a short pointer to
@skills/understanding-development/SKILL.mdmatches the intended Claude-discovery pattern while keeping content tool‑agnostic. No issues.packages/core/src/dev.ts (1)
1-2: Reordered exports are harmless and slightly more ergonomicReordering to
export { z, zodToSchema, type JSONSchema }preserves the public surface while makingzthe first named export, which is intuitive for consumers. No type or runtime impact.packages/server/destinations/datamanager/src/index.ts (1)
17-37: Auth client logging via shared logger is appropriateUsing
logger.debug('Auth client created')aftercreateAuthClientintegrates this destination with the shared logging pattern and avoids console usage, without altering control flow. Looks good..claude/skills/using-logger/SKILL.md (1)
1-16: Logger skill metadata matches the new logging conventionsThis skill metadata correctly describes when/how to use the walkerOS logger and points to the canonical
skills/using-logger/SKILL.md. It fits the overall AI‑readiness/documentation pattern in this PR..claude/skills/understanding-flow/SKILL.md (1)
1-16: Metadata reference file—LGTM.Proper YAML frontmatter and clear reference to primary content location. Consistent with the skill discovery pattern.
skills/understanding-development/SKILL.md (1)
1-41: Comprehensive and well-structured onboarding guide.Clear overview of XP principles, commands, and core utilities aligned with walkerOS architecture. Proper emphasis on strict conventions (no
any, TDD-first, DRY)..claude/skills/understanding-mapping/SKILL.md (1)
1-16: Metadata reference—consistent pattern.YAML frontmatter and reference pointer properly formatted and aligned with other skill metadata files in the PR.
skills/understanding-destinations/SKILL.md (1)
1-125: Well-crafted destination reference guide.Clear interface documentation, env pattern explanation with working examples, and proper testing guidance. Code blocks are properly marked with language identifiers and examples align with walkerOS patterns.
.claude/skills/understanding-destinations/SKILL.md (1)
1-16: Metadata reference—consistent with pattern.YAML frontmatter and discoverability reference properly formatted.
.claude/skills/testing-strategy/SKILL.md (1)
1-16: Metadata reference—follows established pattern.Properly formatted YAML frontmatter and clear reference pointer.
.claude/skills/debugging/SKILL.md (1)
1-16: Metadata reference—consistent pattern maintained.YAML frontmatter and reference pointer properly structured.
skills/debugging/SKILL.md (2)
1-125: Exceptional debugging guide—practical and comprehensive.Clear diagnosis table, four distinct debugging strategies with examples, and six common issues with specific fixes. All TypeScript examples properly marked. Debugging checklist provides actionable verification sequence. Excellent alignment with walkerOS architecture patterns (env pattern, consent, mapping, consent).
225-237: Debugging checklist—excellent verification pattern.The 8-step checklist (lines 225-237) provides a systematic approach to event troubleshooting. Well-ordered progression from console errors → event flow → mapping → data → consent → SDK → network → vendor.
packages/collector/src/__tests__/destination.test.ts (2)
44-55: Logger mocking setup correctly models scoped loggingUsing
createMockLoggerplus a dedicatedscopedMockLoggerreturned fromlogger.scopegives you a clean way to assert on scoped debug calls while still matching the core logger interface. This is a reasonable pattern for these collector-level tests and aligns with the new destination logging behavior.
123-163: Lifecycle logging tests accurately verify init/push behaviorThe
logs init lifecycleandlogs push lifecycletests nicely validate the new logging contract: they assertlogger.scope('unknown')is used and that the scoped logger recordsinit/init doneandpush/push donewith the expectedeventpayload. This provides good regression protection for the logging behavior introduced indestinationInit/destinationPush.packages/collector/src/destination.ts (1)
218-247: Scoped debug logging around init, push, and batch looks solidThe added
destLogger/batchLoggerdebug calls arounddestinationInit, per-eventdestinationPush, andpushBatch(including counts and lifecycle “done” markers) give clear observability without changing existing control flow or consent behavior. This matches the logger scoping pattern used elsewhere and will make destination issues much easier to diagnose.Also applies to: 273-284, 296-322, 330-340
.claude/skills/create-destination/SKILL.md (1)
1-15: Metadata shim correctly forwards to the canonical skillThe frontmatter and pointer to
@skills/create-destination/SKILL.mdare clear and keep content tool-agnostic while enabling discovery. No changes needed.packages/server/destinations/gcp/src/bigquery/push.ts (1)
7-14: Logger integration in BigQuery push improves error handling and observabilityWiring
loggerinto the push context and usinglogger.throwfor missingclient/datasetId/tableIdplus the debug logs aroundinsert()give you clear, structured visibility into configuration problems and API calls, without changing the existing row-shaping behavior. This aligns well with the broader destination logging pattern in the collector.Also applies to: 30-39
skills/testing-strategy/SKILL.md (1)
20-227: Testing strategy doc aligns well with env + dev examples patternThis skill document clearly encodes the AGENT/testing guidance: env-based mocking instead of Jest module mocks, tests wired to
devexamples, and explicit rules around integration/unit/contract tests. ThemockEnvanddev.tsstructures shown are consistent with existing patterns and should be a good reference for future contributors..claude/skills/create-source/SKILL.md (1)
1-15: Claude skill locator looks consistent and minimal.Front matter and the pointer to
skills/create-source/SKILL.mdmatch the documented pattern for Claude skill references; nothing to change here..claude/skills/understanding-events/SKILL.md (1)
1-16: Understanding-events Claude reference is correctly wired.The file follows the standard pattern (front matter + pointer to
skills/understanding-events/SKILL.md) and looks good as a discovery aid..claude/skills/understanding-sources/SKILL.md (1)
1-16: Understanding-sources Claude reference matches the intended pattern.Front matter and the redirect to
skills/understanding-sources/SKILL.mdare consistent with the rest of the skills setup..claude/skills/mapping-configuration/SKILL.md (1)
1-15: Mapping-configuration Claude reference is clean and consistent.The metadata and redirect to
skills/mapping-configuration/SKILL.mdfollow the same convention as the other skills; nothing to adjust.skills/understanding-sources/SKILL.md (1)
1-115: Sources skill aligns well with Source → Collector → Destination modelThis doc clearly describes sources as capture-only, push-based components and points to the canonical
source.tstypes and concrete paths; it matches the Source → Collector → Destination architecture and push() usage described in AGENT.md. Based on learnings, this looks consistent and ready to ship.skills/mapping-configuration/SKILL.md (1)
1-280: Mapping recipes are consistent with the core mapping DSLThe patterns here (string paths,
{ value },{ fn },{ map },{ loop }, consent-gated fields, wildcard entity/action) match the flexible mapping strategies described in AGENT.md and implemented inpackages/core/src/mapping.ts. Referencingapps/quickstart/src/mappings/as validated examples is also in line with the “tests first, docs second” guidance. Based on learnings, this looks solid.skills/understanding-events/SKILL.md (1)
1-177: Event model doc matches the canonical walkerOS event contractThis skill file cleanly codifies the
"entity action"requirement, the full event property set, and the stateless/vendor‑agnostic design, exactly as described in AGENT.md and the coreevent.ts/schema files. Theelb('page view', …)examples also match the documented collector API. Based on learnings, this is accurate and ready to merge.packages/server/destinations/aws/src/firehose/lib/firehose.ts (1)
1-78: Firehose helper’s PushContext + logger integration looks correct
pushFirehosecleanly switches toDestination.PushContext, derivesenvvia the existingisAWSEnvironmentguard, and adds before/after debug logs around theclient.send(...)call, in line with the “external API call” pattern from the logger skill. The existing behavior of constructing the client fromenv.AWS.FirehoseClientor falling back to a dynamic SDK import is preserved, so this change reads as a safe, backwards‑compatible logging upgrade.
| ``` | ||
| packages/ | ||
| ├── core/ # Platform-agnostic types and utilities | ||
| ├── collector/ # Central event collection and processing | ||
| ├── config/ # Shared configuration (eslint, jest, tsconfig, tsup) | ||
| ├── web/ | ||
| │ ├── core/ # Web-specific utilities | ||
| │ ├── sources/ # Data sources (browser DOM, dataLayer) | ||
| │ └── destinations/ # Web destinations (gtag, meta, api, piwikpro, plausible) | ||
| └── server/ | ||
| ├── core/ # Server-specific utilities | ||
| ├── sources/ # Server sources (gcp) | ||
| └── destinations/ # Server destinations (aws, gcp, meta) | ||
| ``` | ||
|
|
||
| ### Applications | ||
|
|
||
| - **apps/walkerjs**: Ready-to-use browser bundle | ||
| - **apps/quickstart**: Code examples and getting started templates | ||
| - **website**: Documentation site built with Docusaurus | ||
|
|
||
| ## Critical Event Model Rules | ||
|
|
||
| ### 1. Entity-Action Event Naming | ||
|
|
||
| **STRICT REQUIREMENT**: All events MUST follow the "ENTITY ACTION" format with | ||
| space separation: | ||
|
|
||
| - ✅ Correct: `"page view"`, `"product add"`, `"order complete"`, | ||
| `"button click"` | ||
| - ❌ Wrong: `"page_view"`, `"purchase"`, `"add_to_cart"`, `"pageview"` | ||
|
|
||
| The event name is parsed as: `const [entity, action] = event.split(' ')` | ||
|
|
||
| ### 2. Universal Push Interface Standard | ||
|
|
||
| **CRITICAL**: All walkerOS components communicate via `push` functions: | ||
|
|
||
| - **Sources**: `source.push()` - Interface to external world (HTTP, DOM events, | ||
| etc.) | ||
| - **Collector**: `collector.push()` - Central event processing | ||
| - **Destinations**: `destination.push()` - Receive processed events | ||
| - **ELB**: `elb()` - Alias for collector.push, used for component wiring | ||
|
|
||
| **Source Push Signatures by Type**: | ||
|
|
||
| - Cloud Functions: `push(req, res) => Promise<void>` (HTTP handler) | ||
| - Browser: `push(event, data) => Promise<void>` (Walker events) | ||
| - DataLayer: `push(event, data) => Promise<void>` (Walker events) | ||
|
|
||
| **Key Principle**: Source `push` IS the handler - no wrappers needed. Example: | ||
| `http('handler', source.push)` for direct deployment. | ||
|
|
||
| ### 3. Event Structure | ||
|
|
||
| All events follow this consistent structure: | ||
|
|
||
| ```typescript | ||
| { | ||
| name: 'product view', // ENTITY ACTION format | ||
| data: { // Entity-specific properties | ||
| id: 'P123', | ||
| name: 'Laptop', | ||
| price: 999 | ||
| }, | ||
| context: { // State/environment information | ||
| stage: ['shopping', 1], | ||
| test: ['variant-A', 0] | ||
| }, | ||
| globals: { // Global properties | ||
| language: 'en', | ||
| currency: 'USD' | ||
| }, | ||
| user: { // User identification | ||
| id: 'user123', | ||
| device: 'device456' | ||
| }, | ||
| nested: [ // Related entities | ||
| { type: 'category', data: { name: 'Electronics' } } | ||
| ], | ||
| consent: { functional: true, marketing: true }, | ||
| // System-generated fields: | ||
| id: '1647261462000-01b5e2-2', | ||
| timestamp: 1647261462000, | ||
| entity: 'product', | ||
| action: 'view' | ||
| } | ||
| ``` | ||
|
|
||
| ## Mapping System Architecture | ||
|
|
||
| ### Core Mapping Functions | ||
|
|
||
| From `/workspaces/walkerOS/packages/core/src/mapping.ts` | ||
|
|
||
| - **`getMappingEvent(event, mappingRules)`**: Finds the appropriate mapping | ||
| configuration for an event | ||
| - **`getMappingValue(value, mappingConfig)`**: Transforms values using flexible | ||
| mapping strategies | ||
|
|
||
| ### Event Mapping Patterns | ||
|
|
||
| ```typescript | ||
| const mapping = { | ||
| // Exact entity-action match | ||
| product: { | ||
| view: { name: 'view_item' }, | ||
| add: { name: 'add_to_cart' }, | ||
| }, | ||
|
|
||
| // Wildcard patterns | ||
| product: { | ||
| '*': { name: 'product_interaction' }, // Matches any action | ||
| }, | ||
| '*': { | ||
| click: { name: 'generic_click' }, // Matches any entity | ||
| }, | ||
| ├── core/ # Types, utilities, schemas (@walkeros/core) | ||
| ├── collector/ # Event processing engine | ||
| ├── config/ # Shared tooling config | ||
| ├── web/ # Browser: sources/, destinations/ | ||
| └── server/ # Node.js: sources/, destinations/ | ||
| // Conditional mappings | ||
| order: { | ||
| complete: [ | ||
| { | ||
| condition: (event) => event.data?.value > 100, | ||
| name: 'high_value_purchase', | ||
| }, | ||
| { name: 'purchase' }, // Fallback | ||
| ], | ||
| }, | ||
| }; | ||
| apps/ | ||
| ├── quickstart/ # Validated examples (source of truth) | ||
| ├── walkerjs/ # Browser bundle | ||
| └── demos/ # Demo applications | ||
| ``` |
There was a problem hiding this comment.
Annotate the package/app tree code fence with a language to satisfy markdownlint.
The packages/apps tree block uses a bare code fence, which trips MD040. Consider:
-```
+```text
packages/
├── core/ # Types, utilities, schemas (@walkeros/core)
...
└── server/ # Node.js: sources/, destinations/That should keep the diagram readable and make markdownlint happy.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In AGENT.md around lines 42 to 54, the package/app tree code fence is missing a
language tag which triggers markdownlint MD040; update the opening triple
backtick to include a language (for example "text") so the block becomes ```text
and leave the diagram content unchanged to satisfy the linter while preserving
formatting and readability.
| ``` | ||
| 1. Research → Understand input format, find SDK/types | ||
| 2. Examples → Create input examples in dev entry FIRST | ||
| 3. Mapping → Define input → walkerOS event transformation | ||
| 4. Scaffold → Copy template and configure | ||
| 5. Implement → Build using examples as test fixtures | ||
| 6. Test → Verify against example variations | ||
| 7. Document → Write README | ||
| ``` |
There was a problem hiding this comment.
Add explicit languages to fenced code blocks to satisfy markdownlint (MD040).
The code fences for the process overview, mapping trace, and directory structure currently use bare ``````, which triggers MD040. Consider annotating them as text (or another appropriate language), e.g.:
-```
+```text
1. Research → Understand input format, find SDK/types
...
-```
+```Apply the same pattern for the mapping trace and the packages/server/sources/[name]/ tree block so lint passes cleanly.
Also applies to: 314-323, 342-363
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In skills/create-source/SKILL.md around lines 32-40 (and also update the other
mentioned ranges 314-323 and 342-363), the fenced code blocks are bare and
trigger markdownlint MD040; edit each triple-backtick block to include an
explicit language token (e.g., text) immediately after the opening backticks so
they read ```text instead of ```; apply the same change to the process overview,
mapping trace, and the directory tree block (packages/server/sources/[name]/) so
all fenced blocks pass MD040.
| test('handles minimal input', () => { | ||
| const result = transformInput(examples.inputs.minimalInput, config); | ||
|
|
||
| // Should handle gracefully (may return undefined or minimal event) | ||
| expect(result).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Align minimalInput example with transformInput behavior and event naming rules.
minimalInput uses event: 'ping' and the test asserts expect(result).toBeDefined(), but the example transformInput splits on a space and returns undefined when it can’t derive both entity and action. Given AGENT’s "entity action" naming non‑negotiable, either:
- Update the minimal example to a valid
"entity action"(e.g.'system ping') and keep thetoBeDefinedassertion, or - Keep
'ping'but change the test/comment to assert that the function returnsundefinedwhile handling input without throwing.
Right now, the documented expectation and the sample implementation don’t match.
Also applies to: 160-163, 375-392
🤖 Prompt for AI Agents
In skills/create-source/SKILL.md around lines 502-507 (also review lines 160-163
and 375-392), the test/example uses minimalInput with event: 'ping' but
transformInput requires "entity action" and returns undefined when it can't
derive both; update the examples/tests so they align: either change
minimalInput.event to a valid two‑word name like 'system ping' and keep
expect(result).toBeDefined(), or keep 'ping' and change the test/comment to
assert that transformInput returns undefined (e.g.,
expect(result).toBeUndefined()) and note that this is acceptable behavior; apply
the chosen change consistently to the referenced line ranges.
| # walkerOS Skills | ||
|
|
||
| Skills are the source of truth for AI assistants working with this repository. | ||
| Tool-agnostic and accessible to Claude, Cursor, Copilot, and other AI tools. | ||
|
|
||
| ## Available Skills | ||
|
|
||
| ### Concept Skills (Understanding) | ||
|
|
||
| | Skill | Description | | ||
| | ------------------------------------------------------------------- | --------------------------------------------------------- | | ||
| | [understanding-development](./understanding-development/SKILL.md) | Build, test, lint, XP principles, folder structure | | ||
| | [understanding-flow](./understanding-flow/SKILL.md) | Architecture, composability, Source→Collector→Destination | | ||
| | [understanding-events](./understanding-events/SKILL.md) | Event model, entity-action naming, properties | | ||
| | [understanding-mapping](./understanding-mapping/SKILL.md) | Event transformation, data/map/loop/condition | | ||
| | [understanding-destinations](./understanding-destinations/SKILL.md) | Destination interface, env pattern, configuration | | ||
| | [understanding-sources](./understanding-sources/SKILL.md) | Source interface, capture patterns | | ||
| | [using-logger](./using-logger/SKILL.md) | Logger access, DRY principles, API logging patterns | | ||
|
|
||
| ### Task Skills | ||
|
|
||
| | Skill | Description | | ||
| | --------------------------------------------------------- | --------------------------------------------- | | ||
| | [testing-strategy](./testing-strategy/SKILL.md) | Testing patterns, env mocking, dev examples | | ||
| | [create-destination](./create-destination/SKILL.md) | Step-by-step destination creation | | ||
| | [create-source](./create-source/SKILL.md) | Step-by-step source creation | | ||
| | [mapping-configuration](./mapping-configuration/SKILL.md) | Mapping recipes for GA4, Meta, custom APIs | | ||
| | [debugging](./debugging/SKILL.md) | Troubleshooting event flow and mapping issues | | ||
|
|
||
| ## Architecture | ||
|
|
||
| ``` | ||
| skills/ <- Primary content (tool-agnostic) | ||
| .claude/skills/ <- Claude Code references (auto-discovery) | ||
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| ### Claude Code | ||
|
|
||
| Skills in `.claude/skills/` are auto-discovered and reference primary files | ||
| here. | ||
|
|
||
| ### Other AI Tools | ||
|
|
||
| Reference skills in this directory directly. | ||
|
|
||
| ## Adding New Skills | ||
|
|
||
| 1. Create `skills/[skill-name]/SKILL.md` with full content | ||
| 2. Create `.claude/skills/[skill-name]/SKILL.md` reference | ||
| 3. Update this README | ||
| 4. Update AGENT.md navigation if needed |
There was a problem hiding this comment.
Great high‑level skills index; fix fenced code block language
The Skills README is well structured and clearly explains concepts vs. tasks and the .claude/skills indirection.
Markdownlint’s MD040 on the directory layout block can be fixed by specifying a language for the fence. For example:
-```
+```text
skills/ <- Primary content (tool-agnostic)
.claude/skills/ <- Claude Code references (auto-discovery)
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In skills/README.md around lines 1 to 53, the fenced code block under
"Architecture" lacks a language identifier which triggers markdownlint MD040;
update the opening fence from ``` to ```text (keeping the same block content and
the closing ```), so the block is explicitly marked as plain text and the linter
warning is resolved.
| ``` | ||
| packages/ | ||
| ├── core/ # Platform-agnostic types, utilities, schemas | ||
| ├── collector/ # Central event processing engine | ||
| ├── config/ # Shared config (eslint, jest, tsconfig, tsup) | ||
| ├── web/ | ||
| │ ├── core/ # Web-specific utilities | ||
| │ ├── sources/ # browser, dataLayer | ||
| │ └── destinations/ # gtag, meta, api, piwikpro, plausible | ||
| └── server/ | ||
| ├── core/ # Server-specific utilities | ||
| ├── sources/ # gcp | ||
| └── destinations/ # aws, gcp, meta | ||
| apps/ | ||
| ├── walkerjs/ # Ready-to-use browser bundle | ||
| ├── quickstart/ # Code examples (source of truth for patterns) | ||
| └── demos/ # Demo applications | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code block at line 44 defining folder structure is missing a language identifier. Since it shows a text-based tree structure, mark it as text or plaintext for proper rendering.
Apply this diff:
## Folder Structure
-```
+```text
packages/
├── core/ # Platform-agnostic types, utilities, schemas🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In skills/understanding-development/SKILL.md around lines 44 to 62, the fenced
code block showing the folder tree is missing a language identifier; update the
opening fence from ``` to ```text (or ```plaintext) so the block is explicitly
marked as plain text for proper rendering — replace the opening triple backticks
with ```text and keep the rest of the block unchanged.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
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 (3)
packages/cli/src/core/docker.ts (2)
165-174: Callingprocess.exit()inside a Promise breaks caller expectations.The function signature returns
Promise<void>, but on failure it callsprocess.exit()instead of rejecting. This:
- Prevents callers from catching errors or performing cleanup
- Makes the function untestable for failure cases
- Breaks composability - any wrapper/retry logic is bypassed
If the intent is to avoid duplicate error messages, reject with a marker error that the CLI entry point can handle silently.
proc.on('exit', (code) => { if (code === 0) { resolve(); } else { - // Docker already logged the error via stdio inherit - // Just exit with same code - no duplicate message - process.exit(code || 1); + // Docker already logged the error via stdio inherit + // Reject with exit code for caller to handle + const error = new Error(`Docker exited with code ${code}`); + (error as any).exitCode = code || 1; + (error as any).silent = true; // Signal to CLI not to log again + reject(error); } });
324-332: Same issue:process.exit()inside Promise executor.This has the same problem as
executeInDocker. Apply the same fix to reject with an error object containing the exit code and a silent flag.proc.on('exit', (code) => { if (code === 0) { resolve(); } else { - // Docker already logged the error via stdio inherit - // Just exit with same code - no duplicate message - process.exit(code || 1); + const error = new Error(`Docker exited with code ${code}`); + (error as any).exitCode = code || 1; + (error as any).silent = true; + reject(error); } });packages/collector/src/destination.ts (1)
129-130: Bug:.concat()result is discarded—skipped events are lost.
Array.prototype.concat()returns a new array and does not mutate the original. The skipped events are never actually added back to the queue.Apply this fix:
// Add skipped events back to the queue - destination.queue.concat(skippedEvents); + destination.queue.push(...skippedEvents);
🧹 Nitpick comments (4)
apps/demos/flows/browser-to-bigquery.json (1)
1-104: Well-structured flow configuration with appropriate demo defaults.The flow configuration correctly defines web and server flows with proper source and destination wiring. Variable references are consistent (COLLECT_URL on line 32, GCP_PROJECT_ID on line 85), and the credentials path (line 90) aligns with the setup instructions in README.md.
Note: The hardcoded GCP_PROJECT_ID and relative credentials path are appropriate for this demo context, but would need parameterization for production use.
Consider externalizing the GCP_PROJECT_ID from the demo flow file into environment variables or a separate .env file for better security and reusability across environments:
"projectId": "${GCP_PROJECT_ID}",This would allow developers to inject their own project IDs without modifying the flow configuration directly.
packages/web/destinations/gtag/src/index.ts (1)
116-119: Consider adding runtime validation for type assertions.The type assertions on lines 118-119 assume
context.dataconforms toWalkerOS.Consentandcontext.config?.settingsconforms toPartial<Settings>, but there's no runtime validation. If the data doesn't match these types, accessing properties later (e.g., in theObject.entries(consent)loop on line 161) could cause runtime errors.Consider adding validation or using type guards to ensure the data structure matches expectations before proceeding.
Example validation approach:
if (type !== 'consent' || !context.data) return; const consent = context.data as WalkerOS.Consent; +if (!consent || typeof consent !== 'object') { + context.logger?.log('Invalid consent data structure'); + return; +} + const settings = (context.config?.settings || {}) as Partial<Settings>;packages/collector/src/destination-code.ts (1)
20-32: Consider returning a value to indicate success/failure.The
pushmethod returnsundefinedin all cases. Other destinations may return a result to indicate success. While the current implementation handles errors gracefully via logging, returning a boolean or result object could help callers understand if the push succeeded.push(event, context) { const { mapping, config, logger } = context; const pushCode = (mapping as CodeMapping | undefined)?.push ?? (config.settings as Settings | undefined)?.push; - if (!pushCode) return; + if (!pushCode) return true; // No code to execute, consider successful try { const fn = new Function('event', 'context', pushCode); fn(event, context); + return true; } catch (e) { logger.error('Code destination push error:', e); + return false; } },packages/collector/src/types/code.ts (1)
3-8: Settings interface allows arbitrary code strings.The
Settingsinterface defines string fields for executable code. This design enables the code destination but inherently carries security implications. Consider adding JSDoc comments to warn developers that these strings will be executed as JavaScript.+/** + * Settings for the code destination. + * WARNING: These strings are executed as JavaScript code. + * Only use with trusted, validated input. + */ export interface Settings { + /** JavaScript code to execute during initialization */ init?: string; + /** JavaScript code to execute on lifecycle events */ on?: string; + /** JavaScript code to execute on push */ push?: string; + /** JavaScript code to execute on batch push */ pushBatch?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.changeset/huge-pens-lay.md(1 hunks).devcontainer/devcontainer.json(2 hunks)apps/demos/flows/.gitignore(1 hunks)apps/demos/flows/README.md(1 hunks)apps/demos/flows/browser-to-bigquery.json(1 hunks)apps/demos/flows/index.html(1 hunks)apps/demos/flows/shared/credentials/.gitkeep(1 hunks)packages/cli/src/commands/run/execution.ts(3 hunks)packages/cli/src/core/docker.ts(2 hunks)packages/cli/src/index.ts(2 hunks)packages/collector/src/__tests__/destination-code.test.ts(1 hunks)packages/collector/src/__tests__/destination.test.ts(9 hunks)packages/collector/src/destination-code.ts(1 hunks)packages/collector/src/destination.ts(7 hunks)packages/collector/src/index.ts(1 hunks)packages/collector/src/on.ts(2 hunks)packages/collector/src/types/code.ts(1 hunks)packages/collector/src/types/index.ts(1 hunks)packages/core/src/types/destination.ts(1 hunks)packages/core/src/types/on.ts(2 hunks)packages/docker/package.json(1 hunks)packages/docker/src/index.ts(5 hunks)packages/docker/src/services/runner.ts(5 hunks)packages/docker/src/services/serve.ts(5 hunks)packages/docker/src/version.ts(1 hunks)packages/web/destinations/gtag/src/__tests__/index.test.ts(9 hunks)packages/web/destinations/gtag/src/index.ts(2 hunks)website/docs/collector/index.mdx(1 hunks)website/docs/destinations/code.mdx(1 hunks)website/docs/destinations/index.mdx(1 hunks)website/docs/getting-started/flow.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- apps/demos/flows/.gitignore
- .changeset/huge-pens-lay.md
- website/docs/destinations/code.mdx
- apps/demos/flows/shared/credentials/.gitkeep
- packages/collector/src/tests/destination-code.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/collector/src/index.tspackages/collector/src/types/index.tspackages/cli/src/index.tspackages/web/destinations/gtag/src/index.tspackages/docker/src/version.tspackages/docker/src/services/serve.tspackages/cli/src/commands/run/execution.tspackages/collector/src/destination-code.tspackages/core/src/types/on.tspackages/docker/src/services/runner.tspackages/collector/src/on.tspackages/core/src/types/destination.tspackages/collector/src/types/code.tspackages/collector/src/destination.tspackages/cli/src/core/docker.tspackages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.tspackages/docker/src/index.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENT.md)
packages/*/src/**/*.ts: Use Higher-Order Functions (HOF) and smart abstractions to avoid redundancy and keep implementations lean and performant
Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Files:
packages/collector/src/index.tspackages/collector/src/types/index.tspackages/cli/src/index.tspackages/docker/src/version.tspackages/docker/src/services/serve.tspackages/cli/src/commands/run/execution.tspackages/collector/src/destination-code.tspackages/core/src/types/on.tspackages/docker/src/services/runner.tspackages/collector/src/on.tspackages/core/src/types/destination.tspackages/collector/src/types/code.tspackages/collector/src/destination.tspackages/cli/src/core/docker.tspackages/collector/src/__tests__/destination.test.tspackages/docker/src/index.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
packages/collector/src/index.tspackages/web/destinations/gtag/src/index.tswebsite/docs/collector/index.mdxwebsite/docs/destinations/index.mdxpackages/collector/src/destination-code.tspackages/core/src/types/on.tspackages/collector/src/on.tspackages/core/src/types/destination.tspackages/collector/src/types/code.tswebsite/docs/getting-started/flow.mdxpackages/collector/src/destination.tspackages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Consent is optional and configurable at multiple levels: destination-level, event mapping level, value mapping level, and policy level - without explicit consent requirements, events process normally
Applied to files:
packages/web/destinations/gtag/src/index.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
packages/docker/src/version.tspackages/core/src/types/on.tspackages/collector/src/on.tspackages/collector/src/types/code.tspackages/docker/src/index.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Use the Source → Collector → Destination(s) architecture pattern for event processing: Sources capture events from various sources (browser DOM, dataLayer, server), Collector handles central event processing with consent management, Destinations transform and send events to third-party tools
Applied to files:
apps/demos/flows/index.htmlwebsite/docs/collector/index.mdxwebsite/docs/destinations/index.mdxpackages/collector/src/on.tswebsite/docs/getting-started/flow.mdxpackages/collector/src/destination.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: All walkerOS components communicate via push functions: sources use source.push(), collector uses collector.push(), destinations use destination.push(), and ELB uses elb() as the alias for collector.push()
Applied to files:
packages/core/src/types/on.tspackages/collector/src/on.tspackages/collector/src/types/code.tspackages/collector/src/destination.tspackages/collector/src/__tests__/destination.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Applied to files:
packages/collector/src/on.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
packages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
packages/collector/src/__tests__/destination.test.tspackages/web/destinations/gtag/src/__tests__/index.test.ts
🧬 Code graph analysis (9)
packages/collector/src/types/index.ts (1)
packages/core/src/types/destination.ts (1)
Code(97-97)
packages/web/destinations/gtag/src/index.ts (2)
packages/web/destinations/gtag/src/types/index.ts (1)
Settings(62-71)packages/web/core/src/environment.ts (1)
getEnv(11-17)
packages/docker/src/version.ts (1)
packages/docker/src/index.ts (1)
VERSION(10-10)
packages/docker/src/services/serve.ts (3)
packages/docker/src/index.ts (3)
runServeMode(116-116)ServeConfig(116-116)VERSION(10-10)packages/cli/src/core/logger.ts (1)
Logger(11-20)packages/docker/src/version.ts (1)
VERSION(7-8)
packages/cli/src/commands/run/execution.ts (3)
packages/core/src/logger.ts (1)
Level(213-213)packages/docker/src/services/runner.ts (1)
runFlow(24-97)packages/docker/src/services/serve.ts (1)
runServeMode(19-94)
packages/collector/src/destination-code.ts (3)
packages/core/src/types/destination.ts (2)
Instance(59-69)Settings(49-49)packages/collector/src/types/code.ts (2)
Settings(3-8)CodeMapping(10-13)packages/cli/demos/programmatic-example.js (1)
event(54-60)
packages/core/src/types/on.ts (4)
packages/collector/src/types/code.ts (3)
OnFn(23-23)Types(15-15)Context(17-17)packages/core/src/types/destination.ts (3)
TypesGeneric(39-44)Types(29-34)Context(113-119)packages/core/src/types/source.ts (2)
TypesGeneric(52-58)Types(35-47)packages/core/src/types/walkeros.ts (1)
PromiseOrValue(106-106)
packages/docker/src/services/runner.ts (1)
packages/cli/src/core/logger.ts (1)
Logger(11-20)
packages/docker/src/index.ts (4)
packages/core/src/logger.ts (1)
Level(213-213)packages/docker/src/version.ts (1)
VERSION(7-8)packages/docker/src/services/runner.ts (1)
runFlow(24-97)packages/docker/src/services/serve.ts (1)
runServeMode(19-94)
🪛 markdownlint-cli2 (0.18.1)
apps/demos/flows/README.md
32-32: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (45)
website/docs/getting-started/flow.mdx (2)
237-256: Documentation update is clear and well-positioned.The addition effectively introduces the built-in Code Destination with a concrete example demonstrating
code: trueconfiguration and string-based push handlers. The placement logically extends the Destinations section.
242-254: Minor inconsistency: Event property access differs across examples.This example accesses
event.nameandevent.data, while the collector documentation (line 81 of collector/index.mdx) uses justevent. Verify whether this is intentional to demonstrate different use patterns or if it should be standardized across all examples for consistency.website/docs/destinations/index.mdx (1)
41-47: Well-structured categorization of destinations by scope.The new "Built-in Destinations" section logically precedes platform-specific destinations and introduces a clear taxonomy. Confirm that the linked documentation (
/docs/destinations/code) exists in the PR and is properly documented.Verify that
website/docs/destinations/code.mdxexists in this PR and contains comprehensive documentation of the Code Destination including:
- Configuration options (settings, mapping, data, consent, policy)
- String-based push handler examples
- any init() or on() context handlers
- Error handling behavior
website/docs/collector/index.mdx (2)
76-85: Update successfully demonstrates simplified setup using built-in Code Destination.The migration from an imported destination to
code: truewith string-based push handler is clear and aligns with the feature goal. The inline comment aids maintainability.
78-82: Verify consistent documentation of event object structure in Code Destination guide.This example accesses the full
eventobject, while the flow.mdx example (line 248) accessesevent.nameandevent.data. Ensure the Code Destination documentation (code.mdx) clarifies what properties are available on the event object and whether these examples reflect different valid patterns or if one should be standardized..devcontainer/devcontainer.json (3)
20-20: Clarify intent: both ChatGPT and Claude Code extensions.The configuration adds both
openai.chatgptand installs@anthropic-ai/claude-codeglobally. Given the PR focus on Claude/skill metadata and AI readiness, verify this is intentional and not a duplicate/competing tooling setup. If the intent is Claude-only, remove the ChatGPT extension.
42-43: Resource constraints are reasonable.The memory (8g with 10g swap) and CPU (4) limits are appropriate for development and should provide sufficient headroom for builds and testing without constraining performance excessively.
46-46: LGTM: postCreateCommand changes.The removal of
@openai/codexand retention of@anthropic-ai/claude-codealigns with the Claude Code focus. The build step is properly retained.packages/web/destinations/gtag/src/__tests__/index.test.ts (1)
429-435: LGTM! Consistent refactoring to structured context.All consent handler test calls have been consistently updated to pass a full context object (collector, config, data, env, logger) instead of just consent data directly. This aligns with the broader architectural change to provide richer runtime context to destination handlers.
The tests maintain their coverage and correctly verify gtag consent behavior with the new API structure.
Also applies to: 466-472, 513-519, 539-545, 574-580, 627-633, 659-665, 692-698, 728-734
apps/demos/flows/index.html (1)
1-28: Well-structured HTML demo UI with sound event architecture.The HTML correctly implements the demo UI with properly-formed event payloads and correct script loading order. The event flow description aligns with the architecture pattern. No issues identified.
packages/web/destinations/gtag/src/index.ts (2)
126-127: LGTM - consistent use of context environment.The change to use
context.envinstead of instance-level environment aligns with the refactoring goal of accessing properties through the context parameter. The use ofgetEnvfrom@walkeros/web-coreis appropriate.
161-171: Clean iteration pattern.The refactored consent mapping logic using
Object.entriesis cleaner and more direct. The handling of both array and single string gtag parameters is appropriate.packages/docker/package.json (1)
30-34: LGTM!Adding
@walkeros/coreas a runtime dependency is appropriate since the docker package now usesLoggertypes and utilities from core at runtime (in runner.ts and serve.ts).packages/cli/src/index.ts (1)
63-71: LGTM!The startup banner with version information is well-implemented. Correctly skips for
--silentand--jsonto avoid interfering with machine-readable output.packages/docker/src/version.ts (1)
1-8: LGTM!Clean implementation of build-time version injection. The
typeofcheck correctly handles cases where__VERSION__isn't defined (e.g., during development), falling back to a sensible default.packages/cli/src/commands/run/execution.ts (1)
1-7: LGTM!Good use of
createLoggerfrom@walkeros/coreas recommended in the coding guidelines. The scoped loggers (logger.scope('runner')andlogger.scope('serve')) provide clear context for log output.packages/docker/src/services/serve.ts (3)
19-22: LGTM!Good use of
import typefor theLoggertype import, following the coding guidelines. The function signature update with the logger parameter enables consistent structured logging throughout the serve mode.
52-61: Good observability enhancement.Including
VERSIONin the health response is excellent for debugging and monitoring. The comprehensive health payload (version, timestamp, mode, file, url) provides valuable runtime introspection.
84-92: Clean shutdown and error handling.The graceful shutdown pattern with signal handlers is well-implemented. Error handling correctly extracts the message and logs before exiting, which is appropriate for a top-level server entry point.
packages/docker/src/index.ts (7)
3-4: LGTM!Correct use of
import typefor type-only imports from @walkeros/core, following TypeScript best practices for tree-shaking.
7-10: LGTM!Centralizing VERSION in a dedicated module and re-exporting for external consumers is good practice for maintainability.
12-14: LGTM!Environment-based log level configuration is appropriate for runtime containers, providing users control over verbosity.
99-112: LGTM!Process error handlers correctly extract error messages and conditionally log stack traces at debug level. The implementation is consistent with the main error handler.
115-116: LGTM!Public API exports align with the updated function signatures and correctly use type-only re-exports for configuration types.
57-60: No action needed — logger.throw() correctly returns never.The logger.throw() method is properly typed to return
neverand always throws an exception. The return statement on line 60 is correct TypeScript idiom for type narrowing and will never be reached, as indicated by the comment. This pattern is appropriate and the code follows best practices.
89-93: No issue found - logger.debug() supports object parameters.The
LogFntype signature acceptscontext?: unknown | Erroras the second parameter, and thenormalizeParams()function explicitly handles object parameters by spreading them into the normalized context. The pattern used at lines 92, 101, and 109 is valid and fully supported.Likely an incorrect or invalid review comment.
packages/docker/src/services/runner.ts (4)
22-28: LGTM!Function signature correctly updated to accept logger parameter with proper JSDoc documentation. The signature aligns with the calling code in index.ts.
29-29: LGTM!Appropriate use of logger.info for status messages with clear, informative output.
Also applies to: 60-63
67-67: LGTM!Logger usage with appropriate levels: info for normal lifecycle events and error for failures.
Also applies to: 74-74, 78-78, 91-91
77-78: LGTM!Error handling correctly extracts messages from Error objects and other types, with conditional stack trace logging at debug level. The implementation is consistent with the main error handler in index.ts.
Also applies to: 90-94
packages/core/src/types/destination.ts (1)
97-103: Clean type abstraction for code-based destinations.The
Code<T>union type provides a flexible way to specify destinations—either as a fullInstance<T>or astruefor the built-in code destination. This maintains type safety while enabling the shorthand pattern used inresolveCode()within the collector package.packages/collector/src/destination.ts (3)
16-18: Clean resolver pattern for code-based destinations.The
resolveCodehelper provides a straightforward way to normalize theCode<T>union type, enabling the shorthandcode: truesyntax while maintaining a consistent internal representation.
235-236: Debug logging additions are consistent and well-structured.The logging follows a consistent pattern: scoped by destination type, using
debuglevel, and including relevant context (event count for batches, event name for single pushes). This aids observability without cluttering production logs.Also applies to: 251-252, 317-319, 327-328, 336-337, 344-345
410-428: LGTM!The
mergeEnvironmentsexport is well-implemented with proper handling of edge cases. The precedence logic (config env overrides destination env) is clearly documented.packages/collector/src/index.ts (1)
9-9: LGTM!The new
destination-codeexport follows the established pattern and is logically grouped with related destination exports.packages/collector/src/types/index.ts (1)
2-2: Namespace export for Code types.The
Codenamespace export groups the code-related types cleanly. Note thatDestination.Code(from@walkeros/core) is a type alias (Instance<T> | true), while thisCodenamespace contains the implementation types (Settings, OnFn, etc.)—the distinction is clear but worth being aware of when importing.packages/core/src/types/on.ts (1)
75-84: OnFn signature provides type-safe lifecycle event handling with complete context.The updated signature correctly implements
(type: Types, context: Destination.Context<T>), enabling destinations to handle lifecycle events (consent, session, ready, run) with full access to collector state, configuration, environment, and logger. Generic typing withT extends Destination.TypesGenericensures type safety. Tests confirm proper integration with context containingdata,collector,config,env, andlogger. Sinceonis optional in the Destination interface, implementations vary by destination need.packages/collector/src/on.ts (1)
81-95: Well-structured destination context with scoped logging.The implementation correctly builds a rich
Destination.Contextwith proper logger scoping for observability. The chainscope(destType).scope('on').scope(type)provides excellent traceability for debugging destination-specific on-event issues.packages/collector/src/__tests__/destination.test.ts (3)
44-47: Proper mock logger setup with scope chaining.The mock logger setup correctly simulates the scope chaining behavior, allowing tests to verify that lifecycle methods properly scope their loggers.
123-163: Good lifecycle logging tests.These tests verify that init and push operations properly log their lifecycle events with the scoped logger. The assertions confirm both the scope call with destination type and the debug messages for start/end of each lifecycle.
205-214: Updated context expectations align with on.ts changes.The test expectations now correctly verify the full structured context object passed to destination
onmethods, includingdata,collector,config,env, andloggerfields. This aligns with the changes inon.ts.packages/collector/src/types/code.ts (2)
1-1: Correct use ofimport typefor type-only imports.Following the coding guidelines, the file correctly uses
import typefor type-only imports, ensuring proper TypeScript compilation and tree-shaking.
10-28: Well-structured type definitions following core patterns.The type aliases and function signatures are well-defined, properly leveraging the generic
Destination.Typespattern from@walkeros/core. This enables type-safe usage of the code destination throughout the codebase.packages/collector/src/destination-code.ts (2)
48-58: Theonmethod signature is intentional and correctly implements the coreOn.OnFntype, which explicitly specifies baseDestination.Contextrather than specialized contexts. This design is appropriate becauseon()serves as a generic event handler for multiple event types (consent, session, ready, run) rather than a push-specific operation. The pattern is consistent across all destination implementations.
4-18: The code destination is an intentional feature, not a security vulnerability.This destination is explicitly designed for developers to execute custom code within the collector. Code strings come from application configuration (not untrusted input), and developers opt-in by setting
code: true. The use ofnew Function()is appropriate here—this is similar to accepting plugins or custom handlers. Whilenew Function()does execute arbitrary code, the threat model assumes trusted, developer-controlled configuration sources, not external or user-supplied input.Likely an incorrect or invalid review comment.
|
|
||
| // Persist Claude Code data across container rebuilds (uses existing .claude folder in repo) | ||
| "containerEnv": { | ||
| "CLAUDE_CONFIG_DIR": "/workspaces/walkerOS/.claude/data" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t d "\.claude" --max-depth 3Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
# Check the entire devcontainer.json file to see postCreateCommand and other build steps
cat -n .devcontainer/devcontainer.jsonRepository: elbwalker/walkerOS
Length of output: 2149
🏁 Script executed:
# Search for any references to .claude or claude initialization in build/setup scripts
rg -r "\.claude" --type json --type sh --type bashRepository: elbwalker/walkerOS
Length of output: 458
🏁 Script executed:
cat -n .gitignore | grep -A 2 -B 2 "claude" || echo "No 'claude' references in .gitignore"Repository: elbwalker/walkerOS
Length of output: 164
🏁 Script executed:
# Search for any documentation or setup scripts that might initialize .claude
find . -type f \( -name "*.md" -o -name "setup*" -o -name "init*" \) | head -20 | xargs grep -l "claude" 2>/dev/null || echo "No setup/init files reference claude"Repository: elbwalker/walkerOS
Length of output: 73
🏁 Script executed:
# Check if .claude is in .gitignore at all
cat .gitignore 2>/dev/null | head -50Repository: elbwalker/walkerOS
Length of output: 879
🏁 Script executed:
# Check CLAUDE.md for any setup guidance
head -50 CLAUDE.mdRepository: elbwalker/walkerOS
Length of output: 485
Create .claude/data directory during container initialization.
The CLAUDE_CONFIG_DIR environment variable is set to /workspaces/walkerOS/.claude/data, but the postCreateCommand does not create this directory. While the devcontainer comment references "existing .claude folder in repo," .claude/data/ is in .gitignore and does not exist. Add directory creation to postCreateCommand to ensure Claude Code can initialize its configuration:
"postCreateCommand": "mkdir -p .claude/data && npm install && npm install -g @anthropic-ai/claude-code && npm run build"
🤖 Prompt for AI Agents
In .devcontainer/devcontainer.json around lines 9 to 13, the CLAUDE_CONFIG_DIR
points to /workspaces/walkerOS/.claude/data but the postCreateCommand does not
create that directory, so add directory creation to ensure the path exists
before installing and building; update postCreateCommand to run a mkdir -p
.claude/data (or equivalent cross-platform creation) first, then run the
existing npm install, globally install @anthropic-ai/claude-code, and npm run
build in the same command sequence.
| npx serve . -p 3000 | ||
| ``` | ||
|
|
||
| 5. Open http://localhost:3000 and click buttons |
There was a problem hiding this comment.
Fix bare URL per markdownlint MD034 rule.
The bare URL on line 32 should be wrapped in angle brackets or formatted as a markdown link for compliance.
Apply this diff:
-5. Open http://localhost:3000 and click buttons
+5. Open <http://localhost:3000> and click buttons📝 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.
| 5. Open http://localhost:3000 and click buttons | |
| 5. Open <http://localhost:3000> and click buttons |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
32-32: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In apps/demos/flows/README.md around line 32, the bare URL
"http://localhost:3000" violates markdownlint MD034; update the line to wrap the
URL in angle brackets or convert it to a markdown link (for example:
<http://localhost:3000> or [http://localhost:3000](http://localhost:3000)) so
the README complies with MD034.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
packages/server/sources/aws/examples/basic-handler.ts (1)
16-22: Consider a type alias for the handler signature.The explicit function type is verbose. A type alias would improve readability and reusability.
+// Handler type for reuse +type LambdaHandler = ( + event: APIGatewayProxyEvent, + context: Context, +) => Promise<APIGatewayProxyResult>; + // Handler singleton - reused across warm invocations -let handler: - | (( - event: APIGatewayProxyEvent, - context: Context, - ) => Promise<APIGatewayProxyResult>) - | null = null; +let handler: LambdaHandler | null = null;packages/server/sources/aws/src/lambda/types.ts (1)
23-25: Convert empty interface to type alias per static analysis.An empty interface is equivalent to
{}. Use a type alias for clarity, or add a comment explaining this is a placeholder for future mapping properties.-export interface Mapping { - // Custom source event mapping properties -} +// Placeholder for custom source event mapping properties +export type Mapping = Record<string, never>;packages/server/sources/aws/src/lambda/__tests__/index.test.ts (1)
871-888: Consider testing the specific error message for invalid timeout.The test verifies that an invalid timeout throws, but doesn't assert on the error message. This could help catch regressions in validation messaging.
await expect( sourceLambda( { settings: { timeout: 999999999, // Exceeds 900000ms Lambda limit }, }, { push: mockPush as never, command: mockCommand as never, elb: jest.fn() as never, logger: mockLogger, }, ), - ).rejects.toThrow(); + ).rejects.toThrow(/timeout/i);packages/server/sources/aws/src/lambda/utils.ts (1)
16-54: Consider extracting header normalization to reduce duplication.The header normalization logic (lines 18-23 and 32-37) is duplicated between the v1 and v2 branches.
+function normalizeHeaders(headers: Record<string, string | undefined> | undefined): Record<string, string> { + const normalized: Record<string, string> = {}; + if (headers) { + Object.entries(headers).forEach(([key, value]) => { + if (value) normalized[key.toLowerCase()] = value; + }); + } + return normalized; +} + export function parseEvent(event: LambdaEvent): ParsedRequest { if (isAPIGatewayV2(event)) { - const headers: Record<string, string> = {}; - if (event.headers) { - Object.entries(event.headers).forEach(([key, value]) => { - if (value) headers[key.toLowerCase()] = value; - }); - } + const headers = normalizeHeaders(event.headers); return { // ... }; } else { - const headers: Record<string, string> = {}; - if (event.headers) { - Object.entries(event.headers).forEach(([key, value]) => { - if (value) headers[key.toLowerCase()] = value; - }); - } + const headers = normalizeHeaders(event.headers); // ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/server/sources/aws/CHANGELOG.md(1 hunks)packages/server/sources/aws/README.md(1 hunks)packages/server/sources/aws/examples/README.md(1 hunks)packages/server/sources/aws/examples/basic-handler.ts(1 hunks)packages/server/sources/aws/examples/cdk-stack.ts(1 hunks)packages/server/sources/aws/examples/sam-template.yaml(1 hunks)packages/server/sources/aws/examples/serverless.yml(1 hunks)packages/server/sources/aws/jest.config.mjs(1 hunks)packages/server/sources/aws/package.json(1 hunks)packages/server/sources/aws/src/dev.ts(1 hunks)packages/server/sources/aws/src/index.ts(1 hunks)packages/server/sources/aws/src/lambda/__tests__/index.test.ts(1 hunks)packages/server/sources/aws/src/lambda/examples/env.ts(1 hunks)packages/server/sources/aws/src/lambda/examples/events.ts(1 hunks)packages/server/sources/aws/src/lambda/examples/index.ts(1 hunks)packages/server/sources/aws/src/lambda/examples/inputs.ts(1 hunks)packages/server/sources/aws/src/lambda/examples/outputs.ts(1 hunks)packages/server/sources/aws/src/lambda/index.ts(1 hunks)packages/server/sources/aws/src/lambda/push.ts(1 hunks)packages/server/sources/aws/src/lambda/schemas/index.ts(1 hunks)packages/server/sources/aws/src/lambda/schemas/primitives.ts(1 hunks)packages/server/sources/aws/src/lambda/schemas/settings.ts(1 hunks)packages/server/sources/aws/src/lambda/types.ts(1 hunks)packages/server/sources/aws/src/lambda/utils.ts(1 hunks)packages/server/sources/aws/tsconfig.json(1 hunks)packages/server/sources/aws/tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/server/sources/aws/CHANGELOG.md
- packages/server/sources/aws/examples/README.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/server/sources/aws/src/lambda/examples/index.tspackages/server/sources/aws/src/lambda/types.tspackages/server/sources/aws/src/lambda/schemas/settings.tspackages/server/sources/aws/src/index.tspackages/server/sources/aws/tsup.config.tspackages/server/sources/aws/src/lambda/push.tspackages/server/sources/aws/src/lambda/examples/events.tspackages/server/sources/aws/src/lambda/examples/outputs.tspackages/server/sources/aws/src/lambda/utils.tspackages/server/sources/aws/src/lambda/schemas/primitives.tspackages/server/sources/aws/src/lambda/examples/inputs.tspackages/server/sources/aws/examples/cdk-stack.tspackages/server/sources/aws/src/lambda/examples/env.tspackages/server/sources/aws/src/dev.tspackages/server/sources/aws/src/lambda/__tests__/index.test.tspackages/server/sources/aws/src/lambda/index.tspackages/server/sources/aws/src/lambda/schemas/index.tspackages/server/sources/aws/examples/basic-handler.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/server/sources/aws/src/lambda/__tests__/index.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Use Higher-Order Functions (HOF) and smart abstractions to avoid redundancy and keep implementations lean and performant
Applied to files:
packages/server/sources/aws/src/lambda/examples/index.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
packages/server/sources/aws/src/lambda/types.tspackages/server/sources/aws/src/lambda/push.tspackages/server/sources/aws/tsconfig.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
packages/server/sources/aws/src/lambda/types.tspackages/server/sources/aws/src/lambda/push.tspackages/server/sources/aws/src/lambda/examples/events.tspackages/server/sources/aws/src/lambda/utils.tspackages/server/sources/aws/src/lambda/schemas/primitives.tspackages/server/sources/aws/src/lambda/examples/inputs.tspackages/server/sources/aws/tsconfig.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: All walkerOS components communicate via push functions: sources use source.push(), collector uses collector.push(), destinations use destination.push(), and ELB uses elb() as the alias for collector.push()
Applied to files:
packages/server/sources/aws/src/lambda/types.tspackages/server/sources/aws/src/lambda/push.tspackages/server/sources/aws/examples/sam-template.yamlpackages/server/sources/aws/examples/cdk-stack.tspackages/server/sources/aws/src/lambda/examples/env.tspackages/server/sources/aws/README.md
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/tsconfig.json : Configure TypeScript with strict settings to enforce type safety and prevent use of `any` types
Applied to files:
packages/server/sources/aws/tsup.config.tspackages/server/sources/aws/tsconfig.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use `import type` for type-only imports to ensure proper TypeScript compilation and tree-shaking
Applied to files:
packages/server/sources/aws/tsup.config.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Applied to files:
packages/server/sources/aws/src/lambda/examples/events.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Applied to files:
packages/server/sources/aws/src/lambda/examples/events.tspackages/server/sources/aws/src/lambda/examples/inputs.tspackages/server/sources/aws/tsconfig.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
packages/server/sources/aws/src/lambda/examples/events.tspackages/server/sources/aws/src/lambda/examples/outputs.tspackages/server/sources/aws/src/lambda/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:24:48.032Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T16:24:48.032Z
Learning: Refer to AGENT.md for all relevant instructions and guidelines when working with the walkerOS codebase
Applied to files:
packages/server/sources/aws/examples/sam-template.yamlpackages/server/sources/aws/examples/cdk-stack.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/server/sources/aws/src/lambda/examples/outputs.tspackages/server/sources/aws/src/lambda/examples/env.tspackages/server/sources/aws/src/lambda/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
packages/server/sources/aws/src/lambda/examples/outputs.tspackages/server/sources/aws/src/lambda/examples/env.tspackages/server/sources/aws/src/lambda/__tests__/index.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Applied to files:
packages/server/sources/aws/tsconfig.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : NEVER use `any` type - always use proper TypeScript types from walkeros/core or package-specific types
Applied to files:
packages/server/sources/aws/tsconfig.json
🧬 Code graph analysis (7)
packages/server/sources/aws/src/lambda/examples/index.ts (1)
packages/cli/demos/programmatic-example.js (1)
events(109-113)
packages/server/sources/aws/src/lambda/schemas/settings.ts (3)
packages/server/sources/aws/src/lambda/schemas/index.ts (2)
SettingsSchema(8-8)Settings(8-8)packages/server/sources/aws/src/lambda/schemas/primitives.ts (1)
CorsOptionsSchema(33-53)packages/server/sources/aws/src/lambda/types.ts (1)
Settings(17-17)
packages/server/sources/aws/src/lambda/push.ts (5)
packages/server/sources/aws/src/lambda/types.ts (1)
EventRequest(57-64)packages/server/sources/aws/src/lambda/examples/env.ts (2)
push(41-52)push(42-44)packages/web/core/src/types/collector.ts (1)
Collector(24-43)packages/cli/src/core/logger.ts (1)
Logger(11-20)packages/core/src/types/walkeros.ts (1)
OrderedProperties(87-89)
packages/server/sources/aws/src/lambda/schemas/primitives.ts (1)
packages/server/sources/aws/src/lambda/types.ts (1)
CorsOptions(18-18)
packages/server/sources/aws/src/lambda/examples/env.ts (3)
packages/core/src/types/walkeros.ts (1)
Elb(4-4)packages/cli/src/core/logger.ts (1)
Logger(11-20)packages/server/sources/aws/src/lambda/types.ts (1)
Env(33-36)
packages/server/sources/aws/src/lambda/schemas/index.ts (1)
packages/server/sources/aws/src/lambda/schemas/settings.ts (1)
SettingsSchema(7-34)
packages/server/sources/aws/examples/basic-handler.ts (2)
packages/collector/src/flow.ts (1)
startFlow(7-59)packages/server/sources/aws/src/lambda/index.ts (1)
sourceLambda(22-170)
🪛 Biome (2.1.2)
packages/server/sources/aws/src/lambda/types.ts
[error] 23-25: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 markdownlint-cli2 (0.18.1)
packages/server/sources/aws/README.md
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (30)
packages/server/sources/aws/README.md (1)
1-299: Comprehensive and well-structured documentation.The README provides thorough coverage of the AWS Lambda source including installation, multiple deployment patterns, configuration options, request/response formats, production considerations, and security guidance. The examples correctly demonstrate the walkerOS pattern of using
startFlow()with sources/destinations and the singleton handler pattern for Lambda warm invocations. The documentation aligns well with walkerOS architecture as reflected in the retrieved learnings.packages/server/sources/aws/jest.config.mjs (1)
1-3: Re-exporting shared Jest config via ESM looks correctImporting the shared Jest config and re‑exporting it as the default keeps test config DRY and consistent across packages. Please just confirm that
@walkeros/config/jestexposes a compatible default export and that this package’s Jest setup is configured to loadjest.config.mjsas an ES module.packages/server/sources/aws/tsconfig.json (1)
1-8: LGTM!The TypeScript configuration properly extends the base Node.js config and defines appropriate source/exclusion paths for the AWS Lambda source package.
packages/server/sources/aws/examples/basic-handler.ts (2)
28-52: LGTM!The singleton initialization pattern is correctly implemented for optimal Lambda warm-start performance. The guard clause prevents re-initialization, and the empty destinations object provides a clear extension point.
54-67: LGTM!The main function correctly delegates to the singleton handler, and the dual export pattern (
mainandhandler) enables both testing and Lambda runtime compatibility.packages/server/sources/aws/src/dev.ts (1)
1-1: LGTM!The namespace export provides a clean entry point for lambda development utilities, aligning with the
./devexport defined in package.json.packages/server/sources/aws/src/lambda/examples/index.ts (1)
1-4: LGTM!Clean barrel file with proper namespace re-exports. This pattern enables convenient imports like
import { env, inputs } from '.../examples'.packages/server/sources/aws/src/index.ts (1)
1-2: LGTM!Clean package entry point that provides both namespace exports and a named alias for the default export, offering flexibility for consumers.
packages/server/sources/aws/src/lambda/schemas/settings.ts (1)
1-36: LGTM!Well-structured Zod schema with:
- Sensible defaults and constraints (timeout max matches AWS Lambda's 15-minute limit)
- Flexible CORS configuration (boolean or detailed options)
- Good use of
.describe()for self-documenting schemas- Proper type inference with
z.inferpackages/server/sources/aws/src/lambda/schemas/index.ts (1)
1-11: LGTM!Clean schema barrel file with proper organization:
- Re-exports primitives and settings
- Uses
typekeyword for type-only export ofSettings- Generates JSON Schema via
zodToSchemafor documentation toolingpackages/server/sources/aws/src/lambda/examples/env.ts (1)
1-52: LGTM!Well-structured mock environment:
- Proper
import typeusage per coding guidelinescreateMockElbFnreturns correctly structuredPushResultnoopLoggerproperly implementsLogger.Instanceincluding the throwing behavior- Getters on
push/command/elbensure fresh mocks per access, which is good for test isolationpackages/server/sources/aws/src/lambda/push.ts (1)
21-28: LGTM! Good structured error handling.The error handling follows best practices with structured logging context and proper
instanceof Errorcheck for the error message extraction.packages/server/sources/aws/src/lambda/schemas/primitives.ts (2)
23-27: LGTM! Clear schema design with good documentation.The
CorsOriginschema clearly documents the allowed values. Whilez.literal('*')is technically redundant withz.string(), it explicitly communicates the wildcard intent which aids readability.
33-53: LGTM! Well-structured CORS options schema.The schema uses appropriate Zod validators (
.int(),.positive()) with descriptive labels for each field. All fields being optional aligns with typical CORS configuration patterns.packages/server/sources/aws/src/lambda/types.ts (1)
1-82: LGTM! Comprehensive type definitions for Lambda source.The type structure follows walkerOS patterns well:
- Proper separation of Lambda-specific types from core types
- Good use of Zod inference for schema-derived types
- Clear convenience aliases for Config types
packages/server/sources/aws/src/lambda/examples/events.ts (1)
9-25: LGTM! Well-structured example events.The example events correctly follow the walkerOS "entity action" naming convention and provide representative test data. Using
Partial<WalkerOS.Event>appropriately indicates these are incomplete payloads for testing.packages/server/sources/aws/src/lambda/__tests__/index.test.ts (3)
114-134: LGTM! Good test setup with proper mocking.The test setup correctly mocks the external push/command interfaces and uses
createMockLoggerfrom core. This follows the coding guidelines for testing sources with mocked collector interfaces.
374-481: LGTM! Comprehensive error handling tests.Good coverage of error scenarios including:
- Missing request body
- Invalid event format
- Processing errors
- Unsupported HTTP methods
Each test verifies both status codes and response body structure.
731-819: LGTM! Proper logging behavior verification.Tests correctly verify:
- No logging on successful requests (delegated to collector)
- Structured error logging with context on failures
- Request ID propagation in error logs
packages/server/sources/aws/src/lambda/utils.ts (3)
10-14: LGTM! Clean type guard implementation.The
isAPIGatewayV2type guard correctly checks for the version property, enabling proper type narrowing in consuming code.
64-74: LGTM! Robust body parsing with base64 support.The function safely handles:
- Non-string inputs (passthrough)
- Base64 decoding when flagged
- JSON parse failures (returns original string)
146-166: LGTM! Well-implemented pixel tracking response.The 1x1 transparent GIF is correctly base64-encoded with appropriate cache-control headers to prevent caching of tracking pixels.
packages/server/sources/aws/src/lambda/index.ts (7)
1-20: LGTM! Clean imports and exports following TypeScript best practices.The use of
import typefor type-only imports ensures proper tree-shaking, and the namespace re-exports provide a clean API surface.
22-34: LGTM! Proper configuration validation with schema parsing.Settings validation happens synchronously before the Lambda handler is created, ensuring invalid configurations are caught early at initialization time rather than during request processing.
35-62: LGTM! Health check and CORS preflight handling are correctly implemented.The health check provides useful diagnostic information, and the OPTIONS handler properly supports CORS preflight requests.
83-136: LGTM! POST event handling with proper validation and error responses.The body validation correctly rejects both falsy values and non-objects, and the
isEventRequesttype guard ensures only valid event requests are processed.
138-163: LGTM! Comprehensive error handling with structured logging.The fallback for unsupported methods and the catch block properly handle edge cases. The structured logging with context (error, requestId, method) and optional chaining for potentially undefined values demonstrate defensive programming.
165-172: LGTM! Clean return structure with default export.The returned
LambdaSourceobject provides a well-structured API, and the default export enables flexible import patterns for consumers.
64-81: Fix null handling in pixel tracking validation.The type check
typeof parsedData === 'object'will incorrectly pass fornullbecause in JavaScript,typeof null === 'object'is true. This could result inenvPushbeing called withnull.Apply this diff to explicitly exclude null:
if (parsed.queryString) { const parsedData = requestToData(parsed.queryString); - if (parsedData && typeof parsedData === 'object') { + if (parsedData && typeof parsedData === 'object' && parsedData !== null) { await envPush(parsedData); } }Alternatively, use a more robust check:
if (parsed.queryString) { const parsedData = requestToData(parsed.queryString); - if (parsedData && typeof parsedData === 'object') { + if (parsedData && isObject(parsedData)) { await envPush(parsedData); } }Based on coding guidelines, prefer using the
isObject()helper from@walkeros/corefor type checking.Likely an incorrect or invalid review comment.
packages/server/sources/aws/src/lambda/examples/inputs.ts (1)
1-144: LGTM! Comprehensive example events for testing and documentation.The examples provide excellent coverage across multiple dimensions:
- Both API Gateway v1 (REST API) and v2 (HTTP API) formats
- Various HTTP methods (POST, GET)
- Normal operation, health checks, and error scenarios
- Edge cases like malformed JSON and missing required fields
The use of spread operators to derive related examples (like
healthCheckEvent) is a clean pattern that maintains consistency while avoiding duplication.
| // API Gateway | ||
| const api = new apigateway.RestApi(this, 'WalkerAPI', { | ||
| restApiName: 'walkerOS Collector', | ||
| description: 'walkerOS event collection API', | ||
| defaultCorsPreflightOptions: { | ||
| allowOrigins: apigateway.Cors.ALL_ORIGINS, | ||
| allowMethods: apigateway.Cors.ALL_METHODS, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Restrict CORS configuration for production deployments.
The API Gateway allows all origins and methods, which is overly permissive. For production, restrict to specific domains and necessary HTTP methods.
Apply this diff:
// API Gateway
const api = new apigateway.RestApi(this, 'WalkerAPI', {
restApiName: 'walkerOS Collector',
description: 'walkerOS event collection API',
defaultCorsPreflightOptions: {
- allowOrigins: apigateway.Cors.ALL_ORIGINS,
- allowMethods: apigateway.Cors.ALL_METHODS,
+ // Production: Replace with specific domains
+ allowOrigins: ['https://yourdomain.com'],
+ allowMethods: ['GET', 'POST', 'OPTIONS'],
},
});📝 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.
| // API Gateway | |
| const api = new apigateway.RestApi(this, 'WalkerAPI', { | |
| restApiName: 'walkerOS Collector', | |
| description: 'walkerOS event collection API', | |
| defaultCorsPreflightOptions: { | |
| allowOrigins: apigateway.Cors.ALL_ORIGINS, | |
| allowMethods: apigateway.Cors.ALL_METHODS, | |
| }, | |
| }); | |
| // API Gateway | |
| const api = new apigateway.RestApi(this, 'WalkerAPI', { | |
| restApiName: 'walkerOS Collector', | |
| description: 'walkerOS event collection API', | |
| defaultCorsPreflightOptions: { | |
| // Production: Replace with specific domains | |
| allowOrigins: ['https://yourdomain.com'], | |
| allowMethods: ['GET', 'POST', 'OPTIONS'], | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In packages/server/sources/aws/examples/cdk-stack.ts around lines 25 to 33, the
API Gateway CORS is currently set to allow all origins and methods; update it to
read allowed origins and allowed methods from configuration or environment
(e.g., process.env.ALLOWED_ORIGINS as a comma-separated list and
process.env.ALLOWED_METHODS), then pass a specific array of origins to
defaultCorsPreflightOptions.allowOrigins and a specific array of apigateway.Cors
methods (e.g., GET, POST, OPTIONS) to allowMethods; include a safe development
fallback (e.g., localhost) when env vars are unset and ensure you do not use
Cors.ALL_ORIGINS or Cors.ALL_METHODS in production.
| // Function URL (simpler, lower cost than API Gateway) | ||
| const fnUrl = collectorFn.addFunctionUrl({ | ||
| authType: lambda.FunctionUrlAuthType.NONE, | ||
| cors: { | ||
| allowedOrigins: ['*'], | ||
| allowedMethods: [lambda.HttpMethod.GET, lambda.HttpMethod.POST], | ||
| allowedHeaders: ['Content-Type', 'Authorization'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add authentication and restrict CORS origins.
The Function URL has no authentication (authType: NONE) and allows all origins ('*'), making it publicly accessible without protection. This is simpler than API Gateway but sacrifices security controls.
Consider:
const fnUrl = collectorFn.addFunctionUrl({
- authType: lambda.FunctionUrlAuthType.NONE,
+ // Production: Use AWS_IAM for authenticated access
+ authType: lambda.FunctionUrlAuthType.AWS_IAM,
cors: {
- allowedOrigins: ['*'],
+ // Production: Specify allowed domains
+ allowedOrigins: ['https://yourdomain.com'],
allowedMethods: [lambda.HttpMethod.GET, lambda.HttpMethod.POST],
allowedHeaders: ['Content-Type', 'Authorization'],
},
});📝 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.
| // Function URL (simpler, lower cost than API Gateway) | |
| const fnUrl = collectorFn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| cors: { | |
| allowedOrigins: ['*'], | |
| allowedMethods: [lambda.HttpMethod.GET, lambda.HttpMethod.POST], | |
| allowedHeaders: ['Content-Type', 'Authorization'], | |
| }, | |
| }); | |
| // Function URL (simpler, lower cost than API Gateway) | |
| const fnUrl = collectorFn.addFunctionUrl({ | |
| // Production: Use AWS_IAM for authenticated access | |
| authType: lambda.FunctionUrlAuthType.AWS_IAM, | |
| cors: { | |
| // Production: Specify allowed domains | |
| allowedOrigins: ['https://yourdomain.com'], | |
| allowedMethods: [lambda.HttpMethod.GET, lambda.HttpMethod.POST], | |
| allowedHeaders: ['Content-Type', 'Authorization'], | |
| }, | |
| }); |
| Resources: | ||
| WalkerOSFunction: | ||
| Type: AWS::Serverless::Function | ||
| Properties: | ||
| CodeUri: ./dist | ||
| Handler: index.handler | ||
| Events: | ||
| # POST endpoint for event collection | ||
| CollectPost: | ||
| Type: Api | ||
| Properties: | ||
| Path: /collect | ||
| Method: POST | ||
|
|
||
| # GET endpoint for pixel tracking | ||
| CollectGet: | ||
| Type: Api | ||
| Properties: | ||
| Path: /collect | ||
| Method: GET | ||
|
|
||
| # Health check endpoint | ||
| Health: | ||
| Type: Api | ||
| Properties: | ||
| Path: /health | ||
| Method: GET |
There was a problem hiding this comment.
Consider adding API Gateway throttling and authentication.
The template deploys publicly accessible endpoints without authentication or rate limiting. While this is an example, production deployments should consider:
- API Gateway usage plans with throttling limits
- API keys or IAM authorization
- WAF rules for DDoS protection
This prevents abuse, cost runaway, and data injection.
Add configuration comments in the template to guide users:
Resources:
WalkerOSFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: ./dist
Handler: index.handler
+ # Production: Consider adding Reserved Concurrent Executions to limit costs
+ # ReservedConcurrentExecutions: 10
Events:
# POST endpoint for event collection
+ # Production: Add API Gateway usage plan with throttling
CollectPost:
Type: Api
Properties:
Path: /collect
Method: POST
+ # Auth: AWS_IAMCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/server/sources/aws/examples/sam-template.yaml around lines 16 to 42,
the API endpoints are deployed publicly without throttling or auth; update the
template to include commented guidance and example resources: add an API Gateway
Stage/UsagePlan + Throttle settings (rate/burst) and show how to attach a
UsagePlan and ApiKey to limit requests, add an Auth block for the Api events (or
a reference to an Authorizer/IAM auth) and mark sensitive endpoints as
ApiKeyRequired/Use Cognito/IAM authorizer, and include a WebAclAssociation
resource example (or a note) to attach an AWS WAF web ACL for basic DDoS/SQL/XSS
protections; keep these as commented examples so users can opt-in for production
deployments.
| functions: | ||
| collector: | ||
| handler: dist/index.handler | ||
| events: | ||
| # HTTP API (v2) - recommended for lower cost | ||
| - httpApi: | ||
| path: /collect | ||
| method: post | ||
| - httpApi: | ||
| path: /collect | ||
| method: get | ||
| - httpApi: | ||
| path: /health | ||
| method: get | ||
|
|
||
| # CloudWatch Logs | ||
| logRetentionInDays: 7 | ||
|
|
||
| # Reserved concurrency to prevent runaway costs | ||
| # reservedConcurrency: 10 |
There was a problem hiding this comment.
Add authorization and consider enabling cost controls.
The HTTP API events lack authorization, allowing public access without authentication. Additionally, the commented reservedConcurrency should be considered for production to prevent cost runaway.
Apply this diff:
functions:
collector:
handler: dist/index.handler
events:
# HTTP API (v2) - recommended for lower cost
- httpApi:
path: /collect
method: post
+ # Production: Add authorization
+ # authorizer:
+ # type: aws_iam
- httpApi:
path: /collect
method: get
+ # authorizer:
+ # type: aws_iam
- httpApi:
path: /health
method: get
# CloudWatch Logs
logRetentionInDays: 7
# Reserved concurrency to prevent runaway costs
- # reservedConcurrency: 10
+ # Production: Uncomment to limit concurrent executions
+ reservedConcurrency: 10📝 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.
| functions: | |
| collector: | |
| handler: dist/index.handler | |
| events: | |
| # HTTP API (v2) - recommended for lower cost | |
| - httpApi: | |
| path: /collect | |
| method: post | |
| - httpApi: | |
| path: /collect | |
| method: get | |
| - httpApi: | |
| path: /health | |
| method: get | |
| # CloudWatch Logs | |
| logRetentionInDays: 7 | |
| # Reserved concurrency to prevent runaway costs | |
| # reservedConcurrency: 10 | |
| functions: | |
| collector: | |
| handler: dist/index.handler | |
| events: | |
| # HTTP API (v2) - recommended for lower cost | |
| - httpApi: | |
| path: /collect | |
| method: post | |
| # Production: Add authorization | |
| # authorizer: | |
| # type: aws_iam | |
| - httpApi: | |
| path: /collect | |
| method: get | |
| # authorizer: | |
| # type: aws_iam | |
| - httpApi: | |
| path: /health | |
| method: get | |
| # CloudWatch Logs | |
| logRetentionInDays: 7 | |
| # Reserved concurrency to prevent runaway costs | |
| # Production: Uncomment to limit concurrent executions | |
| reservedConcurrency: 10 |
🤖 Prompt for AI Agents
In packages/server/sources/aws/examples/serverless.yml around lines 22 to 41,
the HTTP API events are exposed without any authorization and
reservedConcurrency is only commented out; update each httpApi event for the
collector function to require an authorizer (e.g., reference an existing
JWT/Cognito/Custom authorizer by name or add a new authorizer block and
reference it under each httpApi event) and enable reservedConcurrency for
production (uncomment and set a sensible default like 5–10 or wire it to a
config variable), ensuring the authorizer is defined elsewhere in the template
and adding a short comment that reservedConcurrency should be tuned per
environment.
| "dependencies": { | ||
| "@walkeros/core": "0.4.2" | ||
| }, | ||
| "peerDependencies": { | ||
| "@types/aws-lambda": "^8.10.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/aws-lambda": "^8.10.145" | ||
| }, |
There was a problem hiding this comment.
Clarify @types/aws-lambda dependency strategy.
The package lists @types/aws-lambda in both peerDependencies and devDependencies, which is unusual. Typically, type packages are only in devDependencies unless the package exports types that consumers need to reference directly.
If consumers don't need the AWS Lambda types directly (they only use your package's types), remove from peerDependencies:
"dependencies": {
"@walkeros/core": "0.4.2"
},
- "peerDependencies": {
- "@types/aws-lambda": "^8.10.0"
- },
"devDependencies": {
"@types/aws-lambda": "^8.10.145"
},If consumers do need to reference AWS Lambda types (e.g., your public API exports APIGatewayProxyEvent), keep the peer dependency but document this requirement in the README.
🤖 Prompt for AI Agents
In packages/server/sources/aws/package.json around lines 22 to 30, the
@types/aws-lambda type package is listed in both peerDependencies and
devDependencies which is redundant; inspect the package's public type exports—if
your public API does not directly expose AWS Lambda types, remove
@types/aws-lambda from peerDependencies and keep it only in devDependencies; if
your public API does export AWS Lambda types (e.g., APIGatewayProxyEvent) then
keep @types/aws-lambda in peerDependencies and remove it from devDependencies,
and add a short note to the package README stating that consumers must install
@types/aws-lambda (including a suggested version range).
| body: expect.stringContaining('"success":true'), | ||
| }; |
There was a problem hiding this comment.
Jest matchers used in non-test module may cause runtime errors.
This file uses expect.stringContaining and expect.objectContaining, which are Jest runtime objects. Since this file is in the examples/ directory (not __tests__/), it may be imported outside a Jest test context where expect is undefined, causing a ReferenceError.
Additionally, the type Partial<APIGatewayProxyResult> is misleading since body should be string, not a Jest AsymmetricMatcher.
Consider one of these approaches:
- Move to
__tests__/directory if these are only used in tests. - Use plain values and perform assertions in test files:
- body: expect.stringContaining('"success":true'),
+ body: JSON.stringify({ success: true }),- Export a helper function that creates matchers when called from tests:
// In test files:
import { successResponse } from './outputs';
expect(result).toMatchObject({
...successResponse,
body: expect.stringContaining('"success":true'),
});Also applies to: 21-21, 27-29, 36-36
| const result = await push({ | ||
| name: eventReq.event, | ||
| data: (eventReq.data || {}) as WalkerOS.Properties, | ||
| context: eventReq.context as WalkerOS.OrderedProperties | undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the push.ts file and examine its imports and type usage
find . -type f -name "push.ts" -path "*aws*lambda*" | head -5Repository: elbwalker/walkerOS
Length of output: 110
🏁 Script executed:
# Find EventRequest type definition
rg -t ts "type EventRequest|interface EventRequest" --max-count=5Repository: elbwalker/walkerOS
Length of output: 305
🏁 Script executed:
# Find OrderedProperties type definition
rg -t ts "type OrderedProperties|interface OrderedProperties" --max-count=5Repository: elbwalker/walkerOS
Length of output: 134
🏁 Script executed:
# Examine the push.ts file to see full context
find . -type f -name "push.ts" -path "*aws*lambda*" -exec wc -l {} \;Repository: elbwalker/walkerOS
Length of output: 113
🏁 Script executed:
# Read the push.ts file in full
cat -n ./packages/server/sources/aws/src/lambda/push.tsRepository: elbwalker/walkerOS
Length of output: 1289
🏁 Script executed:
# Read EventRequest type from AWS lambda types
cat -n ./packages/server/sources/aws/src/lambda/types.tsRepository: elbwalker/walkerOS
Length of output: 2741
🏁 Script executed:
# Read OrderedProperties type definition
cat -n ./packages/core/src/types/walkeros.ts | head -100Repository: elbwalker/walkerOS
Length of output: 2993
🏁 Script executed:
# Find push function definition to see what it expects for context parameter
rg -t ts "export.*function push|const push.*=" -A 10 | head -50Repository: elbwalker/walkerOS
Length of output: 3415
🏁 Script executed:
# Check Express EventRequest to see if context handling is different
cat -n ./packages/server/sources/express/src/types.tsRepository: elbwalker/walkerOS
Length of output: 2259
🏁 Script executed:
# Check GCP EventRequest to see if context handling is different
cat -n ./packages/server/sources/gcp/src/cloudfunction/types.tsRepository: elbwalker/walkerOS
Length of output: 2525
🏁 Script executed:
# Check how Collector.PushFn is defined to see what it expects
rg -t ts "type PushFn|interface PushFn" -A 5 packages/coreRepository: elbwalker/walkerOS
Length of output: 856
🏁 Script executed:
# Search for EventRequest parsing/validation to see if there's transformation
rg -t ts "EventRequest" packages/server/sources/aws/src/lambda/ -B 2 -A 2 | grep -E "(parse|transform|validate|EventRequest)" | head -20Repository: elbwalker/walkerOS
Length of output: 1941
🏁 Script executed:
# Check if there's any validation schema for EventRequest
find ./packages/server/sources/aws/src/lambda -name "*.ts" -exec grep -l "schema\|validate\|zod" {} \;Repository: elbwalker/walkerOS
Length of output: 393
🏁 Script executed:
# Look at how the HTTP body is parsed into EventRequest in the lambda handler
rg -t ts "parseEvent\|parseBody\|JSON.parse" packages/server/sources/aws/src/lambda/ -B 2 -A 5 | head -40Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
# Read the isEventRequest function and surrounding context
cat -n ./packages/server/sources/aws/src/lambda/utils.tsRepository: elbwalker/walkerOS
Length of output: 5660
🏁 Script executed:
# Check the schemas for any EventRequest validation
cat -n ./packages/server/sources/aws/src/lambda/schemas/index.tsRepository: elbwalker/walkerOS
Length of output: 507
🏁 Script executed:
# Look at test cases to see what format is expected for EventRequest
cat -n ./packages/server/sources/aws/src/lambda/__tests__/index.test.ts | head -100Repository: elbwalker/walkerOS
Length of output: 3464
🏁 Script executed:
# Check if there's any context transformation or validation elsewhere in the code
rg -t ts "context.*\[" packages/server/sources/aws/src/lambda/ -B 2 -A 2Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
# Check how push function actually uses context to understand if the bug manifests
rg -t ts "function push|const push" packages/core/src -A 20 | grep -A 20 "context" | head -30Repository: elbwalker/walkerOS
Length of output: 44
Unsafe type cast: context from AnyObject to OrderedProperties without validation.
EventRequest.context is typed as WalkerOS.AnyObject but cast to WalkerOS.OrderedProperties without any transformation. The isEventRequest guard only validates that event is a string—it does not enforce that context matches the required OrderedProperties structure { [key: string]: [Property, number] | undefined }.
A client could send { event: "test", context: { foo: "bar" } } and it would pass validation, then fail at runtime if the collector expects context values to be tuples with counts.
Either validate and transform context to the correct format before casting, or update EventRequest.context type to WalkerOS.OrderedProperties | undefined if the input is guaranteed to be in that format.
🤖 Prompt for AI Agents
In packages/server/sources/aws/src/lambda/push.ts around line 14, the code
unsafely casts eventReq.context (WalkerOS.AnyObject) to
WalkerOS.OrderedProperties without validation; either validate/transform context
at runtime to the required shape or change the EventRequest.context type to
allow OrderedProperties | undefined. Fix by adding a runtime guard that iterates
eventReq.context keys, verifies each value is a two-element tuple [Property,
number] (and transforms or rejects entries that are invalid), then assign the
resulting safe object to the context variable (or set it to undefined) before
passing it on; alternatively update the EventRequest.context TypeScript type to
WalkerOS.OrderedProperties | undefined if you can guarantee callers supply the
correct shape and remove the unsafe cast.
| if (corsOptions.origin) { | ||
| const origin = Array.isArray(corsOptions.origin) | ||
| ? corsOptions.origin.join(', ') | ||
| : corsOptions.origin; | ||
| headers['Access-Control-Allow-Origin'] = origin; | ||
| } |
There was a problem hiding this comment.
CORS multi-origin handling may not work as expected.
When corsOptions.origin is an array, joining with comma produces "origin1, origin2" which is not valid for Access-Control-Allow-Origin. The spec only allows a single origin or *.
For multiple allowed origins, the standard pattern is to check the request's Origin header against the allowlist and return that specific origin if it matches.
- if (corsOptions.origin) {
- const origin = Array.isArray(corsOptions.origin)
- ? corsOptions.origin.join(', ')
- : corsOptions.origin;
- headers['Access-Control-Allow-Origin'] = origin;
- }
+ if (corsOptions.origin) {
+ // For array origins, you'd need to check against request Origin header
+ // and return the matching one. For now, take the first or use '*'.
+ const origin = Array.isArray(corsOptions.origin)
+ ? corsOptions.origin[0] // Consider passing request origin for dynamic matching
+ : corsOptions.origin;
+ headers['Access-Control-Allow-Origin'] = origin;
+ }A proper solution would pass the request's Origin header to getCorsHeaders and match against the allowlist.
📝 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.
| if (corsOptions.origin) { | |
| const origin = Array.isArray(corsOptions.origin) | |
| ? corsOptions.origin.join(', ') | |
| : corsOptions.origin; | |
| headers['Access-Control-Allow-Origin'] = origin; | |
| } | |
| if (corsOptions.origin) { | |
| // For array origins, you'd need to check against request Origin header | |
| // and return the matching one. For now, take the first or use '*'. | |
| const origin = Array.isArray(corsOptions.origin) | |
| ? corsOptions.origin[0] // Consider passing request origin for dynamic matching | |
| : corsOptions.origin; | |
| headers['Access-Control-Allow-Origin'] = origin; | |
| } |
🤖 Prompt for AI Agents
In packages/server/sources/aws/src/lambda/utils.ts around lines 100 to 105, the
current code joins an array of allowed origins into a comma string and sets
Access-Control-Allow-Origin, which is invalid; instead change getCorsHeaders to
accept the incoming request's Origin header (or accept an origin parameter), and
when corsOptions.origin is an array validate the request Origin against that
allowlist and set Access-Control-Allow-Origin to the matching origin only (or
omit the header/fall back to safe default) rather than joining the list; keep
existing behavior for string or '*' origins.
| const responseHeaders: Record<string, string> = { | ||
| 'Content-Type': | ||
| typeof body === 'object' ? 'application/json' : 'text/plain', | ||
| ...headers, | ||
| }; |
There was a problem hiding this comment.
Potential issue: typeof null === 'object' in JavaScript.
The check typeof body === 'object' returns true for null, which would set Content-Type: application/json and call JSON.stringify(null) (producing "null"). This may not be the intended behavior.
const responseHeaders: Record<string, string> = {
'Content-Type':
- typeof body === 'object' ? 'application/json' : 'text/plain',
+ typeof body === 'object' && body !== null ? 'application/json' : 'text/plain',
...headers,
};📝 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.
| const responseHeaders: Record<string, string> = { | |
| 'Content-Type': | |
| typeof body === 'object' ? 'application/json' : 'text/plain', | |
| ...headers, | |
| }; | |
| const responseHeaders: Record<string, string> = { | |
| 'Content-Type': | |
| typeof body === 'object' && body !== null ? 'application/json' : 'text/plain', | |
| ...headers, | |
| }; |
🤖 Prompt for AI Agents
In packages/server/sources/aws/src/lambda/utils.ts around lines 128 to 132, the
Content-Type check uses typeof body === 'object' which is true for null; change
the condition to explicitly exclude null (e.g. body !== null && typeof body ===
'object') so null is not treated as JSON, update the header selection
accordingly, and ensure any later JSON.stringify usage uses the same null-aware
check.
| export default defineConfig([ | ||
| buildModules({ | ||
| terserOptions: {}, // Don't mangle here | ||
| }), |
There was a problem hiding this comment.
Clarify terser configuration to match intent.
The comment indicates mangling should be disabled, but an empty terserOptions object typically enables terser with default settings (which includes mangling). To preserve function/variable names for better Lambda debugging and stack traces, explicitly disable mangling.
Apply this diff:
export default defineConfig([
buildModules({
- terserOptions: {}, // Don't mangle here
+ terserOptions: {
+ mangle: false, // Preserve names for Lambda debugging
+ },
}),
]);📝 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.
| export default defineConfig([ | |
| buildModules({ | |
| terserOptions: {}, // Don't mangle here | |
| }), | |
| export default defineConfig([ | |
| buildModules({ | |
| terserOptions: { | |
| mangle: false, // Preserve names for Lambda debugging | |
| }, | |
| }), |
🤖 Prompt for AI Agents
In packages/server/sources/aws/tsup.config.ts around lines 3 to 6, the config
currently passes an empty terserOptions which enables Terser defaults (including
mangling) despite the comment; update terserOptions to explicitly disable
mangling and preserve names (e.g. set mangle: false and keep_fnames: true (and
optionally keep_classnames: true)) so function/variable/class names are
preserved for Lambda debugging and stack traces.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/server/sources/fetch/src/schemas/primitives.ts (1)
19-25: Clarify whethercors.maxAgeshould allow0or strictly positive valuesThe schema currently uses
z.number().int().positive()formaxAge, which rejects0. CORSAccess-Control-Max-Agetechnically allows0, so if you ever want to express “no caching” or a disabled value via0, this will block it.If that’s not intentional, consider relaxing the constraint:
- maxAge: z.number().int().positive().optional(), + maxAge: z.number().int().nonnegative().optional(),Or add a short comment that
maxAgemust be> 0so callers know the contract.packages/server/sources/fetch/src/examples/requests.ts (1)
6-59: Good coverage of request shapes; optionally introduce a shared example typeThe examples nicely cover single, batch, pixel, health, CORS preflight, invalid JSON, and oversized payloads and are fine as-is.
If you want a bit more compile-time safety and reuse, you could introduce a lightweight type for these examples and apply it to all exports, e.g.:
type HttpRequestExample = { method: string; url: string; headers?: Record<string, string>; body?: string; }; export const validPostRequest: HttpRequestExample = { /* ... */ };This keeps the “contract” for the dev examples explicit without adding runtime cost.
packages/server/sources/fetch/src/__tests__/validation.test.ts (1)
4-26: Solid schema coverage; consider slightly more robust error assertionThe tests nicely validate single events, a fully populated event, and a batch, plus a negative case for missing
name. That gives good confidence inEventSchema.For the “without name” case you rely on the first issue:
if (!result.success) { expect(result.error.issues[0].path).toEqual(['name']); }If the schema ever grows more validations that can also fail, this could become brittle. Optionally, you could assert that some issue targets
name:if (!result.success) { expect(result.error.issues.some((i) => i.path[0] === 'name')).toBe(true); }Not required, but it makes the test more future‑proof.
packages/server/sources/fetch/src/__tests__/index.test.ts (1)
463-489: Consider renaming shadowed variable for clarity.Line 465 declares a new
mockPushthat shadows themockPushfrom the outerbeforeEachscope. While this works correctly, it could be confusing during maintenance.describe('example-driven tests', () => { it('should process examples.inputs.pageView', async () => { - const mockPush = jest + const examplePush = jest .fn() .mockResolvedValue({ event: { id: 'test-id' } }); const source = await sourceFetch( {}, { - push: mockPush as never, + push: examplePush as never, command: jest.fn() as never, elb: jest.fn() as never, logger: createMockLogger(), }, ); // ... request creation ... await source.push(request); - expect(mockPush).toHaveBeenCalledWith(examples.inputs.pageView); + expect(examplePush).toHaveBeenCalledWith(examples.inputs.pageView); }); });packages/server/sources/fetch/src/schemas/event.ts (1)
4-7: Avoidz.any()- use stricter typing.The use of
z.any()for nested records violates the coding guideline "NEVER useanytype". Consider using a more specific recursive type orz.unknown()with refinement.const PropertiesSchema = z.record( z.string(), - z.union([z.string(), z.number(), z.boolean(), z.record(z.string(), z.any())]), + z.union([z.string(), z.number(), z.boolean(), z.record(z.string(), z.unknown())]), );Apply the same change to
OrderedPropertiesSchemaon line 17. As per coding guidelines, avoid usinganytypes.packages/server/sources/fetch/src/utils.ts (2)
3-52: Tighten CORS handling aroundmaxAgeand dynamic originsOverall shape looks good; a couple of small edge cases are worth tightening:
maxAgeis only applied when truthy (if (corsConfig.maxAge)), so a valid0value would be silently ignored. Prefer a nullish check instead.- When you pick an origin from an array based on
requestOrigin, caches/proxies can serve mismatched CORS headers unlessVary: Originis set. That’s often recommended wheneverAccess-Control-Allow-Originvaries per request.Example diff:
- if (corsConfig.maxAge) { - headers.set('Access-Control-Max-Age', String(corsConfig.maxAge)); - } + if (corsConfig.maxAge != null) { + headers.set('Access-Control-Max-Age', String(corsConfig.maxAge)); + } + + if (corsConfig.origin && Array.isArray(corsConfig.origin)) { + headers.set('Vary', 'Origin'); + }You might also defensively handle an empty
originarray to avoid accidentally setting'undefined'as the header value if misconfigured.
55-70: Consider runtime portability foratoband precomputing the pixel bytesThe pixel response logic is correct, but two small tweaks could improve robustness and efficiency:
atobisn’t available in all server runtimes (especially older Node.js without full web globals). If this source is intended to run outside browser/edge-style environments, consider usingBuffer.from(TRANSPARENT_GIF_BASE64, 'base64')(or similar) behind an environment check.- You recompute the
Uint8Arrayon everycreatePixelResponsecall. If this endpoint is hot, precomputing the bytes once at module load and reusing them would avoid repeated base64 decoding.For example:
-export function createPixelResponse(corsHeaders?: Headers): Response { - const binaryString = atob(TRANSPARENT_GIF_BASE64); - const bytes = new Uint8Array(binaryString.length); - for (let i = 0; i < binaryString.length; i++) { - bytes[i] = binaryString.charCodeAt(i); - } +const TRANSPARENT_GIF_BYTES = (() => { + const binaryString = atob(TRANSPARENT_GIF_BASE64); + const bytes = new Uint8Array(binaryString.length); + for (let i = 0; i < binaryString.length; i++) { + bytes[i] = binaryString.charCodeAt(i); + } + return bytes; +})(); + +export function createPixelResponse(corsHeaders?: Headers): Response { + const bytes = TRANSPARENT_GIF_BYTES;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/server/sources/fetch/README.md(1 hunks)packages/server/sources/fetch/jest.config.mjs(1 hunks)packages/server/sources/fetch/package.json(1 hunks)packages/server/sources/fetch/src/__tests__/index.test.ts(1 hunks)packages/server/sources/fetch/src/__tests__/logging.test.ts(1 hunks)packages/server/sources/fetch/src/__tests__/validation.test.ts(1 hunks)packages/server/sources/fetch/src/dev.ts(1 hunks)packages/server/sources/fetch/src/examples/env.ts(1 hunks)packages/server/sources/fetch/src/examples/index.ts(1 hunks)packages/server/sources/fetch/src/examples/inputs.ts(1 hunks)packages/server/sources/fetch/src/examples/requests.ts(1 hunks)packages/server/sources/fetch/src/index.ts(1 hunks)packages/server/sources/fetch/src/schemas/event.ts(1 hunks)packages/server/sources/fetch/src/schemas/index.ts(1 hunks)packages/server/sources/fetch/src/schemas/primitives.ts(1 hunks)packages/server/sources/fetch/src/schemas/settings.ts(1 hunks)packages/server/sources/fetch/src/types.ts(1 hunks)packages/server/sources/fetch/src/utils.ts(1 hunks)packages/server/sources/fetch/tsconfig.json(1 hunks)packages/server/sources/fetch/tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/server/sources/fetch/src/examples/env.ts
- packages/server/sources/fetch/README.md
- packages/server/sources/fetch/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/server/sources/fetch/src/schemas/primitives.tspackages/server/sources/fetch/src/__tests__/validation.test.tspackages/server/sources/fetch/src/__tests__/logging.test.tspackages/server/sources/fetch/src/__tests__/index.test.tspackages/server/sources/fetch/src/schemas/index.tspackages/server/sources/fetch/src/schemas/settings.tspackages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/schemas/event.tspackages/server/sources/fetch/src/examples/index.tspackages/server/sources/fetch/src/dev.tspackages/server/sources/fetch/src/examples/requests.tspackages/server/sources/fetch/tsup.config.tspackages/server/sources/fetch/src/types.tspackages/server/sources/fetch/src/index.tspackages/server/sources/fetch/src/utils.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/server/sources/fetch/src/__tests__/validation.test.tspackages/server/sources/fetch/src/__tests__/logging.test.tspackages/server/sources/fetch/src/__tests__/index.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
packages/server/sources/fetch/src/__tests__/validation.test.tspackages/server/sources/fetch/src/__tests__/logging.test.tspackages/server/sources/fetch/src/__tests__/index.test.tspackages/server/sources/fetch/src/examples/requests.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
packages/server/sources/fetch/src/__tests__/validation.test.tspackages/server/sources/fetch/src/__tests__/logging.test.tspackages/server/sources/fetch/src/__tests__/index.test.tspackages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/dev.tspackages/server/sources/fetch/src/examples/requests.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/server/sources/fetch/src/__tests__/validation.test.tspackages/server/sources/fetch/src/__tests__/logging.test.tspackages/server/sources/fetch/src/__tests__/index.test.tspackages/server/sources/fetch/src/examples/requests.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
packages/server/sources/fetch/src/schemas/index.tspackages/server/sources/fetch/package.jsonpackages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/schemas/event.tspackages/server/sources/fetch/src/types.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Applied to files:
packages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/schemas/event.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
packages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/tsup.config.tspackages/server/sources/fetch/src/types.tspackages/server/sources/fetch/src/index.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Applied to files:
packages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/schemas/event.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: All walkerOS components communicate via push functions: sources use source.push(), collector uses collector.push(), destinations use destination.push(), and ELB uses elb() as the alias for collector.push()
Applied to files:
packages/server/sources/fetch/src/examples/inputs.tspackages/server/sources/fetch/src/types.tspackages/server/sources/fetch/src/index.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Always use space-separated 'ENTITY ACTION' format for event names (e.g., 'page view', 'product add', 'button click'). Event names are parsed as: const [entity, action] = event.split(' ')
Applied to files:
packages/server/sources/fetch/src/examples/inputs.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/tsconfig.json : Configure TypeScript with strict settings to enforce type safety and prevent use of `any` types
Applied to files:
packages/server/sources/fetch/tsup.config.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Applied to files:
packages/server/sources/fetch/tsup.config.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Use the Source → Collector → Destination(s) architecture pattern for event processing: Sources capture events from various sources (browser DOM, dataLayer, server), Collector handles central event processing with consent management, Destinations transform and send events to third-party tools
Applied to files:
packages/server/sources/fetch/src/index.ts
🧬 Code graph analysis (9)
packages/server/sources/fetch/src/__tests__/validation.test.ts (1)
packages/server/sources/fetch/src/schemas/event.ts (1)
EventSchema(65-91)
packages/server/sources/fetch/src/__tests__/logging.test.ts (3)
packages/core/src/mockLogger.ts (1)
createMockLogger(41-65)packages/server/sources/fetch/src/index.ts (1)
sourceFetch(11-222)packages/server/sources/gcp/src/cloudfunction/types.ts (1)
Request(6-11)
packages/server/sources/fetch/src/__tests__/index.test.ts (3)
packages/server/sources/fetch/src/index.ts (1)
sourceFetch(11-222)packages/core/src/types/walkeros.ts (1)
DeepPartialEvent(10-10)packages/server/sources/fetch/src/examples/inputs.ts (1)
completeEvent(58-98)
packages/server/sources/fetch/src/schemas/settings.ts (1)
packages/server/sources/fetch/src/schemas/primitives.ts (1)
CorsOptionsSchema(19-25)
packages/server/sources/fetch/src/examples/inputs.ts (1)
packages/core/src/types/walkeros.ts (1)
DeepPartialEvent(10-10)
packages/server/sources/fetch/src/dev.ts (1)
packages/server/sources/gcp/src/dev.ts (1)
examples(1-1)
packages/server/sources/fetch/tsup.config.ts (1)
packages/config/tsup/index.d.ts (1)
defineConfig(11-11)
packages/server/sources/fetch/src/types.ts (1)
packages/server/sources/gcp/src/cloudfunction/types.ts (2)
Request(6-11)Response(13-18)
packages/server/sources/fetch/src/utils.ts (2)
packages/server/sources/fetch/src/types.ts (1)
CorsOptions(6-6)packages/server/sources/gcp/src/cloudfunction/types.ts (1)
Response(13-18)
🪛 Biome (2.1.2)
packages/server/sources/fetch/src/types.ts
[error] 9-9: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (21)
packages/server/sources/fetch/tsup.config.ts (2)
1-1: LGTM!The imports are correct and follow the expected pattern for tsup configuration files.
3-11: Configuration is valid; dev entry point exists.The
src/dev.tsfile exists and properly exports from schemas and examples, confirming the build configuration will work as intended. The two-buildModules pattern correctly separates minification settings from the explicit dev entry point configuration.packages/server/sources/fetch/src/examples/index.ts (1)
1-2: Examples namespace re-exports look goodNamespacing
inputsandrequestsfrom a centralexamplesindex keeps the dev API tidy and discoverable. No issues from my side here.packages/server/sources/fetch/src/dev.ts (1)
1-2: Dev entrypoint export surface is clear and consistentRe-exporting
./schemasand namespacedexamplesfrom here aligns well with the rest of the package structure and makes dev/testing ergonomics straightforward.packages/server/sources/fetch/src/schemas/index.ts (1)
1-3: Schema index re-exports are straightforward and usefulExporting
primitives,settings, andeventfrom a single index keeps consumers’ imports clean (Schemas.*) and matches the intended public surface.packages/server/sources/fetch/src/schemas/settings.ts (1)
4-14: Settings schema is well-structured and matches fetch source needsThe
SettingsSchemacovers the key knobs (path, CORS, health path, request/batch limits) with sensible defaults and a clear union forcors(boolean vs detailed options). Typing viaSettings = z.infer<typeof SettingsSchema>keeps runtime and type-level contracts in sync. No changes requested.packages/server/sources/fetch/src/__tests__/logging.test.ts (1)
32-56: Good test coverage for routine operation logging behavior.This test correctly validates that successful event processing doesn't emit unnecessary info/debug logs, aligning with the source implementation's design to avoid noisy logs for routine operations.
packages/server/sources/fetch/jest.config.mjs (1)
1-3: LGTM!Clean re-export of the shared Jest configuration, maintaining consistency with other server source packages (e.g., AWS source).
packages/server/sources/fetch/src/__tests__/index.test.ts (4)
1-4: LGTM on imports.Good use of
import typeforWalkerOSas per coding guidelines. The imports are properly structured.
26-74: Comprehensive initialization tests.Good coverage of default settings verification and custom settings merging. The assertions correctly validate the expected configuration shape.
76-236: Thorough POST request handling test coverage.Good coverage of:
- Valid single events
- Invalid JSON rejection
- Non-object body rejection
- Complete event with all properties
- Collector error handling
The tests properly verify both success and error paths.
238-289: Good pixel tracking test coverage.Tests correctly verify:
- Query parameter parsing to event data
- 1x1 GIF response with proper Content-Type
- Cache-Control headers for no-caching
packages/server/sources/fetch/src/schemas/event.ts (3)
23-32: LGTM on UserSchema.The schema correctly defines the standard WalkerOS user identification fields with appropriate optional modifiers and passthrough for extensibility.
37-47: Good recursive schema design for nested entities.The use of
z.lazy()for the recursiveEntitySchemais the correct pattern for handling self-referential types in Zod.
64-91: Well-structured EventSchema.The schema correctly implements the WalkerOS event structure with:
- Required
namefield with non-empty validation- All core properties (data, context, globals, custom, user, nested, consent)
- System-generated fields as optional for incoming events
- Passthrough for extensibility
packages/server/sources/fetch/src/index.ts (4)
1-9: Good use of type imports.Correctly uses
import typeforWalkerOS,Collector,FetchSource,PartialConfig, andTypesas per coding guidelines.
50-199: Well-structured POST request handling.Good implementation with:
- Defensive double size checking (Content-Length header + actual body)
- Safe JSON parsing with proper error logging
- Schema validation using
safeParsefor graceful error handling- Clear separation between batch and single event processing
224-286: Solid helper function implementations.Both
processEventandprocessBatchare well-implemented:
- Proper error catching and message extraction
- Per-event validation and error logging in batch processing
- Sequential processing maintains ordering guarantees
288-291: Clean module exports.Good public API surface with type re-exports, utilities, schemas, and examples all properly exposed.
packages/server/sources/fetch/src/utils.ts (1)
72-81: JSON response helper looks solid
createJsonResponseis straightforward and correctly setsContent-Type: application/jsonwhile allowing CORS headers to be merged via theHeadersconstructor. No issues from my side here.packages/server/sources/fetch/src/types.ts (1)
1-8: Core fetch source typings are consistent and well-wiredThe way you bind
Settings,InitSettings,Env, andPushintoCoreSource.Typesand then exposeConfig,PartialConfig, andFetchSourcematches the standard WalkerOS source pattern and keeps the public surface clear. No issues spotted here.Also applies to: 13-29, 31-36
| { | ||
| "name": "@walkeros/server-source-fetch", | ||
| "description": "Web Standard Fetch API source for walkerOS (Cloudflare Workers, Vercel Edge, Deno, Bun)", | ||
| "version": "0.4.2", | ||
| "license": "MIT", | ||
| "main": "./dist/index.js", | ||
| "module": "./dist/index.mjs", | ||
| "types": "./dist/index.d.ts", | ||
| "files": [ | ||
| "dist/**" | ||
| ], | ||
| "scripts": { | ||
| "build": "tsup --silent", | ||
| "clean": "rm -rf .turbo && rm -rf node_modules && rm -rf dist", | ||
| "dev": "jest --watchAll --colors", | ||
| "lint": "tsc && eslint \"**/*.ts*\"", | ||
| "test": "jest", | ||
| "update": "npx npm-check-updates -u && npm update" | ||
| }, | ||
| "dependencies": { | ||
| "@walkeros/core": "*" | ||
| }, | ||
| "devDependencies": {}, | ||
| "repository": { | ||
| "url": "git+https://github.com/elbwalker/walkerOS.git", | ||
| "directory": "packages/server/sources/fetch" | ||
| }, | ||
| "author": "elbwalker <hello@elbwalker.com>", | ||
| "homepage": "https://github.com/elbwalker/walkerOS#readme", | ||
| "bugs": { | ||
| "url": "https://github.com/elbwalker/walkerOS/issues" | ||
| }, | ||
| "keywords": [ | ||
| "walker", | ||
| "walkerOS", | ||
| "source", | ||
| "server", | ||
| "fetch", | ||
| "edge", | ||
| "cloudflare", | ||
| "vercel", | ||
| "deno", | ||
| "bun" | ||
| ], | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.mjs", | ||
| "require": "./dist/index.js" | ||
| }, | ||
| "./dev": { | ||
| "types": "./dist/dev.d.ts", | ||
| "import": "./dist/dev.mjs", | ||
| "require": "./dist/dev.js" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check workspace configuration and devDependencies patterns
#!/bin/bash
# Check the root package.json for shared devDependencies
echo "=== Root package.json devDependencies (first 15 entries) ==="
jq -r '.devDependencies | keys[] | "\(.)"' package.json 2>/dev/null | head -15
echo -e "\n=== Root package.json structure (key sections) ==="
jq 'keys' package.json 2>/dev/null
# Check if this is a workspaces/lerna setup
echo -e "\n=== Workspace configuration ==="
jq -r '.workspaces // "No workspaces field"' package.json 2>/dev/null
# Check if pnpm-workspace exists
echo -e "\n=== pnpm-workspace.yaml check ==="
ls -la pnpm-workspace.yaml 2>/dev/null || echo "File not found"
# Check other source packages for devDependencies pattern
echo -e "\n=== Other source package.json devDependencies ==="
fd 'package\.json' 'packages/server/sources' --type f | while read file; do
echo "$file:"
jq -r '.devDependencies | if length == 0 then "EMPTY" else (keys | join(", ")) end' "$file"
done
# Check @walkeros/core version across all source packages
echo -e "\n=== @walkeros/core dependency versions ==="
fd 'package\.json' 'packages/server/sources' --type f | while read file; do
echo "$file: $(jq -r '.dependencies["@walkeros/core"] // "not found"' "$file")"
doneRepository: elbwalker/walkerOS
Length of output: 1464
Inconsistent dependency versioning across source packages requires alignment.
The fetch package uses a wildcard version (*) for @walkeros/core, while aws and gcp source packages pin to 0.4.2. Additionally, the empty devDependencies object differs from aws (which declares @types/aws-lambda) and express (which declares @types/cors and @types/express).
Align the @walkeros/core version to match aws and gcp packages (pin to 0.4.2 rather than wildcard), and verify whether type definitions for standard APIs need to be declared as devDependencies, consistent with other source packages.
🤖 Prompt for AI Agents
packages/server/sources/fetch/package.json lines 1-57: the package uses a
wildcard dependency for @walkeros/core and has an empty devDependencies object
which is inconsistent with other source packages; change the @walkeros/core
version from "*" to "0.4.2" to match aws and gcp packages, and add any missing
devDependencies for type definitions used by this source (e.g., add the same
@types packages other sources declare if this package uses those APIs) so
dependency versions and dev type declarations are aligned across source
packages.
| import { createMockLogger } from '@walkeros/core'; | ||
|
|
||
| describe('logger usage', () => { | ||
| it('should use logger.throw for validation errors', async () => { |
There was a problem hiding this comment.
Test description is misleading.
The test description says "should use logger.throw for validation errors" but the test actually verifies logger.error is called (line 29). Update the description to match the actual behavior being tested.
- it('should use logger.throw for validation errors', async () => {
+ it('should use logger.error for validation errors', async () => {📝 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.
| it('should use logger.throw for validation errors', async () => { | |
| it('should use logger.error for validation errors', async () => { |
🤖 Prompt for AI Agents
In packages/server/sources/fetch/src/__tests__/logging.test.ts around line 5,
the test description "should use logger.throw for validation errors" is
incorrect because the test asserts that logger.error is called (line 29); update
the it(...) description to accurately reflect the behavior being tested (e.g.,
"should log validation errors using logger.error") so the test name matches the
assertion.
| // Simple page view event | ||
| export const pageView: WalkerOS.DeepPartialEvent = { | ||
| name: 'page view', | ||
| data: { | ||
| title: 'Home Page', | ||
| path: '/', | ||
| referrer: 'https://google.com', | ||
| }, | ||
| user: { | ||
| id: 'user-123', | ||
| session: 'session-456', | ||
| }, | ||
| timestamp: 1700000000000, | ||
| }; | ||
|
|
||
| // E-commerce event with nested entities | ||
| export const productAdd: WalkerOS.DeepPartialEvent = { | ||
| name: 'product add', | ||
| data: { | ||
| id: 'P-123', | ||
| name: 'Laptop', | ||
| price: 999.99, | ||
| quantity: 1, | ||
| }, | ||
| context: { | ||
| stage: ['shopping', 1], | ||
| }, | ||
| globals: { | ||
| language: 'en', | ||
| currency: 'USD', | ||
| }, | ||
| user: { | ||
| id: 'user-123', | ||
| }, | ||
| nested: [ | ||
| { | ||
| entity: 'category', | ||
| data: { | ||
| name: 'Electronics', | ||
| path: '/electronics', | ||
| }, | ||
| }, | ||
| ], | ||
| consent: { | ||
| functional: true, | ||
| marketing: true, | ||
| }, | ||
| }; | ||
|
|
||
| // Complete event with all optional fields | ||
| export const completeEvent: WalkerOS.DeepPartialEvent = { | ||
| name: 'order complete', | ||
| data: { | ||
| id: 'ORDER-123', | ||
| total: 999.99, | ||
| currency: 'USD', | ||
| }, | ||
| context: { | ||
| stage: ['checkout', 3], | ||
| test: ['variant-A', 0], | ||
| }, | ||
| globals: { | ||
| language: 'en', | ||
| country: 'US', | ||
| }, | ||
| custom: { | ||
| campaignId: 'summer-sale', | ||
| source: 'email', | ||
| }, | ||
| user: { | ||
| id: 'user-123', | ||
| email: 'user@example.com', | ||
| session: 'session-456', | ||
| }, | ||
| nested: [ | ||
| { | ||
| entity: 'product', | ||
| data: { | ||
| id: 'P-123', | ||
| price: 999.99, | ||
| }, | ||
| }, | ||
| ], | ||
| consent: { | ||
| functional: true, | ||
| marketing: true, | ||
| analytics: false, | ||
| }, | ||
| trigger: 'click', | ||
| group: 'ecommerce', | ||
| }; | ||
|
|
||
| // Minimal valid event | ||
| export const minimal: WalkerOS.DeepPartialEvent = { | ||
| name: 'ping', | ||
| }; | ||
|
|
||
| // Batch of events | ||
| export const batch: WalkerOS.DeepPartialEvent[] = [ | ||
| pageView, | ||
| productAdd, | ||
| { name: 'button click', data: { id: 'cta' } }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align minimal event name with the ENTITY ACTION naming convention
These examples are well-structured and match the WalkerOS event shape (data/context/globals/user/nested/consent, etc.), which is great for using them as a public “contract”.
One inconsistency: by convention events should use the space‑separated "ENTITY ACTION" format, but the minimal example uses just 'ping':
export const minimal: WalkerOS.DeepPartialEvent = {
name: 'ping',
};To avoid confusing downstream parsing like const [entity, action] = name.split(' '), consider renaming this to something like:
-export const minimal: WalkerOS.DeepPartialEvent = {
- name: 'ping',
-};
+export const minimal: WalkerOS.DeepPartialEvent = {
+ name: 'system ping',
+};(or any ENTITY ACTION variant that fits your semantics).
🤖 Prompt for AI Agents
In packages/server/sources/fetch/src/examples/inputs.ts around lines 8 to 110,
the minimal example event uses the single-word name 'ping' which breaks the
project convention of space‑separated "ENTITY ACTION" event names; change the
minimal event's name to an ENTITY ACTION form (e.g., "system ping" or "service
ping") to match the rest of the examples and parsing expectations, and update
any local usages or tests that reference the old 'ping' string.
| const push = async (request: Request): Promise<Response> => { | ||
| const startTime = Date.now(); |
There was a problem hiding this comment.
Remove unused startTime variable.
startTime is declared on line 19 but never used. This appears to be leftover from removed timing/logging code.
const push = async (request: Request): Promise<Response> => {
- const startTime = Date.now();
-
try {📝 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.
| const push = async (request: Request): Promise<Response> => { | |
| const startTime = Date.now(); | |
| const push = async (request: Request): Promise<Response> => { | |
| try { |
🤖 Prompt for AI Agents
In packages/server/sources/fetch/src/index.ts around lines 18 to 19, the
variable `startTime` is declared but never used; remove the unused declaration
to clean up dead code. Edit the push function to delete the line that sets
`const startTime = Date.now();`, run TypeScript/ESLint to ensure no other
references remain, and update any nearby comments if they referenced timing.
| export type CorsOptions = z.infer<typeof CorsOptionsSchema>; | ||
| export type InitSettings = Partial<Settings>; | ||
|
|
||
| export interface Mapping {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace empty Mapping interface with a type alias to satisfy Biome
Biome is right to flag the empty interface here; it’s effectively equivalent to {} and will trip noEmptyInterface. If you don’t yet need mappings for this source, a simple alias is enough and keeps the intent clear.
For example:
-export interface Mapping {}
+export type Mapping = Record<string, never>;or, if you prefer to allow future keys without committing structure yet:
-export interface Mapping {}
+export type Mapping = Record<string, unknown>;The first option encodes “no mapping” more precisely; the second leaves room for extension without more refactors.
📝 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.
| export interface Mapping {} | |
| export type Mapping = Record<string, never>; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 9-9: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🤖 Prompt for AI Agents
In packages/server/sources/fetch/src/types.ts around line 9, replace the empty
interface declaration "export interface Mapping {}" with a type alias to satisfy
Biome's noEmptyInterface rule; either use a closed alias for "no mapping" (e.g.,
export type Mapping = Record<string, never>) or a permissive alias for future
extension (e.g., export type Mapping = Record<string, unknown>), and update any
imports/usages if necessary.
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/cli/src/commands/bundle/bundler.ts (1)
523-605: Fix mismatch between explicit code imports and default import generation for packages with mixed usageWhen a package appears in
explicitCodeImports(because at least one source or destination specifies an explicitcodefield),generateImportStatementsemits only named imports and skips the default import. However,buildConfigObjectstill attempts to use the default import variable (viapackageNameToVariable) for any entry in that package that lacks acodefield, resulting in aReferenceErrorat runtime.Example:
sources: { source1: { package: 'pkg-a', code: 'customCode' }, source2: { package: 'pkg-a' } // no code field }Generated imports:
import { customCode } from 'pkg-a';(no default import)
Generated config for source2:code: gtagDestination(undefined variable)The issue stems from the
else ifchain ingenerateImportStatements(lines 593–604). WhenhasExplicitCodeis true at the package level, the default import branch never executes, even though other entries for that package may need it.Refactor to track per-package whether a default import is needed: emit the default import whenever any source or destination entry for that package lacks an explicit
codefield, in addition to named imports for entries with explicit code.
🧹 Nitpick comments (9)
packages/cli/src/core/asset-resolver.ts (1)
63-81: Consider removing or documenting the unusedassetTypeparameter.The
assetTypeparameter is declared but never used within the function body. Line 71 hardcodes the path to the'examples'subdirectory regardless of the asset type value.Either:
- Remove the parameter if it's no longer needed (breaking change requiring version bump)
- Document why it's retained (e.g., for API compatibility or future extensibility)
If the parameter is truly unnecessary, apply this diff to remove it:
/** * Resolve asset path using unified strategy * * Resolution rules: * 1. Bare names (no / or \) → Package asset (examples) * 2. Relative paths (./ or ../) → User asset relative to base directory * 3. Absolute paths → Use as-is * * @param assetPath - Path to resolve - * @param assetType - Type of asset (determines package subdirectory) * @param baseDir - Base directory for relative paths (defaults to cwd) * @returns Absolute path to asset */ export function resolveAsset( assetPath: string, - assetType: AssetType, baseDir?: string, ): string {Otherwise, add a JSDoc comment explaining why the parameter is retained despite not being used in the current implementation.
packages/core/src/__tests__/flow.test.ts (2)
1266-1293: Add a test for digit-leading unscoped package namesCurrent tests cover scoped/unscoped, hyphen/slash, and numeric-in-name cases, but not a package that starts with a digit. Given how
packageNameToVariableis used, adding such a test would protect against invalid identifiers if someone ever uses"123-utils"or similar:test('handles unscoped package names starting with digits', () => { expect(packageNameToVariable('123-utils')).toBe('_123Utils'); });(Exact expected value depends on the chosen fix in
packageNameToVariable.)
1295-1393: Consider renaming this describe to match current behaviorThe block title
"resolveCodeFromPackage - default export fallback"is now slightly misleading: the implementation no longer performs any fallback, it just preserves explicitcodeand otherwise leaves it undefined. The individual tests are correct, but the heading might confuse future readers.Renaming the describe (e.g. to
"resolveCodeFromPackage - explicit code preservation") would better reflect the behavior.packages/cli/examples/flow-simple.json (1)
7-13: Verify destinationDemo is still resolvable after removing importsHere
@walkeros/destination-demono longer declaresimports: ["destinationDemo"], while thedemodestination still only specifiescode: "destinationDemo"and nopackage.Depending on how the bundler now infers imports, this example might no longer map
destinationDemoto the correct package.Please double-check that:
- Running the CLI with this example still bundles/executes correctly, and
- If the new mechanism requires a
packageon the destination (as inserver-collect.json) or some other mapping, update this example accordingly to avoid confusing users.packages/cli/examples/flow-order-complete.json (1)
7-13: Keep flow-order-complete in sync with the new code/import mapping strategyAs with
flow-simple.json, this example now has@walkeros/destination-demowithoutimportswhiledestinations.demoonly referencescode: "destinationDemo".Please confirm this still works end-to-end with the updated bundler. If the new pattern is to rely on
package+code(likeserver-collect.json), consider either:
- Adding
"package": "@walkeros/destination-demo"todestinations.demo, or- Documenting that this particular example is intentionally using a different (pure code) mechanism.
packages/cli/src/__tests__/bundle/bundler.test.ts (1)
295-447: New bundler helper tests give good coverage of config, wrappers, and entrypointThe added suites for
buildConfigObject,generatePlatformWrapper, andcreateEntryPointcollectively cover:
- Explicit
codepreservation when provided.- Default code variable naming for package-backed sources.
- Browser IIFE vs. Node
export default async functionwrappers.- End-to-end entrypoint generation with the expected imports and config wiring.
This should catch most regressions in the new bundling flow.
If you want to harden them further later, you could:
- Parse the generated source with a real parser instead of relying solely on
toContain, and/or- Add a browser-platform
createEntryPointtest mirroring the server one.But as-is, these tests are already valuable and aligned with the new design.
packages/cli/src/commands/bundle/bundler.ts (3)
433-458: Destination/source and explicit-code detectors look good; consider small type tighteningThe new
detectSourcePackages/detectExplicitCodeImportshelpers correctly guard against non-object configs and only consider entries with explicitpackage+codestrings. This keeps auto-import behavior explicit and predictable. You might consider tightening the intermediate cast to a more specificFlow.Config-shaped helper type (rather thanRecord<string, unknown>) to make it harder to regress the structure accidentally, but functionally this looks solid.Also applies to: 460-516
765-777: Config serialization is JSON‑only despite comment about “existing serializer utilities”
processConfigValuecurrently reduces allconfig,env, andcollectorvalues toJSON.stringify(value, null, 2), then inlines that JSON into the generated code. This is fine for plain JSON data, but:
- Any functions, Dates, RegExps, or other non‑JSON structures inside configs will be lost or stringified unexpectedly.
- The comment above mentions “Uses existing serializer utilities”, but none are actually used here.
If Flow configs are guaranteed to be JSON‑serializable, updating the comment to reflect that would avoid confusion. If not, consider delegating to a central serializer from
@walkeros/core(or wherever the “existing utilities” live) so bundling respects the full config surface.Also applies to: 783-785
799-821: Optional: surface unhandled errors in browser IIFE wrapperThe browser wrapper is an async IIFE whose returned promise is never observed. Any rejection from
startFlow(config)will become an unhandled rejection, which can be noisy and hard to trace in some environments.If you care about better DX here, you could wrap the IIFE body in a try/catch and
console.errora concise message on failure, or expose the promise towindowfor callers to await. Not critical, but might improve debuggability for complex flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/demos/flows/browser-to-bigquery.json(1 hunks)packages/cli/README.md(2 hunks)packages/cli/examples/flow-order-complete.json(1 hunks)packages/cli/examples/flow-simple.json(1 hunks)packages/cli/examples/server-collect.json(2 hunks)packages/cli/package.json(0 hunks)packages/cli/src/__tests__/bundle/bundler-helpers.test.ts(0 hunks)packages/cli/src/__tests__/bundle/bundler.test.ts(2 hunks)packages/cli/src/__tests__/bundle/programmatic.test.ts(0 hunks)packages/cli/src/__tests__/bundle/serializer.test.ts(0 hunks)packages/cli/src/__tests__/bundle/template-engine.test.ts(0 hunks)packages/cli/src/__tests__/config-loader.test.ts(0 hunks)packages/cli/src/__tests__/push/fixtures/configs/server-basic.json(1 hunks)packages/cli/src/__tests__/push/fixtures/configs/web-basic.json(1 hunks)packages/cli/src/commands/bundle/bundler.ts(5 hunks)packages/cli/src/commands/bundle/serializer.ts(0 hunks)packages/cli/src/commands/bundle/template-engine.ts(0 hunks)packages/cli/src/config/build-defaults.ts(0 hunks)packages/cli/src/core/asset-resolver.ts(5 hunks)packages/cli/src/index.ts(2 hunks)packages/cli/src/types/bundle.ts(0 hunks)packages/cli/src/types/template.ts(0 hunks)packages/cli/templates/server.hbs(0 hunks)packages/cli/templates/web.hbs(0 hunks)packages/core/src/__tests__/flow.test.ts(5 hunks)packages/core/src/flow.ts(1 hunks)
💤 Files with no reviewable changes (13)
- packages/cli/src/tests/bundle/programmatic.test.ts
- packages/cli/src/config/build-defaults.ts
- packages/cli/package.json
- packages/cli/src/types/template.ts
- packages/cli/src/commands/bundle/serializer.ts
- packages/cli/templates/web.hbs
- packages/cli/src/tests/bundle/serializer.test.ts
- packages/cli/src/types/bundle.ts
- packages/cli/templates/server.hbs
- packages/cli/src/tests/bundle/template-engine.test.ts
- packages/cli/src/tests/config-loader.test.ts
- packages/cli/src/commands/bundle/template-engine.ts
- packages/cli/src/tests/bundle/bundler-helpers.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/index.ts
- apps/demos/flows/browser-to-bigquery.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/core/src/flow.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.tspackages/cli/src/core/asset-resolver.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENT.md)
packages/*/src/**/*.ts: Use Higher-Order Functions (HOF) and smart abstractions to avoid redundancy and keep implementations lean and performant
Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Files:
packages/core/src/flow.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.tspackages/cli/src/core/asset-resolver.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
packages/cli/examples/flow-simple.jsonpackages/cli/examples/flow-order-complete.jsonpackages/cli/src/__tests__/push/fixtures/configs/web-basic.jsonpackages/cli/src/commands/bundle/bundler.tspackages/cli/examples/server-collect.jsonpackages/cli/src/__tests__/push/fixtures/configs/server-basic.json
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
packages/cli/src/commands/bundle/bundler.tspackages/cli/src/__tests__/bundle/bundler.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use `import type` for type-only imports to ensure proper TypeScript compilation and tree-shaking
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
packages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
packages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/cli/src/__tests__/bundle/bundler.test.tspackages/core/src/__tests__/flow.test.ts
🧬 Code graph analysis (1)
packages/core/src/flow.ts (1)
packages/core/src/types/flow.ts (1)
Packages(37-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test
- GitHub Check: AWS Amplify Console Web Preview
- GitHub Check: Build and test
🔇 Additional comments (8)
packages/cli/src/core/asset-resolver.ts (2)
48-48: No action needed—all'template'AssetType usages have been removed.The removal of
'template'from theAssetTypeunion is confirmed as complete. No remaining code in the codebase attempts to pass'template'as an asset type, and no orphaned references exist.
25-43: Theexamples/directory change is correctly implemented with no outstanding concerns.The transition from
templates/toexamples/is cleanly applied in thegetAssetDir()function. The directory-walking algorithm will correctly traverse frompackages/cli/src/core/up through the package directory structure to locate theexamples/directory at the repository root. No remaining references to the oldtemplatesdirectory structure exist in the codebase, and theAssetTypedefinition has been properly updated to exclude the'template'option. The migration is complete and consistent.packages/core/src/flow.ts (1)
113-125: resolveCodeFromPackage behavior matches non-generative goalThe updated implementation now strictly preserves explicit
codevalues and never infers them frompackages, which aligns with the new tests and reduces surprising auto-magic. No issues spotted here.packages/core/src/__tests__/flow.test.ts (1)
1126-1206: getFlowConfig code-resolution tests align with new non-generative behaviorThese tests clearly encode the new contract that
codeis only present when explicitly provided, regardless ofpackagescontents, including the multi-source/destination case. This is a good regression guard for future changes to code resolution.packages/cli/src/__tests__/push/fixtures/configs/server-basic.json (1)
6-11: Fixture update is consistent with relaxed package metadataUsing an empty object for
@walkeros/destination-demokeeps the key present while delegating version/import semantics to the tooling, and the test destination still referencescode: "destinationDemo". This looks consistent with the new configuration model.packages/cli/src/__tests__/push/fixtures/configs/web-basic.json (1)
6-11: Web push fixture change mirrors server-basic and looks safeAligning the web fixture with the server one by making
@walkeros/destination-demoan empty object keeps tests focused on push behavior rather than version/import details. No functional issues from this change alone.packages/cli/examples/server-collect.json (1)
7-16: Explicit code + package wiring matches new flow semanticsThis example now encodes
codedirectly on the source/destination while keeping version info inpackages, which lines up with the non-generativeresolveCodeFromPackageand the new bundler behavior around explicit code imports. This is a clear, self-contained example of the intended pattern.Also applies to: 19-36
packages/cli/src/__tests__/bundle/bundler.test.ts (1)
3-8: Importing new bundler helpers keeps tests close to real behaviorPulling
buildConfigObject,generatePlatformWrapper, andcreateEntryPointfrom the same module asbundleensures the tests exercise the actual public surface of the bundler. This is a good place to validate the new code-generation pipeline.
| export async function createEntryPoint( | ||
| flowConfig: Flow.Config, | ||
| buildOptions: BuildOptions, | ||
| packagePaths: Map<string, string>, | ||
| ): Promise<string> { | ||
| // Detect destination packages for auto-importing examples | ||
| // Detect packages used by destinations and sources | ||
| const destinationPackages = detectDestinationPackages(flowConfig); | ||
| const sourcePackages = detectSourcePackages(flowConfig); | ||
| const explicitCodeImports = detectExplicitCodeImports(flowConfig); | ||
|
|
||
| // Generate import statements (examples generation removed) | ||
| // Generate import statements | ||
| const { importStatements } = generateImportStatements( | ||
| buildOptions.packages, | ||
| destinationPackages, | ||
| sourcePackages, | ||
| explicitCodeImports, | ||
| ); | ||
|
|
||
| // No longer generate examples in bundles - simulator loads them dynamically | ||
| const examplesObject = ''; | ||
| const importsCode = importStatements.join('\n'); | ||
| const hasFlow = destinationPackages.size > 0 || sourcePackages.size > 0; | ||
|
|
||
| // Process template or use code directly | ||
| const templatedCode = await processTemplate(flowConfig, buildOptions); | ||
| // If no sources/destinations, just return user code with imports (no flow wrapper) | ||
| if (!hasFlow) { | ||
| const userCode = buildOptions.code || ''; | ||
| return importsCode ? `${importsCode}\n\n${userCode}` : userCode; | ||
| } | ||
|
|
||
| // Wrap code for specific output format | ||
| const wrappedCode = wrapCodeForFormat( | ||
| templatedCode, | ||
| buildOptions.format, | ||
| !!buildOptions.template, | ||
| // Build config object programmatically (DRY - single source of truth) | ||
| const configObject = buildConfigObject(flowConfig, explicitCodeImports); | ||
|
|
||
| // Generate platform-specific wrapper | ||
| const wrappedCode = generatePlatformWrapper( | ||
| configObject, | ||
| buildOptions.code || '', | ||
| buildOptions as { | ||
| platform: string; | ||
| windowCollector?: string; | ||
| windowElb?: string; | ||
| }, | ||
| ); | ||
|
|
||
| // Assemble final entry point code | ||
| return assembleFinalCode( | ||
| importStatements, | ||
| examplesObject, | ||
| wrappedCode, | ||
| buildOptions.format, | ||
| ); | ||
| // Assemble final code | ||
| return importsCode ? `${importsCode}\n\n${wrappedCode}` : wrappedCode; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/commands/bundle/bundler.ts | sed -n '600,680p'Repository: elbwalker/walkerOS
Length of output: 3153
🏁 Script executed:
cat -n packages/cli/src/commands/bundle/bundler.ts | sed -n '1,100p'Repository: elbwalker/walkerOS
Length of output: 3897
🏁 Script executed:
rg "generateImportStatements" packages/cli/src/commands/bundle/ -A 10Repository: elbwalker/walkerOS
Length of output: 1855
🏁 Script executed:
fd -t f "\.ts" packages/cli/src/commands/bundle/Repository: elbwalker/walkerOS
Length of output: 241
🏁 Script executed:
find packages/cli -name "*bundler*test*" -o -name "*bundler*spec*"Repository: elbwalker/walkerOS
Length of output: 169
🏁 Script executed:
tail -100 packages/cli/src/commands/bundle/bundler.tsRepository: elbwalker/walkerOS
Length of output: 2869
🏁 Script executed:
find packages -type f -name "*.ts" | grep bundle | head -20Repository: elbwalker/walkerOS
Length of output: 507
🏁 Script executed:
git ls-files | grep bundlerRepository: elbwalker/walkerOS
Length of output: 213
🏁 Script executed:
find . -type d -name packagesRepository: elbwalker/walkerOS
Length of output: 72
🏁 Script executed:
pwd && ls -laRepository: elbwalker/walkerOS
Length of output: 2057
🏁 Script executed:
sed -n '450,550p' packages/cli/src/commands/bundle/bundler.tsRepository: elbwalker/walkerOS
Length of output: 3235
🏁 Script executed:
sed -n '550,612p' packages/cli/src/commands/bundle/bundler.tsRepository: elbwalker/walkerOS
Length of output: 2398
🏁 Script executed:
rg "startFlow" packages/cli/src --type ts -B 2 -A 2 | head -80Repository: elbwalker/walkerOS
Length of output: 5240
🏁 Script executed:
grep -n "packages\|buildOptions\|createEntryPoint" packages/cli/src/__tests__/bundle/bundler.test.ts | head -30Repository: elbwalker/walkerOS
Length of output: 1426
🏁 Script executed:
grep -n "startFlow\|export" packages/core/src/index.ts | head -20Repository: elbwalker/walkerOS
Length of output: 682
🏁 Script executed:
find packages/core -type f -name "*.ts" | grep -v test | head -20Repository: elbwalker/walkerOS
Length of output: 683
🏁 Script executed:
find packages/core -name "*.ts" -type f | xargs grep -l "Config\|config" | head -5Repository: elbwalker/walkerOS
Length of output: 242
🏁 Script executed:
ls -la packages/cli/src/config/Repository: elbwalker/walkerOS
Length of output: 478
🏁 Script executed:
cat packages/cli/src/types/bundle.tsRepository: elbwalker/walkerOS
Length of output: 3349
🏁 Script executed:
cat packages/cli/src/config/build-defaults.tsRepository: elbwalker/walkerOS
Length of output: 1941
🏁 Script executed:
find packages/cli -name "*.md" -type f | xargs cat 2>/dev/nullRepository: elbwalker/walkerOS
Length of output: 22781
Correct startFlow import source and clarify required configuration
generatePlatformWrapper unconditionally calls startFlow(config) in both browser and server branches, but startFlow must be imported from @walkeros/collector, not @walkeros/core. The function will only be available if:
@walkeros/collectoris explicitly included inbuildOptions.packageswithimports: ['startFlow']configured, OR- The user code imports it directly
This is documented as a required pattern (all examples show it), but there is no validation to prevent incomplete configurations. If a developer omits the collector package or forgets the import declaration, the generated bundle will throw ReferenceError: startFlow is not defined at runtime. Consider adding validation to ensure @walkeros/collector with startFlow import is present when hasFlow is true.
Additionally, hasFlow only checks for destination and source packages. Flows with only collector configuration (no destinations/sources) will take the early return path (line 640-642) without calling generatePlatformWrapper, silently skipping the collector execution. Document whether collector-only flows are unsupported or if having at least one destination/source is a requirement.
Suggested improvements:
- Add validation that
@walkeros/collectoris in packages withstartFlowimport when destinations/sources exist - Clarify collector-only flow behavior: either support it by updating
hasFlowlogic or document the requirement - Improve error messages if packages are misconfigured
| /** | ||
| * Convert package name to valid JavaScript variable name. | ||
| * Used for deterministic default import naming. | ||
| * @example | ||
| * packageNameToVariable('@walkeros/server-destination-api') | ||
| * // → '_walkerosServerDestinationApi' | ||
| */ | ||
| export function packageNameToVariable(packageName: string): string { | ||
| const hasScope = packageName.startsWith('@'); | ||
| const normalized = packageName | ||
| .replace('@', '') | ||
| .replace(/[/-]/g, '_') | ||
| .split('_') | ||
| .filter((part) => part.length > 0) | ||
| .map((part, i) => | ||
| i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1), | ||
| ) | ||
| .join(''); | ||
|
|
||
| return hasScope ? '_' + normalized : normalized; | ||
| } |
There was a problem hiding this comment.
Ensure packageNameToVariable always returns a valid JS identifier
For an unscoped package like "123-utils", this will currently produce "123Utils", which is not a valid identifier and would break generated import code. Scoped numeric names ("@123/utils") are safe because of the leading _, but unscoped ones are not.
You can harden this by normalizing the leading @ more explicitly and then guarding the first character:
-export function packageNameToVariable(packageName: string): string {
- const hasScope = packageName.startsWith('@');
- const normalized = packageName
- .replace('@', '')
- .replace(/[/-]/g, '_')
- .split('_')
- .filter((part) => part.length > 0)
- .map((part, i) =>
- i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1),
- )
- .join('');
-
- return hasScope ? '_' + normalized : normalized;
-}
+export function packageNameToVariable(packageName: string): string {
+ const hasScope = packageName.startsWith('@');
+ const normalized = packageName
+ .replace(/^@/, '')
+ .replace(/[/-]/g, '_')
+ .split('_')
+ .filter((part) => part.length > 0)
+ .map((part, i) =>
+ i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1),
+ )
+ .join('');
+
+ const base = hasScope ? `_${normalized}` : normalized;
+ return /^[A-Za-z_$]/.test(base) ? base : `_${base}`;
+}📝 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.
| /** | |
| * Convert package name to valid JavaScript variable name. | |
| * Used for deterministic default import naming. | |
| * @example | |
| * packageNameToVariable('@walkeros/server-destination-api') | |
| * // → '_walkerosServerDestinationApi' | |
| */ | |
| export function packageNameToVariable(packageName: string): string { | |
| const hasScope = packageName.startsWith('@'); | |
| const normalized = packageName | |
| .replace('@', '') | |
| .replace(/[/-]/g, '_') | |
| .split('_') | |
| .filter((part) => part.length > 0) | |
| .map((part, i) => | |
| i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1), | |
| ) | |
| .join(''); | |
| return hasScope ? '_' + normalized : normalized; | |
| } | |
| /** | |
| * Convert package name to valid JavaScript variable name. | |
| * Used for deterministic default import naming. | |
| * @example | |
| * packageNameToVariable('@walkeros/server-destination-api') | |
| * // → '_walkerosServerDestinationApi' | |
| */ | |
| export function packageNameToVariable(packageName: string): string { | |
| const hasScope = packageName.startsWith('@'); | |
| const normalized = packageName | |
| .replace(/^@/, '') | |
| .replace(/[/-]/g, '_') | |
| .split('_') | |
| .filter((part) => part.length > 0) | |
| .map((part, i) => | |
| i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1), | |
| ) | |
| .join(''); | |
| const base = hasScope ? `_${normalized}` : normalized; | |
| return /^[A-Za-z_$]/.test(base) ? base : `_${base}`; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/flow.ts (1)
98-111: Harden identifier generation for edge cases.As flagged in a previous review, this function can generate invalid JavaScript identifiers for unscoped packages starting with digits (e.g.,
"123-utils"→"123Utils"). While scoped packages like"@123/utils"are safe due to the leading underscore, unscoped numeric-leading names are not.Apply the previously suggested fix to guard against invalid identifiers:
export function packageNameToVariable(packageName: string): string { const hasScope = packageName.startsWith('@'); const normalized = packageName - .replace('@', '') + .replace(/^@/, '') .replace(/[/-]/g, '_') .split('_') .filter((part) => part.length > 0) .map((part, i) => i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1), ) .join(''); - return hasScope ? '_' + normalized : normalized; + const base = hasScope ? `_${normalized}` : normalized; + return /^[A-Za-z_$]/.test(base) ? base : `_${base}`; }packages/cli/src/commands/bundle/bundler.ts (1)
628-671: Add validation for requiredstartFlowimport.The refactored
createEntryPointis cleaner and more maintainable, but the issue from the previous review remains:generatePlatformWrapperunconditionally callsstartFlow(config)without validating that@walkeros/collectoris present with thestartFlowimport.Add validation after line 647:
const importsCode = importStatements.join('\n'); const hasFlow = destinationPackages.size > 0 || sourcePackages.size > 0; + // Validate required startFlow import when flow exists + if (hasFlow) { + const collectorPkg = buildOptions.packages['@walkeros/collector']; + if (!collectorPkg || !collectorPkg.imports?.includes('startFlow')) { + throw new Error( + 'Flow configuration requires @walkeros/collector with startFlow import. ' + + 'Add "@walkeros/collector": { "imports": ["startFlow"] } to packages config.' + ); + } + } + // If no sources/destinations, just return user code with imports (no flow wrapper) if (!hasFlow) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli/src/commands/bundle/bundler.ts(5 hunks)packages/core/src/__tests__/flow.test.ts(5 hunks)packages/core/src/flow.ts(1 hunks)packages/core/src/schemas/flow.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
packages/core/src/schemas/flow.tspackages/core/src/flow.tspackages/core/src/__tests__/flow.test.tspackages/cli/src/commands/bundle/bundler.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENT.md)
packages/*/src/**/*.ts: Use Higher-Order Functions (HOF) and smart abstractions to avoid redundancy and keep implementations lean and performant
Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Implement event mapping with flexible patterns: exact entity-action matches, wildcard patterns using '*', conditional mappings with functions, and value mapping strategies including string key extraction, static values, custom functions, object transformation, array processing, and consent-based mapping
Files:
packages/core/src/schemas/flow.tspackages/core/src/flow.tspackages/core/src/__tests__/flow.test.tspackages/cli/src/commands/bundle/bundler.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.test.{ts,tsx}: Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Files:
packages/core/src/__tests__/flow.test.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Before documenting code patterns, create functional, executable tests to validate them - use /workspaces/walkerOS/apps/quickstart as the source of truth for working examples
Applied to files:
packages/core/src/__tests__/flow.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external APIs in destination tests (gtag, fbq, etc.) and verify they are called correctly with the expected data - focus on verifying external API calls, not return values
Applied to files:
packages/core/src/__tests__/flow.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.test.{ts,tsx} : Write component-level integration tests with mocked external APIs and boundaries: test destinations by mocking external APIs (gtag, fbq, etc.), test collector with mocked destinations and sources, test sources with mocked collector interface
Applied to files:
packages/core/src/__tests__/flow.test.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use core functions from walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
📚 Learning: 2025-11-25T16:25:10.439Z
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to **/*.{ts,tsx} : Use `import type` for type-only imports to ensure proper TypeScript compilation and tree-shaking
Applied to files:
packages/cli/src/commands/bundle/bundler.ts
🧬 Code graph analysis (2)
packages/core/src/flow.ts (1)
packages/core/src/types/flow.ts (1)
Packages(37-44)
packages/core/src/__tests__/flow.test.ts (1)
packages/core/src/flow.ts (2)
packageNameToVariable(98-111)getFlowConfig(192-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test
- GitHub Check: AWS Amplify Console Web Preview
- GitHub Check: Build and test
🔇 Additional comments (7)
packages/core/src/schemas/flow.ts (1)
115-120: LGTM! Schema additions are well-structured.The optional
codefield additions to bothSourceReferenceSchemaandDestinationReferenceSchemaare well-documented and appropriately optional. The descriptions clearly explain the purpose and default behavior.Also applies to: 160-165
packages/core/src/flow.ts (1)
113-132: LGTM! Code resolution logic is sound.The updated
resolveCodeFromPackagecorrectly prioritizes explicit code and falls back to auto-generated names from package names. The logic is clear and handles the undefined cases appropriately.packages/core/src/__tests__/flow.test.ts (1)
1301-1396: LGTM! Comprehensive test coverage for code resolution.The new tests thoroughly cover the
resolveCodeFromPackagebehavior including:
- Auto-generation when code is not provided
- Explicit code preservation
- Named exports handling
- Multiple destinations sharing the same package
The tests are functional and executable, aligning with the codebase testing patterns.
packages/cli/src/commands/bundle/bundler.ts (4)
433-458: LGTM! Source package detection is well-implemented.The
detectSourcePackagesfunction correctly mirrors thedetectDestinationPackagespattern, extracting package names from sources with explicit 'package' fields.
547-616: LGTM! Import generation logic handles all cases correctly.The updated
generateImportStatementsproperly handles three distinct import scenarios:
- Explicit imports from package config (utilities like
startFlow)- Explicit code imports from destinations/sources
- Auto-generated default imports for packages used by destinations/sources
The logic correctly skips unused packages, maintaining optimal bundle size.
717-795: LGTM! Config serialization is clean and maintainable.The
buildConfigObjectandprocessConfigValuefunctions provide a clean programmatic approach to config serialization. The logic correctly distinguishes between explicit and auto-generated codes, and the JSON serialization is appropriate for config values.
797-841: LGTM! Platform wrappers are well-structured.The
generatePlatformWrapperfunction cleanly separates browser and server concerns:
- Browser: IIFE with optional window assignments for
collectorandelb- Server: Default export function with context parameter for extensibility
Both wrappers correctly inject user code and call
startFlow, though validation for thestartFlowimport should be added upstream (as noted in thecreateEntryPointcomment).
| // Only treat as explicit if code doesn't match auto-generated pattern | ||
| // Auto-generated code starts with '_' (from packageNameToVariable) | ||
| const isAutoGenerated = destConfig.code.startsWith('_'); | ||
| if (!isAutoGenerated) { | ||
| if (!explicitCodeImports.has(destConfig.package)) { | ||
| explicitCodeImports.set(destConfig.package, new Set()); | ||
| } | ||
| explicitCodeImports.get(destConfig.package)!.add(destConfig.code); | ||
| } |
There was a problem hiding this comment.
Fragile auto-generated code detection.
The logic to distinguish explicit from auto-generated codes assumes all auto-generated identifiers start with _ (lines 486, 514). However, packageNameToVariable only prefixes scoped packages (e.g., @walkeros/foo → _walkerosFoo), leaving unscoped packages unchanged (e.g., lodash → lodash).
This means auto-generated codes for unscoped packages will be incorrectly treated as explicit imports, potentially generating named imports instead of default imports.
Consider one of these approaches:
- Track auto-generated codes explicitly: Pass a set of auto-generated codes from
resolveCodeFromPackageto avoid inference - Normalize all auto-generated codes: Always prefix with
_inpackageNameToVariable, even for unscoped packages - Document the limitation: If unscoped packages are not expected in walkerOS flows, document this assumption
Example fix (option 2):
export function packageNameToVariable(packageName: string): string {
- const hasScope = packageName.startsWith('@');
const normalized = packageName
.replace(/^@/, '')
.replace(/[/-]/g, '_')
// ... rest of normalization
- return hasScope ? `_${normalized}` : normalized;
+ // Always prefix to ensure consistent detection of auto-generated codes
+ return `_${normalized}`;
}Also applies to: 512-520
🤖 Prompt for AI Agents
In packages/cli/src/commands/bundle/bundler.ts around lines 484-492 (and also
update usage at ~512-520), the code incorrectly infers auto-generated
identifiers by checking for a leading '_' which fails for unscoped packages;
update the flow so auto-generated codes are tracked explicitly instead of
inferred: change resolveCodeFromPackage (or its caller) to return (or populate)
a Set of autoGeneratedCodes alongside the resolved code, then replace the
startsWith('_') checks with membership checks against that Set (use the Set when
deciding whether to add to explicitCodeImports), and propagate this Set to any
other places that currently rely on the '_' heuristic.
| describe('packageNameToVariable', () => { | ||
| test('converts scoped package names to valid variable names', () => { | ||
| expect(packageNameToVariable('@walkeros/server-destination-api')).toBe( | ||
| '_walkerosServerDestinationApi', | ||
| ); | ||
| }); | ||
|
|
||
| test('converts unscoped package names', () => { | ||
| expect(packageNameToVariable('lodash')).toBe('lodash'); | ||
| }); | ||
|
|
||
| test('handles multiple hyphens and slashes', () => { | ||
| expect(packageNameToVariable('@custom/my-helper')).toBe('_customMyHelper'); | ||
| expect(packageNameToVariable('@scope/package-name-test')).toBe( | ||
| '_scopePackageNameTest', | ||
| ); | ||
| }); | ||
|
|
||
| test('handles package names with numbers', () => { | ||
| expect(packageNameToVariable('@walkeros/web-destination-gtag')).toBe( | ||
| '_walkerosWebDestinationGtag', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test for numeric-leading unscoped package names.
The test suite covers scoped packages, unscoped packages, hyphens, and numbers, but doesn't test the edge case of unscoped packages starting with digits (e.g., "123-utils"), which would currently generate invalid JavaScript identifiers like "123Utils".
Add this test case to verify the fix suggested for packageNameToVariable:
test('handles package names with numbers', () => {
expect(packageNameToVariable('@walkeros/web-destination-gtag')).toBe(
'_walkerosWebDestinationGtag',
);
});
+
+ test('handles unscoped packages starting with digits', () => {
+ expect(packageNameToVariable('123-utils')).toBe('_123Utils');
+ expect(packageNameToVariable('9to5-scheduler')).toBe('_9to5Scheduler');
+ });
});🤖 Prompt for AI Agents
In packages/core/src/__tests__/flow.test.ts around lines 1272 to 1294, the suite
lacks a test for unscoped package names that start with digits; add a test case
inside the existing describe('packageNameToVariable') block that calls
packageNameToVariable('123-utils') (and/or similar) and asserts the result is a
valid identifier (e.g. '_123Utils'), thereby verifying the implementation
prefixes numeric-leading unscoped names with an underscore to produce a valid JS
variable name.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/docs/guides/session.mdx (1)
122-122: Consider using "whether" for better formality.Based on learnings from static analysis tools. When presenting alternatives with "or not", "whether" is more formal than "if" in technical documentation.
Apply this diff:
-is used to decide if storage access is allowed or not. If set, it registers an +is used to decide whether storage access is allowed or not. If set, it registers an
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/docs/guides/session.mdx(1 hunks)website/sidebars.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENT.md)
**/*.{ts,tsx}: NEVER useanytype - always use proper TypeScript types from @walkeros/core or package-specific types
Implement event structure with consistent properties: name (ENTITY ACTION format), data (entity-specific), context (state/environment), globals (global properties), user (identification), nested (related entities), consent, and system-generated fields (id, timestamp, entity, action)
Use core functions from @walkeros/core for common operations: getEvent(), createEvent() for event creation, getMappingEvent(), getMappingValue() for transformations, isString(), isObject(), isDefined() for type checking, assign(), clone() for object operations, tryCatch(), tryCatchAsync() for error handling
Useimport typefor type-only imports to ensure proper TypeScript compilation and tree-shaking
Avoid inline change comments like '// Fixed for version X' or '// Changed due to Y' - Git tracks history, use migrations and documentation for breaking changes instead
Use Root-level Turborepo commands for parallel operations across packages, navigate to individual packages for focused development, and test changes across all affected packages due to core package dependencies
Files:
website/sidebars.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: elbwalker/walkerOS PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-25T16:25:10.439Z
Learning: Applies to packages/*/src/**/*.ts : Implement Destination interface with standard structure: type, init(), push(), pushBatch() methods and config object with optional settings, mapping, data, consent, policy, queue, and dryRun properties
🪛 LanguageTool
website/docs/guides/session.mdx
[style] ~122-~122: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...tecting a new session is used to decide if storage access is allowed or not. If se...
(IF_WHETHER)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (4)
website/sidebars.ts (1)
127-127: LGTM! Documentation properly linked.The new session guide is correctly added to the sidebar navigation and follows the established pattern for guide entries.
website/docs/guides/session.mdx (3)
1-408: Excellent documentation structure and content.The session guide is comprehensive, well-organized, and includes:
- Clear explanations of session detection modes
- Detailed configuration tables
- Practical code examples
- Visual flowcharts for detection processes
- Appropriate warnings and notes for consent/storage
The documentation effectively guides users through session management features.
123-124: Cross-reference link is valid.The documentation reference to
/docs/collector/commands#oncorrectly points to the "## on" heading inwebsite/docs/collector/commands.mdx(line 85). No issues found.
11-14: The PackageLink pathpackages/web/core/src/sessionis valid and exists in the repository.
Summary by CodeRabbit
Documentation
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.