feat: v0.3.0 — continue_on_error, email connectors, browser automation#45
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…int recovery When a step with continue_on_error: true fails after exhausting retries, workflow execution continues instead of halting. The failed step's error and partial output are exposed to downstream steps via CEL (steps['name'].error / steps['name'].output). Checkpoint recovery re-populates failed-continued steps so resume is correct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add AdvanceExecution to Orchestrator — takes the workflow step definitions, remaps failed-but-continued steps to "completed" in the DAG status view, and schedules downstream steps instead of cascading cancellations. Includes TestDistributed_ContinueOnError to verify the end-to-end behaviour with a real Postgres instance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces parseIMAPCredential and imapDial helpers in internal/connector/imap.go, using go-imap/v2 (imapclient). Includes unit tests for credential parsing and a testcontainers-based integration test using GreenMail (skipped under -short). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements EmailReceiveConnector that selects a folder, searches for messages by filter (unseen/all/flagged/recent), fetches envelopes and body text, optionally marks messages as \Seen, and returns structured output. Registered as email/receive in the connector registry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extends the Trigger struct with email-specific fields (Mailbox, Folder, Filter, PollInterval) and adds validation ensuring mailbox is required, filter is one of the allowed values, and poll_interval is a valid duration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… and Prometheus metrics - Add EmailTriggerPoller (internal/server/email_trigger.go): one goroutine per enabled email trigger, persistent IMAP connection with exponential backoff (5s→5min), configurable poll_interval, marks seen after fire, enforces maxConnections=5 cap, Reload() for hot-reload on apply - Wire into Server: constructor creates poller, Start() calls poller.Start(), Shutdown() calls poller.Stop() - Extend TriggerRecord/TriggerInput with Mailbox/Folder/Filter/PollInterval; SyncTriggers INSERT now includes the four email columns; add ListEmailTriggers + scanEmailTriggers (dedicated scan, not reusing scanTriggers); add nullableString helper - Add email trigger Prometheus metrics: connections_active gauge, poll_duration histogram, poll_errors/triggers/connection_errors counters - Add audit actions: email.trigger.fired, email.connection.established, email.connection.failed - Add 10 unit tests covering start/stop, reload bookkeeping, search criteria, input building, body extraction, and address formatting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements BrowserRunConnector that accepts a user Playwright script, wraps it in language-appropriate boilerplate (JS/TS or Python), passes it via stdin to the official Microsoft Playwright Docker image, and delegates execution to DockerRunConnector. Supports output_format=json to parse stdout as JSON. Registered as browser/run in the connector registry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…failure notification
…nstream CEL error access Exercises the three-step scenario: http/request fails with continue_on_error:true, email/send mock receives the error message via CEL interpolation of steps['step-1'].error, and a conditional step is skipped because the error is non-null. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…error handling, bounds checking - Enforce TLS 1.2 minimum in imap.go and email_trigger.go dialIMAP - Add --experimental-strip-types to TypeScript browser/run containerCmd - Handle initial poll error in pollLoop (was silently ignored) - Add overflow/NaN/Inf bounds checking in extractUID for float64/int/int64 - Fix Python browser/run docs example to use sync Playwright API (no await) - Correct scanEmailTriggers comment from "12 columns" to "10 columns" - Add log.Printf warning when falling back from UIDExpunge to Expunge Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… limits, metrics - MakeGlobalStepExecutor now loads failed-continued steps into CEL context, matching the sequential resumeExecution path so downstream expressions can reference error/output from continue_on_error steps - email/receive: cap limit param at 200 to prevent unbounded fetches - email_receive fetchMessages: remove redundant defer cmd.Close() that caused a double-close after the explicit close at end of function - metrics: add EmailTriggersSkippedTotal counter; increment in EmailTriggerPoller.startPoller when maxConnections limit is reached Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…il delivery semantics - Fix email/move output: success → moved - Fix email/delete output: success → deleted - Fix email/flag output: success → updated, add action field - Fix email/receive and trigger filter values: remove seen, add recent, change default all → unseen - Remove fictional engine.email config keys; document compile-time default of 5 connections - Add email trigger type fields (mailbox, folder, filter, poll_interval) to workflow-reference/index.md - Remove _credential params from email/receive, email/move, email/delete, email/flag; add step-level credential notes - Fix Python browser/run example to use pre-created page object instead of calling browser.new_page() - Add at-least-once delivery section explaining crash-recovery re-trigger risk and idempotency guidance - Clarify steps.error is only practically useful when continue_on_error: true Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deploying mantle with
|
| Latest commit: |
107cc62
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://23874f09.mantle-cxy.pages.dev |
| Branch Preview URL: | https://feature-v030-release.mantle-cxy.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IMAP-backed email triggers/connectors, a containerized Playwright browser runner, per-step continue_on_error support with DB migration and engine/orchestrator changes, metrics and audit entries, validation/tests, example workflows, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Poller as Email Trigger Poller
participant IMAP as IMAP Server
participant Engine as Workflow Engine
participant DB as Database
Poller->>IMAP: Dial & Login (persistent connection)
activate IMAP
loop Poll Interval
Poller->>IMAP: Search for messages (filter)
IMAP-->>Poller: UIDs
alt Messages Found
Poller->>IMAP: Fetch envelope, flags, body
IMAP-->>Poller: Message details
Poller->>Engine: Fire workflow with message inputs
activate Engine
Engine->>DB: Record execution / create steps
Engine-->>Poller: Execution started
deactivate Engine
Poller->>IMAP: Mark messages as \Seen
IMAP-->>Poller: Confirmed
else No Messages
Poller->>Poller: Wait until next tick
end
end
deactivate IMAP
sequenceDiagram
participant Orch as Orchestrator
participant DB as Database
participant Executor as Step Executor
Orch->>DB: Read step statuses
Orch->>Orch: Remap failed steps with continue_on_error -> treat as completed
Orch->>Orch: Build DAG & compute ready steps
Orch->>Executor: Create pending executions for ready steps
Executor-->>DB: Update step status (failed or completed)
Orch->>Orch: Cascade cancellations for non-continued failures
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…p for deferred invocation
… retry IMAP connections - browser/run: change JS/TS and Python container commands to install the playwright npm/pip package inline before executing the script. The Microsoft Playwright Docker images ship browsers but not the Node API package, causing MODULE_NOT_FOUND errors in CI. - TestIMAPDial: add a 5-attempt retry loop (2s sleep) around imapDial. GreenMail may not accept logins immediately after the "Starting GreenMail" log line appears in CI. - fetchFirstUID: replace the single-shot call with a 5-attempt retry loop (2s sleep) so that SMTP-to-IMAP visibility delay in CI doesn't cause TestEmailDelete_DeletesMessage and TestEmailMove_MovesMessage to fail with "no messages found". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/engine/engine.go (1)
724-726:⚠️ Potential issue | 🟡 MinorMissing
"error": nilkey in completed steps CEL context.In
MakeGlobalStepExecutor, completed steps are seeded with only"output", but the sequential execution path (line 143) and failed-continued steps (lines 734-737) both include the"error"key. This inconsistency could cause CEL expressions likesteps['name'].errorto fail for completed steps in the distributed worker path.🐛 Proposed fix for consistency
for name, output := range completedSteps { - celCtx.Steps[name] = map[string]any{"output": output} + celCtx.Steps[name] = map[string]any{"output": output, "error": nil} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/engine.go` around lines 724 - 726, In MakeGlobalStepExecutor the completedSteps seeding only sets {"output": output} which is inconsistent with other paths that include an "error" key; update the loop that populates celCtx.Steps from completedSteps to set celCtx.Steps[name] = map[string]any{"output": output, "error": nil} so every step has both keys (reference the completedSteps variable and celCtx.Steps map in MakeGlobalStepExecutor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/browser.go`:
- Around line 21-30: The wrapper scripts assume the Playwright package is
available but the container images only include browsers; update the wrapper
generation to ensure Playwright is installed before requiring/importing it: in
buildJSWrapper, run an installation step (e.g., execute "npm install
playwright") or ensure the generated script invokes an install command or uses a
base image with playwright preinstalled before the "require('playwright')" call;
apply the same fix to the Python wrapper(s) that produce "from
playwright.sync_api import sync_playwright" (install via "pip install
playwright" or use a prebuilt image) as referenced around the other wrappers at
the noted lines so runtime require/imports succeed.
In `@internal/connector/email_delete_test.go`:
- Around line 112-117: The test currently uses unchecked type assertions on
result["deleted"] and result["uid"] which can panic; update the assertions in
email_delete_test.go to use the comma-ok form (e.g., check value, ok :=
result["deleted"].(bool) and value, ok := result["uid"].(uint32)), fail the test
with a clear message (t.Fatalf or t.Errorf) if the type assertion fails, and
then compare the extracted values to expected (deleted == true and retUID ==
uid) to produce actionable failures instead of panics.
In `@internal/connector/email_delete.go`:
- Line 6: Replace the standard "log" import and any uses of log.* in this file
with the project's structured logger (do not use package "log"); remove the
"log" import and instead use the existing project logger instance (e.g.,
processLogger or the package's logger) in the email_delete.go functions that
currently call log.* so that logs include structured fields like workflowID and
executionID (attach them as fields/context on each log call) and follow the
project's logging pattern for consistency and observability.
- Around line 58-67: The current fallback to client.Expunge() in the UID
deletion path is unsafe because UIDExpunge failure will cause Expunge to remove
all messages marked \Deleted; update the deletion flow (the block calling
client.UIDExpunge(uidSet).Close()) to instead return a clear error stating
UIDExpunge is required (do not call client.Expunge()), and add an audit/metric
emission when UIDExpunge is not supported so the event is observable (e.g., emit
an audit log/metric in the same conditional where you currently log the
fallback). Also add a short comment in the EmailDeleteConnector/UID delete logic
documenting that UIDExpunge is required and that callers should not rely on
Expunge fallback.
In `@internal/connector/email_move.go`:
- Around line 69-95: Move the extractUID helper into the shared IMAP utilities
(e.g., imap.go) and extend its type switch to accept uint and uint64 (in
addition to uint32, float64, int, int64) validating non-zero and <=
math.MaxUint32 for all integer types and rejecting NaN/Inf for float64; keep the
same error messages and return uint32, and update callers (e.g., email_move.go
and email_delete.go) to call the relocated extractUID from the shared IMAP
utilities.
In `@internal/connector/email_receive.go`:
- Around line 126-135: The code is calling client.Store(...) with a UIDSet
(uidSet), which operates on sequence numbers; replace these calls with
client.UIDStore(...) so the STORE command is sent using UIDs: for example change
client.Store(uidSet, storeFlags, nil).Close() to client.UIDStore(uidSet,
storeFlags, nil).Close(); apply the same replacement for any other places where
client.Store is invoked with a UIDSet (look for variables named uidSet,
storeFlags, markSeen or similar) so flag, delete and trigger flows use UIDStore
instead of Store.
In `@internal/connector/imap.go`:
- Around line 46-68: The imapDial function lacks a context parameter and so
cannot enforce timeouts/cancellations; change imapDial to accept ctx
context.Context and use a net.Dialer (e.g., &net.Dialer{Timeout:...}) passed
into imapclient.Options.Dialer so you call imapclient.DialTLS/DialInsecure with
those options (or perform dialer.DialContext(ctx, ...) and wrap the net.Conn
with imapclient.New(conn, opts)); ensure Login uses the returned client and that
the dial and error handling remain the same but are context-aware.
In `@internal/engine/distributed_test.go`:
- Around line 294-362: The test currently only verifies step-2 is scheduled and
completed but doesn't assert that the failed step's persisted error/partial
output is written and visible to downstream steps; update the test so the Worker
StepExecutor (the executor closure used by Worker) makes step-2 read the
persisted state for step-1 (via the database row in step_executions for execID
and step_name "step-1" or via the same mechanism the runtime uses to surface
errors/outputs) and assert that the error string and/or partial output fields
contain the expected values (e.g., "deliberate failure" and any partial output)
before returning success; ensure you reference and read the same columns the
production code persists and keep the check inside the executor for step-2 so
the test fails if the distributed checkpoint did not persist
step-1.error/output.
In `@internal/engine/engine.go`:
- Around line 902-927: The current loadFailedContinuedSteps function can receive
output::text == "null" (a JSON null) which causes json.Unmarshal to fail when
expecting an object; after scanning outputStr in loadFailedContinuedSteps, guard
against JSON null by normalizing it before unmarshalling (e.g., if
strings.TrimSpace(outputStr) == "null" || outputStr == "" then set outputStr =
"{}"), then proceed to json.Unmarshal into the map; this fixes cases where
output is JSON null while preserving existing handling of SQL NULL via COALESCE.
In `@internal/server/email_trigger_test.go`:
- Around line 187-200: Remove the unused imapclientFetchBufferStub type and
simplify the wrapper: delete the imapclientFetchBufferStub declaration and
update buildEmailTriggerInputsFromStub to no longer accept or ignore a parameter
(or remove the wrapper entirely and call
buildEmailTriggerInputs(&imapclient.FetchMessageBuffer{}, "INBOX") at the test
call sites); locate symbols imapclientFetchBufferStub and
buildEmailTriggerInputsFromStub and update callers to use
buildEmailTriggerInputs(&imapclient.FetchMessageBuffer{}, "INBOX") or the new
parameterless helper that returns the same map from buildEmailTriggerInputs.
In `@internal/server/email_trigger.go`:
- Around line 368-382: The warning log when Store fails after firing the
workflow lacks the message UID, making it hard to trace duplicates; update the
log in the error branch where client.Store(uidSet, storeFlags, nil).Close() is
checked to include the UID(s) from uidSet (or the specific UID used to fetch the
message) alongside existing fields like "trigger_id" (t.ID) and "workflow"
(t.WorkflowName), so the call to e.server.Logger.Warn includes a "uid" or "uids"
key with the identifier(s) to aid debugging duplicate executions.
- Around line 388-427: In fireWorkflow, the audit emission is using the original
ctx instead of the team-scoped context teamCtx; change the call to
e.server.Auditor.Emit to pass teamCtx so context-aware auditor middleware
receives the TeamID; specifically update the Auditor.Emit invocation (currently
e.server.Auditor.Emit(ctx, audit.Event{...})) to use teamCtx and keep the rest
of the audit.Event (Actor, Action, Resource, Metadata, TeamID) unchanged.
- Around line 312-322: The current code builds an imap.UIDSet from
searchData.AllUIDs() without a cap, which can exhaust memory for large
mailboxes; limit the number of UIDs before constructing uidSet by introducing a
maxFetch constant (e.g., 200) and truncating the uids slice to the most recent
maxFetch entries (e.g., if len(uids) > maxFetch, set uids =
uids[len(uids)-maxFetch:]) before the loop that adds them to uidSet; update any
variable references (uids, uidSet) used by the subsequent fetch logic in
email_trigger.go so the fetch operates only on the capped set while keeping
metrics.EmailPollDuration and t.WorkflowName/folder usage unchanged.
In `@internal/server/server.go`:
- Around line 287-291: After successfully calling s.emailPoller.Start(ctx) you
must ensure s.emailPoller.Stop(ctx) is called on all early returns; add a
rollback cleanup (e.g., a deferred stop or an explicit stop before each return)
immediately after Start succeeds to guarantee the poller is stopped if later
code returns (for example in the errCh/http bind failure branch). Locate the
Start/Stop calls on s.emailPoller (s.emailPoller.Start and s.emailPoller.Stop)
and either add defer s.emailPoller.Stop(ctx) right after Start or ensure every
non-shutdown return path calls s.emailPoller.Stop(ctx) before returning so the
poller does not leak when initialization fails later.
In `@internal/workflow/validate_test.go`:
- Around line 700-724: TestValidate_ContinueOnError currently uses
steps['risky-step'].error but the implicit-dependency extractor tests only cover
.output; update the test-suite to ensure ExtractImplicitDeps/Validate recognizes
steps.*.error as creating a dependency on that step. Add a unit case (or extend
existing implicit-deps table) that includes an expression referencing
steps['name'].error and assert the resulting dependency list for the consumer
step includes the producer step; ensure the extractor logic referenced by
ExtractImplicitDeps is exercised so .error is treated like .output and the
notify step cannot run before risky-step.
In `@internal/workflow/validate.go`:
- Around line 219-256: The branch that validates the "browser/run" step
currently skips checks when step.Params is nil and silently accepts non-string
types for language/output_format; update the validation block in validate.go so
that if step.Action == "browser/run" and step.Params == nil you append a
ValidationError for prefix + ".params" (and/or ".params.script") indicating
params/script are required, and change the language/output_format checks to
explicitly assert types (e.g., lang, ok := step.Params["language"].(string); if
!ok && step.Params["language"] != nil { append a type ValidationError for prefix
+ ".params.language" } else if ok { validate against validLanguages }) and
similarly for output_format (use format, ok :=
step.Params["output_format"].(string) with a type error when present but not a
string, then validate values), keeping the existing script presence/emptiness
checks on step.Params["script"].(string) unchanged but ensuring they run even
when step.Params was originally nil by handling that nil case first.
- Around line 104-152: The trigger validation loop in Validate (iterating
w.Triggers / trig) only enforces type and email fields, so cron and webhook
triggers lack required field checks; update the switch over trig.Type to add
case "cron" that ensures trig.Schedule (or similarly named field) is present and
non-empty and that its schedule parses/validates, and add case "webhook" that
ensures trig.Path (and any required method or auth fields) is present and valid;
alternatively call the JSON Schema validation for triggers here so all kinds
(cron, webhook, email) are validated consistently before returning errs.
In `@site/src/content/docs/workflow-reference/connectors.md`:
- Around line 383-390: The email connector examples use the `trigger.uid`
namespace but the rest of the PR documents trigger data as `trigger.payload.*`;
update the samples (e.g., the `archive-message` example and the delete/flag
examples) to use the documented namespace such as `trigger.payload.uid` (or
alternatively update the expressions.md to document `trigger.uid` if you prefer
that naming) so the trigger field names are consistent across docs; ensure the
YAML `params` entries reference `trigger.payload.*` everywhere to match
`expressions.md`.
- Around line 743-751: The example uses page.goto() and page.pdf() without
creating a Page object; insert a call to create the page from the injected
browser (e.g., page = browser.new_page()) before invoking page.goto() and
page.pdf() so the script initializes a Page instance prior to navigation and PDF
generation.
---
Outside diff comments:
In `@internal/engine/engine.go`:
- Around line 724-726: In MakeGlobalStepExecutor the completedSteps seeding only
sets {"output": output} which is inconsistent with other paths that include an
"error" key; update the loop that populates celCtx.Steps from completedSteps to
set celCtx.Steps[name] = map[string]any{"output": output, "error": nil} so every
step has both keys (reference the completedSteps variable and celCtx.Steps map
in MakeGlobalStepExecutor).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20c29e75-f1d9-40f4-a5ed-3ace2af64719
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
examples/browser-form-submit.yamlexamples/browser-scrape.yamlexamples/docker-volume-backup.yamlexamples/email-inbox-triage.yamlexamples/error-notification.yamlgo.modinternal/audit/audit.gointernal/connector/browser.gointernal/connector/browser_test.gointernal/connector/connector.gointernal/connector/email_delete.gointernal/connector/email_delete_test.gointernal/connector/email_flag.gointernal/connector/email_flag_test.gointernal/connector/email_move.gointernal/connector/email_move_test.gointernal/connector/email_receive.gointernal/connector/email_receive_test.gointernal/connector/imap.gointernal/connector/imap_test.gointernal/db/migrations/014_continue_on_error.sqlinternal/db/migrations/015_email_triggers.sqlinternal/engine/distributed_test.gointernal/engine/engine.gointernal/engine/engine_test.gointernal/engine/orchestrator.gointernal/metrics/metrics.gointernal/server/email_trigger.gointernal/server/email_trigger_test.gointernal/server/server.gointernal/server/trigger.gointernal/workflow/validate.gointernal/workflow/validate_test.gointernal/workflow/workflow.gosite/src/content/docs/concepts/expressions.mdsite/src/content/docs/server-guide/triggers.mdsite/src/content/docs/workflow-reference/connectors.mdsite/src/content/docs/workflow-reference/index.md
| import ( | ||
| "context" | ||
| "fmt" | ||
| "log" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider structured logging instead of log package.
Using the standard log package loses structured context (workflow ID, execution ID, etc.). Consider using the project's logging pattern for consistency and observability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/email_delete.go` at line 6, Replace the standard "log"
import and any uses of log.* in this file with the project's structured logger
(do not use package "log"); remove the "log" import and instead use the existing
project logger instance (e.g., processLogger or the package's logger) in the
email_delete.go functions that currently call log.* so that logs include
structured fields like workflowID and executionID (attach them as fields/context
on each log call) and follow the project's logging pattern for consistency and
observability.
There was a problem hiding this comment.
Agreed. Tracked as #39 in the v0.3.1 milestone (The Locksmith Update).
There was a problem hiding this comment.
@michaelmcnees, thanks for tracking this! I'll look forward to seeing the structured logging improvements land in #39 as part of the v0.3.1 milestone. 👍
| // extractUID extracts a uint32 UID value from params, accepting uint32 or float64. | ||
| func extractUID(params map[string]any, key string) (uint32, error) { | ||
| switch v := params[key].(type) { | ||
| case uint32: | ||
| if v == 0 { | ||
| return 0, fmt.Errorf("%s must be a non-zero UID", key) | ||
| } | ||
| return v, nil | ||
| case float64: | ||
| if math.IsNaN(v) || math.IsInf(v, 0) || v <= 0 || v > math.MaxUint32 || v != math.Floor(v) { | ||
| return 0, fmt.Errorf("%s must be a non-zero UID (valid uint32 integer)", key) | ||
| } | ||
| return uint32(v), nil | ||
| case int: | ||
| if v <= 0 || v > math.MaxUint32 { | ||
| return 0, fmt.Errorf("%s must be a non-zero UID (valid uint32)", key) | ||
| } | ||
| return uint32(v), nil | ||
| case int64: | ||
| if v <= 0 || v > math.MaxUint32 { | ||
| return 0, fmt.Errorf("%s must be a non-zero UID (valid uint32)", key) | ||
| } | ||
| return uint32(v), nil | ||
| default: | ||
| return 0, fmt.Errorf("%s is required and must be a uint32", key) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
extractUID should be in a shared file and handle additional integer types.
This helper is used by multiple connectors (email_move.go, email_delete.go). Consider moving it to imap.go with other shared IMAP utilities. Also, uint and uint64 types are not handled, which could cause unexpected errors if params originate from different sources.
♻️ Extended type handling
func extractUID(params map[string]any, key string) (uint32, error) {
switch v := params[key].(type) {
case uint32:
if v == 0 {
return 0, fmt.Errorf("%s must be a non-zero UID", key)
}
return v, nil
+ case uint:
+ if v == 0 || v > math.MaxUint32 {
+ return 0, fmt.Errorf("%s must be a non-zero UID (valid uint32)", key)
+ }
+ return uint32(v), nil
+ case uint64:
+ if v == 0 || v > math.MaxUint32 {
+ return 0, fmt.Errorf("%s must be a non-zero UID (valid uint32)", key)
+ }
+ return uint32(v), nil
case float64:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/email_move.go` around lines 69 - 95, Move the extractUID
helper into the shared IMAP utilities (e.g., imap.go) and extend its type switch
to accept uint and uint64 (in addition to uint32, float64, int, int64)
validating non-zero and <= math.MaxUint32 for all integer types and rejecting
NaN/Inf for float64; keep the same error messages and return uint32, and update
callers (e.g., email_move.go and email_delete.go) to call the relocated
extractUID from the shared IMAP utilities.
There was a problem hiding this comment.
Agreed on consolidation. Tracked as part of #37 (Consolidate duplicated IMAP config and dial logic) in v0.3.1.
There was a problem hiding this comment.
@michaelmcnees, sounds good! Consolidating extractUID alongside the other shared IMAP utilities in v0.3.1 makes sense — grouping it with parseIMAPCredential and imapDial in imap.go will keep the shared logic in one place. The uint/uint64 type extensions can be included at the same time.
| if markSeen && len(uids) > 0 { | ||
| storeFlags := &imap.StoreFlags{ | ||
| Op: imap.StoreFlagsAdd, | ||
| Flags: []imap.Flag{imap.FlagSeen}, | ||
| Silent: true, | ||
| } | ||
| if err := client.Store(uidSet, storeFlags, nil).Close(); err != nil { | ||
| return nil, fmt.Errorf("email/receive: marking messages as seen: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check go-imap/v2 API for Store vs UIDStore
rg -n 'func.*Store\(' --type go
ast-grep --pattern 'client.Store($_, $_, $_)'
# Check if UIDStore exists in the library
rg -n 'UIDStore' internal/connector/Repository: dvflw/mantle
Length of output: 998
🌐 Web query:
go-imap v2 imapclient Store vs UIDStore UID operations
💡 Result:
In the go-imap v2 library (github.com/emersion/go-imap/v2), the IMAPClient provides methods for STORE operations following standard IMAP conventions for sequence numbers vs UIDs. The Client.Store(numSet imap.NumSet, store *imap.StoreFlags, options *imap.StoreOptions) method sends a STORE command using message sequence numbers (NumSet). Sequence numbers are temporary indices that change as messages are added, expunged, or modified. For UID-based operations, which use permanent unique identifiers (UIDs) that remain stable across sessions (per UIDPLUS RFC 4315), use Client.UIDStore(uidSet imap.UIDSet, store *imap.StoreFlags, options *imap.StoreOptions). This sends a UID STORE command, interpreting the set as UIDs instead of sequence numbers. UID operations are preferred for reliable message targeting, especially in concurrent or changing mailboxes. Examples from docs and issues show patterns like c.Fetch(seqset or uidset, items...) and c.UIDFetch(uidset, items...), confirming UID-prefixed methods for UID sets. Store follows this: use UIDStore for UID operations to avoid sequence number shifts. Note: v2 is in beta (latest v2.0.0-beta.8 as of 2026); always check pkg.go.dev/github.com/emersion/go-imap/v2/imapclient for exact signatures. Other libraries like github.com/demisto/go-imap explicitly have UIDStore vs Store.
Citations:
- 1: https://pkg.go.dev/github.com/emersion/go-imap/v2/imapclient
- 2: go-imap v2 emersion/go-imap#260
- 3: https://stackoverflow.com/questions/24851303/how-to-fetch-the-uids-using-golang-imap
- 4: https://github.com/emersion/go-imap
- 5: Get the UID of the current message in a range emersion/go-imap#307
- 6: https://pkg.go.dev/github.com/demisto/go-imap/imap
Replace client.Store(uidSet, ...) with client.UIDStore(uidSet, ...).
The code uses client.Store() with a UIDSet, but go-imap v2's Store() method expects sequence numbers. For UID-based operations, use Client.UIDStore() instead, which sends the UID STORE command. Using the wrong method will operate on incorrect messages based on temporary sequence numbers rather than permanent UIDs.
This same issue appears in internal/connector/email_flag.go:77, internal/connector/email_delete.go:54, and internal/server/email_trigger.go:374.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/email_receive.go` around lines 126 - 135, The code is
calling client.Store(...) with a UIDSet (uidSet), which operates on sequence
numbers; replace these calls with client.UIDStore(...) so the STORE command is
sent using UIDs: for example change client.Store(uidSet, storeFlags,
nil).Close() to client.UIDStore(uidSet, storeFlags, nil).Close(); apply the same
replacement for any other places where client.Store is invoked with a UIDSet
(look for variables named uidSet, storeFlags, markSeen or similar) so flag,
delete and trigger flows use UIDStore instead of Store.
There was a problem hiding this comment.
Verified this is a false positive. go-imap v2's client.Store(numSet, ...) method dispatches UID STORE vs plain STORE automatically based on the runtime type of numSet — it checks imapwire.NumSetKind. Since all call sites pass imap.UIDSet (not imap.SeqSet), the wire protocol already sends UID STORE. There is no separate UIDStore method in go-imap v2.
There was a problem hiding this comment.
@michaelmcnees, thanks for the detailed verification. You're correct — go-imap v2's Client.Store accepts imap.NumSet as its parameter, and the library dispatches UID STORE vs plain STORE on the wire based on the runtime type of the numSet argument. There is no separate UIDStore method in go-imap v2's imapclient — the unified Store method handles both cases. My comment was incorrectly based on the go-imap v1 API, which did have a separate UidStore method. Apologies for the noise.
✏️ Learnings added
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/connector/email_receive.go:126-135
Timestamp: 2026-03-25T19:44:56.827Z
Learning: In go-imap v2 (github.com/emersion/go-imap/v2/imapclient), the `Client.Store(numSet imap.NumSet, store *imap.StoreFlags, options *imap.StoreOptions)` method automatically dispatches `UID STORE` vs plain `STORE` on the wire based on the runtime type of `numSet`: passing `imap.UIDSet` results in `UID STORE`, and passing `imap.SeqSet` results in plain `STORE`. There is no separate `UIDStore` method in go-imap v2 (unlike v1, which had `UidStore`). Do not flag uses of `client.Store(uidSet, ...)` as incorrect when the variable is of type `imap.UIDSet`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func TestValidate_ContinueOnError(t *testing.T) { | ||
| result := mustParse(t, ` | ||
| name: test-continue | ||
| steps: | ||
| - name: risky-step | ||
| action: http/request | ||
| continue_on_error: true | ||
| params: | ||
| url: https://example.com | ||
| - name: notify | ||
| action: slack/send | ||
| if: "steps['risky-step'].error != null" | ||
| params: | ||
| channel: "#alerts" | ||
| text: "Step failed" | ||
| `) | ||
| errs := Validate(result) | ||
| assertNoErrors(t, errs) | ||
| if result.Workflow.Steps[0].ContinueOnError != true { | ||
| t.Errorf("expected ContinueOnError to be true for steps[0], got %v", result.Workflow.Steps[0].ContinueOnError) | ||
| } | ||
| if result.Workflow.Steps[1].ContinueOnError != false { | ||
| t.Errorf("expected ContinueOnError to be false for steps[1], got %v", result.Workflow.Steps[1].ContinueOnError) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add implicit-dependency coverage for steps.*.error.
TestValidate_ContinueOnError parses steps['risky-step'].error, but the implicit-dependency tests in this file only cover .output references. If ExtractImplicitDeps misses .error, the notify step can run too early and this new test still passes.
🧪 Minimal case to pin the extractor
+ {
+ name: "if condition references step error",
+ step: Step{
+ If: "steps['risky-step'].error != null",
+ },
+ want: []string{"risky-step"},
+ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/workflow/validate_test.go` around lines 700 - 724,
TestValidate_ContinueOnError currently uses steps['risky-step'].error but the
implicit-dependency extractor tests only cover .output; update the test-suite to
ensure ExtractImplicitDeps/Validate recognizes steps.*.error as creating a
dependency on that step. Add a unit case (or extend existing implicit-deps
table) that includes an expression referencing steps['name'].error and assert
the resulting dependency list for the consumer step includes the producer step;
ensure the extractor logic referenced by ExtractImplicitDeps is exercised so
.error is treated like .output and the notify step cannot run before risky-step.
There was a problem hiding this comment.
Good suggestion. The existing stepRefPattern regex detects steps.X references for dependency validation. Adding a test that verifies steps['x'].error is detected would improve coverage. Deferring to v0.3.1.
There was a problem hiding this comment.
@michaelmcnees, understood — good to know stepRefPattern already covers the steps.X pattern broadly. Would you like me to open a follow-up GitHub issue to track adding the steps['x'].error implicit-dependency test case for v0.3.1?
| // Validate triggers. | ||
| validTriggerTypes := map[string]bool{"cron": true, "webhook": true, "email": true} | ||
| validEmailFilters := map[string]bool{"unseen": true, "all": true, "flagged": true, "recent": true} | ||
| for i, trig := range w.Triggers { | ||
| trigPrefix := fmt.Sprintf("triggers[%d]", i) | ||
| if trig.Type == "" { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".type", | ||
| Message: "trigger type is required", | ||
| }) | ||
| continue | ||
| } | ||
| if !validTriggerTypes[trig.Type] { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".type", | ||
| Message: fmt.Sprintf("trigger type must be one of: cron, webhook, email (got %q)", trig.Type), | ||
| }) | ||
| continue | ||
| } | ||
| switch trig.Type { | ||
| case "email": | ||
| if trig.Mailbox == "" { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".mailbox", | ||
| Message: "mailbox is required for email triggers", | ||
| }) | ||
| } | ||
| if trig.Filter != "" && !validEmailFilters[trig.Filter] { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".filter", | ||
| Message: fmt.Sprintf("filter must be one of: unseen, all, flagged, recent (got %q)", trig.Filter), | ||
| }) | ||
| } | ||
| if trig.PollInterval != "" { | ||
| d, err := time.ParseDuration(trig.PollInterval) | ||
| if err != nil { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".poll_interval", | ||
| Message: fmt.Sprintf("invalid duration: %v", err), | ||
| }) | ||
| } else if d <= 0 { | ||
| errs = append(errs, ValidationError{ | ||
| Field: trigPrefix + ".poll_interval", | ||
| Message: "poll_interval must be a positive duration", | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate still accepts malformed cron and webhook triggers.
This block only enforces type and email-specific fields, so a cron trigger with no schedule or a webhook trigger with no path still passes validation and fails later at runtime. Please add the missing type-specific checks here, or move trigger validation behind the workflow JSON Schema so all trigger kinds stay aligned. As per coding guidelines, "Validate workflow YAML against JSON Schema in the validate command."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/workflow/validate.go` around lines 104 - 152, The trigger validation
loop in Validate (iterating w.Triggers / trig) only enforces type and email
fields, so cron and webhook triggers lack required field checks; update the
switch over trig.Type to add case "cron" that ensures trig.Schedule (or
similarly named field) is present and non-empty and that its schedule
parses/validates, and add case "webhook" that ensures trig.Path (and any
required method or auth fields) is present and valid; alternatively call the
JSON Schema validation for triggers here so all kinds (cron, webhook, email) are
validated consistently before returning errs.
There was a problem hiding this comment.
This is a pre-existing gap — cron and webhook trigger validation was not part of the v0.3.0 scope. We added email trigger validation in this PR. Adding required-field checks for cron (schedule) and webhook (path) is a good improvement but out of scope for this release. Will create a separate issue.
There was a problem hiding this comment.
@michaelmcnees, understood — that's a reasonable scoping decision. Would you like me to open a GitHub issue to track adding required-field validation for cron (schedule) and webhook (path) triggers in internal/workflow/validate.go? I can file it now so it's ready for a follow-up PR.
🧠 Learnings used
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T01:18:15.022Z
Learning: Applies to **/*.yaml : Validate workflow YAML against JSON Schema in the validate command
| // Validate params for browser/run steps. | ||
| if step.Action == "browser/run" && step.Params != nil { | ||
| // Validate script is present and non-empty. | ||
| script, hasScript := step.Params["script"].(string) | ||
| if !hasScript { | ||
| errs = append(errs, ValidationError{ | ||
| Field: prefix + ".params.script", | ||
| Message: "script param is required and must be a string", | ||
| }) | ||
| } else if strings.TrimSpace(script) == "" { | ||
| errs = append(errs, ValidationError{ | ||
| Field: prefix + ".params.script", | ||
| Message: "script param must not be empty", | ||
| }) | ||
| } | ||
|
|
||
| // Validate language (if present). | ||
| if lang, ok := step.Params["language"].(string); ok && lang != "" { | ||
| validLanguages := map[string]bool{"javascript": true, "typescript": true, "python": true} | ||
| if !validLanguages[lang] { | ||
| errs = append(errs, ValidationError{ | ||
| Field: prefix + ".params.language", | ||
| Message: fmt.Sprintf("language must be one of: javascript, typescript, python (got %q)", lang), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Validate output_format (if present). | ||
| if format, ok := step.Params["output_format"].(string); ok && format != "" { | ||
| validFormats := map[string]bool{"json": true, "text": true} | ||
| if !validFormats[format] { | ||
| errs = append(errs, ValidationError{ | ||
| Field: prefix + ".params.output_format", | ||
| Message: fmt.Sprintf("output_format must be one of: json, text (got %q)", format), | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
browser/run can still bypass validation with missing or mistyped params.
Because the whole branch is gated on step.Params != nil, { action: browser/run } passes validation even though the connector requires params.script. Also, language and output_format are only checked after a successful .(string), so values like language: 123 or output_format: false are silently accepted and then defaulted at execution time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/workflow/validate.go` around lines 219 - 256, The branch that
validates the "browser/run" step currently skips checks when step.Params is nil
and silently accepts non-string types for language/output_format; update the
validation block in validate.go so that if step.Action == "browser/run" and
step.Params == nil you append a ValidationError for prefix + ".params" (and/or
".params.script") indicating params/script are required, and change the
language/output_format checks to explicitly assert types (e.g., lang, ok :=
step.Params["language"].(string); if !ok && step.Params["language"] != nil {
append a type ValidationError for prefix + ".params.language" } else if ok {
validate against validLanguages }) and similarly for output_format (use format,
ok := step.Params["output_format"].(string) with a type error when present but
not a string, then validate values), keeping the existing script
presence/emptiness checks on step.Params["script"].(string) unchanged but
ensuring they run even when step.Params was originally nil by handling that nil
case first.
There was a problem hiding this comment.
The validation runs at mantle validate time against the static YAML. Dynamic CEL expressions ({{ }}) are resolved at runtime, so static validation can't catch a missing script that comes from a CEL expression. This is consistent with how all other connectors work — params are validated structurally at parse time, not at execution time. The runtime connector already validates script is non-empty and returns a clear error. No change needed.
There was a problem hiding this comment.
@michaelmcnees, that makes sense — thanks for the clarification. Static validation can only reason about literal YAML values, and requiring script at validate-time would break workflows that supply it dynamically via a CEL expression. The runtime connector's own guard is the right place for that check. I'll note this design pattern for future reviews.
(^_^) ∫
✏️ Learnings added
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/workflow/validate.go:219-256
Timestamp: 2026-03-25T19:46:50.273Z
Learning: In dvflw/mantle, `mantle validate` performs static structural validation against literal YAML values only. Connector params (e.g., `script` for `browser/run`) are not required to be present at validate time because they can be supplied via CEL expressions (`{{ }}`), which are resolved at runtime. Runtime connectors are responsible for validating their own required params and returning clear errors. This is consistent across all connectors and intentional by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T01:18:15.022Z
Learning: Applies to **/*.yaml : Validate workflow YAML against JSON Schema in the validate command
| ```yaml | ||
| - name: archive-message | ||
| action: email/move | ||
| credential: company-inbox | ||
| params: | ||
| uid: "{{ trigger.uid }}" | ||
| source_folder: INBOX | ||
| target_folder: Archive |
There was a problem hiding this comment.
Align the email examples with the documented trigger namespace.
These samples use trigger.uid, but site/src/content/docs/concepts/expressions.md in the same PR still documents trigger data as trigger.payload.*. Please make the namespace consistent or document the email-trigger shape here. The same mismatch repeats in the delete/flag examples below.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/src/content/docs/workflow-reference/connectors.md` around lines 383 -
390, The email connector examples use the `trigger.uid` namespace but the rest
of the PR documents trigger data as `trigger.payload.*`; update the samples
(e.g., the `archive-message` example and the delete/flag examples) to use the
documented namespace such as `trigger.payload.uid` (or alternatively update the
expressions.md to document `trigger.uid` if you prefer that naming) so the
trigger field names are consistent across docs; ensure the YAML `params` entries
reference `trigger.payload.*` everywhere to match `expressions.md`.
There was a problem hiding this comment.
Will review and align in the docs fix push.
There was a problem hiding this comment.
@michaelmcnees — Sounds good! When you do the fix push, the places to update are:
email/moveexample —trigger.uid→ consistent namespaceemail/deleteexample — sameemail/flagexample — same
Whether you settle on trigger.uid (flat email-trigger shape) or trigger.payload.uid (generic payload shape), just make sure site/src/content/docs/concepts/expressions.md and site/src/content/docs/server-guide/triggers.md all agree with whichever convention is chosen. Happy to re-check once the push lands!
| script: | | ||
| import os | ||
| import json | ||
|
|
||
| page.goto('https://example.com/report') | ||
| page.pdf(path='/mantle/artifacts/report.pdf') | ||
|
|
||
| file_size = os.path.getsize('/mantle/artifacts/report.pdf') | ||
| print(json.dumps({'generated': True, 'size_bytes': file_size})) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Playwright Python, given a Browser instance, should a docs example create a page with page = browser.new_page()before callingpage.goto()andpage.pdf()?
💡 Result:
Yes, in Playwright Python, given a Browser instance, documentation examples consistently create a page with page = browser.new_page before calling page.goto and page.pdf. The official Browser API docs show examples like: browser = chromium.launch; page = browser.new_page; page.goto("https://example.com"). Similar patterns appear in Page, Playwright intro, and PDF generation tutorials from sources like pdforge.com (2025). browser.new_page is a convenience method that creates a new page in a new context, suitable for single-page scenarios, though production code may prefer explicit context.new_page for lifecycle control.
Citations:
- 1: https://playwright.dev/python/docs/api/class-page
- 2: https://playwright.dev/python/docs/next/pages
- 3: https://playwright.dev/python/docs/pages
- 4: https://playwright.dev/python/docs/api/class-browser
- 5: https://playwright.dev/python/docs/next/api/class-browser
- 6: https://pdforge.com/blog/generate-pdf-from-html-using-playwright-python
- 7: https://playwright.dev/python/docs/api/class-playwright
Add page creation before navigation and PDF operations.
The example calls page.goto() and page.pdf() without first creating a page instance. Add page = browser.new_page() to initialize the page from the injected browser instance.
Fix
import os
import json
+ page = browser.new_page()
page.goto('https://example.com/report')
page.pdf(path='/mantle/artifacts/report.pdf')📝 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.
| script: | | |
| import os | |
| import json | |
| page.goto('https://example.com/report') | |
| page.pdf(path='/mantle/artifacts/report.pdf') | |
| file_size = os.path.getsize('/mantle/artifacts/report.pdf') | |
| print(json.dumps({'generated': True, 'size_bytes': file_size})) | |
| script: | | |
| import os | |
| import json | |
| page = browser.new_page() | |
| page.goto('https://example.com/report') | |
| page.pdf(path='/mantle/artifacts/report.pdf') | |
| file_size = os.path.getsize('/mantle/artifacts/report.pdf') | |
| print(json.dumps({'generated': True, 'size_bytes': file_size})) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/src/content/docs/workflow-reference/connectors.md` around lines 743 -
751, The example uses page.goto() and page.pdf() without creating a Page object;
insert a call to create the page from the injected browser (e.g., page =
browser.new_page()) before invoking page.goto() and page.pdf() so the script
initializes a Page instance prior to navigation and PDF generation.
There was a problem hiding this comment.
Already fixed in commit 107cc62 — the Python example was updated to use the pre-created page object from the wrapper.
There was a problem hiding this comment.
@michaelmcnees, thanks for the follow-up! That makes sense — since the Python wrapper in BrowserRunConnector already pre-creates a page instance (analogous to how browser is injected for JS/TS), the example correctly reflects the actual runtime context. Acknowledged!
…ead of npm install
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/server/email_trigger.go (2)
411-426:⚠️ Potential issue | 🟡 MinorAudit event uses original
ctxinstead of team-scopedteamCtx.The
fireWorkflowmethod createsteamCtx(line 390) but the audit event emission at line 412 usesctx. WhileTeamIDis explicitly set, any context-aware auditor middleware won't see the team context.🐛 Proposed fix
if e.server.Auditor != nil { - _ = e.server.Auditor.Emit(ctx, audit.Event{ + _ = e.server.Auditor.Emit(teamCtx, audit.Event{ Actor: "email-poller", Action: audit.ActionEmailTriggerFired,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 411 - 426, In fireWorkflow, the audit Emit call uses the original ctx instead of the team-scoped teamCtx; update the call to use teamCtx (i.e., call e.server.Auditor.Emit(teamCtx, audit.Event{...})) so any context-aware auditor middleware receives the team context; keep the existing e.server.Auditor != nil guard and the audit.Event payload (WorkflowName/ID/TeamID) unchanged.
312-322:⚠️ Potential issue | 🟠 MajorUnbounded UID fetch may cause memory/performance issues with large mailboxes.
The code fetches all UIDs from
searchData.AllUIDs()without applying the 200-message cap mentioned in the PR summary. Large mailboxes (e.g., after an outage) could cause memory exhaustion or timeouts.🛡️ Proposed fix to cap fetched messages
+const maxEmailsPerPoll = 200 + uids := searchData.AllUIDs() if len(uids) == 0 { metrics.EmailPollDuration.WithLabelValues(t.WorkflowName, folder).Observe(time.Since(start).Seconds()) return nil } + // Cap the number of messages processed per poll to avoid memory exhaustion. + if len(uids) > maxEmailsPerPoll { + e.server.Logger.Warn("email poller: truncating UID list", + "trigger_id", t.ID, + "total_uids", len(uids), + "processing", maxEmailsPerPoll) + uids = uids[:maxEmailsPerPoll] + } + // Fetch envelope + body text for each message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 312 - 322, The code currently builds a UID set from searchData.AllUIDs() without limiting count; change it to cap to the most recent 200 UIDs before creating uidSet to avoid unbounded memory usage. After calling uids := searchData.AllUIDs(), if len(uids) > 200 slice it to keep only the last 200 entries (e.g., uids = uids[len(uids)-200:]) and then iterate those to populate uidSet (uidSet.AddNum). Keep existing metrics.EmailPollDuration behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/email_trigger.go`:
- Around line 461-497: The imapDialConfig struct and dialIMAP function duplicate
connector.imapConfig/connector.imapDial which will drift; refactor by moving the
shared definitions into a single internal/shared package or by exporting the
connector types so both packages import the same symbols (e.g., replace local
imapDialConfig and dialIMAP with a single exported type and dial function in a
common package and update callers to use
connector.ImapDialConfig/connector.DialIMAP or
internal.ImapDialConfig/internal.DialIMAP), ensuring the implementation (TLS
options, Login/Close behavior and error wrapping) remains identical.
---
Duplicate comments:
In `@internal/server/email_trigger.go`:
- Around line 411-426: In fireWorkflow, the audit Emit call uses the original
ctx instead of the team-scoped teamCtx; update the call to use teamCtx (i.e.,
call e.server.Auditor.Emit(teamCtx, audit.Event{...})) so any context-aware
auditor middleware receives the team context; keep the existing e.server.Auditor
!= nil guard and the audit.Event payload (WorkflowName/ID/TeamID) unchanged.
- Around line 312-322: The code currently builds a UID set from
searchData.AllUIDs() without limiting count; change it to cap to the most recent
200 UIDs before creating uidSet to avoid unbounded memory usage. After calling
uids := searchData.AllUIDs(), if len(uids) > 200 slice it to keep only the last
200 entries (e.g., uids = uids[len(uids)-200:]) and then iterate those to
populate uidSet (uidSet.AddNum). Keep existing metrics.EmailPollDuration
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 759c01d1-3c9f-4a0c-9aa6-c5a0b3c348c9
📒 Files selected for processing (4)
internal/connector/browser.gointernal/connector/email_move_test.gointernal/connector/imap_test.gointernal/server/email_trigger.go
| // imapDialConfig holds parameters for dialling an IMAP server. | ||
| // It mirrors connector.imapConfig but is defined here to avoid a cross-package | ||
| // dependency on an unexported type. | ||
| type imapDialConfig struct { | ||
| Host string | ||
| Port string | ||
| Username string | ||
| Password string | ||
| UseTLS bool | ||
| } | ||
|
|
||
| // dialIMAP connects to an IMAP server and authenticates. | ||
| func dialIMAP(cfg *imapDialConfig) (*imapclient.Client, error) { | ||
| addr := net.JoinHostPort(cfg.Host, cfg.Port) | ||
| var ( | ||
| client *imapclient.Client | ||
| err error | ||
| ) | ||
| if cfg.UseTLS { | ||
| client, err = imapclient.DialTLS(addr, &imapclient.Options{ | ||
| TLSConfig: &tls.Config{ | ||
| ServerName: cfg.Host, | ||
| MinVersion: tls.VersionTLS12, | ||
| }, | ||
| }) | ||
| } else { | ||
| client, err = imapclient.DialInsecure(addr, nil) | ||
| } | ||
| if err != nil { | ||
| return nil, fmt.Errorf("connecting to IMAP server %s: %w", addr, err) | ||
| } | ||
| if err := client.Login(cfg.Username, cfg.Password).Wait(); err != nil { | ||
| _ = client.Close() | ||
| return nil, fmt.Errorf("IMAP login failed: %w", err) | ||
| } | ||
| return client, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared IMAP dial logic to avoid drift.
The imapDialConfig struct and dialIMAP function duplicate connector.imapConfig and connector.imapDial. While the comment at line 462 acknowledges this is to avoid cross-package dependency on an unexported type, consider exporting the connector types or extracting to a shared internal package to prevent future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/email_trigger.go` around lines 461 - 497, The imapDialConfig
struct and dialIMAP function duplicate connector.imapConfig/connector.imapDial
which will drift; refactor by moving the shared definitions into a single
internal/shared package or by exporting the connector types so both packages
import the same symbols (e.g., replace local imapDialConfig and dialIMAP with a
single exported type and dial function in a common package and update callers to
use connector.ImapDialConfig/connector.DialIMAP or
internal.ImapDialConfig/internal.DialIMAP), ensuring the implementation (TLS
options, Login/Close behavior and error wrapping) remains identical.
There was a problem hiding this comment.
Agreed. Tracked as #37 in v0.3.1 (The Locksmith Update).
There was a problem hiding this comment.
@michaelmcnees, noted — I'll avoid flagging this duplication in future reviews of these files since the refactor is already tracked for v0.3.1.
✏️ Learnings added
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/server/email_trigger.go:472-508
Timestamp: 2026-03-25T19:46:51.137Z
Learning: In `internal/server/email_trigger.go`, the `imapDialConfig` struct and `dialIMAP` function intentionally duplicate `connector.imapConfig` and `connector.imapDial` from `internal/connector/imap.go` to avoid a cross-package dependency on unexported types. The refactor (exporting shared types or extracting to a common internal package) is tracked in issue `#37` for v0.3.1 (The Locksmith Update). Do not flag this duplication in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/connector/imap.go:46-68
Timestamp: 2026-03-25T19:45:51.680Z
Learning: In `internal/connector/imap.go`, the `imapDial` function intentionally omits a `context.Context` parameter for v0.3.0. The default go-imap v2 dialer already enforces a 30-second timeout, which is considered adequate. Context-aware dialing for cancellation support is deferred to v0.3.1 and tracked in issue `#37`. Do not flag the missing context parameter in future reviews of this file.
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/connector/email_delete.go:58-67
Timestamp: 2026-03-25T19:44:44.878Z
Learning: In `internal/connector/email_delete.go`, the fallback from `UIDExpunge` to `Expunge` is intentional for v0.3.0 to maintain compatibility with older IMAP servers that do not support the UIDPLUS extension. A loud warning log is emitted when the fallback is taken. A `require_uidplus` config flag is planned for v0.3.1 to allow users to opt into stricter behavior. Do not flag this fallback as a bug in future reviews.
Cap UIDs fetched per poll cycle to 200 to prevent memory exhaustion on large mailboxes. Fix audit event in fireWorkflow to use teamCtx (which carries TeamID) instead of the bare ctx. Note: go-imap v2 client.Store() already issues UID STORE automatically when passed an imap.UIDSet (via uidCmdName dispatch) — no UIDStore method exists in this API version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/browser.go`:
- Around line 33-46: The Python wrapper (buildPythonWrapper) currently creates a
page variable via browser.new_page(), but the JavaScript/TypeScript wrapper only
exposes browser, causing API inconsistency; update the JS/TS wrapper that
constructs the Playwright script to also pre-create and expose a page variable
(using browser.newPage()) alongside browser (so both language wrappers provide
browser and page), ensuring the JS wrapper's boilerplate mirrors
buildPythonWrapper's structure and names.
- Around line 10-13: The Docker images referenced by playwrightNodeImage and
playwrightPythonImage only include browsers and system deps, not the Playwright
npm package, so relying on NODE_PATH to find the module will fail; update the
connector to either (A) use a custom image that installs the Playwright npm
package (build an image FROM mcr.microsoft.com/playwright:v1.52.0-noble and run
npm install playwright@1.52.0 during build) and point playwrightNodeImage to
that custom image, or (B) change runtime startup to install the npm package
before requiring it (e.g., run npm ci/npm install playwright into the container
at init), and remove the NODE_PATH workaround in the code that attempts to load
Playwright so the module can be resolved reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c917be31-1ccd-4b86-817a-9cdcbbcf08d4
📒 Files selected for processing (1)
internal/connector/browser.go
| // buildPythonWrapper wraps the user's Python Playwright script with boilerplate | ||
| // that launches Chromium and tears it down. | ||
| func buildPythonWrapper(userScript string) string { | ||
| // Indent each line of the user script by 8 spaces to sit inside the try block. | ||
| indented := indentLines(userScript, " ") | ||
| return "from playwright.sync_api import sync_playwright\n" + | ||
| "with sync_playwright() as p:\n" + | ||
| " browser = p.chromium.launch()\n" + | ||
| " try:\n" + | ||
| " page = browser.new_page()\n" + | ||
| indented + "\n" + | ||
| " finally:\n" + | ||
| " browser.close()\n" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider API consistency between language wrappers.
The Python wrapper pre-creates a page variable (line 42), but the JavaScript wrapper (lines 21-31) only provides browser. This means:
- Python users: Have
browserandpageavailable - JS/TS users: Have only
browser, must create page themselves
This asymmetry could confuse users switching between languages or following documentation.
Option 1: Add page to JS wrapper for consistency
func buildJSWrapper(userScript string) string {
return "const { chromium } = require('playwright');\n" +
"(async () => {\n" +
" const browser = await chromium.launch();\n" +
" try {\n" +
+ " const page = await browser.newPage();\n" +
" " + userScript + "\n" +
" } finally {\n" +
" await browser.close();\n" +
" }\n" +
"})();\n"
}Option 2: Remove page from Python wrapper
func buildPythonWrapper(userScript string) string {
indented := indentLines(userScript, " ")
return "from playwright.sync_api import sync_playwright\n" +
"with sync_playwright() as p:\n" +
" browser = p.chromium.launch()\n" +
" try:\n" +
- " page = browser.new_page()\n" +
indented + "\n" +
" finally:\n" +
" browser.close()\n"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // buildPythonWrapper wraps the user's Python Playwright script with boilerplate | |
| // that launches Chromium and tears it down. | |
| func buildPythonWrapper(userScript string) string { | |
| // Indent each line of the user script by 8 spaces to sit inside the try block. | |
| indented := indentLines(userScript, " ") | |
| return "from playwright.sync_api import sync_playwright\n" + | |
| "with sync_playwright() as p:\n" + | |
| " browser = p.chromium.launch()\n" + | |
| " try:\n" + | |
| " page = browser.new_page()\n" + | |
| indented + "\n" + | |
| " finally:\n" + | |
| " browser.close()\n" | |
| } | |
| // buildPythonWrapper wraps the user's Python Playwright script with boilerplate | |
| // that launches Chromium and tears it down. | |
| func buildPythonWrapper(userScript string) string { | |
| // Indent each line of the user script by 8 spaces to sit inside the try block. | |
| indented := indentLines(userScript, " ") | |
| return "from playwright.sync_api import sync_playwright\n" + | |
| "with sync_playwright() as p:\n" + | |
| " browser = p.chromium.launch()\n" + | |
| " try:\n" + | |
| indented + "\n" + | |
| " finally:\n" + | |
| " browser.close()\n" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/browser.go` around lines 33 - 46, The Python wrapper
(buildPythonWrapper) currently creates a page variable via browser.new_page(),
but the JavaScript/TypeScript wrapper only exposes browser, causing API
inconsistency; update the JS/TS wrapper that constructs the Playwright script to
also pre-create and expose a page variable (using browser.newPage()) alongside
browser (so both language wrappers provide browser and page), ensuring the JS
wrapper's boilerplate mirrors buildPythonWrapper's structure and names.
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 `@internal/server/email_trigger.go`:
- Around line 93-100: The loop that checks e.pollers under e.mu and then
releases the lock before calling startPoller can race and start the same trigger
twice; change this by moving the check-and-start under a single lock: add a
helper (e.g. startPollerIfNotRunning) that acquires e.mu, checks
e.pollers[t.ID], and if absent performs the start logic (or calls an internal
start routine that does not re-acquire the lock) so the existence check and
insertion are atomic; update the loop to call startPollerIfNotRunning(ctx, t)
instead of unlocking then calling startPoller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2160427c-2681-44e8-accb-b7e1493ac0e2
📒 Files selected for processing (1)
internal/server/email_trigger.go
|
Race condition in The check-and-start in Fixed by splitting into three methods:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/server/email_trigger.go (1)
93-100:⚠️ Potential issue | 🟡 MinorRace condition between checking and starting pollers persists.
The lock is released at line 96 before calling
startPollerat line 98. IfReloadis called concurrently, multiple threads could pass the check and callstartPollerfor the same trigger ID. SincestartPollerdoesn't verify the trigger ID is absent before inserting at line 129, it will overwrite the cancel function, orphaning the first goroutine.🛡️ Proposed fix: consolidate check-and-start under single lock
// Start pollers for newly added triggers. for _, t := range triggers { - e.mu.Lock() - _, running := e.pollers[t.ID] - e.mu.Unlock() - if !running { - e.startPoller(ctx, t) - } + e.startPollerIfAbsent(ctx, t) }Add a helper that holds the lock for both check and start:
func (e *EmailTriggerPoller) startPollerIfAbsent(ctx context.Context, t TriggerRecord) { e.mu.Lock() defer e.mu.Unlock() if _, exists := e.pollers[t.ID]; exists { return // already running } if len(e.pollers) >= e.maxConnections { e.server.Logger.Warn("email poller: maxConnections reached, skipping trigger", "trigger_id", t.ID, "workflow", t.WorkflowName, "max", e.maxConnections) metrics.EmailTriggersSkippedTotal.Inc() return } pollCtx, cancel := context.WithCancel(ctx) e.pollers[t.ID] = cancel e.wg.Add(1) go func() { defer e.wg.Done() defer func() { e.mu.Lock() delete(e.pollers, t.ID) e.mu.Unlock() }() e.runPoller(pollCtx, t) }() }Also applies to: 115-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 93 - 100, The current check-then-start in Reload risks races because the lock is released before calling startPoller; create a helper (e.g., startPollerIfAbsent) on EmailTriggerPoller that holds e.mu for the whole check-and-start sequence: under the lock verify e.pollers[t.ID] is absent and enforce maxConnections, create the pollCtx and store the cancel in e.pollers[t.ID], then call e.wg.Add and launch the goroutine that calls runPoller; ensure the goroutine deletes e.pollers[t.ID] under e.mu when it exits. Replace the existing unlock-then-start pattern in Reload (the loop around startPoller) with a call to this new startPollerIfAbsent to eliminate the race.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/email_trigger.go`:
- Around line 593-613: In parseRawHeaders, you redundantly call
textproto.CanonicalMIMEHeaderKey(key) even though tp.ReadMIMEHeader() already
returns canonicalized keys; remove the extra canonicalization and assign
headers[key] = values[0] (or otherwise use the key directly) when populating the
headers map to simplify the code while preserving behavior (locate this in
function parseRawHeaders where textproto.CanonicalMIMEHeaderKey(key) is used).
In `@internal/server/server.go`:
- Around line 287-291: If s.emailPoller.Start(ctx) fails, stop the previously
started cron scheduler to avoid leaking goroutines: after the failed call to
s.emailPoller.Start(ctx) and before returning the error, call the cron shutdown
method (e.g. s.cron.Stop() or s.cron.Stop(ctx) depending on the cron API) and
handle/propagate any stop error appropriately (log or wrap) so the cron is
always stopped on startup failure.
---
Duplicate comments:
In `@internal/server/email_trigger.go`:
- Around line 93-100: The current check-then-start in Reload risks races because
the lock is released before calling startPoller; create a helper (e.g.,
startPollerIfAbsent) on EmailTriggerPoller that holds e.mu for the whole
check-and-start sequence: under the lock verify e.pollers[t.ID] is absent and
enforce maxConnections, create the pollCtx and store the cancel in
e.pollers[t.ID], then call e.wg.Add and launch the goroutine that calls
runPoller; ensure the goroutine deletes e.pollers[t.ID] under e.mu when it
exits. Replace the existing unlock-then-start pattern in Reload (the loop around
startPoller) with a call to this new startPollerIfAbsent to eliminate the race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8049cae-7a15-41a1-b814-bc53cbc7a6ae
📒 Files selected for processing (2)
internal/server/email_trigger.gointernal/server/server.go
…ler start failure
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/server.go (1)
311-318:⚠️ Potential issue | 🟡 MinorConsider stopping email poller before cron on context cancellation path.
On context cancellation (line 313), the code proceeds to graceful shutdown which stops cron first (line 324), then email poller (line 327). However, on the
errChpath (lines 315-317), only the email poller is stopped before returning. For consistency and to ensure cron is also stopped on HTTP bind failures:♻️ Proposed fix
case err := <-errCh: + s.cron.Stop() s.emailPoller.Stop() return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` around lines 311 - 318, On shutdown paths, ensure the email poller is stopped before the cron and that both are stopped on the errCh path: in the select, when ctx.Done() is received call s.emailPoller.Stop() before proceeding to the cron shutdown call (the cron stop/shutdown invocation), and in the case handling err := <-errCh also call the cron shutdown method in addition to s.emailPoller.Stop() before returning err (use the existing cron stop/shutdown symbol in the file).
♻️ Duplicate comments (1)
internal/connector/browser.go (1)
22-32: 🧹 Nitpick | 🔵 TrivialAPI inconsistency: Python wrapper pre-creates
page, but JavaScript wrapper does not.The Python wrapper (line 43) creates a
pagevariable viabrowser.new_page(), while the JavaScript wrapper only exposesbrowser. Users switching between languages or following shared documentation may expect both wrappers to provide the same variables.Consider aligning the wrappers for consistency:
♻️ Option: Add page to JS wrapper
func buildJSWrapper(userScript string) string { return "const { chromium } = require('playwright');\n" + "(async () => {\n" + " const browser = await chromium.launch();\n" + " try {\n" + + " const page = await browser.newPage();\n" + " " + userScript + "\n" + " finally {\n" + " await browser.close();\n" + " }\n" + "})();\n" }Also applies to: 36-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/connector/browser.go` around lines 22 - 32, The JavaScript wrapper returned by buildJSWrapper currently exposes only browser, causing an API mismatch with the Python wrapper which pre-creates a page; update buildJSWrapper to create a page variable (e.g., const page = await browser.newPage() or await browser.newPage(); depending on Playwright API in use) inside the async block before injecting userScript so userScript can use page just like in the Python wrapper, and ensure the page is closed in the finally block (await page.close()) alongside await browser.close().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/browser.go`:
- Around line 22-31: The buildJSWrapper function inserts userScript inline
without indenting subsequent lines, which breaks readability and stack traces
for multi-line scripts; update buildJSWrapper to indent every line of userScript
(e.g., using the same indentLines helper used by the Python wrapper or implement
a similar line-by-line prefixing) before concatenation so each line is prefixed
with the wrapper indentation (match the existing " " spacing) and ensure
the final wrapped string preserves trailing newline/closing braces.
- Around line 163-174: The JSON parsing block should be skipped when the script
exited with a non-zero code to avoid misleading "stdout is not valid JSON"
errors; update the logic around outputFormat == "json" to first extract and
normalize the exit code from result (e.g., result["exit_code"] as int/float64)
and only attempt json.Unmarshal on stdout when exitCode == 0, otherwise skip
parsing (or attach raw stdout) and return the actual exit failure instead of a
JSON parse error; refer to the variables result, outputFormat, stdout, output
and the json.Unmarshal call to locate and change the code.
In `@internal/connector/email_receive_test.go`:
- Around line 97-110: The test currently only checks the first element of c.Flag
and c.NotFlag, allowing extra unexpected flags; update the assertions in the
test loop to also assert the flag slice length is exactly 1 when tt.wantFlag or
tt.wantNotFlag is non-empty and exactly 0 when they are empty, and still assert
that c.Flag[0] == tt.wantFlag and c.NotFlag[0] == tt.wantNotFlag; modify the
checks around tt.wantFlag, c.Flag, tt.wantNotFlag, and c.NotFlag so they
validate both length and value rather than only the first element.
- Around line 161-168: The current retry loop in the test exits on the first
successful c.Execute(context.Background(), params) even if the mailbox is still
empty; update the loop in email_receive_test.go to continue retrying until the
returned result contains the expected message_count (not just until err == nil).
After each successful Execute, check result["message_count"] (or the appropriate
field on the returned type) against the expected value and only break when it
matches; otherwise log the current count and continue retrying with the same
attempt limit and 2s sleep. Ensure you still surface the final result and error
after the loop.
In `@internal/server/email_trigger.go`:
- Around line 365-383: The loop currently calls e.fireWorkflow as each msgData
is fetched, which causes partial processing if a fetch error occurs; change the
logic in the Fetch handling (the block using client.Fetch, cmd.Next,
msgData.Collect) to first collect all successful message buffers into a slice
(e.g., gather results from msgData.Collect into a []bufList) while still
tracking fetchErr and handling cmd.Close, and only after the fetch loop
completes and fetchErr is nil iterate over that collected slice to call
buildEmailTriggerInputs and e.fireWorkflow for each message; ensure you still
propagate/return fetchErr when present and preserve existing closeErr handling
so no workflows are fired when a fetch failed partway.
- Around line 571-577: The current fallback uses buf.BodySection[0].Bytes which
may be the HEADER section; change the logic in the block that looks up the TEXT
specifier (using imap.FetchItemBodySection and buf.FindBodySection) so that if
FindBodySection returns nil you do NOT fall back to
buf.BodySection[0].Bytes—instead leave trigger["body"] empty (or unset) to avoid
placing headers into the body; keep using extractEmailBody(rawText) when rawText
is non-nil and reference buf.FindBodySection, buf.BodySection, extractEmailBody,
and imap.FetchItemBodySection to locate the code to modify.
- Around line 526-538: The "recent" case in buildEmailSearchCriteria (case
"recent" returning imap.Flag(`\Recent`)) lacks unit test coverage and needs
documentation about RFC 9051 deprecation; add a unit test exercising
buildEmailSearchCriteria("recent") and asserting it returns a SearchCriteria
with Flag containing imap.Flag(`\Recent`), and update comments or README near
buildEmailSearchCriteria to note RFC 9051 warnings (session-scoped, cleared on
select, inconsistent across servers) and recommend using SINCE or "unseen"
(UNSEEN/NotFlag: imap.FlagSeen) or time-based searches for production.
---
Outside diff comments:
In `@internal/server/server.go`:
- Around line 311-318: On shutdown paths, ensure the email poller is stopped
before the cron and that both are stopped on the errCh path: in the select, when
ctx.Done() is received call s.emailPoller.Stop() before proceeding to the cron
shutdown call (the cron stop/shutdown invocation), and in the case handling err
:= <-errCh also call the cron shutdown method in addition to
s.emailPoller.Stop() before returning err (use the existing cron stop/shutdown
symbol in the file).
---
Duplicate comments:
In `@internal/connector/browser.go`:
- Around line 22-32: The JavaScript wrapper returned by buildJSWrapper currently
exposes only browser, causing an API mismatch with the Python wrapper which
pre-creates a page; update buildJSWrapper to create a page variable (e.g., const
page = await browser.newPage() or await browser.newPage(); depending on
Playwright API in use) inside the async block before injecting userScript so
userScript can use page just like in the Python wrapper, and ensure the page is
closed in the finally block (await page.close()) alongside await
browser.close().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d69c0571-5e7c-461e-bdf9-74fd851215bf
📒 Files selected for processing (5)
internal/connector/browser.gointernal/connector/browser_test.gointernal/connector/email_receive_test.gointernal/server/email_trigger.gointernal/server/server.go
| func buildJSWrapper(userScript string) string { | ||
| return "const { chromium } = require('playwright');\n" + | ||
| "(async () => {\n" + | ||
| " const browser = await chromium.launch();\n" + | ||
| " try {\n" + | ||
| " " + userScript + "\n" + | ||
| " } finally {\n" + | ||
| " await browser.close();\n" + | ||
| " }\n" + | ||
| "})();\n" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
User scripts with newlines will break the JS wrapper indentation.
The JS wrapper embeds the user script inline with a single " " + userScript (line 27). Multi-line scripts will have only the first line indented, potentially causing readability issues in error traces. The Python wrapper correctly uses indentLines, but the JS wrapper does not.
♻️ Proposed fix
func buildJSWrapper(userScript string) string {
+ indented := indentLines(userScript, " ")
return "const { chromium } = require('playwright');\n" +
"(async () => {\n" +
" const browser = await chromium.launch();\n" +
" try {\n" +
- " " + userScript + "\n" +
+ indented + "\n" +
" } finally {\n" +
" await browser.close();\n" +
" }\n" +
"})();\n"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/browser.go` around lines 22 - 31, The buildJSWrapper
function inserts userScript inline without indenting subsequent lines, which
breaks readability and stack traces for multi-line scripts; update
buildJSWrapper to indent every line of userScript (e.g., using the same
indentLines helper used by the Python wrapper or implement a similar
line-by-line prefixing) before concatenation so each line is prefixed with the
wrapper indentation (match the existing " " spacing) and ensure the final
wrapped string preserves trailing newline/closing braces.
| if tt.wantFlag != "" { | ||
| if len(c.Flag) == 0 || string(c.Flag[0]) != tt.wantFlag { | ||
| t.Errorf("filter %q: Flag = %v, want %q", tt.filter, c.Flag, tt.wantFlag) | ||
| } | ||
| } else if len(c.Flag) != 0 { | ||
| t.Errorf("filter %q: unexpected Flag = %v", tt.filter, c.Flag) | ||
| } | ||
| if tt.wantNotFlag != "" { | ||
| if len(c.NotFlag) == 0 || string(c.NotFlag[0]) != tt.wantNotFlag { | ||
| t.Errorf("filter %q: NotFlag = %v, want %q", tt.filter, c.NotFlag, tt.wantNotFlag) | ||
| } | ||
| } else if len(c.NotFlag) != 0 { | ||
| t.Errorf("filter %q: unexpected NotFlag = %v", tt.filter, c.NotFlag) | ||
| } |
There was a problem hiding this comment.
Make filter assertions strict on flag count.
Current checks validate only the first element; tests can still pass with extra unexpected flags.
Suggested tightening
- if tt.wantFlag != "" {
- if len(c.Flag) == 0 || string(c.Flag[0]) != tt.wantFlag {
+ if tt.wantFlag != "" {
+ if len(c.Flag) != 1 || string(c.Flag[0]) != tt.wantFlag {
t.Errorf("filter %q: Flag = %v, want %q", tt.filter, c.Flag, tt.wantFlag)
}
} else if len(c.Flag) != 0 {
t.Errorf("filter %q: unexpected Flag = %v", tt.filter, c.Flag)
}
- if tt.wantNotFlag != "" {
- if len(c.NotFlag) == 0 || string(c.NotFlag[0]) != tt.wantNotFlag {
+ if tt.wantNotFlag != "" {
+ if len(c.NotFlag) != 1 || string(c.NotFlag[0]) != tt.wantNotFlag {
t.Errorf("filter %q: NotFlag = %v, want %q", tt.filter, c.NotFlag, tt.wantNotFlag)
}
} else if len(c.NotFlag) != 0 {
t.Errorf("filter %q: unexpected NotFlag = %v", tt.filter, c.NotFlag)
}📝 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 tt.wantFlag != "" { | |
| if len(c.Flag) == 0 || string(c.Flag[0]) != tt.wantFlag { | |
| t.Errorf("filter %q: Flag = %v, want %q", tt.filter, c.Flag, tt.wantFlag) | |
| } | |
| } else if len(c.Flag) != 0 { | |
| t.Errorf("filter %q: unexpected Flag = %v", tt.filter, c.Flag) | |
| } | |
| if tt.wantNotFlag != "" { | |
| if len(c.NotFlag) == 0 || string(c.NotFlag[0]) != tt.wantNotFlag { | |
| t.Errorf("filter %q: NotFlag = %v, want %q", tt.filter, c.NotFlag, tt.wantNotFlag) | |
| } | |
| } else if len(c.NotFlag) != 0 { | |
| t.Errorf("filter %q: unexpected NotFlag = %v", tt.filter, c.NotFlag) | |
| } | |
| if tt.wantFlag != "" { | |
| if len(c.Flag) != 1 || string(c.Flag[0]) != tt.wantFlag { | |
| t.Errorf("filter %q: Flag = %v, want %q", tt.filter, c.Flag, tt.wantFlag) | |
| } | |
| } else if len(c.Flag) != 0 { | |
| t.Errorf("filter %q: unexpected Flag = %v", tt.filter, c.Flag) | |
| } | |
| if tt.wantNotFlag != "" { | |
| if len(c.NotFlag) != 1 || string(c.NotFlag[0]) != tt.wantNotFlag { | |
| t.Errorf("filter %q: NotFlag = %v, want %q", tt.filter, c.NotFlag, tt.wantNotFlag) | |
| } | |
| } else if len(c.NotFlag) != 0 { | |
| t.Errorf("filter %q: unexpected NotFlag = %v", tt.filter, c.NotFlag) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/email_receive_test.go` around lines 97 - 110, The test
currently only checks the first element of c.Flag and c.NotFlag, allowing extra
unexpected flags; update the assertions in the test loop to also assert the flag
slice length is exactly 1 when tt.wantFlag or tt.wantNotFlag is non-empty and
exactly 0 when they are empty, and still assert that c.Flag[0] == tt.wantFlag
and c.NotFlag[0] == tt.wantNotFlag; modify the checks around tt.wantFlag,
c.Flag, tt.wantNotFlag, and c.NotFlag so they validate both length and value
rather than only the first element.
| for attempt := 1; attempt <= 5; attempt++ { | ||
| result, err = c.Execute(context.Background(), params) | ||
| if err == nil { | ||
| break | ||
| } | ||
| t.Logf("attempt %d: Execute() error: %v (retrying in 2s)", attempt, err) | ||
| time.Sleep(2 * time.Second) | ||
| } |
There was a problem hiding this comment.
Retry loop can exit before message visibility, causing flakiness.
The loop stops on first successful Execute(), even when the mailbox is still empty. Retry until message_count reaches the expected value.
Suggested fix
var result map[string]any
var err error
+ var count int
for attempt := 1; attempt <= 5; attempt++ {
result, err = c.Execute(context.Background(), params)
- if err == nil {
- break
+ if err == nil {
+ var ok bool
+ count, ok = result["message_count"].(int)
+ if ok && count >= 1 {
+ break
+ }
+ err = fmt.Errorf("message not visible in IMAP yet")
}
t.Logf("attempt %d: Execute() error: %v (retrying in 2s)", attempt, err)
time.Sleep(2 * time.Second)
}
if err != nil {
t.Fatalf("Execute() error after retries: %v", err)
}
-
- count, ok := result["message_count"].(int)
+ count, ok := result["message_count"].(int)
if !ok {
t.Fatalf("message_count is not an int: %T", result["message_count"])
}📝 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.
| for attempt := 1; attempt <= 5; attempt++ { | |
| result, err = c.Execute(context.Background(), params) | |
| if err == nil { | |
| break | |
| } | |
| t.Logf("attempt %d: Execute() error: %v (retrying in 2s)", attempt, err) | |
| time.Sleep(2 * time.Second) | |
| } | |
| var result map[string]any | |
| var err error | |
| var count int | |
| for attempt := 1; attempt <= 5; attempt++ { | |
| result, err = c.Execute(context.Background(), params) | |
| if err == nil { | |
| var ok bool | |
| count, ok = result["message_count"].(int) | |
| if ok && count >= 1 { | |
| break | |
| } | |
| err = fmt.Errorf("message not visible in IMAP yet") | |
| } | |
| t.Logf("attempt %d: Execute() error: %v (retrying in 2s)", attempt, err) | |
| time.Sleep(2 * time.Second) | |
| } | |
| if err != nil { | |
| t.Fatalf("Execute() error after retries: %v", err) | |
| } | |
| count, ok := result["message_count"].(int) | |
| if !ok { | |
| t.Fatalf("message_count is not an int: %T", result["message_count"]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/email_receive_test.go` around lines 161 - 168, The current
retry loop in the test exits on the first successful
c.Execute(context.Background(), params) even if the mailbox is still empty;
update the loop in email_receive_test.go to continue retrying until the returned
result contains the expected message_count (not just until err == nil). After
each successful Execute, check result["message_count"] (or the appropriate field
on the returned type) against the expected value and only break when it matches;
otherwise log the current count and continue retrying with the same attempt
limit and 2s sleep. Ensure you still surface the final result and error after
the loop.
| cmd := client.Fetch(uidSet, fetchOptions) | ||
| var fetchErr error | ||
| for { | ||
| msgData := cmd.Next() | ||
| if msgData == nil { | ||
| break | ||
| } | ||
| buf, err := msgData.Collect() | ||
| if err != nil { | ||
| fetchErr = err | ||
| break | ||
| } | ||
|
|
||
| inputs := buildEmailTriggerInputs(buf, folder) | ||
| e.fireWorkflow(ctx, t, inputs, imap.UID(buf.UID)) | ||
| } | ||
| if closeErr := cmd.Close(); closeErr != nil && fetchErr == nil { | ||
| fetchErr = closeErr | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider batching workflow executions to avoid partial processing on fetch errors.
The current loop fires workflows one at a time as messages are fetched (lines 367-380). If a fetch error occurs mid-batch (fetchErr set at line 374), some workflows will have already fired but the messages won't be marked as seen (lines 385-391 return early). On reconnect, those messages will be re-fetched and re-triggered.
While the at-least-once semantics are documented, consider collecting all messages first before firing any workflows, or documenting this behavior more explicitly in the logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/email_trigger.go` around lines 365 - 383, The loop currently
calls e.fireWorkflow as each msgData is fetched, which causes partial processing
if a fetch error occurs; change the logic in the Fetch handling (the block using
client.Fetch, cmd.Next, msgData.Collect) to first collect all successful message
buffers into a slice (e.g., gather results from msgData.Collect into a
[]bufList) while still tracking fetchErr and handling cmd.Close, and only after
the fetch loop completes and fetchErr is nil iterate over that collected slice
to call buildEmailTriggerInputs and e.fireWorkflow for each message; ensure you
still propagate/return fetchErr when present and preserve existing closeErr
handling so no workflows are fired when a fetch failed partway.
| // buildEmailSearchCriteria constructs IMAP search criteria matching the | ||
| // connector package's filter semantics. | ||
| func buildEmailSearchCriteria(filter string) *imap.SearchCriteria { | ||
| switch filter { | ||
| case "flagged": | ||
| return &imap.SearchCriteria{Flag: []imap.Flag{imap.FlagFlagged}} | ||
| case "recent": | ||
| return &imap.SearchCriteria{Flag: []imap.Flag{imap.Flag(`\Recent`)}} | ||
| case "all": | ||
| return &imap.SearchCriteria{} | ||
| default: // "unseen" | ||
| return &imap.SearchCriteria{NotFlag: []imap.Flag{imap.FlagSeen}} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
IMAP \Recent flag searchable server compatibility
💡 Result:
The IMAP \Recent flag indicates messages that are "recently arrived" in a mailbox for the current session (first session notified about them). It is a standard system flag defined in RFC 3501 (IMAP4rev1) section 2.3.2 and RFC 9051 (IMAP4rev2, where it is deprecated but still supported for compatibility). The flag cannot be set or cleared by clients; it is managed server-side and is session-specific. The SEARCH command (RFC 3501 section 6.4.4) explicitly supports searching for \Recent: "RECENT" matches messages with the \Recent flag set. "NEW" matches \Recent but not \Seen (equivalent to "RECENT UNSEEN"). This is part of core IMAP4rev1, so standards-compliant servers must support it. Server compatibility is generally good for standards-compliant open-source servers: - Dovecot: Fully supports RECENT in SEARCH and doveadm queries. - Cyrus IMAP: Supports core IMAP4rev1 SEARCH including RECENT. - Libraries like ImapFlow, PHP imap_search, Chilkat: Document and use RECENT/NEW searches. However, \Recent is unreliable across servers due to its session-only, multi-client sensitive nature: - Once any session SELECTs the mailbox (read-write), \Recent is cleared for subsequent sessions. - Without IDLE (not always supported), new messages may arrive after SELECT but before SEARCH, affecting results. - Commercial servers often have quirks: - Outlook/Exchange: RECENT/NEW unreliable (node-imap issues); prefer date-based searches. - Gmail: Works but uses custom extensions; RECENT flaky in multi-client scenarios. - IBM Lotus Domino: Broken \Recent handling. - Older Dovecot versions had disappearing \Recent bugs. IMAP4rev2 (RFC 9051) deprecates \Recent/RECENT responses, advising clients to ignore untagged RECENT and use alternatives like SINCE/YOUNGER (RFC 5032 WITHIN), UNSEEN, or UIDNEXT for "new" messages. Recommendation: Avoid relying on SEARCH RECENT/NEW for production due to flakiness. Use time-based searches (SINCE ), UNSEEN, or track UIDNEXT/HIGHESTMODSEQ (CONDSTORE/QRESYNC, RFC 7162).
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc3501
- 2: https://imapflow.com/docs/guides/searching/
- 3: https://tools.ietf.org/html/rfc3501
- 4: https://rfc-editor.org/rfc/rfc9051.html
- 5: https://www.php.net/manual/en/function.imap-search.php
- 6: https://www.ietf.org/rfc/rfc3501.txt
- 7: https://doc.dovecot.org/main/core/man/doveadm-search-query.7.html
- 8: https://www.chilkatsoft.com/imap_search_criteria.asp
- 9: https://www.cyrusimap.org/imap/rfc-support.html
🏁 Script executed:
# Find where buildEmailSearchCriteria is used
rg "buildEmailSearchCriteria" --type go -A 3 -B 3Repository: dvflw/mantle
Length of output: 3195
🏁 Script executed:
# Check for any documentation or comments about IMAP filters in the file
rg -A 5 -B 5 "\"recent\"" internal/server/email_trigger.goRepository: dvflw/mantle
Length of output: 486
🏁 Script executed:
# Look for related issues or discussions about IMAP server compatibility
fd -e md -e txt | xargs rg "recent|RECENT" -l 2>/dev/null | head -20Repository: dvflw/mantle
Length of output: 505
🏁 Script executed:
# Check if "recent" is documented in the docs
rg -i "recent" site/src/content/docs/server-guide/triggers.md site/src/content/docs/workflow-reference/connectors.mdRepository: dvflw/mantle
Length of output: 561
🏁 Script executed:
# Look for any test specifically for the "recent" filter
rg "TestBuildEmailSearchCriteria.*Recent|recent.*filter" internal/server/email_trigger_test.goRepository: dvflw/mantle
Length of output: 38
🏁 Script executed:
# Check the full test file to see if there's a recent test
cat -n internal/server/email_trigger_test.go | grep -A 10 -B 2 "TestBuildEmailSearch"Repository: dvflw/mantle
Length of output: 1437
Add test coverage for the "recent" filter and document RFC 9051 deprecation concerns.
The \Recent flag is documented as a supported filter option, but it lacks unit test coverage unlike the other filters. Additionally, RFC 9051 (IMAP4rev2) deprecates \Recent due to its unreliability: it is session-specific, cleared when any session selects the mailbox, and has inconsistent behavior across servers including commercial implementations (Outlook/Exchange, Gmail). If this filter is intended for production use, consider either (1) adding test coverage to validate its behavior with your supported IMAP servers, or (2) offering more reliable alternatives like time-based searches (SINCE) or the UNSEEN flag per RFC 9051 recommendations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/email_trigger.go` around lines 526 - 538, The "recent" case
in buildEmailSearchCriteria (case "recent" returning imap.Flag(`\Recent`)) lacks
unit test coverage and needs documentation about RFC 9051 deprecation; add a
unit test exercising buildEmailSearchCriteria("recent") and asserting it returns
a SearchCriteria with Flag containing imap.Flag(`\Recent`), and update comments
or README near buildEmailSearchCriteria to note RFC 9051 warnings
(session-scoped, cleared on select, inconsistent across servers) and recommend
using SINCE or "unseen" (UNSEEN/NotFlag: imap.FlagSeen) or time-based searches
for production.
|
Request timed out after 900000ms (requestId=13e1a351-7188-4270-86d6-f4393bb4fe2b) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
internal/server/email_trigger.go (3)
571-577:⚠️ Potential issue | 🟡 MinorFallback body extraction may incorrectly return headers.
If
FindBodySectionfails to locate the TEXT section (line 573), the fallback at lines 575-576 usesbuf.BodySection[0].Byteswithout checking which section it is. Since the HEADER section is also requested (lines 354-357), the first section could be headers rather than body text.🐛 Proposed fix: Remove unreliable fallback
// Extract body text from the TEXT body section. bodySection := &imap.FetchItemBodySection{Specifier: imap.PartSpecifierText} if rawText := buf.FindBodySection(bodySection); rawText != nil { trigger["body"] = extractEmailBody(rawText) - } else if len(buf.BodySection) > 0 { - trigger["body"] = extractEmailBody(buf.BodySection[0].Bytes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 571 - 577, The current fallback assigns trigger["body"] from buf.BodySection[0].Bytes which can be a HEADER part; update the logic in the email extraction block so you only set trigger["body"] when a proper TEXT body part is found: rely on buf.FindBodySection(bodySection) result and remove the unconditional fallback that uses buf.BodySection[0].Bytes, or alternatively iterate buf.BodySection to locate the PartSpecifierText/actual body bytes before calling extractEmailBody; adjust the code around imap.FetchItemBodySection, FindBodySection, buf.BodySection and extractEmailBody accordingly so headers are never passed to extractEmailBody.
526-539: 🧹 Nitpick | 🔵 TrivialAdd test coverage for the "recent" filter and document RFC 9051 deprecation.
The
\Recentflag (line 533) is supported but lacks unit test coverage unlike other filters. Per RFC 9051,\Recentis deprecated due to unreliability: it's session-specific, cleared when any session selects the mailbox, and behaves inconsistently across servers (Outlook/Exchange, Gmail). Consider adding test coverage and documenting this caveat for users relying on the "recent" filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 526 - 539, The buildEmailSearchCriteria function returns a SearchCriteria for the "recent" case using imap.Flag(`\Recent`) but lacks unit test coverage and no documentation about RFC 9051 deprecation; add a unit test that exercises buildEmailSearchCriteria("recent") and asserts it returns &imap.SearchCriteria{Flag: []imap.Flag{imap.Flag(`\Recent`)}}, and update the function comment (or nearby package docs) to note RFC 9051 deprecation and the caveats (session-specific, cleared on mailbox select, inconsistent across servers like Exchange/Gmail) so callers know "recent" is unreliable.
365-392: 🧹 Nitpick | 🔵 TrivialPartial processing on fetch error may cause duplicate workflow executions.
The loop fires workflows as messages are fetched (line 379). If a fetch error occurs mid-batch (lines 373-375), workflows already fired won't be marked as seen (the function returns at line 391), causing re-triggers on reconnect. While at-least-once semantics are documented, consider collecting all messages first before firing workflows to avoid partial processing.
♻️ Optional: Collect-then-fire pattern
+ type fetchedMsg struct { + buf *imapclient.FetchMessageBuffer + uid imap.UID + } + var msgs []fetchedMsg + cmd := client.Fetch(uidSet, fetchOptions) var fetchErr error for { msgData := cmd.Next() if msgData == nil { break } buf, err := msgData.Collect() if err != nil { fetchErr = err break } - - inputs := buildEmailTriggerInputs(buf, folder) - e.fireWorkflow(ctx, t, inputs, imap.UID(buf.UID)) + msgs = append(msgs, fetchedMsg{buf: buf, uid: imap.UID(buf.UID)}) } if closeErr := cmd.Close(); closeErr != nil && fetchErr == nil { fetchErr = closeErr } if fetchErr != nil { // ... error handling unchanged return fetchErr } + + // Fire workflows only after successful fetch of all messages. + for _, m := range msgs { + inputs := buildEmailTriggerInputs(m.buf, folder) + e.fireWorkflow(ctx, t, inputs, m.uid) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/email_trigger.go` around lines 365 - 392, The current loop uses cmd.Next()/msgData.Collect() and calls e.fireWorkflow immediately (via buildEmailTriggerInputs and e.fireWorkflow), so if a fetch error occurs mid-iteration already-fired messages won't be marked seen and will be reprocessed; change to a collect-then-fire pattern: first iterate over cmd.Next() collecting each msgData.Collect() result into a local slice (use the same identifiers buildEmailTriggerInputs and cmd.Next/msgData.Collect to locate the code), handle and propagate any Collect or cmd.Close() error after collection, then in a separate loop iterate the collected buffers to call buildEmailTriggerInputs(...) and e.fireWorkflow(...) so partial fetch failures do not cause already-fired messages to be re-triggered.internal/connector/browser.go (1)
163-174:⚠️ Potential issue | 🟠 MajorSkip JSON decoding after a failed browser run.
If the script exits non-zero, attempting to unmarshal partial stdout turns the real browser failure into “stdout is not valid JSON” and drops the exit code/stderr that
continue_on_errorconsumers need.♻️ Suggested fix
// Optionally parse stdout as JSON. if outputFormat == "json" { + switch v := result["exit_code"].(type) { + case int64: + if v != 0 { + return output, nil + } + case int: + if v != 0 { + return output, nil + } + case float64: + if v != 0 { + return output, nil + } + } stdout, _ := result["stdout"].(string) stdout = strings.TrimSpace(stdout) if stdout != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/connector/browser.go` around lines 163 - 174, The code unmarshals stdout to JSON regardless of whether the browser run failed, which masks the real failure; in the block that checks outputFormat == "json" (using the local variables result, stdout and output), first detect a failed run (e.g., non-zero exit code or a non-empty stderr field in result) and skip JSON decoding when the run failed so you preserve and return the original exit code/stderr for continue_on_error consumers; only attempt json.Unmarshal when result indicates success (zero exit) and stdout is non-empty, then set output["json"] with the parsed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/browser_test.go`:
- Around line 126-134: The current TestBuildJSWrapper_TypeScriptPassthrough only
checks string passthrough via buildJSWrapper; add an execution-focused test for
language: "typescript" that verifies the TypeScript execution branch is actually
used (e.g., assert the produced wrapper/command includes the TypeScript runner
flag such as "node --experimental-strip-types" or whatever TS-specific
invocation your code emits) so a broken TS execution path fails the test; update
or add a test that calls buildJSWrapper (and the function that assembles the
runtime command if separate) and checks for the TS-specific command token rather
than only the original TypeScript source string.
In `@internal/connector/browser.go`:
- Line 65: The env map (documented as map[string]string) must be normalized to
map[string]any before being passed to docker/run to avoid silently dropping
variables; update the code that forwards env (the local variable named env) to
create a normalized map[string]any (handle nil safely) and copy each key/value
from the map[string]string into it, then pass that normalized map into
docker/run (the docker/run call sites around the current env usage and the
second occurrence at the later call near lines 145-146). Ensure the conversion
is applied in both places where env is forwarded.
- Around line 115-119: In the case "typescript" branch the containerCmd
currently silences npm install stderr, losing useful diagnostics; update the
containerCmd assignment used with playwrightNodeImage/wrapperScript to run "npm
install --no-save --silent playwright@"+playwrightVersion without redirecting
stderr (remove "2>/dev/null") so npm registry/auth/network errors are preserved,
keeping the rest of the command that pipes into node --experimental-strip-types
intact; modify the containerCmd variable in that case branch accordingly.
---
Duplicate comments:
In `@internal/connector/browser.go`:
- Around line 163-174: The code unmarshals stdout to JSON regardless of whether
the browser run failed, which masks the real failure; in the block that checks
outputFormat == "json" (using the local variables result, stdout and output),
first detect a failed run (e.g., non-zero exit code or a non-empty stderr field
in result) and skip JSON decoding when the run failed so you preserve and return
the original exit code/stderr for continue_on_error consumers; only attempt
json.Unmarshal when result indicates success (zero exit) and stdout is
non-empty, then set output["json"] with the parsed value.
In `@internal/server/email_trigger.go`:
- Around line 571-577: The current fallback assigns trigger["body"] from
buf.BodySection[0].Bytes which can be a HEADER part; update the logic in the
email extraction block so you only set trigger["body"] when a proper TEXT body
part is found: rely on buf.FindBodySection(bodySection) result and remove the
unconditional fallback that uses buf.BodySection[0].Bytes, or alternatively
iterate buf.BodySection to locate the PartSpecifierText/actual body bytes before
calling extractEmailBody; adjust the code around imap.FetchItemBodySection,
FindBodySection, buf.BodySection and extractEmailBody accordingly so headers are
never passed to extractEmailBody.
- Around line 526-539: The buildEmailSearchCriteria function returns a
SearchCriteria for the "recent" case using imap.Flag(`\Recent`) but lacks unit
test coverage and no documentation about RFC 9051 deprecation; add a unit test
that exercises buildEmailSearchCriteria("recent") and asserts it returns
&imap.SearchCriteria{Flag: []imap.Flag{imap.Flag(`\Recent`)}}, and update the
function comment (or nearby package docs) to note RFC 9051 deprecation and the
caveats (session-specific, cleared on mailbox select, inconsistent across
servers like Exchange/Gmail) so callers know "recent" is unreliable.
- Around line 365-392: The current loop uses cmd.Next()/msgData.Collect() and
calls e.fireWorkflow immediately (via buildEmailTriggerInputs and
e.fireWorkflow), so if a fetch error occurs mid-iteration already-fired messages
won't be marked seen and will be reprocessed; change to a collect-then-fire
pattern: first iterate over cmd.Next() collecting each msgData.Collect() result
into a local slice (use the same identifiers buildEmailTriggerInputs and
cmd.Next/msgData.Collect to locate the code), handle and propagate any Collect
or cmd.Close() error after collection, then in a separate loop iterate the
collected buffers to call buildEmailTriggerInputs(...) and e.fireWorkflow(...)
so partial fetch failures do not cause already-fired messages to be
re-triggered.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3296fed1-8c14-41d9-91ec-e042ec26f280
📒 Files selected for processing (5)
internal/connector/browser.gointernal/connector/browser_test.gointernal/connector/email_receive_test.gointernal/server/email_trigger.gointernal/server/server.go
| func TestBuildJSWrapper_TypeScriptPassthrough(t *testing.T) { | ||
| // TypeScript uses the same JS wrapper since the Node Playwright image can | ||
| // execute TS syntax via the wrapper route. | ||
| tsScript := "const page: any = await browser.newPage();" | ||
| wrapper := buildJSWrapper(tsScript) | ||
| if !strings.Contains(wrapper, tsScript) { | ||
| t.Errorf("JS/TS wrapper should contain the user TypeScript script") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an execution test for language: "typescript".
This only verifies string passthrough. The actual TypeScript branch runs a different command from JavaScript, so a broken node --experimental-strip-types path can ship without any test failing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/browser_test.go` around lines 126 - 134, The current
TestBuildJSWrapper_TypeScriptPassthrough only checks string passthrough via
buildJSWrapper; add an execution-focused test for language: "typescript" that
verifies the TypeScript execution branch is actually used (e.g., assert the
produced wrapper/command includes the TypeScript runner flag such as "node
--experimental-strip-types" or whatever TS-specific invocation your code emits)
so a broken TS execution path fails the test; update or add a test that calls
buildJSWrapper (and the function that assembles the runtime command if separate)
and checks for the TS-specific command token rather than only the original
TypeScript source string.
| // - language (string) "javascript" | "typescript" | "python" (default "javascript") | ||
| // - script (string, required) the user's Playwright script | ||
| // - output_format (string) "json" | "text" (default "text") | ||
| // - env (map[string]string) environment variables |
There was a problem hiding this comment.
Normalize env before forwarding to docker/run.
The comment documents env as map[string]string, but docker/run only normalizes map[string]any. Passing the documented shape here silently drops every environment variable.
♻️ Suggested fix
- if envRaw, ok := params["env"]; ok {
- dockerParams["env"] = envRaw
- }
+ if envRaw, ok := params["env"]; ok {
+ switch env := envRaw.(type) {
+ case map[string]any:
+ dockerParams["env"] = env
+ case map[string]string:
+ converted := make(map[string]any, len(env))
+ for k, v := range env {
+ converted[k] = v
+ }
+ dockerParams["env"] = converted
+ default:
+ return nil, fmt.Errorf("browser/run: env must be an object")
+ }
+ }Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/connector/browser.go` at line 65, The env map (documented as
map[string]string) must be normalized to map[string]any before being passed to
docker/run to avoid silently dropping variables; update the code that forwards
env (the local variable named env) to create a normalized map[string]any (handle
nil safely) and copy each key/value from the map[string]string into it, then
pass that normalized map into docker/run (the docker/run call sites around the
current env usage and the second occurrence at the later call near lines
145-146). Ensure the conversion is applied in both places where env is
forwarded.
…body fallback, add TS branch test
|
Addressing latest review feedback (commit 73c5631): Fixed:
Duplicate comments — status:
|
1 participant
Summary
continue_on_errorstep option — Steps can now setcontinue_on_error: trueto allow workflow execution to continue after failure. Failed step error and partial output are exposed to downstream steps viasteps['name'].errorin CEL expressions. Includes checkpoint recovery, distributed worker support, audit events, and Prometheus metrics.Email connectors & trigger — Four new IMAP connectors (
email/receive,email/move,email/delete,email/flag) plus a persistent IMAP polling trigger that fires workflow executions on new emails. Supports folder selection, message filtering, custom tags/keywords, and connection monitoring via Prometheus metrics.Browser automation —
browser/runconnector wraps Playwright scripts (JavaScript, TypeScript, Python) inside Docker containers. Delegates to the existing Docker connector for container lifecycle, security hardening, and artifact collection.Additional changes
Test plan
go test ./... -short)go vet ./...cleango build ./cmd/mantle/succeeds.release-reviews/RELEASE-REVIEW-2026-03-25-v030.mdRelease review verdict
CONDITIONAL GO — all conditions addressed. See full report for details.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Examples
Documentation
Database & Metrics