feat(server): answer Forgejo comments via webhook-triggered sessions #8
No reviewers
Labels
No labels
Agent
Ignore
Agent
In Progress
Agent
Planned
Agent
Planning
Agent
Ready
Agent
Triage
Compat
Breaking
Kind
Bug
Kind
Documentation
Kind
Enhancement
Kind
Feature
Kind
Security
Kind
Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
lucas/omp-deck!8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/forgejo-comment-driven"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Forgejo comments can now start agent sessions. A signed webhook at
/api/forgejo/webhookqueues issue and PR conversation comments. A dispatcher works through the queue: one trigger at a time per repository, different repositories in parallel. It answers comments that @mention the agent or reply to one of its comments, but only from authors with write access. Each accepted comment starts a new unattended session in the workspace whose gitoriginis that repository. These sessions get a fixed tool set: git read tools, Forgejo read tools, and comments on the triggering issue or PR only. They never prompt for approval and can't enter plan or idea mode, and that policy is stored so it still applies after a restart. Settings → Forgejo gains a Webhook section (payload URL, generated secret, setup steps) and a list of recent deliveries. When something goes wrong, the agent posts a Forgejo comment; if that fails, it creates an inbox ticket or a notification instead.Decisions
rowidbreaks ties). The generated ids only sort by time to the millisecond, and a test showed the order flipping.agent_endthe SDK marks as non-terminal (isTerminal: false, meaning queued work follows). It also re-checks one tick later whether the session is still streaming.ForgejoDispatcher.whenIdle()was added so the tests can wait on real signals instead of polling with timers.toolNameslist isn't filtered against the tools that are actually registered. The SDK already drops names it doesn't know (sdk.ts~4009).Verification
bun run typecheck: protocol, server and web pass.cd apps/server && bun test: 689 pass, 0 fail. New tests cover:onAcceptedfires;cd apps/web && bun test: 229 pass.The design holds up well overall. Restricting the tool surface when the session is created, rather than in the permission filter, is the right call. Resume failing closed when the trigger row can't be read, and the per-repo queue, are both solid. The inline comments cover one gap in the policy boundary (the repository is never checked, only the number), a dedupe question, a misclassification on older Forgejo versions, and a few operational points. Nits are marked as such.
@ -0,0 +76,4 @@decide(toolName, params, scope) {if (Object.hasOwn(FORGEJO_COMMENT_TOOLS, toolName)) {// Both tools are accepted for issues and PRs alike: PR comments use issue routes.if ((params as { number?: unknown } | null)?.number === scope.number) return "allow";The policy compares only
numberand ignoresscope.owner/scope.repo. The comment tools post to whichever repositorybuildForgejoTools(cwd)resolves from the checkout's currentorigin, both when the session is created and again on every resume. The workspace index, on the other hand, caches the origin and never re-checks it (seeworkspace-index.ts). If a checkout'soriginchanges, the dispatcher can match a workspace fora/xwhile the session's tools point atb/y, and#Nthen gets a comment in the wrong repository. The same can happen when an old trigger session is resumed after its origin was changed.Suggestion: in
createSession/resumeSession, when a trigger is set, compare the repositorybuildForgejoToolsresolved againsttrigger.scopeand refuse the session (or skip the Forgejo tools) if they differ.Fixed in
d3cb733.buildForgejoToolsnow also returns the repository it resolved. When a trigger is set,createSessionandresumeSessioncallassertTriggerRepoand refuse the session if that repository isn'tscope.owner/repo(case-insensitive) or the checkout has no Forgejo tools at all. A resumed session whose origin has drifted therefore fails closed as well. The dispatcher records the refusal as a failed trigger.@ -0,0 +159,4 @@const longestRun = Math.max(0, ...(body.match(/`+/g) ?? []).map((r) => r.length));const fence = "`".repeat(Math.max(3, longestRun + 1));return [`Forgejo comment on ${t.repo} ${t.isPull ? "PR" : "issue"} #${t.number}: "${issue?.title ?? "?"}"`,Nit: the comment body is fenced, but the issue title goes into the seed unfenced, and the issue author doesn't need write access. The system context marks comment text as untrusted, but not titles. Consider marking the title as untrusted too.
Done in
d3cb733. The untrusted-input rule in the orientation now covers comment text, issue and PR titles, and anything else written by Forgejo users.@ -0,0 +215,4 @@buildConversationSeed({ repo: row.repo, number: row.number, isPull: row.isPull, reason, payload }),);if (outcome === "ok") {finishForgejoTrigger(row.id, "done", reason);The session stays live after its turn. The bridge never disposes idle sessions, so every accepted comment leaves a full SDK session in memory until the server restarts. Since there's no user on the other end, consider
handle.dispose()(non-destructive: the JSONL stays and "Open session" can still resume it) once the turn has settled, on both the success and failure paths.Done in
69d517e. The dispatcher callshandle.dispose()once the turn has settled, on success, failure and timeout alike. This is non-destructive: the JSONL stays, and opening the session from Recent deliveries resumes it.@ -0,0 +308,4 @@if (api) {const [owner = "", repo = ""] = row.repo.split("/");const body =`omp-deck failed to handle this comment: ${reason}.` +reasongoes into a public Forgejo comment as is. On theprocess()catch path, that iserr.messagefrom any exception (bridge, SDK, DB), which can include local paths or other internal details. On public repositories this leaks information. Suggestion: post a generic message plus the deck session id or trigger id, and keep the detailed reason in the row and the server log, where the deliveries list already shows it.Fixed in
69d517e. The public comment now carries only a fixed label per failure kind (restart, timeout, agent turn failed, Forgejo API error, internal error) plus the trigger and session ids. The detailed reason stays in the row (Recent deliveries), the server log and the fallback inbox ticket. The test asserts that the provider error text doesn't appear in the comment.@ -0,0 +34,4 @@* `issue_comment` still classifies as an issue comment.*/export function resolveEventType(eventTypeHeader: string | undefined, eventHeader: string | undefined): string {return eventTypeHeader ?? eventHeader ?? "";In the fallback for Forgejo versions that send no
X-Forgejo-Event-Type, the module doc says inline review comments arrive withX-Forgejo-Event: pull_request_comment, while conversation comments arrive asissue_comment. So in the fallback,pull_request_commentmeans an inline review comment, butclassifyForgejoDeliveryaccepts it as a conversation comment. The fallback should mappull_request_commentto "ignore" (or only fall back forissue_comment). A test for this would help.Right, fixed in
69d517e. WithoutX-Forgejo-Event-Type,resolveEventTypenow maps an Event ofpull_request_commenttopull_request_review_comment, so it is ignored.issue_commentstill falls back as before. Tests cover both fallback cases.@ -0,0 +55,4 @@/** Index a newly registered workspace. No-op until the index has been built. */export function noteWorkspaceAdded(ws: Workspace): void {if (!index) return;Two races, either of which leaves a newly registered workspace unmatched until restart (and posts "not connected" in the meantime):
listWorkspaces()snapshot may already be out of date.repoKeylookup isn't awaited, so a trigger that arrives right after registration misses the workspace.Possible fix: on a miss, check the DB workspaces that aren't in the index before returning nothing.
Both races fixed in
fbb891e. Every lookup now first resolves the registered workspaces the index has no entry for yet, then matches. That covers registrations during the first build and lookups that arrive beforenoteWorkspaceAddedhas resolved. Workspaces that are already indexed are still not re-verified, which is the documented origin-drift gap, now backed by the repo check from 16001. Test added.@ -0,0 +8,4 @@* neither the body nor the headers are ever logged.** Accepted deliveries become `pending` rows in `forgejo_triggers`, deduped by* `X-Forgejo-Delivery` so Forgejo's manual redeliver is safe.Two concerns with deduplicating by
X-Forgejo-Delivery:Deduplicating on the signed content (for example
comment.idplusactionplus a hash of the body, orcomment.updated_at) would cover both cases.Confirmed: Forgejo's
ReplayHookTaskcopies the payload into a new hook task, andCreateHookTaskassigns a fresh UUID (models/webhook/hooktask.go). The old comment was therefore wrong.Fixed in
69d517e. Rows are now deduplicated ondedupe_key, a SHA-256 over signed body fields: repo,comment.id,action,comment.updated_atand the body.updated_atkeeps successive edits that restore an earlier body apart.delivery_idis still stored, for display only. New tests cover a replay with a forged delivery id (duplicate) and a later edit of the same comment (new row). Migration 010 was changed in place because it hasn't been merged yet. Anyone who already ran this branch needs to dropforgejo_triggersand itsschema_migrationsrow.@ -0,0 +34,4 @@const cfg = readForgejoConfig();if (!secret || !cfg) return c.json({ error: "forgejo webhook not configured" }, 503);const body = new Uint8Array(await c.req.arrayBuffer());Nit: the whole body is buffered before the signature is checked, and Bun's default body limit is 128 MB. Since this is the one route meant to be reachable from outside, a small explicit limit (comment payloads are a few KB) would be cheap insurance.
Added in
69d517e: Hono'sbodyLimitat 1 MiB on the route, which answers 413 before the body is buffered for the signature check. Test added.@ -193,0 +196,4 @@1. In Settings → Forgejo → **Webhook**, click **Generate secret** (stored as `forgejo.webhook_secret`; **Regenerate** replaces it and invalidates the old one).2. In Forgejo's repository, organization or system webhook settings, add a **Forgejo** webhook: target URL = the payload URL shown in the panel (`<deck origin>/api/forgejo/webhook`), method POST, content type `application/json`, secret = the generated secret, trigger on custom events **Issue comments** and **Pull request comments**.3. Forgejo must be able to reach that URL. `/api/forgejo/webhook` is the one deck route meant for an external caller; exposing it (and exempting it from any front-end auth) is up to your reverse proxy. Its only authentication is the HMAC-SHA256 signature over the body, checked against the secret.I think this should be stated plainly: the deck's
/apihas no authentication of its own, and it is only safe because the server binds to loopback by default. The reverse proxy must expose onlyPOST /api/forgejo/webhook. Proxying the whole origin hands the full API (sessions, settings, tokens, the webhook secret) to anyone who can reach it.Agreed, rewritten in
fafd5d9. The doc now says plainly that/apihas no authentication of its own and is only safe because the server binds to loopback by default. The proxy must expose onlyPOST /api/forgejo/webhook, and proxying the whole origin exposes the full API, including the token and the webhook secret. The changelog entry mentions this too.@ -0,0 +88,4 @@"",`You were started by a comment on ${isPull ? "pull request" : "issue"} #${number}. Nobody is watching this session: your chat output is never read. The only way to answer is a Forgejo comment.`,"- Read the thread first (`forgejo_list_issue_comments`, or `forgejo_list_pr_comments_and_reviews` for a pull request).",`- Reply only with \`forgejo_create_issue_comment\` or \`forgejo_create_pr_comment\` on #${number}.`,This is missing the third tool, that's used to reply to review threads.
Added
forgejo_create_pr_thread_commentto the conversation policy ind3cb733. It's limited to the triggering PR like the other two comment tools, and the tool itself checks thatcomment_idis an inline comment of that PR. The orientation now names it for answering inline review comments.