Skip to content

Feature/enhance automation nodes - #375

Merged
pikann merged 7 commits into
masterfrom
feature/enhance-automation-nodes
Aug 7, 2026
Merged

Feature/enhance automation nodes#375
pikann merged 7 commits into
masterfrom
feature/enhance-automation-nodes

Conversation

@pikann

@pikann pikann commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes #366

Summary

Adds synchronous AI-agent execution and the rest of the automation node types requested in #366, then simplifies the automation status model and cleans up a few things found along the way.

New automation capabilities

  • Synchronous AI agent execution — a trigger_ai_agent action now pauses the graph walk until the conversation it started actually finishes, instead of continuing to the next node the instant the conversation is created. Resumes via a new durable StreamAgentConversationStatus stream once the conversation reaches a terminal status.
  • Wait/Delay action node — pauses a graph walk for a configured number of minutes before continuing, polled by a new WaitScheduler (same leader-lock/ticker pattern as the existing due-date/cron schedulers).
  • Sprint automation — four new sprint triggers (created/started/completed/deleted), five new sprint-scoped condition fields, and two new actions (update_sprint, complete_sprint), fed by a new durable StreamSprintActivities stream so sprint events reach the automation engine the same way task events already do.
  • {{variable}} interpolation — node config fields (agent messages, Call API URL/body/headers, task titles, sprint name/goal) can reference {{task.title}}, {{sprint.name}}, and similar placeholders from the current run.

Automation status: active/inactive only

Replaced the three-state draft/active/archived lifecycle with a single active/inactive toggle. archived previously locked a graph from being edited at all, which no longer made sense next to the pause/resume nodes above — inactive automations stay fully editable, matching how draft already worked. The three lifecycle endpoints (activate/archive/revert-to-draft) collapse to two idempotent ones (activate/deactivate), and the builder page's badge-plus-three-buttons cluster becomes a single switch.

Other fixes and cleanup

  • Bugfix: a sprint-triggered automation that paused at a Wait or trigger_ai_agent node lost its sprint context on resume, failing the downstream update_sprint/complete_sprint action with "no sprint in context". The resume path only ever reconstructed the walk's task, never its sprint.
  • Generalized the pause/resume context (previously one column per field — task_id, then sprint_id) into a single context JSONB column, so a future context type doesn't need its own migration.
  • Consolidated the pending-wait table migrations into one, since none had shipped anywhere outside this branch yet.
  • Builder page header split into two rows (identity + status; view tabs + actions) instead of one crowded strip, and the trigger picker groups its 13 built-in types into Task events / Sprint events / Schedule & webhook sections.
  • All new/changed strings translated across all 9 locales, including a couple of pre-existing gaps found while auditing.

Testing

  • go build ./... && go vet ./... && gofmt -l . && go test ./...
  • Full e2e suite (go test ./test/e2e/...), including new coverage for synchronous agent execution, Wait nodes, Sprint triggers/conditions/actions, variable interpolation, and the active/inactive lifecycle
  • Frontend tsc --noEmit and biome check .

pikann and others added 3 commits August 7, 2026 12:52
…orkflows

- Added WaitScheduler to periodically poll for wait action nodes and resume their paused graph walks.
- Introduced new database migrations to support pending agent waits and delays.
- Enhanced existing test cases to cover scenarios involving wait nodes and agent conversations.
- Updated e2e tests to validate the behavior of automation workflows with wait nodes and agent interactions.
- Refactored fake repository implementations to accommodate new methods for handling run steps and pending waits.
- Changed automation status from three states (draft, active, archived) to two (active, inactive).
- Updated related API endpoints and service methods to reflect the new status model.
- Removed references to draft and archived statuses in the codebase.
- Adjusted UI components to handle the new active/inactive toggle.
- Updated database schema to support the new status definitions.
- Modified tests to ensure compatibility with the new automation lifecycle.
CI lint job was failing on E501 in executor.py and streams.py.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #375. This is a large but well-scoped change: sprint-scoped triggers/conditions/actions, wait/trigger_ai_agent pause/resume, variable interpolation, and collapsing the automation lifecycle to active/inactive.

The test coverage is strong and the migrations are safe, but I found a critical durability bug in the pause/resume path and a high-severity fan-out AI-agent semantics bug that should be fixed before merge. I’ve also flagged a handful of medium/low issues inline.

Blocking issues

  1. Pause/resume can permanently stall a run — both ClaimPendingAgentWait and ClaimDueDelays delete the pending row before the graph walk resumes. If the process crashes or resumeWalk/resumeAfterDelay errors after the delete, the row is gone and the run can never continue.
  2. Fan-out trigger_ai_agent resumes downstream on the first finished conversation — when the action fans out to multiple conversations, each terminal status event walks the same outgoing edges, so downstream actions run before the remaining conversations have resolved.

