packages/engine/internal/engine/workflow_connector_test.go (1)

453-466: Unused helper function.

applyWorkflowVersioned is defined but never called in this test file. Either use it or remove it to avoid dead code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/workflow_connector_test.go` around lines 453
- 466, The helper function applyWorkflowVersioned is unused; either remove it or
replace existing test calls that manually parse/save workflows to use this
helper. Locate applyWorkflowVersioned and either delete the function to
eliminate dead code, or refactor tests that call workflow.ParseBytes and
workflow.Save to invoke applyWorkflowVersioned(t, database, yamlContent) so
parsing and saving are centralized; ensure any returned version and error are
handled where tests expect them.

packages/site/src/content/docs/workflow-reference/connectors.md (1)

623-625: Add language specifier to fenced code block.

The CEL path example code block lacks a language identifier. Consider using cel or text for clarity.

-```
+```cel
 steps['my-step'].output.steps['child-step'].output.field

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @packages/site/src/content/docs/workflow-reference/connectors.md around lines
623 - 625, The fenced code block containing the CEL path example
(steps['my-step'].output.steps['child-step'].output.field) is missing a language
specifier; update the markdown in connectors.md to add a language tag to the
triple-backtick fence (e.g., cel or text) immediately before the example
line so the block reads with the chosen specifier and renders correctly.

</details>

</blockquote></details>
<details>
<summary>packages/engine/internal/cli/logs.go (1)</summary><blockquote>

`266-268`: **Consider logging query errors instead of silent return.**

Silently returning on database query failure could mask connectivity or schema issues. While this matches the existing error-handling style in similar code, adding a debug log would improve troubleshooting.

<details>
<summary>🔧 Optional: Add debug logging</summary>

```diff
 	if err != nil {
+		// Optionally log for debugging: fmt.Fprintf(cmd.ErrOrStderr(), "debug: failed to query child executions: %v\n", err)
 		return
 	}
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cli/logs.go` around lines 266 - 268, Replace the
silent early return in the block that reads "if err != nil { return }" (the
error check immediately after the database query) with a logged message that
includes the error and contextual text, e.g., using the file's existing logger
(log or logger) to emit a debug/error log like "query failed: %v" or similar,
then return; keep the same flow but ensure err is logged for troubleshooting.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @packages/engine/internal/cli/cancel.go:

• Around line 31-40: The DB-only cancel currently flips
  workflow_executions.status via the cancel CLI (the ExecContext call updating
  status), but running executions in engine.go (look for the execution loop /
  methods like ExecuteWorkflow, runExecution, or the main loop in engine.Engine)
  never re-check workflow_executions.status, so they continue invoking connectors
  and may later overwrite 'cancelled'. Fix: add a cancellation check in the live
  execution path by querying the workflow_executions.status (or a dedicated
  cancelled flag) at safe interruption points inside ExecuteWorkflow/runExecution
  (before starting a task, before retries, and between connector calls) and
  abort/cleanup when status == 'cancelled', and ensure any code that writes final
  status (completed/failed) respects a prior cancelled state (skip overwriting if
  DB shows cancelled).
• Around line 31-40: The recursive cancel path currently issues multiple raw
  ExecContext calls (the CTE updating workflow_executions and the subsequent
  step_executions update) and ignores the second error and audit emission;
  refactor into a single helper that opens a DB transaction, performs the
  recursive SELECT/UPDATE for workflow_executions and the related step_executions
  update inside that transaction, returns any error to the caller, and for every
  updated row emits an AuditEmitter event (use the AuditEmitter interface already
  used elsewhere) before committing; ensure the helper is called from the cancel
  command in cancel.go and that all ExecContext errors are checked and propagated
  so the operation is atomic and auditable.

In @packages/engine/internal/cli/status.go:

• Around line 102-135: The current query in the status command uses WHERE
  parent_execution_id = $1 and only returns direct children; replace it with a
  recursive CTE that starts from execID and recursively selects all descendants,
  emitting a depth (level) column; update the childExec struct to include Depth
  (int), scan the extra depth field from childRows, and when rendering use Depth
  to indent (e.g., multiply spaces or prefix with └─) so the full descendant tree
  (not just direct children) is shown; keep using
  database.QueryContext/cmd.Context(), preserve ORDER BY (e.g., started_at) inside
  or after the CTE, and continue using statusIcon(c.Status) when printing each
  entry.

In @packages/engine/internal/config/config.go:

• Around line 134-135: The MaxWorkflowDepth field (MaxWorkflowDepth) is not
  bound to a CLI flag in the Load function, so add a BindPFlag call in Load next
  to the other engine settings to expose engine.max_workflow_depth to CLI
  overrides; locate the Load function and the existing engine flag BindPFlag calls
  (the same block that binds other engine fields) and add a BindPFlag for the
  MaxWorkflowDepth flag name that matches your flag naming convention (so Viper
  will pick up the CLI flag for MaxWorkflowDepth), and repeat the same fix where
  similar engine bindings are missing (the other engine binding blocks
  referenced).

In @packages/engine/internal/engine/workflow_connector.go:

• Around line 22-28: The doc comment for Execute incorrectly documents a
  token_budget param that isn't implemented; remove the - token_budget (int64, optional): token budget override for the child line from the Execute function's
  comment so the documentation matches the current implementation (the Execute
  method and its use of params/map[string]any in the workflow_connector.go file,
  notably the params handling around the existing extraction logic). If/when you
  implement token budget extraction in Execute (or related helpers), reintroduce
  the doc entry then.

In @packages/site/src/content/docs/workflow-reference/connectors.md:

• Line 609: The docs claim a token_budget param but the implementation never
  reads or forwards it; either remove it from the doc or implement extraction and
  forwarding: modify the function that reads params (the block that currently
  extracts workflow, version, and inputs) to also pull token_budget from
  params, validate/coerce it to an integer (e.g., positive number or null), and
  then pass that value into the downstream call to resumeExecution (the existing
  resumeExecution invocation) so the child execution receives the budget;
  reference the params extraction code and the resumeExecution call when
  making the change.

---

Nitpick comments:
In @packages/engine/internal/cli/logs.go:

• Around line 266-268: Replace the silent early return in the block that reads
  "if err != nil { return }" (the error check immediately after the database
  query) with a logged message that includes the error and contextual text, e.g.,
  using the file's existing logger (log or logger) to emit a debug/error log like
  "query failed: %v" or similar, then return; keep the same flow but ensure err is
  logged for troubleshooting.

In @packages/engine/internal/engine/workflow_connector_test.go:

• Around line 453-466: The helper function applyWorkflowVersioned is unused;
  either remove it or replace existing test calls that manually parse/save
  workflows to use this helper. Locate applyWorkflowVersioned and either delete
  the function to eliminate dead code, or refactor tests that call
  workflow.ParseBytes and workflow.Save to invoke applyWorkflowVersioned(t,
  database, yamlContent) so parsing and saving are centralized; ensure any
  returned version and error are handled where tests expect them.

In @packages/site/src/content/docs/workflow-reference/connectors.md:

• Around line 623-625: The fenced code block containing the CEL path example
  (steps['my-step'].output.steps['child-step'].output.field) is missing a language
  specifier; update the markdown in connectors.md to add a language tag to the
  triple-backtick fence (e.g., cel or text) immediately before the example
  line so the block reads with the chosen specifier and renders correctly.

</details>

<details>
<summary>🪄 Autofix (Beta)</summary>

Fix all unresolved CodeRabbit comments on this PR:

- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Organization UI

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `08727fbe-fc5a-49e2-8f18-631976d188cf`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 81daf35c1165482482298c6be8253f7d85852274 and 712c8f7911c692bf9793f64fd8012d8ece311ec6.

</details>

<details>
<summary>📒 Files selected for processing (12)</summary>

* `packages/engine/internal/audit/audit.go`
* `packages/engine/internal/cli/cancel.go`
* `packages/engine/internal/cli/logs.go`
* `packages/engine/internal/cli/run.go`
* `packages/engine/internal/cli/serve.go`
* `packages/engine/internal/cli/status.go`
* `packages/engine/internal/config/config.go`
* `packages/engine/internal/db/migrations/016_workflow_composition.sql`
* `packages/engine/internal/engine/engine.go`
* `packages/engine/internal/engine/workflow_connector.go`
* `packages/engine/internal/engine/workflow_connector_test.go`
* `packages/site/src/content/docs/workflow-reference/connectors.md`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

packages/engine/internal/cli/cancel.go (1)

84-113: ⚠️ Potential issue | 🟠 Major

Don't make cancellation audits best-effort.

This transaction mutates both workflow_executions and step_executions, but only emits execution-level audit events after Commit, and _ = auditor.Emit(...) drops failures. A successful cancel can therefore leave no durable audit trail, and the step cancellations are never audited at all. Please emit through an audit.Emitter before commit and treat audit failures as part of the operation. As per coding guidelines, "Every state-changing operation must emit an audit event via the AuditEmitter interface."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cli/cancel.go` around lines 84 - 113, The cancel
path currently commits DB changes then emits audit events and ignores failures;
instead collect the cancelled workflow execution IDs (cancelledIDs) and the
cancelled step execution IDs (from the same recursive query or by returning ids
from the UPDATE) while still in the transaction, construct an audit.Emitter (use
audit.Emitter interface or the existing audit.PostgresEmitter with DB but call
Emit before committing), call auditor.Emit(...) for each workflow execution and
for each cancelled step execution inside the transaction and treat any Emit
error as a failure (return the error) so the commit never proceeds if auditing
fails; do not swallow errors from auditor.Emit and ensure Emit is invoked before
tx.Commit() rather than after.

