feat(controller): dispatch agent runs for incident.io webhook events#45
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughProcessIncidentEvent now launches idempotent agent TaskRuns and Kubernetes Jobs per incident.io webhook (keyed by incident_id:event_type), selects the configured triage engine (default claude-code), applies a per-call append_system_prompt override, and adds tests and operator-facing docs. ChangesIncident.io Webhook Dispatch
Sequence Diagram(s)sequenceDiagram
participant IncidentIO as incident.io (Webhook)
participant Reconciler as Reconciler.ProcessIncidentEvent
participant TaskRunStore as TaskRunStore
participant Engine as Engine.BuildExecutionSpec
participant K8s as K8sClient (Create Job)
IncidentIO->>Reconciler: POST event (incident_id, event_type, payload)
Reconciler->>TaskRunStore: check idempotency key (incident_id:event_type)
alt no existing running TaskRun
Reconciler->>Engine: BuildExecutionSpec(engine, EngineConfig with append_system_prompt)
Engine-->>Reconciler: execution spec / error
Reconciler->>K8s: jobBuilder -> Create Job
K8s-->>Reconciler: Job created / error
Reconciler->>TaskRunStore: persist TaskRun (running), record mappings
else duplicate running
Reconciler-->>IncidentIO: skip duplicate (no new Job)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/controller/incident.go (2)
194-196: 💤 Low valueStream reader conditional may be overly restrictive.
The stream reader is only started when
engineName == defaultIncidentEngine(i.e.,"claude-code"). If the config explicitly setsIncidentTriage.Engine = "claude-code", this works. However, if other streaming-capable engines are added in the future, this hardcoded check would skip streaming for them.Consider checking an engine capability flag instead, or document this as intentional for Step 1b.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/incident.go` around lines 194 - 196, The current conditional only starts streaming for a hardcoded engine name (engineName == defaultIncidentEngine), which prevents streaming-capable engines added later from using startStreamReader; change the check to use an engine capability flag or helper (e.g., replace the equality check with a capability query like engineSupportsStreaming(engineName) or consult IncidentTriage.Engine's configured capability) and call r.startStreamReader(ctx, tr) when that capability is true; update or add the helper (engineSupportsStreaming) to reference the engine registry/config so future engines that support streaming will work without modifying this code.
139-144: 💤 Low valueConsider whether store failures should abort job creation.
When
taskRunStore.Savefails (line 139), the function logs but continues to create the K8s Job. If the store is the primary source of truth for recovery after restarts, this could lead to orphaned jobs that aren't tracked.The pattern appears intentional to prioritise availability over consistency, but verify this aligns with the recovery model. If store persistence is critical, returning the error would be safer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/incident.go` around lines 139 - 144, The code currently logs failures from r.taskRunStore.Save(ctx, tr) but proceeds to create the K8s Job; if persistent store writes are required for recovery, change this to abort job creation by returning the error (or a wrapped error) after logging so the caller knows the save failed; specifically, in the function containing r.taskRunStore.Save, replace the current log-only branch with r.logger.ErrorContext(...) followed by returning the error (or fmt.Errorf("save task run: %w", err)) so that the downstream Job creation code is not executed when r.taskRunStore.Save fails.internal/controller/incident_test.go (1)
1-265: ⚡ Quick winConsider adding test for terminal run re-processing.
The implementation allows re-launching a task run when the prior run with the same idempotency key is terminal (lines 84-91 in
incident.go), but this behaviour isn't tested. A test that transitions an existing TaskRun to a terminal state and verifies a new run can be created would complete the idempotency coverage.📋 Suggested test case
func TestProcessIncidentEvent_RelaunchAfterTerminal(t *testing.T) { cfg := incidentTestConfig() logger := testLogger() k8s := fake.NewSimpleClientset() eng := &recordingEngine{name: "claude-code"} jb := &mockJobBuilder{} r := NewReconciler(cfg, logger, WithEngine(eng), WithJobBuilder(jb), WithK8sClient(k8s), WithNamespace("test-ns"), ) ctx := context.Background() evt := incidentTestEvent("01HZTERMINAL") // First run require.NoError(t, r.ProcessIncidentEvent(ctx, evt)) // Simulate terminal state tr, ok := r.GetTaskRun("01HZTERMINAL:" + webhook.EventIncidentCreatedV2) require.True(t, ok) require.NoError(t, tr.Transition(taskrun.StateSucceeded)) // Second run should create a new job since prior is terminal require.NoError(t, r.ProcessIncidentEvent(ctx, evt)) jobs, err := k8s.BatchV1().Jobs("test-ns").List(ctx, metav1.ListOptions{}) require.NoError(t, err) assert.Len(t, jobs.Items, 2, "terminal run should allow re-launch") assert.Equal(t, 2, eng.buildCalled) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/incident_test.go` around lines 1 - 265, Add a new unit test named TestProcessIncidentEvent_RelaunchAfterTerminal that mirrors the existing ProcessIncidentEvent idempotency tests: create cfg via incidentTestConfig(), test logger, fake k8s client, a recordingEngine and mockJobBuilder, construct the Reconciler via NewReconciler(..., WithEngine(eng), WithJobBuilder(jb), WithK8sClient(k8s), WithNamespace("test-ns")), call r.ProcessIncidentEvent(ctx, evt) for evt := incidentTestEvent("01HZTERMINAL"), fetch the TaskRun with r.GetTaskRun("01HZTERMINAL:"+webhook.EventIncidentCreatedV2), transition it to a terminal state using tr.Transition(taskrun.StateSucceeded), then call r.ProcessIncidentEvent(ctx, evt) again and assert that two k8s jobs exist and eng.buildCalled == 2; ensure assertions follow the style of other tests (require.NoError/require.True/assert.Len/assert.Equal).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/incident.go`:
- Around line 78-94: ProcessIncidentEvent has a TOCTOU race: the read lock is
released after checking r.taskRuns[idempotencyKey] and a concurrent caller can
insert the same key before the write-path inserts, causing duplicate TaskRuns;
fix by performing an atomic check-and-insert while holding the write lock (or
use a sync.Map with LoadOrStore) — i.e., when computing idempotencyKey in
ProcessIncidentEvent, replace the separate RUnlock/read and later insert
sequence with a single critical section that either (a) upgrades to or acquires
the write lock and checks r.taskRuns[idempotencyKey] and inserts a placeholder
TaskRun (e.g., Queued) before releasing the lock, or (b) swap r.taskRuns to a
sync.Map and call LoadOrStore(idempotencyKey, placeholder) to ensure only one
creation proceeds.
---
Nitpick comments:
In `@internal/controller/incident_test.go`:
- Around line 1-265: Add a new unit test named
TestProcessIncidentEvent_RelaunchAfterTerminal that mirrors the existing
ProcessIncidentEvent idempotency tests: create cfg via incidentTestConfig(),
test logger, fake k8s client, a recordingEngine and mockJobBuilder, construct
the Reconciler via NewReconciler(..., WithEngine(eng), WithJobBuilder(jb),
WithK8sClient(k8s), WithNamespace("test-ns")), call r.ProcessIncidentEvent(ctx,
evt) for evt := incidentTestEvent("01HZTERMINAL"), fetch the TaskRun with
r.GetTaskRun("01HZTERMINAL:"+webhook.EventIncidentCreatedV2), transition it to a
terminal state using tr.Transition(taskrun.StateSucceeded), then call
r.ProcessIncidentEvent(ctx, evt) again and assert that two k8s jobs exist and
eng.buildCalled == 2; ensure assertions follow the style of other tests
(require.NoError/require.True/assert.Len/assert.Equal).
In `@internal/controller/incident.go`:
- Around line 194-196: The current conditional only starts streaming for a
hardcoded engine name (engineName == defaultIncidentEngine), which prevents
streaming-capable engines added later from using startStreamReader; change the
check to use an engine capability flag or helper (e.g., replace the equality
check with a capability query like engineSupportsStreaming(engineName) or
consult IncidentTriage.Engine's configured capability) and call
r.startStreamReader(ctx, tr) when that capability is true; update or add the
helper (engineSupportsStreaming) to reference the engine registry/config so
future engines that support streaming will work without modifying this code.
- Around line 139-144: The code currently logs failures from
r.taskRunStore.Save(ctx, tr) but proceeds to create the K8s Job; if persistent
store writes are required for recovery, change this to abort job creation by
returning the error (or a wrapped error) after logging so the caller knows the
save failed; specifically, in the function containing r.taskRunStore.Save,
replace the current log-only branch with r.logger.ErrorContext(...) followed by
returning the error (or fmt.Errorf("save task run: %w", err)) so that the
downstream Job creation code is not executed when r.taskRunStore.Save fails.
🪄 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: CHILL
Plan: Pro
Run ID: 33365c2f-2663-40c6-af35-bb726417ca28
📒 Files selected for processing (4)
CHANGELOG.mddocs/api/webhooks.mdinternal/controller/incident.gointernal/controller/incident_test.go
💤 Files with no reviewable changes (1)
- docs/api/webhooks.md
ProcessIncidentEvent's idempotency map check only no-ops when the existing TaskRun is non-terminal — once a prior run reaches Succeeded/Failed/TimedOut, a fresh event with the same key should spawn a new TaskRun and Job. The path existed in the dispatch code restored by the previous commit but had no test coverage, raised by CodeRabbit on PR #45. The test transitions the first run Running → Succeeded, then issues a second event for the same incident_id:event_type and asserts a second Job is created. A 2ms sleep separates the calls because the TaskRun ID embeds time.Now().UnixMilli() and the derived K8s Job name would otherwise collide on the same-millisecond path. In production, two webhook deliveries colliding inside a millisecond is implausible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
⚙️ Automated reply from Claude Code (running locally as the PR author's assistant). The author has reviewed and approved this response before posting. Addressing the three nitpicks from the review: Relaunch-after-terminal test ( Stream-reader hardcoded to
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/incident_test.go (1)
300-305: ⚖️ Poor tradeoffReplace
time.Sleepwith deterministic ID generation or mocked time.The test relies on wall-clock timing to avoid TaskRun ID collision (line 301: "embeds
time.Now().UnixMilli()"). This introduces non-determinism and potential flakiness. Tests should be fully deterministic.Consider injecting a clock interface into the reconciler or introducing a sequence counter for test TaskRun IDs to eliminate the timing dependency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/incident_test.go` around lines 300 - 305, Replace the non-deterministic time.Sleep(2 * time.Millisecond) used to avoid TaskRun ID collisions by making TaskRun IDs deterministic in tests: either inject a clock interface (replace calls to time.Now().UnixMilli() used in TaskRun ID generation) into the reconciler so tests can supply a mocked clock with predictable ticks, or add a simple test-only sequence counter / ID provider that the test can set when constructing TaskRun IDs; update the test to use the mock clock or sequence provider instead of sleeping so the derived K8s Job name no longer depends on real time and collisions are avoided deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/incident_test.go`:
- Around line 79-315: Refactor the seven standalone tests that all call
Reconciler.ProcessIncidentEvent into one or more table-driven tests using t.Run
subtests: extract the common setup (incidentTestConfig, testLogger,
fake.NewSimpleClientset, recordingEngine/mockJobBuilder, NewReconciler, ctx,
incidentTestEvent) and create a table of cases (e.g. "success-created",
"idempotency-duplicate", "distinct-event-types", "default-engine",
"engine-not-registered", "build-spec-error", "relaunch-after-terminal") where
each case supplies inputs and expected outcomes; inside each t.Run initialize or
reuse the shared setup as appropriate, call r.ProcessIncidentEvent, and perform
the case-specific assertions (checking task run in r.GetTaskRun, job counts from
k8s.BatchV1().Jobs(...).List, eng.buildCalled, error messages, and state
transitions like tr.Transition), preserving the original expectations and unique
behaviors for each scenario (use identifiers like ProcessIncidentEvent,
GetTaskRun, recordingEngine, mockJobBuilder, incidentTestEvent,
webhook.EventIncidentCreatedV2/EventIncidentStatusUpdatedV2 to locate code to
change).
---
Nitpick comments:
In `@internal/controller/incident_test.go`:
- Around line 300-305: Replace the non-deterministic time.Sleep(2 *
time.Millisecond) used to avoid TaskRun ID collisions by making TaskRun IDs
deterministic in tests: either inject a clock interface (replace calls to
time.Now().UnixMilli() used in TaskRun ID generation) into the reconciler so
tests can supply a mocked clock with predictable ticks, or add a simple
test-only sequence counter / ID provider that the test can set when constructing
TaskRun IDs; update the test to use the mock clock or sequence provider instead
of sleeping so the derived K8s Job name no longer depends on real time and
collisions are avoided deterministically.
🪄 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: CHILL
Plan: Pro
Run ID: b75aa533-6e71-4427-9b9a-d3308ee6c153
📒 Files selected for processing (1)
internal/controller/incident_test.go
PR #44 (Step 1a) shipped the receiver side of the incident.io webhook flow — endpoint, signature verification, parsing, server wiring, config schema, controller adapter — but deliberately omitted the dispatch body of Reconciler.ProcessIncidentEvent. That deferral was paired with operator-side wiring (a classifier skill + incident_triage.append_system_prompt in helm values), which is now in place separately. This change restores dispatch. Reconciler.ProcessIncidentEvent now: - Keys idempotency on incident_id:event_type so created and status_updated for the same incident produce distinct task runs; duplicate events while the existing run is non-terminal are no-ops. - Selects the engine from config.IncidentTriage.Engine (default claude-code) and returns an error if unregistered. - Builds an engine.Task with the incident name, summary, and permalink, but no RepoURL — so the engine's BuildPrompt no-repo fallback fires and the agent operates from system prompt + skill. - Carries osmia:source:incident-io and osmia:event:<type> labels for downstream observability. - Generates a TaskRun ID of the form tr-incident-<lowercased-id>-<created|updated>-<millis> that stays inside the K8s 63-char limit even with a 26-char ULID. - Layers config.IncidentTriage.AppendSystemPrompt onto the per-call EngineConfig via the override pattern used in the deleted dispatch code. - Drives the standard BuildExecutionSpec -> JobBuilder.Build -> Jobs().Create() path, transitions the TaskRun to Running, and starts the stream reader for claude-code runs. Tests: brought back TestProcessIncidentEvent, _Idempotency, _DistinctEventTypes, _DefaultsEngineToClaudeCode, _EngineNotRegistered, _BuildExecutionSpecError, and the recordingEngine mock that captures AppendSystemPrompt for the per-call override assertion. Removed TestProcessIncidentEvent_DispatchDisabled, which was a regression guard against accidentally returning to the no-op state. Docs: removed the "Dispatch is currently disabled" admonition from docs/api/webhooks.md and replaced the corresponding CHANGELOG entry with a description of the active dispatch behaviour. Known limitation, deferred to the use-case abstraction follow-up: handleJobComplete calls ticketing.MarkComplete with the incident UUID as the TicketID, which the configured ticketing backend will not recognise. The resulting "not found" error is logged but non-fatal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ProcessIncidentEvent's idempotency map check only no-ops when the existing TaskRun is non-terminal — once a prior run reaches Succeeded/Failed/TimedOut, a fresh event with the same key should spawn a new TaskRun and Job. The path existed in the dispatch code restored by the previous commit but had no test coverage, raised by CodeRabbit on PR #45. The test transitions the first run Running → Succeeded, then issues a second event for the same incident_id:event_type and asserts a second Job is created. A 2ms sleep separates the calls because the TaskRun ID embeds time.Now().UnixMilli() and the derived K8s Job name would otherwise collide on the same-millisecond path. In production, two webhook deliveries colliding inside a millisecond is implausible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f4de946 to
525e48f
Compare
The incident.io section of docs/api/webhooks.md showed an example incident_triage block that referenced /incident-classifier but never told operators that they have to author the skill themselves. The example skill name and the matching engines.<engine>.skills[] entry were also in separate docs (webhook config in webhooks.md, skill mechanics in plugins/engines.md), forcing operators to derive the wiring from first principles. Adds an "Authoring the classifier skill" subsection that: - States explicitly that Osmia does not ship a classifier — operators provide one to suit their incident taxonomy and response surface. - Shows a complete worked example pairing the incident_triage block with a matching ConfigMap-backed engines.<engine>.skills[] entry, plus the kubectl create configmap invocation. - Lays out the typical structure of a classifier skill (input contract, classifications, output contract, bail-out, operational constraints) without prescribing categories — that's deployment-specific. - Notes how to iterate on the skill without redeploying the controller (update the ConfigMap; next agent Job picks it up). - Cross-references the upstream incident.io API docs for operators who need to derive additional fields from the event payload. No code changes; this is purely operator-facing documentation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/webhooks.md`:
- Around line 487-488: The snippet currently contains a literal ellipsis
(`kubectl create configmap … -o yaml --dry-run=client | kubectl apply -f -`)
which is not runnable; update that fragment in docs/api/webhooks.md to a
concrete, executable example by replacing `…` with a clear placeholder (e.g.,
`<CONFIGMAP_NAME>` and `<FILE_OR_DIR>`) or a concrete name, and show the full
command form such as `kubectl create configmap <CONFIGMAP_NAME>
--from-file=<FILE_OR_DIR> -o yaml --dry-run=client | kubectl apply -f -`; ensure
placeholders are angle-bracketed and documented inline so readers can copy/paste
and run the command.
🪄 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: CHILL
Plan: Pro
Run ID: 900cc8db-83bd-4d31-bb35-94e66cf18e80
📒 Files selected for processing (2)
CHANGELOG.mddocs/api/webhooks.md
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
The "Iterating on the skill" subsection used a literal Unicode ellipsis in the inline kubectl invocation as a placeholder. The ellipsis is a real character, not a shell idiom, so anyone copy-pasting the snippet would get a syntax error. Raised by CodeRabbit on PR #45. Replaces the inline backticked one-liner with a proper bash fenced block containing the full create-then-apply pattern, including the namespace and the --from-file= argument. The earlier "Authoring the classifier skill" section already shows the plain `kubectl create configmap` form for first-time creation; this section shows the idempotent re-apply form for subsequent updates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Wires the controller's
Reconciler.ProcessIncidentEventto dispatch an agent run for each incident.io webhook event. The receiver-side plumbing forPOST /webhooks/incident-ioalready delivers signed, parsed events to this entry point; this PR makes the entry point actually launch a K8s Job and TaskRun rather than logging-and-returning.Also:
recordingEnginemock that captures the per-callEngineConfigso theAppendSystemPromptoverride is asserted.docs/api/webhooks.mdand rewrites the corresponding CHANGELOG entry to describe active dispatch.What dispatch now does
For each incident.io webhook event reaching
ProcessIncidentEvent:incident_id:event_typesoincident_created_v2andincident_status_updated_v2for the same incident produce distinct task runs; duplicate events while the existing run is non-terminal are no-ops. Once the prior run reaches a terminal state, a fresh delivery with the same key launches a new run.config.IncidentTriage.Engine(defaultclaude-code); returns an error if the engine isn't registered.engine.Taskcarrying the incident name, summary, and permalink — but noRepoURL, so the agent operates from system prompt + skill rather than a repository / merge-request lifecycle. Labelsosmia:source:incident-ioandosmia:event:<type>for downstream observability.tr-incident-<lowercased-id>-<created|updated>-<millis>— stays inside the K8s 63-char label/name limit even with a 26-char ULID.AppendSystemPromptoverride fromIncidentTriage.AppendSystemPromptlayered ontobaseEngineConfig. This is the surface operators use to direct the agent at a specific skill.BuildExecutionSpec→JobBuilder.Build→Jobs().Create(); transitions the TaskRun to Running, persists, and starts the stream reader forclaude-coderuns.Known limitation
When one of these incident-flow TaskRuns completes,
handleJobCompletecallsticketing.MarkCompletewith the incident UUID as theTicketID. The configured ticketing backend won't recognise it; the resulting "not found" is logged but non-fatal. The clean fix is a future use-case abstraction that dispatches completion to handlers aware of the originating flow.Test plan
go build ./...clean.go test ./internal/controller/... -run TestProcessIncidentEvent -v— all 7 dispatch tests pass.go test ./...— full suite green, no regressions.gofmt -lclean on the touched files;go vet ./internal/controller/...clean.incident_created_v2(e.g. via incident.io's "Send test webhook" button) and confirm a Job appears and the configured dispatch surface receives a post.Summary by CodeRabbit
New Features
Documentation
Tests