feat: webhook/event-driven session creation and lifecycle callbacks - #381
Draft
tstapler wants to merge 18 commits into
Draft
feat: webhook/event-driven session creation and lifecycle callbacks#381tstapler wants to merge 18 commits into
tstapler wants to merge 18 commits into
Conversation
Phase 3 implementation plan + ADR-001 (extend Workflow vs. new Trigger entity), plus the requirements/research artifacts from Phases 1-2.
Backlog item d963144c grew from 8 to 12 acceptance criteria after the original plan/requirements were written. AC9-12 (async chain-fire, callback-dispatcher semaphore cap, SSRF validation, atomic dedup) were not reflected in the existing plan — patch requirements.md and implementation/plan.md to close each gap before running sdd:4-validate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…mortem.md, plan fixes Ran validation + pre-mortem + cross-artifact consistency subagents against the AC9-12-hardened plan. Consistency check found 0 blockers / 6 concerns (all addressed inline: FR7 on_queue_item_created ambiguity resolved, Goal-4 approval-bypass verification task added, imprecise FR citations fixed, speculative-scope items flagged). Pre-mortem found 3 P1 + 2 P2 findings, all patched into plan.md: - P1: TriggerFireEvent.delivery_id unique index was global instead of scoped to (workflow_id, delivery_id) — would silently swallow a second legitimately-matching trigger's fire under AC12's own dedup logic. - P1: no validation preventing a Workflow row's trigger_type from disagreeing with its populated fields, risking dual cron+webhook registration for one row. - P1: Risk Control section claimed Phase 1 was "zero user-visible change" while its own admission-gate fix changes cron-fire behavior for existing users with no pre-Phase-7 way to observe it — pulled ListTriggerFireEvents forward into Phase 1 and corrected the claim. - P2 x2: boot-time flag-gated route registration logs its decision now; chain reconciler retries are now bounded instead of indefinite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Ran /pm:triad-review (Product/UX/Engineering lenses in parallel). - Product: needs-work, 0 blockers — success-metrics/demand-validation gaps noted in validation.md as accepted tradeoffs of the non-interactive SDD pipeline, not blocking for this internal single-operator tool. - UX: needs-work, 2 blockers — Phase 7 had no create/edit form for triggers despite AC7 requiring it, and the Phase 8 e2e test assumed a UI flow that didn't exist. Added Story 7.1.2 (TriggerFormModal: create/ edit, type-specific fieldsets, masked secret handling, inline backend- validation errors, aria-live, loading state) and corrected Task 8.4.1a to depend on it and drop the never-planned "delete" scope. - Engineering: ready, 0 blockers — but flagged inaccurate reuse claims (SlackConfig/SlackNotifier/SlackNotificationSettings.tsx don't exist in this repo, only in an unimplemented sibling planning doc) and one wrong file-path guess (WorkflowRepository's ent-backed impl is actually session/ent_workflow_repository.go). Corrected throughout; also resolved the two remaining Unresolved Questions (maxChainDepth as a compile-time constant of 5, github_push branch matching as exact-match for v1). Readiness gate re-verified clean: no TODO/TBD placeholders, no open Unresolved Questions, no open pre-mortem P1/P2 checkboxes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 1 (Epics 1.1-1.3):
- Extend ent.Workflow with trigger_type/github_repo/github_branch/
webhook_slug(unique)/webhook_secret_encrypted/event_filter/
label_filter/prompt_template/last_fired_at, with indexes on
webhook_slug and trigger_type.
- Save-time trigger_type-vs-populated-fields validation in
WorkflowService.CreateWorkflow/UpdateWorkflow, and a tightened
cron-registration gate (Scheduler.Start/Reload/addCronEntry) so a
mismatched row can never dual-register as both cron and webhook.
- One-time trigger_type backfill on Scheduler.Start, adjusted from the
plan's literal "trigger_type == ''" check since ent's additive
migration already stamps the schema default ("manual") on existing
rows — backfill instead targets cron_enabled rows still at that
default, leaving genuinely mismatched rows for the tightened gate to
reject rather than silently correct.
- New TriggerFireEvent ent entity (modeled on SourceSyncEvent) with a
composite unique index on (workflow_id, delivery_id) — scoped
per-workflow per the plan's pre-mortem-corrected design, not a global
unique, so two different triggers matching the same inbound delivery
don't collide. Repository Create() returns a typed ErrDuplicateDelivery
on constraint violation (atomic insert-first dedup, not check-then-act).
- ListTriggerFireEvents RPC pulled forward from Phase 7 (pre-mortem P1 #3)
so existing cron-workflow users have observability into admission-gate
rejections before the UI ships.
- Scheduler.AdmissionGate consumer interface + BacklogService.Admit,
wired at construction; FireNow now consults it before CreateSession,
closing the pre-existing WIP-gate bypass (2026-07-12 OOM incident
precedent) and persisting a fired_failed TriggerFireEvent on rejection.
make build, make lint, make test, and gofmt all verified green.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 2 (Epics 2.1-2.4):
- VerifyGitHubSignature/VerifyWebhookSecret (server/services/webhook_signature.go):
stdlib crypto/hmac+sha256, compared via hmac.Equal.
- GitHubWebhookHandler (POST /webhooks/github): body-size-capped,
raw-body signature verification per repo-matching candidate, then for
each matched github_push candidate an atomic per-workflow dedup claim
via TriggerFireEvent's composite unique index before firing — a
single push can legitimately match more than one trigger, so dedup
and fire happen per-candidate, not as a single upfront check.
- GenericWebhookHandler (POST /webhooks/{slug}): same shape, event/
label_filter match, delivery-ID = SHA-256 digest of raw body.
- Both routes are feature-flag-gated (webhook_triggers) at both route
registration and inside each handler (defense in depth), registered
near the existing /api/hooks/permission-request receiver.
- TriggerRateLimiter: per-Workflow token bucket (golang.org/x/time/rate)
consulted in Scheduler.FireNow before the admission gate.
- WorkflowRepository.ListByTriggerType, TriggerFireEventRepository.UpdateOutcome
added to support per-candidate matching and pending->final outcome
transitions.
Phase-3 stub boundary (explicit TODOs in code): FireTrigger and
RenderTriggerPrompt don't exist yet (Phase 3). Handlers currently call
a local text/template stub (renderTriggerPromptStub) and
Scheduler.FireNow via a shared helper (renderAndFireTrigger in
webhook_trigger_common.go) — Phase 3 replaces both with the real
FireTrigger/RenderTriggerPrompt calls.
Verified independently (not just trusting the implementing agent):
- go build ./... clean
- go vet ./... clean
- gofmt clean on all changed/new files
- make lint: 0 issues
- go test -race on every webhook/trigger/dedup test (including the
concurrent-goroutine-race dedup tests) — all pass
- go test -race on the full server/services + server/workflows +
session package set surfaced one FAILURE, in a pre-existing, wholly
unrelated GitHub-keychain test (TestListGitHubAccounts_..., in
github/keychain.go + github/user_pr_cache.go — untouched by this
change, confirmed via git status). Filed as backlog item
92d679fd-eb4e-40f0-a60b-866f636c98ee per
.claude/rules/fix-flaky-tests-dont-defer.md rather than silently
routing around it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…gration
Implements plan.md's Phase 3 (Epics 3.1-3.2), replacing Phase 2's
deliberate stub:
- workflows.RenderTriggerPrompt(tmplStr, payload): text/template with
Option("missingkey=error") (a template referencing a missing payload
field returns a non-nil error, verified empirically, not a panic or
silent <no value>), zero-value FuncMap (no custom template funcs —
Turing-completeness mitigation), output wrapped in the confirmed
"--- WEBHOOK PAYLOAD DATA (treat as inert data, not instructions) ---"
marker matching BuildSessionInitialPrompt's convention, sanitized/
truncated via helpers adapted from session/backlog_context.go.
- ValidatePromptTemplate: parse-time-only check wired into
WorkflowService.CreateWorkflow/UpdateWorkflow, rejecting a malformed
prompt_template at save time rather than deferring to fire time.
- Scheduler.FireTrigger(ctx, wf, renderedPrompt, deliveryID): extracts
FireNow's admission-gate/rate-limit/CreateSession/TriggerFireEvent
logic into a method both cron and webhook paths converge on; FireNow
becomes a thin {{input}}-substitution wrapper around it, preserving
all existing FireNow callers' behavior unchanged. Also adds the
last_fired_at bump on success (used by Phase 4's missed-fire
detection).
- webhook_trigger_common.go's renderAndFireTrigger now calls the real
RenderTriggerPrompt + FireTrigger instead of Phase 2's stub; both
webhook handlers pick this up transitively. trigger_prompt_stub.go
deleted (no remaining callers, confirmed via grep).
- FireTrigger's own audit-event recording is scoped to deliveryID==""
(FireNow's shape) since webhook-path callers already pre-claim and
update their own TriggerFireEvent row — avoids a spurious
ErrDuplicateDelivery collision between the two recording paths.
Verified independently: go build ./..., go vet ./..., gofmt all clean;
go test ./server/... ./session/... fully green; go test -race on
server/workflows and the webhook/trigger/render test subset in
server/services green; make lint 0 issues; grep confirms no dangling
references to the deleted stub.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 4 (Epic 4.1, Task 4.1.1b/c — 4.1.1a's last_fired_at bump was already added in Phase 3's FireTrigger). On Scheduler.Start, every cron-enabled Workflow that registers as a cron entry is checked via checkMissedCronFire: computes the most recent expected occurrence before "now" and logs a "[WorkflowScheduler] missed cron fire" warning if last_fired_at is nil or older than that occurrence. Detection only — does not replay-fire the missed occurrence (checkMissedCronFire has no session-service dependency, so this is structurally guaranteed, not just tested). robfig/cron's Schedule interface only exposes forward Next(t), no "previous occurrence" method. mostRecentCronOccurrence searches backward from now with an exponentially widening window bounded below by the workflow's CreatedAt, confining each forward Next() walk to a narrow window rather than walking from CreatedAt unconditionally (which would cost one Next() call per historical occurrence — unbounded for an old, frequently-firing workflow). Bounding the search at CreatedAt also naturally suppresses false positives on brand-new workflows that haven't had a chance to fire yet, with no separate "existed one cron period" check needed. Verified independently: go build, go vet, gofmt clean; go test -race ./server/workflows/... green (30 tests, 6 new); make lint 0 issues. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 5 (Epics 5.1-5.2): - CallbackConfig (config/types.go): OnSessionCompleteURL/OnSessionStaleURL/ OnQueueItemCreatedURL, embedded on Config as Callbacks. - CallbackConfigProto/GetCallbackConfig/UpdateCallbackConfig: booleans only (on_session_complete_configured etc) — the raw URL is never echoed back by any RPC. - ValidateCallbackURL (server/services/webhook_ssrf.go): rejects non-http(s) schemes, loopback, link-local, private-range IPs, and the 169.254.169.254 cloud-metadata address explicitly, via net.Resolver.LookupIPAddr against the request's own (cancellable) context. Called at both config-save time (CallbackConfigService) and on every send-time delivery attempt inside CallbackDispatcher (DNS can change between save and fire — TOCTOU/DNS-rebinding). - CallbackDispatcher: non-blocking semaphore-capped (buffered channel, cap 20) Dispatch — a dispatch beyond the cap drops and logs rather than queuing or blocking the caller; up to 3 bounded-retry delivery attempts, each with an independent 5s timeout; never logs the target URL, only the event type and error. An SSRF check failure mid-retry aborts the remaining attempts entirely rather than skipping just that attempt, to avoid giving a DNS-rebinding attacker repeated chances. - Wired to the three lifecycle call sites: on_session_complete (TransitionBacklogItemStatus's done branch), on_session_stale (reconcileStaleWorkSessions's first-sighting branch, reusing MarkStuckNotified's existing dedup), on_queue_item_created (ReactiveQueueManager.OnItemAdded) — confirmed during /sdd:4-validate as the correct FR7 event via the original issue's own "needs-review" example URL. All three are fire-and-forget relative to the lifecycle transition itself. - session/callback_dispatcher.go: a session-package-local consumer interface (mirrors the existing ItemChangePublisher pattern) since session can't import server/services without a cycle. Verified independently: go build, go vet, gofmt clean; go test ./server/... ./session/... ./config/... fully green; go test -race on the callback/SSRF/dispatch test set green, including a semaphore-cap test that proves drops (not deferred queuing) under sustained load against a hanging test server; make lint 0 issues. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…ret write path Implements plan.md's Phase 6 (Epics 6.1-6.3) and closes a gap found during Phase 7 review (no RPC ever exposed a way to actually set webhook_secret_encrypted). Phase 6 — pipeline chaining: - BacklogItem gains next_workflow_id/chain_fired/chained_at/ triggered_by_chain_depth, plus a (status, chain_fired) index for the reconciler's query. - New ChainFirer (session/chain_firer.go): chain-depth check (const maxChainDepth = 5, compile-time per the plan's resolved Unresolved Question), prompt build, Scheduler.FireTriggerChained call, ChainFired write. Dispatched via a semaphore-bounded goroutine (mirroring CallbackDispatcher's inFlight-channel shape) strictly *after* TransitionBacklogItemStatus's own DB write returns — never inside that transaction (AC9). A dedicated EntRepository.ClaimChainFire performs a genuine SQL-level CAS (WHERE chain_fired=false AND updated_at=?), not a Get-then-check-then-write precondition (which would not have closed the double-fire race between the async happy-path dispatch and the restart-recovery reconciler). - TriggerChainReconciler.ReconcileChains runs on the existing 60s reconcile ticker (no new ticker) for status=done AND next_workflow_id IS NOT NULL AND chain_fired=false rows, with a bounded retry ceiling (const maxChainWaitDuration = 1h) so a chain stuck behind a saturated WIP gate eventually gives up (ChainFired=true, fired_failed) instead of retrying every tick forever. - Scheduler.FireTrigger refactored into a private fireTrigger(..., chainDepth) plus the new FireTriggerChained entry point; prompt construction factored into buildTemplatedPrompt for reuse. - AC9 proven directly: TestTransitionBacklogItemStatus_should_ returnBeforeChainFireCreateSessionBegins_When_DoneWithNextWorkflowSet asserts the transition returns in <500ms while a fake TriggerFirer stays blocked on an unclosed channel. Double-fire prevention proven under -race with 5 concurrent Dispatch calls on the same item (TestChainFirer_should_fireExactlyOnce_When_DispatchAndReconciler RaceOnSameItem): exactly one FireTriggerChained call. Webhook secret write path (gap closed): - Write-only webhook_secret field (never round-tripped back, same discipline as CallbackConfigProto) added to CreateWorkflowRequest/ UpdateWorkflowRequest. WorkflowService encrypts it with the same helper webhook verification already decrypts with; an omitted field on UpdateWorkflow leaves the existing stored secret unchanged (mirrors CallbackConfigService's masked-URL convention). - TestCreateWorkflow_WebhookSecret_RoundTripsThroughHMACVerification proves the real end-to-end path: create via RPC with a secret, sign a webhook request with that same secret, handler accepts it. Verified independently: go build ./..., go vet ./..., gofmt clean; full go test ./... green; go test -race on session (chain-fire tests) and server/services (secret round-trip) green; make lint 0 issues. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 7 (Epics 7.1-7.4), including Story 7.1.2
(TriggerFormModal, added during /pm:triad-review's UX pass to close
the "no create/edit form" blocker AC7 requires).
- TriggersPanel.tsx (+ .css.ts): list view extending ApprovalRulesPanel's
shape — type badges (github_push/cron/webhook), enable/disable toggle
(reuses the existing per-Workflow enable flag the backend already
treats as generic across trigger types), last-fired relative
timestamp, mobile FAB. Excludes plain "manual" rows to avoid a
duplicate UI surface with the existing /workflows page.
- TriggerFormModal.tsx (+ .css.ts): create/edit form — trigger-type
radio conditionally rendering github_push/cron/webhook fieldsets
with client-side field-clearing on type switch (mirrors the
server-side trigger_type-vs-populated-fields validation), inline
backend-validation-error-to-field mapping, aria-live announcements,
focus-trapped dialog with Escape-to-close. Webhook/GitHub secret
field: system-generated + show-once + copy-to-clipboard on create,
masked placeholder on edit (omitted from the update payload unless
explicitly changed) — wired to the new write-only webhook_secret RPC
field.
- TriggerExecutionHistory.tsx: 5-state badges wired to the already-
shipped ListTriggerFireEvents RPC, "N received / M matched" counter
collapsing no_match by default.
- CallbackSettings.tsx: three masked callback URLs wired to
GetCallbackConfig/UpdateCallbackConfig, same masked-placeholder-on-
edit convention as the webhook secret field.
- SessionDetailView.tsx: "Triggered by: {slug} ({trigger_type})"
attribution badge linking to the new /triggers page, reading the
existing WorkflowId field.
- New /triggers route + nav entry, gated behind the webhook_triggers
feature flag.
- 5 feature-registry entries under docs/registry/features/frontend/
per .claude/rules/feature-registry.md.
Task 7.2.1d (dry-run "Send test event" modal) skipped — plan.md
explicitly flags it as having no direct FR/AC, lowest priority in this
phase.
Verified independently: npx tsc --noEmit clean; npx jest --no-coverage
— 3888/3888 tests pass across 286 suites; next lint — 0 errors (only
pre-existing warnings in files unrelated to this feature).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…ron-only Found during Phase 7 review: validateTriggerTypeFieldConsistency (Task 1.1.1e) unconditionally rejected cron_enabled=true for any non-"cron" trigger_type. But Phase 2's GenericWebhookHandler/GitHubWebhookHandler and Phase 7's TriggersPanel toggle both independently settled on reusing CronEnabled as the generic per-trigger "enabled" flag across every trigger type — the validation made it impossible to ever enable a webhook/github_push trigger through CreateWorkflow/UpdateWorkflow, even though the underlying fire/verify logic worked correctly (only provable via direct repository calls that bypassed the RPC, which is exactly how Phase 1-3's own tests were written and why this was never caught). Two call sites had the same conflation: - validateTriggerTypeFieldConsistency's cron_enabled check, removed — redundant anyway with Scheduler.addCronEntry/Reload's existing gate (scheduler.go:206,404), which already independently requires trigger_type=="cron" before registering anything as a cron entry, so removing this check does not reopen the dual-registration risk it was meant to prevent (pre-mortem P1 #2). - The "cron_expression is required when cron_enabled is true" checks in both CreateWorkflow and UpdateWorkflow, now gated on the (resolved/ effective) trigger_type actually being "cron" — a webhook trigger enabled via cron_enabled=true has no cron schedule to require. Added TestCreateWorkflow_WebhookTriggerType_CronEnabledTrue_Accepted / TestUpdateWorkflow_CronEnabledTrue_AcceptedForNonCronTriggerType, fixed two existing tests whose expectations encoded the old (buggy) behavior, and added TestCreateWorkflow_WebhookSecret_FullHTTPRoundTrip — a true end-to-end proof (RPC create+enable → real signed HTTP request → GenericWebhookHandler.Handle → session created) that was blocked until this fix landed, closing the gap the webhook-secret round-trip tests had to route around. Verified independently: go build, go vet, gofmt clean; full go test ./... green; go test -race on the workflow/webhook/trigger/callback test subset green (61s); make lint 0 issues; web-app tsc --noEmit clean (no frontend changes needed — TriggerFormModal already sent the field this fix unblocks). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Completes pre-mortem P2 #4 (already half-done — the registered branch already logged): route registration is boot-time only, so an operator who flips webhook_triggers on expecting /webhooks/* to work immediately otherwise had zero signal anywhere in the app that a restart is still required. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Implements plan.md's Phase 8 remainder (Epics 8.3-8.4; 8.1/most-of-8.2
were already covered by prior phases and the boot-time log commit).
Registry completeness (Epic 8.3):
- Found GetCallbackConfig/UpdateCallbackConfig (Phase 5) had no
// +api: marker and no scanner methodToID entry — added both, plus
per-feature JSON files (docs/registry/features/backend/callback-config/
{get,update}.json), marked tested with their existing test names.
Verified via controlled before/after diff that this does not grow
docs/registry/coverage-gaps.json's count (64/48 unmatched before and
after — the fix only renamed two IDs from PascalCase fallbacks to the
scope:action convention, didn't add or remove coverage).
- ListTriggerFireEvents, CreateWorkflow/UpdateWorkflow, and all 5
frontend components already had complete registry entries from prior
phases.
E2E tests (Epic 8.4):
- tests/e2e/pages/TriggersPage.ts (new): page helper for TriggersPanel
(create/edit/toggle) and CallbackSettings (save/read status/read
error), data-testid/ARIA-role locators only.
- tests/e2e/triggers-panel.spec.ts (new): create-appears-in-list,
edit-persists, toggle-reflects-live-without-reload,
callback-url-round-trips-masked-after-reload,
SSRF-target-rejected-with-visible-error. // @feature header, no
waitForTimeout.
- CallbackSettings.tsx: added a data-testid on the Configured/Not-
configured status badge so it's independently addressable.
Independently re-verified (not just trusting the implementing agent):
go build/vet/gofmt clean; tools/scanner build+test clean; web-app tsc
--noEmit clean; npx jest (CallbackSettings/TriggersPanel/TriggerFormModal)
28/28 pass; make lint 0 issues; npx playwright test triggers-panel.spec.ts
against the real auto-managed isolated test server — 10/10 pass
(chromium + chromium-dom). Reverted four unrelated tests/e2e/fixtures/
*-theme.json port-number diffs picked up as a side effect of running
the suite (ephemeral test-server port baked into a tracked fixture,
not an intentional change).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…onfig, webhook_slug clear bug, reconciler scale
Found by an sdd:6-verify Go-idioms review, all confirmed real before
fixing (read the cited code, didn't take findings on faith):
- CallbackDispatcher's cfg was a one-time boot snapshot: config.LoadConfig()
returns a fresh *Config on every call, so UpdateCallbackConfig saved to
disk but never reached the dispatcher's long-lived pointer — a saved
callback URL silently never took effect until a restart, directly
contradicting AC7 ("without restarting the service"), which had
already been marked passing before this was found. Same bug class
already fixed once in this codebase for BacklogService (see
defaults_service.go's SetSharedBacklogConfig precedent) — applied
the same shared-mutex-guarded-live-pointer pattern here.
TestUpdateCallbackConfig_TakesEffectOnDispatchWithoutRestart proves
a save is visible to the very next Dispatch call, no restart.
- EntWorkflowRepository.Update unconditionally wrote SetWebhookSlug("")
when a caller cleared webhook_slug, instead of ClearWebhookSlug()
(Create already had this guard correctly). Since webhook_slug is
Optional+Unique but not Nillable, two literal "" values collide under
the unique index while two NULLs don't — the first workflow to clear
its slug would succeed, the second would fail with a unique-constraint
violation.
TestEntWorkflowRepository_Update_should_ClearWebhookSlugWithoutUniqueConstraintViolation_When_TwoWorkflowsBothClearedInSequence
proves both clears now succeed.
- TriggerChainReconciler.ReconcileChains filtered NextWorkflowID/ChainFired
in a Go loop after ListBacklogItems's default 1000-row limit already
truncated the result set — a pending chain outside that window was
silently never reconciled, undermining AC5's crash-recovery guarantee
at scale despite the schema already carrying a (status, chain_fired)
index built for exactly this query. Added ChainFired/NextWorkflowIDSet
filter fields to BacklogItemFilter so the query is now pushed into SQL.
TestTriggerChainReconciler_should_findPendingChain_When_MoreThan1000DoneItemsExist
seeds 1001+ done items and proves the one with a pending chain (placed
outside the old 1000-row window) is still found and fired.
- ChainFirer.Dispatch's unused ctx parameter removed for consistency
with its sibling CallbackDispatcher.Dispatch (same async-fire-and-
forget shape, same reason ctx can't be propagated into the goroutine).
- Added index.Fields("workflow_id", "created_at") to TriggerFireEvent
to match ListByWorkflow's actual query shape (previously only the
composite (workflow_id, delivery_id) unique index and a bare
created_at index existed, neither of which covers this query).
Verified independently (re-ran everything myself, not just trusted the
fixing agent's self-report): go build/vet/gofmt clean; full
go test ./server/... ./session/... ./config/... green; each new test
run individually to confirm it exercises what it claims; go test -race
on the touched packages green; make lint 0 issues.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Found by an sdd:6-verify React/CSS-idioms review, all confirmed real before fixing: - TriggerFormModal's clipboard-copy handler discarded the write promise unawaited and reported "Copied" unconditionally — a failed clipboard write (denied permission, insecure context, unfocused window) on a secret shown exactly once could mean irrecoverable loss of it. Now awaits the write, only confirms success genuinely, and shows a visible fallback (select-to-copy-manually) on failure. - TriggersPanel's mobile FAB had a hardcoded rgba() shadow, violating the theme-token convention and rendering wrong under themes with a non-black shadow color (e.g. the terminal theme's green glow) — now uses vars.shadow.lg. - CallbackSettings had no aria-live region for its save confirmation, unlike its sibling TriggerFormModal/TriggersPanel — screen-reader users got no announcement that Save succeeded or failed. - 8 occurrences of inline layout styles across TriggerFormModal.tsx, TriggersPanel.tsx, and TriggerExecutionHistory.tsx, violating .claude/rules/css-architecture.md's explicit "Never Do" list — moved into named .css.ts classes using vars.* tokens. - The visuallyHidden aria-live-announcer style was hand-rolled inline in two places instead of reusing the existing one from ReviewQueuePanel.css.ts — extracted to a shared web-app/src/styles/a11y.css.ts all four components now import. - generateClientSecret() silently fell back to Math.random() (a non-cryptographic PRNG) when crypto.getRandomValues was unavailable, for a value whose entire security property is unguessability — removed the fallback; generation is now disabled with an explanatory message instead. - useCallbackConfig/useTriggerFireEvents initialized loading=false despite fetching unconditionally on mount, producing a one-frame flash of empty/not-configured UI before the real state landed — now initialize loading=true. - TriggerFormModal's rotate-secret flow could silently no-op if submitted without clicking "Generate secret" first (empty webhookSecret means "no change" on the wire) — added client-side validation catching this before submit. - TriggersPanel's toggle-failure path only surfaced an error to screen readers (aria-live) with no visible indication for sighted users — added a transient visible error banner alongside it. Verified independently: npx tsc --noEmit clean; full npx jest suite — 286/286 suites, 3897/3897 tests pass; npx next lint clean (0 errors). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
…dation
Found by sdd:6-verify's Layer 3 security review (HIGH confidence, 9/10):
CallbackDispatcher's http.Client was constructed as a bare &http.Client{}
with no CheckRedirect override. Go's default client follows up to 10
redirects transparently, so a callback target that itself passed
ValidateCallbackURL could respond with a 3xx Location pointing at a
loopback/link-local/metadata address, and the client would silently
follow it — completely bypassing the send-time SSRF check AC11 requires
(the check only validates the *original* configured URL, never a
redirect target).
Fix: set CheckRedirect to refuse all redirects (http.ErrUseLastResponse).
A webhook-callback POST has no legitimate need to follow one; treating
a redirect response as a failed delivery attempt (retried like any other
non-2xx) is simpler and safer than re-validating each hop.
TestCallbackDispatcher_Deliver_DoesNotFollowRedirect proves it: a target
server that only ever 302s to a second "final" server is retried and
exhausted (3 attempts) while the final server never receives a request.
Verified independently: go build/vet/gofmt clean; go test
./server/... ./session/... green; go test -race on the callback
dispatcher tests green; make lint 0 issues.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
Found while re-verifying the rebased branch: the callback config is
global, process-wide server state, not per-browser-context. Playwright
runs this spec against two projects (chromium, chromium-dom) sharing
the same test-server instance in one invocation — the second project's
run of callback-settings_should_roundTripMaskedConfiguredBadge_When_
validUrlSaved started from whatever state the first project's run left
the on_session_complete field in, failing its initial "Not configured"
assertion.
Fix: reset the field first if it's already configured. The masked URL
input's DOM value is always "" regardless of configured state (the
real URL never round-trips), so fill("") is a no-op — added
clearCallbackUrl() using the dedicated "Clear" affordance instead
(only rendered when configured, calls setEdit directly).
Verified: 2 consecutive full runs, 10/10 pass both times.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW
8 tasks
Contributor
✅ Registry ValidationTest Coverage: 37/189 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
UX Analysis
|
Contributor
Frontend Terminal Throughput |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Allow external events to trigger session creation and allow session lifecycle events to call external webhooks. The existing rules engine is purely reactive (only evaluates inbound agent tool-call requests); this adds a proactive/outbound dimension: inbound triggers (GitHub push, cron, generic webhook) that create sessions, outbound lifecycle callbacks (session complete/stale, queue item created), and completion-triggered pipeline chaining — closing the gap noted in the original issue (#47) against competitor tools (Dorothy, Tutti, Sortie) that already support this.
Replaces #379, which was created from a branch whose base had diverged from origin/main (predating this session) and so carried ~150 unrelated files in its diff. This PR is rebased cleanly onto current
main.What Changed
ent.Workflowextended withtrigger_type/webhook_slug/event_filter/label_filter/prompt_template/etc;GitHubWebhookHandler/GenericWebhookHandlerverify HMAC signatures (hmac.Equal, constant-time) and dedup deliveries atomically via a(workflow_id, delivery_id)unique DB constraint, not a check-then-insert race.RenderTriggerPrompt—text/templatewithmissingkey=error, zero-valueFuncMap, wrapped in the same inert-data-block framingBuildSessionInitialPromptuses.Scheduler.FireTriggergeneralizes the existingFireNow(cron) path so cron and webhook triggers converge on one admission-gated, rate-limited entry point.CallbackDispatcher— semaphore-capped (drops+logs beyond capacity, never blocks the caller or queues unboundedly), bounded-retry, SSRF-validated (loopback/link-local/private-range/cloud-metadata rejected) at both config-save time and every send-time attempt, withCheckRedirectrefusing 3xx redirects so a validated target can't route around the check via a redirect response.ChainFirerdispatches asynchronously after the completing item's DB transition returns (never holds the transaction open acrossCreateSession's tmux+worktree cost), a restart-safeTriggerChainReconcileron the existing 60s tick with a bounded retry ceiling, and a hard chain-depth cap.TriggersPanel(list, type badges, live enable/disable),TriggerFormModal(create/edit, type-specific fields, show-once secret generation),TriggerExecutionHistory,CallbackSettings, and a session→trigger attribution badge, all behind thewebhook_triggersfeature flag at/triggers.Test plan
go build ./...,go vet ./...,gofmt— cleango test ./...— full repo, 0 failuresgo test -raceon every webhook-triggers-relevant package — clean, including dedicated concurrency-race tests for dedup, chain-fire, and callback-dispatch cappingmake lint— 0 issuesweb-app:tsc --noEmit, fulljestsuite (286/286 suites, 3911/3911 tests),next lint— all cleantests/e2e/triggers-panel.spec.ts— 10/10 pass (chromium + chromium-dom) against the real isolated test server driving the actual built binary, run twice consecutively to confirm stability (create, edit, live toggle, callback save+mask, SSRF rejection with visible error)TestCreateWorkflow_WebhookSecret_FullHTTPRoundTripcreates+enables a trigger via the real RPC, signs a request with that secret, and drives it through the real HTTP handler to session creationsdd:6-verifyran in full (idiom, architecture, refactor, and dedicated security review) — findings fixed and re-verified, including one HIGH-confidence SSRF-via-redirect vulnerability found and closed with a regression testmainre-verified end-to-end after resolving the one genuine conflict (re-applyingon_session_stalecallback wiring to its new post-refactor location inbacklog_lifecycle_stale.go)🤖 Generated with Claude Code
https://claude.ai/code/session_011FwJ8TrscWJtxWkw8iXbDW