packages/engine/internal/engine/engine.go (1)

181-189: ⚠️ Potential issue | 🟠 Major

Cancellation is still only observed between steps.

This poll prevents starting the next step, but a step already inside executeStepLogic can keep retrying/backing off after mantle cancel, and its later updateStep call will overwrite the step_executions.status = 'cancelled' written by packages/engine/internal/cli/cancel.go. Please re-check cancellation inside the attempt loop / step update path so in-flight work stops promptly and cancelled step rows stay cancelled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/engine.go` around lines 181 - 189, The
current cancellation check in engine.go only runs between steps, so in-flight
work inside executeStepLogic can keep retrying and later calls to updateStep can
overwrite a cancelled step; fix by adding cancellation checks inside the
attempt/retry loop in executeStepLogic (and immediately before calling
updateStep) by querying the step_executions.status (e.g., SELECT status FROM
step_executions WHERE id = $1) via e.DB.QueryRowContext(ctx, ...) or by
observing a cancelled context and aborting retries, and modify updateStep to
perform a conditional update (or check current status first / use WHERE status
!= 'cancelled') so it does not overwrite an existing 'cancelled' status written
by the cancel handler.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/engine/internal/engine/engine.go (1)

993-999: Missing team_id filter and potential TOCTOU race.

1. The SELECT query doesn't include team_id, while the subsequent UPDATE does filter by it. This is inconsistent.

2. There's a small TOCTOU window between the SELECT and UPDATE: if cancellation occurs between these two queries, the UPDATE could overwrite the "cancelled" status. For a robust solution, consider using a single atomic UPDATE with a WHERE status != 'cancelled' clause.

♻️ Proposed atomic update

Replace the read-then-write pattern with a conditional UPDATE:

 func (e *Engine) updateExecutionStatus(ctx context.Context, execID, status, errMsg string) error {
-	// Don't overwrite a cancelled status.
-	var existing string
-	if err := e.DB.QueryRowContext(ctx, "SELECT status FROM workflow_executions WHERE id = $1", execID).Scan(&existing); err == nil {
-		if existing == "cancelled" {
-			return nil // Already cancelled, don't overwrite.
-		}
-	}
-
 	var completedAt any
 	if status == "completed" || status == "failed" || status == "cancelled" || status == "timed_out" {
 		completedAt = time.Now()
 	}

 	teamID := auth.TeamIDFromContext(ctx)
-	_, err := e.DB.ExecContext(ctx,
-		`UPDATE workflow_executions SET status = $1, completed_at = $2, updated_at = NOW() WHERE id = $3 AND team_id = $4`,
-		status, completedAt, execID, teamID,
+	result, err := e.DB.ExecContext(ctx,
+		`UPDATE workflow_executions SET status = $1, completed_at = $2, updated_at = NOW() 
+		 WHERE id = $3 AND team_id = $4 AND status != 'cancelled'`,
+		status, completedAt, execID, teamID,
 	)
 	if err != nil {
 		return fmt.Errorf("updating execution %s status to %s: %w", execID, status, err)
 	}
+	// Zero rows affected means execution was already cancelled — this is expected.
+	_ = result
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/engine.go` around lines 993 - 999, The
SELECT-then-UPDATE in the cancellation path (using e.DB.QueryRowContext and
execID) misses the team_id filter and has a TOCTOU risk; replace the
read-then-write with a single atomic UPDATE on workflow_executions that includes
both id and team_id and a WHERE clause preventing overwrite of cancelled status
(e.g. WHERE id = $1 AND team_id = $2 AND status != 'cancelled'), then check the
result via RowsAffected to determine if the row was already cancelled or
successfully updated; remove the prior SELECT block and use the UPDATE's
affected count to decide returning nil vs handling not-found/already-cancelled
cases.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/engine/internal/engine/engine.go (1)

