refactor ai and sync engine - #123
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtracted piece registry and metadata into a new package ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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)
docs/architecture/master/tasks.md (1)
35-44:⚠️ Potential issue | 🟡 MinorDocument still has stale package name references elsewhere.
Good update at Line 35 and Line 44, but this file still lists
infra-adapterslater (Line 790), which conflicts with the newpackages/infranaming. Please align that stale reference in the same doc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/master/tasks.md` around lines 35 - 44, The document contains stale package name references: replace any occurrences of the old "infra-adapters" name with the new "packages/infra" naming to keep the doc consistent (search for "infra-adapters" around the later sections, e.g., where adapters/files are listed) and update related mentions such as folder listings and package names in the section that currently references "infra-adapters" so they match the entries like `packages/infra/src/encryption/**`, `packages/infra/src/index.ts`, and `packages/infra/package.json`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/stitches.module.ts`:
- Around line 6-12: Combine the two separate imports from the same package into
a single import statement: replace the individual imports of PiecesModule and
MetadataModule from "@nexiom/piece-registry" with one consolidated import that
includes both symbols (PiecesModule, MetadataModule); ensure other imports
(ConnectionsModule, StitchesController, StitchesAdminController,
FieldMappingsController, StitchesService) remain unchanged and that any module
references in the file still match the consolidated names.
In `@docs/architecture/master/implementation-plan.md`:
- Line 738: Update the architecture doc entry for packages/infra to remove or
clarify the "SQS client factory" responsibility: edit the table row listing
`packages/infra` so it only mentions `LocalCryptoAdapter` and `AwsKmsAdapter`
(or add a parenthetical note that SQS client creation lives in
`packages/queue`), ensuring the phrase "SQS client factory" is deleted or
replaced to reflect that queue responsibilities are owned by `packages/queue`;
verify references to `LocalCryptoAdapter` and `AwsKmsAdapter` remain unchanged.
In `@docs/architecture/sync_strategy/sync_strategy.md`:
- Line 441: The doc has inconsistent package naming: the Platform row uses
`packages/infra` while the directory layout still lists
`packages/infra-adapters`; pick one canonical package name (prefer the new
`packages/infra`) and update all occurrences to match. Edit the table row that
currently shows `engine/platform/core/ + packages/infra + packages/cache` and
the directory layout entry that references `packages/infra-adapters/` so both
use the same package identifier (`packages/infra`), and ensure any descriptive
text mentioning infra adapters is adjusted to the chosen name (search for
`packages/infra-adapters` and `packages/infra` and normalize).
In `@engine/ai/core/package.json`:
- Around line 28-31: Update the zod dependency in package.json: replace the
current "zod": "^3.24.1" entry with a supported version (e.g., "zod": "^3.25.76"
or bump to "^4.1.8") so it satisfies the peer range expected by the ai package;
run your lockfile update (npm/yarn/pnpm) afterwards to ensure the new version is
installed and type checks pick up the stricter Zod typings.
In `@engine/ai/core/src/services/orchestrator.service.ts`:
- Around line 204-206: The tool definitions currently use unsafe `as any` casts
for inputSchema and handler args; extract each tool's z.object(...) schema into
a named constant (e.g., hydratorInputSchema, actionExecutorInputSchema), set the
tool's inputSchema to that constant (no cast), and change the handler signature
to use z.infer<typeof thatSchema> for args (for example: execute: async (args:
z.infer<typeof hydratorInputSchema>) => { ... }) so the handler has a statically
typed contract; update both occurrences referenced (the hydrator tool and the
action executor tool) and remove the `as any` casts on inputSchema and args.
---
Outside diff comments:
In `@docs/architecture/master/tasks.md`:
- Around line 35-44: The document contains stale package name references:
replace any occurrences of the old "infra-adapters" name with the new
"packages/infra" naming to keep the doc consistent (search for "infra-adapters"
around the later sections, e.g., where adapters/files are listed) and update
related mentions such as folder listings and package names in the section that
currently references "infra-adapters" so they match the entries like
`packages/infra/src/encryption/**`, `packages/infra/src/index.ts`, and
`packages/infra/package.json`.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 739690a0-6c74-4ff9-a365-f4e0daa54cd1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (110)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/modules/ai/ai.module.tsapps/api/src/modules/ai/controllers/ai.controller.spec.tsapps/api/src/modules/ai/controllers/ai.controller.tsapps/api/src/modules/ai/interceptors/ai-ratelimit.guard.tsapps/api/src/modules/ai/interceptors/ai-telemetry.interceptor.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/callback.controller.spec.tsapps/api/src/modules/connections/connections/callback.controller.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connections/token-refresh.service.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/metadata/metadata.controller.spec.tsapps/api/src/modules/metadata/metadata.controller.tsapps/api/src/modules/metadata/metadata.module.tsapps/api/src/modules/scheduler/poll-sync-runner.spec.tsapps/api/src/modules/scheduler/poll-sync-runner.tsapps/api/src/modules/scheduler/scheduler.module.tsapps/api/src/modules/stitches/stitches.module.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/poller.service.spec.tsapps/api/src/modules/trigger/poller.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/modules/trigger/webhooks.controller.spec.tsapps/api/src/modules/trigger/webhooks.controller.tsapps/api/src/modules/webhooks/webhook-signature.guard.spec.tsapps/api/src/modules/webhooks/webhook-signature.guard.tsapps/api/src/modules/webhooks/webhooks.module.tsapps/worker/package.jsonapps/worker/src/app.module.tsapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/normalization.service.spec.tsapps/worker/src/modules/pipeline/normalization.service.tsapps/worker/src/modules/pipeline/pipeline.module.tsdocs/architecture/master/implementation-plan.mddocs/architecture/master/tasks.mddocs/architecture/sync_strategy/sync_strategy.mdengine/ai/core/package.jsonengine/ai/core/src/ai-engine.module.tsengine/ai/core/src/constants/prompts.tsengine/ai/core/src/index.tsengine/ai/core/src/services/orchestrator.service.spec.tsengine/ai/core/src/services/orchestrator.service.tsengine/ai/core/src/types/chat-request.types.tsengine/ai/core/tsconfig.jsonengine/ai/core/vitest.config.tsengine/sync/application/README.mdengine/sync/application/mapping/package.jsonengine/sync/application/mapping/src/config-applicator.tsengine/sync/application/mapping/src/index.tsengine/sync/application/mapping/src/jsonata-extensions.tsengine/sync/application/mapping/src/mapping-engine.spec.tsengine/sync/application/mapping/src/mapping-engine.tsengine/sync/application/mapping/src/mapping.types.tsengine/sync/application/mapping/tsconfig.jsonengine/sync/application/mapping/vitest.config.tsengine/sync/platform/README.mdengine/sync/platform/core/package.jsonengine/sync/platform/core/src/evaluator.tsengine/sync/platform/core/src/hydrator.tsengine/sync/platform/core/src/index.tsengine/sync/platform/core/src/state/cursor-manager.service.spec.tsengine/sync/platform/core/src/state/cursor-manager.service.tsengine/sync/platform/core/src/state/cursor-manager.types.tsengine/sync/platform/core/src/storage-resolver/storage-resolver.module.tsengine/sync/platform/core/src/storage-resolver/storage-resolver.service.spec.tsengine/sync/platform/core/src/storage-resolver/storage-resolver.service.tsengine/sync/platform/core/tsconfig.jsonengine/sync/platform/core/vitest.config.tspackages/credentials/package.jsonpackages/credentials/src/crypto/encryption.interface.tspackages/credentials/src/crypto/encryption.service.spec.tspackages/credentials/src/crypto/encryption.service.tspackages/credentials/src/http-client/host-http-client.spec.tspackages/credentials/src/http-client/host-http-client.tspackages/credentials/src/index.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/tsconfig.jsonpackages/credentials/tsconfig.spec.jsonpackages/credentials/vitest.config.tspackages/infra/eslint.config.mjspackages/infra/package.jsonpackages/infra/src/adapters/aws-kms.adapter.tspackages/infra/src/adapters/local-crypto.adapter.tspackages/infra/src/aws-kms.adapter.spec.tspackages/infra/src/constants.tspackages/infra/src/encryption.module.spec.tspackages/infra/src/encryption.module.tspackages/infra/src/index.tspackages/infra/src/interfaces/encryption-service.interface.tspackages/infra/src/local-crypto.adapter.spec.tspackages/infra/tsconfig.jsonpackages/infra/vitest.config.tspackages/pieces/registry/package.jsonpackages/pieces/registry/src/index.tspackages/pieces/registry/src/metadata/metadata-discovery.service.spec.tspackages/pieces/registry/src/metadata/metadata-discovery.service.tspackages/pieces/registry/src/metadata/metadata.module.tspackages/pieces/registry/src/pieces/piece-loader.service.tspackages/pieces/registry/src/pieces/piece-registry.service.spec.tspackages/pieces/registry/src/pieces/piece-registry.service.tspackages/pieces/registry/src/pieces/pieces.module.tspackages/pieces/registry/tsconfig.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (3)
- apps/api/src/modules/metadata/metadata.module.ts
- apps/api/src/modules/metadata/metadata.controller.spec.ts
- apps/api/src/modules/metadata/metadata.controller.ts
| | Concern | Code | Responsibility | | ||
| | :--- | :--- | :--- | | ||
| | **Platform** | `engine/platform/core/` + `packages/infra-adapters` + `packages/cache` | Manages AWS KMS decryption of tenant credentials, acquires the Redis Distributed Refresh Lock, hosts the Activepieces runtime environment that runs piece actions | | ||
| | **Platform** | `engine/platform/core/` + `packages/infra` + `packages/cache` | Manages AWS KMS decryption of tenant credentials, acquires the Redis Distributed Refresh Lock, hosts the Activepieces runtime environment that runs piece actions | |
There was a problem hiding this comment.
This rename is correct, but the same file still has an old path reference.
Line 441 now uses packages/infra, but the directory layout section still lists packages/infra-adapters/ (Line 66). Please normalize both sections to one package name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/sync_strategy/sync_strategy.md` at line 441, The doc has
inconsistent package naming: the Platform row uses `packages/infra` while the
directory layout still lists `packages/infra-adapters`; pick one canonical
package name (prefer the new `packages/infra`) and update all occurrences to
match. Edit the table row that currently shows `engine/platform/core/ +
packages/infra + packages/cache` and the directory layout entry that references
`packages/infra-adapters/` so both use the same package identifier
(`packages/infra`), and ensure any descriptive text mentioning infra adapters is
adjusted to the chosen name (search for `packages/infra-adapters` and
`packages/infra` and normalize).
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 6 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 6 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/modules/stitches/stitches.module.ts (1)
14-21: 🧹 Nitpick | 🔵 TrivialOptional: Consider removing redundant
PiecesModuleimport.Since
PiecesModuleis registered as global in the app module (viaPiecesModule.forRoot({ anchorUrl: import.meta.url })), explicitly importing it here on line 18 is redundant. Global modules in NestJS are automatically available to all modules without explicit imports.However, this redundancy pre-existed the refactor and maintaining it doesn't cause any functional issues. Consider removing it in a future cleanup PR to simplify the module dependencies.
♻️ Optional refactor to remove redundant import
`@Module`({ imports: [ DbModule, AuthModule, CacheModule, - PiecesModule, ConnectionsModule, MetadataModule, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/stitches/stitches.module.ts` around lines 14 - 21, The PiecesModule import is redundant because it's registered global; remove its explicit reference from the imports array in the StitchesModule (delete the PiecesModule entry from the imports: [...] list) and also remove any corresponding unused top-level import statement for PiecesModule to keep the file clean (look for the PiecesModule identifier in this module file and remove both the array entry and the import declaration).engine/ai/core/src/services/orchestrator.service.ts (1)
482-498:⚠️ Potential issue | 🟠 MajorDo not forward
confirmedinto connector action props.At Line 497,
propsValuereceives the fullargs, which includes orchestration-onlyconfirmed. That can break strict action prop validation or create unintended behavior downstream.♻️ Proposed fix
- const argsWithConfirm = args as Record<string, unknown> & { + const argsWithConfirm = args as Record<string, unknown> & { confirmed?: boolean; }; if (argsWithConfirm.confirmed !== true) { return { success: false, error: 'Action requires explicit user confirmation. Please set confirmed=true to proceed.', connectionName: `connection-${conn.id}`, }; } try { + const { confirmed: _confirmed, ...propsValue } = argsWithConfirm; const result = (await action.run({ auth: creds, - propsValue: args as Record<string, unknown>, + propsValue, })) as unknown;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/ai/core/src/services/orchestrator.service.ts` around lines 482 - 498, The call to action.run forwards orchestration-only flag confirmed inside propsValue (args), which can break action prop validation; in the orchestrator where argsWithConfirm is defined, strip out confirmed before calling action.run (e.g., extract confirmed via const { confirmed, ...props } = argsWithConfirm) and pass props (not the original args) as propsValue to action.run; update any typing to keep propsValue as Record<string, unknown> so the action receives only its intended properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/master/implementation-plan.md`:
- Line 811: The file currently ends immediately after the line "Then open
`http://localhost:3000` → Route Intelligence → see the L1→L6 green trace."
without a trailing newline; add a single newline character at the end of the
file (after the closing backticks) so the document ends with one trailing
newline to satisfy MD047 and standard POSIX/markdown newline expectations.
In `@docs/architecture/sync_strategy/sync_strategy.md`:
- Line 466: The file ends without a trailing newline causing MD047; add a single
newline character at EOF so the last table row ("L6 | Destination Gateway |
`outbound_gateway` log, GEM update | — (standardised) |") is followed by one
newline (i.e., ensure the file ends with "\n") to satisfy markdown compliance.
In `@engine/ai/core/src/services/orchestrator.service.ts`:
- Around line 199-207: The execute handler created in dynamicTool currently
forwards the entire args (z.infer<typeof hydratorInputSchema>) including the
orchestration-only confirmed field to action.run; remove confirmed before
calling action.run by destructuring or copying args and deleting or omitting the
confirmed property, then call action.run({ propsValue: sanitizedArgs }) so
confirmed never reaches the action layer (update the execute implementation
around the dynamicTool -> execute -> action.run call).
---
Outside diff comments:
In `@apps/api/src/modules/stitches/stitches.module.ts`:
- Around line 14-21: The PiecesModule import is redundant because it's
registered global; remove its explicit reference from the imports array in the
StitchesModule (delete the PiecesModule entry from the imports: [...] list) and
also remove any corresponding unused top-level import statement for PiecesModule
to keep the file clean (look for the PiecesModule identifier in this module file
and remove both the array entry and the import declaration).
In `@engine/ai/core/src/services/orchestrator.service.ts`:
- Around line 482-498: The call to action.run forwards orchestration-only flag
confirmed inside propsValue (args), which can break action prop validation; in
the orchestrator where argsWithConfirm is defined, strip out confirmed before
calling action.run (e.g., extract confirmed via const { confirmed, ...props } =
argsWithConfirm) and pass props (not the original args) as propsValue to
action.run; update any typing to keep propsValue as Record<string, unknown> so
the action receives only its intended properties.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0421e143-a6fa-4d53-ae7f-9d710fe07adb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/api/src/modules/stitches/stitches.module.tsdocs/architecture/master/implementation-plan.mddocs/architecture/sync_strategy/sync_strategy.mdengine/ai/core/package.jsonengine/ai/core/src/services/orchestrator.service.ts
| ``` | ||
|
|
||
| Then open `http://localhost:3000` → Route Intelligence → see the L1→L6 green trace. | ||
| Then open `http://localhost:3000` → Route Intelligence → see the L1→L6 green trace. No newline at end of file |
There was a problem hiding this comment.
Add trailing newline for markdown compliance.
The file should end with a single newline character to comply with markdown formatting standards (MD047).
📝 Proposed fix
Add a newline after the closing backticks on line 811:
Then open `http://localhost:3000` → Route Intelligence → see the L1→L6 green trace.
+🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 811-811: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/master/implementation-plan.md` at line 811, The file
currently ends immediately after the line "Then open `http://localhost:3000` →
Route Intelligence → see the L1→L6 green trace." without a trailing newline; add
a single newline character at the end of the file (after the closing backticks)
so the document ends with one trailing newline to satisfy MD047 and standard
POSIX/markdown newline expectations.
| | L4 | Outbound Prep | Fat JSON assembly, Sync Condition evaluation | MappingEngine (80%) + Git Shard VM (20%) | | ||
| | L5 | Delivery Engine | KMS decrypt, Redis lock, Activepieces runtime | `run()` → HTTP request to destination API | | ||
| | L6 | Destination Gateway | `outbound_gateway` log, GEM update | — (standardised) | | ||
| | L6 | Destination Gateway | `outbound_gateway` log, GEM update | — (standardised) | No newline at end of file |
There was a problem hiding this comment.
Add trailing newline for markdown compliance.
The file should end with a single newline character to comply with markdown formatting standards (MD047).
📝 Proposed fix
Add a newline after the last row of the table on line 466:
| L6 | Destination Gateway | `outbound_gateway` log, GEM update | — (standardised) |
+📝 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.
| | L6 | Destination Gateway | `outbound_gateway` log, GEM update | — (standardised) | | |
| | L6 | Destination Gateway | `outbound_gateway` log, GEM update | — (standardised) | | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 466-466: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/sync_strategy/sync_strategy.md` at line 466, The file ends
without a trailing newline causing MD047; add a single newline character at EOF
so the last table row ("L6 | Destination Gateway | `outbound_gateway` log, GEM
update | — (standardised) |") is followed by one newline (i.e., ensure the file
ends with "\n") to satisfy markdown compliance.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/ai/core/src/services/orchestrator.service.ts`:
- Around line 495-497: The destructuring that strips the orchestration-only
confirmed field should mark the discarded variable as intentionally unused to
satisfy linters: change the destructuring in orchestrator.service.ts from const
{ confirmed, ...sanitizedArgs } = argsWithConfirm; to use a prefixed discard
like const { confirmed: _confirmed, ...sanitizedArgs } = argsWithConfirm; so the
confirmed value is removed but ESLint no-unused-vars is avoided while keeping
sanitizedArgs for action.run().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 998da0d2-abe8-419a-9d68-58267d126eac
📒 Files selected for processing (1)
engine/ai/core/src/services/orchestrator.service.ts
Summary by CodeRabbit
Refactor
Chores
Documentation
Note