Other concerns

  • Front-end wait node accepts fractional minutes while the backend stores an *int.
  • *target = *updated in applyUpdateSprint mutates a repository-returned pointer in place.
  • call_api interpolates user values into URLs/headers without encoding/sanitization.
  • Sprint status from the activity stream is cast without enum validation.
  • WaitScheduler.Start takes a context.Context it never uses.
  • Custom-field diff uses fmt.Sprintf("%v", …) comparison.
  • Removing /archive and /revert-to-draft is a breaking API change; it should be documented or aliased for one release.

Please address the two blocking issues at minimum; the rest can be handled here or in follow-ups.

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on the review above with line-anchored details on the blocking correctness/durability issues. Please address at least the two blocking items (pause/resume durability and fan-out AI-agent semantics) before merging; the rest are inline suggestions.

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog

pullfrog Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Line-anchored follow-up to the review above. The two blocking issues are #1 and #2; please address them before merging.


1. Critical — pause/resume can permanently stall a run (automation_consumer.go:982)

ClaimPendingAgentWait deletes the pending row before resumeWalk runs. If the consumer crashes or resumeWalk errors after the DELETE, the stream message is redelivered, but the row is gone (ClaimPendingAgentWait returns nil), so the handler just acks and returns. The run is left running with no pending waits and downstream actions never execute.

Fix: Do not delete the pending row until resumeWalk succeeds, or use a lease/outbox pattern so resume is retryable.

2. High — fan-out trigger_ai_agent resumes downstream on the first finished conversation (automation_consumer.go:1528)

When trigger_ai_agent fans out to multiple conversations, each terminal status event calls resumeWalkFrom and walks the same node's outgoing edges. The first finished conversation therefore executes downstream actions while the other conversations are still in flight. There is no test covering this case.

Fix: Track the pending conversation count per (run_id, node_id) and only walk outgoing edges after the last pending wait is cleared.

3. Medium — wait node accepts fractional minutes in the UI (automation-node-config-panel.tsx:2190)

const minutes = Number(waitMinutes); accepts 1.5, but the backend stores WaitMinutes as *int, so the API will reject fractional values.

Fix: Enforce integer minutes (e.g., parseInt, Math.floor, or step={1}).

4. Medium — sprint pointer mutated in place (automation_consumer.go:2268)

*target = *updated mutates the sprint pointer returned by resolveSprintFor, which may come from the repository/cache.

Fix: Copy the updated sprint into a fresh local variable.

5. Low — variable interpolation is not URL/header-safe (automation_consumer.go:1767)

vartemplate.Render interpolates user values directly into URLs and headers. Values with spaces, &, or newlines can produce malformed URLs or header injection.

Fix: URL-encode values in the URL path/query and strip newlines from header values.

6. Low — sprint status from stream is not validated (automation_consumer.go:665)

Status: sprintdom.SprintStatus(status) casts the stream value directly. Malformed events can propagate invalid statuses through condition evaluation.

Fix: Validate against the allowed enum and ack/drop bad messages.

7. Low — WaitScheduler.Start ignores its context (wait_scheduler.go:70)

Start(ctx context.Context) accepts a context but run() uses context.Background(), so cancellation cannot shut it down cleanly.

Fix: Use the passed context as the parent.

8. Low — loose custom-field comparison (automation_consumer.go:2108)

fmt.Sprintf("%v", existing) != fmt.Sprintf("%v", v) can miscompare numeric types or structured values.

Fix: Use a normalized comparison such as JSON marshal or reflect.DeepEqual.

9. Note — breaking API change

/archive and /revert-to-draft were removed in favor of /deactivate. Please document this in release notes, or keep the old endpoints as aliases for one release if backward compatibility matters.

Pullfrog  | View workflow run | via Pullfrog | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

pikann and others added 2 commits August 7, 2026 14:27
The persistence fixture mocked publish_realtime and publish_event but
was never updated for the newer publish_conversation_status, so
test_llm_failure_marks_conversation_failed fell through to a real
Valkey connection and failed on DNS resolution in CI (no valkey
service in that job — this suite mocks out Postgres/Valkey entirely
by design, per its own module docstring).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… 6 other issues

Verified and fixed the 9 issues from the automated PR review:

- Critical: ClaimPendingAgentWait/ClaimDueDelays deleted their row before
  the resume was confirmed to succeed, so a crash or error mid-resume lost
  it forever with no retry possible. Replaced with find/list-then-delete-
  on-success across both the trigger_ai_agent and wait node paths, and
  fixed the resulting finalizeRunIfDone double-counting (it now runs after
  the delete, not before).