625-629: ⚠️ Potential issue | 🟡 Minor

Missing team_id filter in retry-loop cancellation check.

Same concern as the main execution loop check—this query should include a team_id filter for consistency with the codebase's defense-in-depth approach.

🛡️ Proposed fix

+		teamID := auth.TeamIDFromContext(ctx)
 		var execStatus string
-		if err := e.DB.QueryRowContext(ctx, "SELECT status FROM workflow_executions WHERE id = $1", execID).Scan(&execStatus); err == nil && execStatus == "cancelled" {
+		if err := e.DB.QueryRowContext(ctx, "SELECT status FROM workflow_executions WHERE id = $1 AND team_id = $2", execID, teamID).Scan(&execStatus); err == nil && execStatus == "cancelled" {
 			return nil, fmt.Errorf("execution cancelled")
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/engine.go` around lines 625 - 629, The
cancellation check in the retry loop uses e.DB.QueryRowContext to query "SELECT
status FROM workflow_executions WHERE id = $1" but omits the team_id filter;
update the query used in that block (around the QueryRowContext call) to include
"AND team_id = $2" and pass the appropriate team identifier (e.g., the existing
teamID variable or method parameter used elsewhere in the function) as the
second argument (keep using execID as the first), leaving the subsequent Scan
and status comparison unchanged so the cancellation check respects team scoping.

packages/engine/internal/engine/engine.go (1)

999-1005: TOCTOU race between SELECT and UPDATE in updateExecutionStatus.

The status check at lines 1001-1005 and the subsequent UPDATE at line 1013 are separate queries, allowing a race where another process could change the status between them. Consider using a single atomic UPDATE with a WHERE status != 'cancelled' predicate, similar to the approach in updateStep.

♻️ Proposed atomic approach

 func (e *Engine) updateExecutionStatus(ctx context.Context, execID, status, errMsg string) error {
-	// Don't overwrite a cancelled status.
-	var existing string
-	if err := e.DB.QueryRowContext(ctx, "SELECT status FROM workflow_executions WHERE id = $1", execID).Scan(&existing); err == nil {
-		if existing == "cancelled" {
-			return nil // Already cancelled, don't overwrite.
-		}
-	}
-
 	var completedAt any
 	if status == "completed" || status == "failed" || status == "cancelled" || status == "timed_out" {
 		completedAt = time.Now()
 	}

 	teamID := auth.TeamIDFromContext(ctx)
-	_, err := e.DB.ExecContext(ctx,
-		`UPDATE workflow_executions SET status = $1, completed_at = $2, updated_at = NOW() WHERE id = $3 AND team_id = $4`,
+	result, err := e.DB.ExecContext(ctx,
+		`UPDATE workflow_executions SET status = $1, completed_at = $2, updated_at = NOW()
+		 WHERE id = $3 AND team_id = $4 AND status != 'cancelled'`,
 		status, completedAt, execID, teamID,
 	)
 	if err != nil {
 		return fmt.Errorf("updating execution %s status to %s: %w", execID, status, err)
 	}
+	// No rows affected means the execution was already cancelled (or not found).
+	// This is not an error—we simply skip the update.
+	_ = result
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/engine.go` around lines 999 - 1005, The
SELECT-then-UPDATE in updateExecutionStatus introduces a TOCTOU race: replace
the two-step logic (the SELECT of workflow_executions.status into variable
existing and then the separate UPDATE) with a single atomic UPDATE that includes
a WHERE clause preventing overwrite when status = 'cancelled' (e.g., UPDATE ...
SET status = $1, ... WHERE id = $2 AND status != 'cancelled'), then check
affected rows to determine if the update occurred; mirror the pattern used by
updateStep to ensure only non-cancelled executions are updated and avoid the
race.

packages/engine/internal/engine/workflow_connector_test.go (3)

203-212: Hardcoded team ID bypasses auth context.

The manual INSERT uses a hardcoded UUID (00000000-0000-0000-0000-000000000001) for team_id rather than using the team from context. This works for testing but creates a subtle inconsistency—the execution at depth 1 has a different team than what auth.TeamIDFromContext(ctx2) would return (empty string for context.Background()).

Consider adding a helper or setting the team context:

ctx2 := auth.WithTeamID(context.Background(), "00000000-0000-0000-0000-000000000001")
ctx2 = WithExecutionID(ctx2, deepExecID)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/workflow_connector_test.go` around lines 203
- 212, The test inserts a workflow_execution row with a hardcoded team UUID
which bypasses the auth context and makes auth.TeamIDFromContext(ctx2)
inconsistent; update the test to set the team in the context used for the deep
execution instead of relying on the hardcoded value — e.g., after creating
deepExecID, call auth.WithTeamID(context.Background(), "<same-uuid>") (or derive
the team id used for insertion) to produce ctx2 and then call
WithExecutionID(ctx2, deepExecID) so QueryRowContext/Scan and subsequent calls
(QueryRowContext, auth.TeamIDFromContext, WithExecutionID) all use the same team
context, or add a small helper to build that context consistently.

---

176-199: Consider simplifying the explanatory comments.

The extensive inline comments explaining the depth calculation logic (lines 176-199) make the test harder to follow. Consider moving this reasoning to a test case name or a brief comment, and letting the test code speak for itself.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/workflow_connector_test.go` around lines 176
- 199, The long inline explanation around the depth math should be replaced with
a short, focused comment and clearer test naming: remove the multi-line
reasoning and replace it with a one-liner like "verify child invocation succeeds
when parentDepth=0 and MaxWorkflowDepth=1" (or create a separate test named to
capture the failure case when parentDepth >= MaxWorkflowDepth); keep the
existing assertions using result.Status and result.Error, and remove the
explanatory references to childDepth/parentDepth/MaxWorkflowDepth from the
body—if you need to preserve the failing-case reasoning, move it to the test
name or a brief top-of-file comment; use the variables mentioned in the diff
(result, parentDepth, MaxWorkflowDepth, childDepth, t.Logf) to locate and update
the code.

---

355-381: Unit test for depth logic is valuable but tests implementation directly.

TestWorkflowConnector_DepthCheck_Unit duplicates the depth calculation logic (childDepth := parentDepth + 1; exceeded := childDepth > maxDepth) rather than invoking actual connector code. If the connector's logic ever diverges, this test would still pass. Consider either:

1. Testing the actual connector method if possible, or
2. Adding a comment noting this tests the expected algorithm, not the implementation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/engine/workflow_connector_test.go` around lines 355
- 381, TestWorkflowConnector_DepthCheck_Unit duplicates the depth calculation
instead of exercising the actual connector logic; update the test to call the
WorkflowConnector's real depth-checking method (the method in the connector that
enforces max depth) with representative parentDepth and maxDepth inputs and
assert its error/boolean result, or if calling the connector isn't possible, add
a clear comment above TestWorkflowConnector_DepthCheck_Unit stating it
intentionally tests the expected algorithm rather than the implementation so
future maintainers understand the limitation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/engine/internal/cli/cancel.go (1)

85-109: ⚠️ Potential issue | 🟠 Major

Audit the step cancellations too.

The execution-side audit fix at Lines 101-109 is good, but Lines 85-93 still mutate step_executions without corresponding audit rows. That leaves part of the recursive cancel path invisible in the audit trail. Return the affected step IDs and emit matching audit events before commit.

As per coding guidelines, "Every state-changing operation must emit an audit event via the AuditEmitter interface."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cli/cancel.go` around lines 85 - 109, The UPDATE
that cancels rows in step_executions (executed via tx.ExecContext) currently
mutates state without emitting audit events; modify the query to RETURN the
affected step_executions.id (or perform a SELECT of ids using the same WITH
RECURSIVE children CTE) so you collect the cancelled step IDs into a slice
(e.g., cancelledStepIDs) inside the same transaction, then call audit.EmitTx for
each cancelledStepID (similar to the existing loop for cancelledIDs) before
committing; ensure you reference the same tx and context and produce
audit.Resource{Type:"step_execution", ID: <id>} and handle errors the same way
as audit.EmitTx for executions.

packages/engine/internal/cli/cancel.go (1)

42-50: Extract the subtree CTE once.

The same WITH RECURSIVE children block appears twice in this command. Any future scope or status change now has to stay in sync in two places, which is easy to miss. A shared helper or SQL constant would make this path safer to maintain.

Also applies to: 86-93

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cli/cancel.go` around lines 42 - 50, Extract the
repeated WITH RECURSIVE children CTE into a single shared SQL constant (e.g.,
childrenCTE or cteChildrenSQL) and reuse that constant in both places where the
long CTE string is currently embedded (the UPDATE ... RETURNING id cancel query
and the other query around lines 86-93). Replace the duplicated literal with
string concatenation or fmt.Sprintf to insert the shared CTE into the existing
query variables so future changes to the subtree CTE only need to be made in one
place.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes