Simon/feat/ci one message per run

[SimonThormeyer]

Just one message per workflow run

[claude]

PR Review: One message per workflow run

Nice change — collapsing the per-event CI spam into a single message that gets edited in place is a clear UX win, and removing the now-redundant check_suite/status handling keeps the surface small. The tests cover both the "first message" and "edit existing message" paths, which is great. A few things worth considering:

Potential bugs / correctness

• Unbounded Redis growth (storage leak). rememberWorkflowRunMessageId writes …:workflow_run:<id>:<conversation> keys (plus an entry in the workflow_run_messages set) with no TTL. After a run completes it is never edited again, but its key lives forever — for an active repo this set grows by one entry per workflow run indefinitely, and entries are only purged when the whole repo goes inactive (removeRepositoryWebhook). Consider setting an expiry on these keys (e.g. SetArgs.Builder.ex(...) a few hours past run completion), since the edit window is short-lived. This is the main concern.

• Race on concurrent deliveries. GitHub fires requested → in_progress → completed for the same run, and Ktor serves requests concurrently. The read (workflowRunMessageId) in sendWorkflowRunMessage and the write (rememberWorkflowRunMessageId) back in the route are separate, non-atomic operations. Two near-simultaneous events for the same run can both read null, both send a fresh Text, and you end up with two messages — defeating the "one message per run" goal. If that ordering is plausible in practice, consider a SETNX/atomic guard, or at least note the risk.

• TextEdited id semantics — worth double-checking. On edit, the code stores the UUID returned by sendMessage(TextEdited) and uses that as replacingMessageId for the next edit (the test asserts the newly stored id). If the Wire SDK expects every edit to reference the original message id rather than the previous edit's id, the 3rd+ update (running → passed → …) would break. Worth confirming against the SDK edit semantics, since a run transitions through several states.

Code quality

• Duplicated condition in Routing.kt. if (event == EVENT_WORKFLOW_RUN && workflowRunId != null) is evaluated twice (lines 91 and 107) around a single forEach body. These can be merged into one branch so the sendWorkflowRunMessage + rememberWorkflowRunMessageId calls live together; the else branch's messageId is then unused and can be dropped. Reads cleaner and removes the implicit coupling between the two checks.

• EVENT_WORKFLOW_RUN is now defined in three places (Routing.kt, TemplateHandler.kt, and the test). Worth hoisting to a single shared constant.

• Minor (test): io.mockk.slot<WireMessage>() is fully qualified inline — an import would match the style of the surrounding every/verify/justRun usage.

Observation (not introduced here)

repositoryEvents() dropping check_suite/status only affects newly provisioned webhooks — ensureWebhookForConversation returns early when a webhook id already exists, so repos subscribed before this change keep emitting those events (which now have no template and are silently ignored). Probably fine for the hackathon scope, just flagging.

Overall this looks solid for the stated goal; the storage-leak/TTL point and the TextEdited id semantics are the two I would want resolved before relying on it long-term.