Hardening: bounded dedup maps, fetch timeouts, buildTaskRequest helper #35
Labels
No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
No milestone
No project
No assignees
1 participant
dev
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
charles/claude-hooks#35
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
User story
As a maintainer, I want three small correctness/readability fixes batched into one PR — bounded in-memory maps, timeouts on every Forgejo fetch, and a single
buildTaskRequesthelper — so that the webhook stops leaking memory under high event volume, stops blocking for 30 s on Forgejo hangs, and stops duplicating the same 12-field object six times.Context
Audit 2026-04-18 surfaced three pre-existing issues in
src/webhook.ts:_fixCiDispatched(line ~460) and_reviewFallbacks(line ~470) grow indefinitely under high event volume;_fixCiDispatchedonly prunes after hitting a hard 200-entry cap.fetch()calls withoutAbortSignal.timeout.repoHasWorkflowsand three insiderequestReviewIfFresh. A Forgejo hang blocks the webhook handler until OS TCP timeout (~30 s).TaskRequestobject literals across event handlers — 12 fields × 6 occurrences = ~72 lines of boilerplate.Acceptance criteria
BoundedMap utility
BoundedMap<K, V>(maxSize, ttlMs?)class (can live insrc/util/bounded-map.tsor inline inwebhook-ci.tsif #A has already landed). Onset, if size exceedsmaxSize, evict the oldest entry. IfttlMsis provided, also evict entries older thanttlMson write._fixCiDispatched(plainMap) withBoundedMap<string, number>(500, 3_600_000)— cap matches the 1 h dedup window plus headroom._reviewFallbacks(plainMap) with eitherBoundedMapor keep as-is if the existing arm-cancels-prior-timer logic is already sufficient. Document the choice in a comment.BoundedMapevicts on size and on TTL; does not mis-evict entries that are still within TTL.Fetch timeouts
signal: AbortSignal.timeout(5_000)to everyfetch()call insrc/webhook.ts(or the split files if #A landed first). If #A landed and introducedsrc/forgejo-api.ts, add the timeout there exactly once and verify all callers go through it.curl https://httpbin.org/delay/10takes ≥ 10 s; confirm our handler bails at 5 s with a clear log line, not a hang.buildTaskRequest helper
buildTaskRequest(agent: Agent, repo: string, issueOrPr: number, task: string, overrides?: Partial<TaskRequest>): TaskRequestinto eithersrc/webhook-handlers.ts(if #A landed) orsrc/webhook.ts. It must:forgejo_token,forgejo_mcp_command,forgejo_url,callback_url,role,git_name,git_email,branch_prefix,system_prompt,model,containerfrom the config for the given agent.stateless_session(review) and other per-event fields.Tests
BoundedMaphas dedicated tests (size eviction, TTL eviction, get/set/has semantics).BoundedMaptests.Out of scope
webhook.tsstructural split (see #A). This issue lives in whatever the current file layout is — if #A lands first, do the work in the new files; otherwise do it inwebhook.ts.References
Dependencies
#A,#C,#D.main