- High: a trigger_ai_agent node fanned out to several conversations
  sharing one NodeID, and each terminal-status event independently walked
  that node's outgoing edges — so the first conversation to finish fired
  downstream actions while its siblings were still in flight. Added
  CountPendingAgentWaitsForNode to gate on every sibling resolving (and
  none failing) before continuing.
- Medium: the wait node's minutes input accepted fractional values in the
  UI while the backend stores an int — now floored, with step=1.
- Medium: applyUpdateSprint/applyCompleteSprint mutated a resolved sprint
  pointer in place unconditionally, including when it came from
  sprintRepo (possibly cache-shared) rather than the walker's own object —
  now only mutates the walker-owned case.
- Low: call_api interpolated {{variable}} values directly into URLs and
  headers with no escaping. Added vartemplate.RenderEscaped, applied via
  url.QueryEscape for the URL and newline-stripping for headers.
- Low: sprint status from the activity stream was cast to SprintStatus
  with no validation — now checked against ValidSprintStatuses.
- Low: WaitScheduler.Start accepted a context it never used — now wired
  through so cancellation actually stops the scheduler.
- Low: custom-field diffing compared values via fmt.Sprintf("%v", ...),
  which treats a number and its string form as equal — switched to
  JSON-marshal comparison.
- Note: the /archive and /revert-to-draft removal is a deliberate,
  already-documented breaking change (see the PR description) with no
  external consumers — no code change needed.

New regression tests for the fan-out gate and custom-field comparison
verified to fail without their respective fixes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ One concurrency issue remains. The prior review's blocking issues (pause/resume durability, fan-out gating, wait-minutes mismatch, call_api escaping, sprint cache aliasing, sprint status validation, custom-field diff, WaitScheduler context) are all resolved in the commits since the last review.

The remaining issue is a read-modify-write race in the fan-out gate: resumeWalk counts pending agent waits before deleting the completed row, so two concurrent consumers can both see stillPending > 1 and skip calling resumeWalkFrom, leaving the run stalled. The fix is to delete the resolved row first, then count remaining siblings.

Suggested inline at services/api/internal/worker/automation_consumer.go:1079.

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog

pullfrog Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Blocking feedback converted to a PR comment: the fan-out gate in resumeWalk has a read-modify-write race.

At services/api/internal/worker/automation_consumer.go:1076-1085, the code counts pending agent waits before deleting the completed row. With multiple consumers, two sibling completions can both see stillPending > 1 and skip resumeWalkFrom, leaving the run stuck.

Suggested change:

	// Delete this wait before counting so the count reflects only siblings,
	// and concurrent resumes see a consistent remaining total.
	if err := c.repo.DeleteAgentWaitForAutomationNode(ctx, wait.ID); err != nil {
		return fmt.Errorf("delete completed agent wait: %w", err)
	}
	stillPending, err := c.repo.CountPendingAgentWaitsForNode(ctx, wait.RunID, wait.NodeID)
	if err != nil {
		return fmt.Errorf("count pending agent waits for node: %w", err)
	}
	if stillPending > 0 {
		return nil
	}

This deletes the resolved wait first, then resumes the parent only when no pending siblings remain.

Pullfrog  | View workflow run | via Pullfrog | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Two sibling trigger_ai_agent waits resolving concurrently on different
consumer replicas could each read a stale sibling count via the old
separate count-then-delete/delete-then-count pattern, causing the run
to either stall forever or fire the downstream node twice. Replace
CountPendingAgentWaitsForNode with a single atomic
DeletePendingAgentWaitAndCountRemaining that deletes-and-counts inside
one transaction serialized by a Postgres advisory lock keyed on
(run_id, node_id).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Closed the remaining read-modify-write race in the trigger_ai_agent fan-out gate by replacing the separate count-then-delete with an atomic DeletePendingAgentWaitAndCountRemaining operation. A transaction-scoped Postgres advisory lock keyed on (runID, nodeID) serializes sibling resolutions across consumer replicas, so the last wait always sees remaining == 0 and proceeds exactly once.

Pullfrog  | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

  • Hardened WaitScheduler's Redis leader lock with per-acquisition tokens, compare-and-delete release, and per-delay lease renewal so a long backlog can't outlive the lock.
  • Made StopConversation publish a terminal "stopped" status to StreamAgentConversationStatus so a trigger_ai_agent node paused waiting on this conversation resumes when the conversation is explicitly stopped.
  • Added E2E regression coverage for the explicit-stop resume path.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread services/api/internal/service/agent/agent_service.go
@pikann
pikann merged commit 646e188 into master Aug 7, 2026
10 checks passed
@pikann
pikann deleted the feature/enhance-automation-nodes branch August 7, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhancing Workflows: Synchronous AI Agent Execution and More Automation Nodes

1 participant