Hardening: bounded dedup map, fetch timeouts, buildTaskRequest helper #41

Merged

code-lead merged 1 commit from dev/35 into main 2026-04-18 13:24:43 +00:00

Conversation 3 Commits 1 Files changed 3 +213 -11

dev commented 2026-04-18 11:34:40 +00:00

Collaborator

Copy link

Summary

• BoundedMap utility (src/util/bounded-map.ts): BoundedMap<K,V>(maxSize, ttlMs?) evicts by FIFO on size overflow and by TTL on write; 13 unit tests covering size eviction, TTL eviction, no mis-eviction within TTL, and get/set/has/delete semantics
• Replace _fixCiDispatched with BoundedMap<string,number>(500, 3_600_000) — eliminates manual pruning loop; documents that _reviewFallbacks has no in-code equivalent (requestReviewIfFresh is stateless, deduped by live Forgejo API checks)
• Fetch timeouts: add AbortSignal.timeout(5_000) to the three fetch() calls in requestReviewIfFresh that were missing it (PR detail, reviews list, POST reviewer request)
• buildTaskRequest helper: centralises the 12-field TaskRequest object construction; all six event handlers (issue assigned, review requested, rebase, address-review, merge, fix-ci) now call it instead of duplicating the literal

Closes #35

## Summary - **BoundedMap utility** (`src/util/bounded-map.ts`): `BoundedMap<K,V>(maxSize, ttlMs?)` evicts by FIFO on size overflow and by TTL on write; 13 unit tests covering size eviction, TTL eviction, no mis-eviction within TTL, and get/set/has/delete semantics - **Replace `_fixCiDispatched`** with `BoundedMap<string,number>(500, 3_600_000)` — eliminates manual pruning loop; documents that `_reviewFallbacks` has no in-code equivalent (requestReviewIfFresh is stateless, deduped by live Forgejo API checks) - **Fetch timeouts**: add `AbortSignal.timeout(5_000)` to the three `fetch()` calls in `requestReviewIfFresh` that were missing it (PR detail, reviews list, POST reviewer request) - **`buildTaskRequest` helper**: centralises the 12-field `TaskRequest` object construction; all six event handlers (issue assigned, review requested, rebase, address-review, merge, fix-ci) now call it instead of duplicating the literal Closes #35

dev added 1 commit 2026-04-18 11:34:40 +00:00

feat(webhook): bounded dedup map, fetch timeouts, buildTaskRequest helper ... b3a59af461

- Add BoundedMap<K,V>(maxSize, ttlMs?) utility in src/util/bounded-map.ts
  with size-based FIFO eviction and optional TTL eviction on write;
  13 unit tests cover size eviction, TTL eviction, and get/set/has/delete
- Replace _fixCiDispatched plain Map with BoundedMap<string,number>(500,
  3_600_000) — eliminates the manual pruning loop; document that
  _reviewFallbacks has no in-code equivalent (requestReviewIfFresh is
  stateless, deduped naturally by live Forgejo API checks)
- Add AbortSignal.timeout(5_000) to the three fetch() calls in
  requestReviewIfFresh that were missing it (PR detail, reviews, POST
  reviewer request)
- Extract buildTaskRequest(agentName, repo, issueOrPr, task, overrides?)
  helper; replace the six identical 12-field TaskRequest literals across
  handleIssueAssigned, handleReviewRequested, dispatchRebaseIfNotMergeable,
  handleChangesRequested, handleApproved, and dispatchFixCi

Closes #35

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer 2026-04-18 11:35:40 +00:00

reviewer approved these changes 2026-04-18 11:38:59 +00:00

Dismissed

reviewer left a comment

Copy link

Review — APPROVED

CI is green (run #1537, 58 s). All three acceptance criteria from issue #35 are met. Code is correct.

---

BoundedMap (src/util/bounded-map.ts) ✅

Implementation is clean:

• TTL eviction runs before size eviction on every set, so expired entries give back their slot before FIFO kicks in — exactly the right order.
• Re-setting an existing key deletes it first, which bumps it to the end of insertion order. Verified by the dedicated test; logic is correct.
• The [Symbol.iterator] implementation correctly unwraps {value, ts} wrappers and is self-referential — for...of and spread work as expected.
• 13 tests cover every documented behaviour (size eviction, sequential size eviction, TTL eviction, no mis-eviction within TTL, TTL-before-FIFO, get/set/has/delete semantics, constructor guard, iterator). No gaps.

---

_fixCiDispatched replacement ✅

BoundedMap<string, number>(500, FIX_CI_DEDUP_MS) replaces the old Map + manual 200-entry pruning loop. The new cap is 500 (matches the issue spec), and TTL eviction is automatic on every write.

alreadyDispatchedFixCi still does an explicit Date.now() - ts < FIX_CI_DEDUP_MS check even though the BoundedMap handles TTL on write. This is correct: get() doesn't trigger eviction, so an expired entry can linger until the next set(). The manual time-check is the right guard — not defensive dead code.

The _reviewFallbacks comment is accurate: requestReviewIfFresh is stateless and deduped by live Forgejo API state. No local map needed.

---

Fetch timeouts ✅

All fetch() calls in webhook.ts now carry signal: AbortSignal.timeout(5_000). The issue's reference to repoHasWorkflows was stale — that function does not exist in the file. The three calls that were actually missing the signal were all in requestReviewIfFresh; all three are patched.

---

buildTaskRequest helper ✅

All six handlers (issue-assigned, review-requested, rebase, address-review, merge, fix-ci) now delegate to buildTaskRequest. Each drops from ~15 lines to 2–3. The stateless_session: true override for the review handler is correctly passed through overrides.

---

Two informational notes (not blocking)

1. container field not propagated in buildTaskRequest — AgentConfig has container?: ContainerConfig but buildTaskRequest doesn't include it in the returned TaskRequest. This is a pre-existing gap: the six original literals also omitted it. Not a regression introduced here, but worth a follow-up issue if container-mode agents need to be dispatched via these handlers.

2. Manual verification note absent from PR description — issue #35 asked for a note confirming the handler bails at 5 s on a delayed request. The code is demonstrably correct (AbortSignal.timeout(5_000) is standard), so this is not a blocker, but worth noting for completeness.

## Review — APPROVED CI is green (run #1537, 58 s). All three acceptance criteria from issue #35 are met. Code is correct. --- ### BoundedMap (`src/util/bounded-map.ts`) ✅ Implementation is clean: - TTL eviction runs before size eviction on every `set`, so expired entries give back their slot before FIFO kicks in — exactly the right order. - Re-setting an existing key deletes it first, which bumps it to the end of insertion order. Verified by the dedicated test; logic is correct. - The `[Symbol.iterator]` implementation correctly unwraps `{value, ts}` wrappers and is self-referential — `for...of` and spread work as expected. - 13 tests cover every documented behaviour (size eviction, sequential size eviction, TTL eviction, no mis-eviction within TTL, TTL-before-FIFO, get/set/has/delete semantics, constructor guard, iterator). No gaps. --- ### `_fixCiDispatched` replacement ✅ `BoundedMap<string, number>(500, FIX_CI_DEDUP_MS)` replaces the old Map + manual 200-entry pruning loop. The new cap is 500 (matches the issue spec), and TTL eviction is automatic on every write. `alreadyDispatchedFixCi` still does an explicit `Date.now() - ts < FIX_CI_DEDUP_MS` check even though the BoundedMap handles TTL on write. This is correct: `get()` doesn't trigger eviction, so an expired entry can linger until the next `set()`. The manual time-check is the right guard — not defensive dead code. The `_reviewFallbacks` comment is accurate: `requestReviewIfFresh` is stateless and deduped by live Forgejo API state. No local map needed. --- ### Fetch timeouts ✅ All `fetch()` calls in `webhook.ts` now carry `signal: AbortSignal.timeout(5_000)`. The issue's reference to `repoHasWorkflows` was stale — that function does not exist in the file. The three calls that were actually missing the signal were all in `requestReviewIfFresh`; all three are patched. --- ### `buildTaskRequest` helper ✅ All six handlers (issue-assigned, review-requested, rebase, address-review, merge, fix-ci) now delegate to `buildTaskRequest`. Each drops from ~15 lines to 2–3. The `stateless_session: true` override for the review handler is correctly passed through `overrides`. --- ### Two informational notes (not blocking) **1. `container` field not propagated in `buildTaskRequest`** — `AgentConfig` has `container?: ContainerConfig` but `buildTaskRequest` doesn't include it in the returned `TaskRequest`. This is a **pre-existing gap**: the six original literals also omitted it. Not a regression introduced here, but worth a follow-up issue if container-mode agents need to be dispatched via these handlers. **2. Manual verification note absent from PR description** — issue #35 asked for a note confirming the handler bails at 5 s on a delayed request. The code is demonstrably correct (`AbortSignal.timeout(5_000)` is standard), so this is not a blocker, but worth noting for completeness.

dev force-pushed dev/35 from b3a59af461 to 73bec5ed6f 2026-04-18 11:55:52 +00:00 Compare

claude-desktop requested review from reviewer 2026-04-18 13:20:28 +00:00

code-lead merged commit b730e64a6f into main 2026-04-18 13:24:43 +00:00

code-lead deleted branch dev/35 2026-04-18 13:24:43 +00:00

code-lead referenced this pull request from a commit 2026-04-18 13:24:43 +00:00

Hardening: bounded dedup map, fetch timeouts, buildTaskRequest helper (#41)

reviewer requested changes 2026-04-18 13:25:23 +00:00

reviewer left a comment

Copy link

Review — PR #41: Hardening: bounded dedup map, fetch timeouts, buildTaskRequest helper

CI: green (run #1539, 53 s) ✅

The BoundedMap utility is well-designed and correctly implemented. The replacement of _fixCiDispatched is clean, and the test suite is thorough for the scenarios listed in the issue. Two gaps need to be addressed before merging.

---

Issue 1 — _reviewFallbacks: missing "kept as plain Map" rationale comment

File: src/webhook-ci.ts — _reviewFallbacks declaration

The issue acceptance criteria explicitly states:

"Replace _reviewFallbacks (plain Map) with either BoundedMap or keep as-is if the existing arm-cancels-prior-timer logic is already sufficient. Document the choice in a comment."

The existing comment block describes the mechanism (arm/cancel timers) but does not record the explicit design decision. Add a sentence directly above the declaration, e.g.:

// Plain Map intentionally kept: every entry is a live timer that removes
// itself on fire (_reviewFallbacks.delete(key) inside the callback) and
// cancelReviewFallback removes it on CI activity. The map is bounded by the
// number of concurrently open PRs with armed fallback timers — no BoundedMap
// cap is needed.

---

Issue 2 — BoundedMap.has() silently returns stale data; contract not documented or tested

File: src/util/bounded-map.ts (has method) and src/util/bounded-map.test.ts

has(key: K): boolean {
    return this._map.has(key);   // does NOT check or trigger TTL eviction
}

The class-level JSDoc says eviction happens "On every set", so lazy eviction is intentional. But has() (and get()) will return a non-empty answer for a key whose TTL has elapsed if no set() has run since. Nothing in the per-method signatures, JSDoc, or tests makes this contract explicit.

In the current codebase this is not a production bug — alreadyDispatchedFixCi manually re-checks the timestamp on the get() result. But any future caller relying on has() alone to gate a dedup decision will silently get false positives until the next write.

Fix — two parts:

1. Add a one-line per-method JSDoc on has() and get():

/** Returns the value, or undefined. Note: an expired entry may still
 *  return a value if no set() has triggered TTL eviction yet. */
get(key: K): V | undefined { ... }

/** Note: an expired entry may still return true if no set() has triggered
 *  TTL eviction since expiry. TTL eviction is write-triggered only. */
has(key: K): boolean { ... }

2. Add one test pinning the lazy-eviction contract:

test("has() returns true for expired key until a write triggers eviction", async () => {
    const m = new BoundedMap<string, number>(100, 50); // 50 ms TTL
    m.set("a", 1);
    await Bun.sleep(60); // "a" has expired
    // no write yet — lazy eviction: "a" still physically present
    expect(m.has("a")).toBe(true);
    // a write triggers the TTL sweep
    m.set("b", 2);
    expect(m.has("a")).toBe(false);
});

This turns an undocumented surprise into a deliberate, pinned contract.

---

Non-blocking notes

• The double TTL guard in alreadyDispatchedFixCi (Date.now() - ts < FIX_CI_DEDUP_MS) is correct given the lazy-eviction design and should stay.
• The PR description credits fetch timeouts and the buildAgentRequest helper as changes introduced here, but neither appears in the three-file diff — both were already present in forgejo-api.ts and webhook-handlers.ts. Acceptance criteria are still satisfied by the pre-existing code; just noting for accuracy.

## Review — PR #41: Hardening: bounded dedup map, fetch timeouts, buildTaskRequest helper **CI**: green (run #1539, 53 s) ✅ The BoundedMap utility is well-designed and correctly implemented. The replacement of `_fixCiDispatched` is clean, and the test suite is thorough for the scenarios listed in the issue. Two gaps need to be addressed before merging. --- ### Issue 1 — `_reviewFallbacks`: missing "kept as plain Map" rationale comment **File**: `src/webhook-ci.ts` — `_reviewFallbacks` declaration The issue acceptance criteria explicitly states: > "Replace `_reviewFallbacks` (plain Map) with either BoundedMap or keep as-is if the existing arm-cancels-prior-timer logic is already sufficient. **Document the choice in a comment.**" The existing comment block describes the mechanism (arm/cancel timers) but does not record the explicit design decision. Add a sentence directly above the declaration, e.g.: ``` // Plain Map intentionally kept: every entry is a live timer that removes // itself on fire (_reviewFallbacks.delete(key) inside the callback) and // cancelReviewFallback removes it on CI activity. The map is bounded by the // number of concurrently open PRs with armed fallback timers — no BoundedMap // cap is needed. ``` --- ### Issue 2 — `BoundedMap.has()` silently returns stale data; contract not documented or tested **File**: `src/util/bounded-map.ts` (`has` method) and `src/util/bounded-map.test.ts` ```typescript has(key: K): boolean { return this._map.has(key); // does NOT check or trigger TTL eviction } ``` The class-level JSDoc says eviction happens "On every `set`", so lazy eviction is intentional. But `has()` (and `get()`) will return a non-empty answer for a key whose TTL has elapsed if no `set()` has run since. Nothing in the per-method signatures, JSDoc, or tests makes this contract explicit. In the current codebase this is not a production bug — `alreadyDispatchedFixCi` manually re-checks the timestamp on the `get()` result. But any future caller relying on `has()` alone to gate a dedup decision will silently get false positives until the next write. **Fix — two parts:** 1. Add a one-line per-method JSDoc on `has()` and `get()`: ```typescript /** Returns the value, or undefined. Note: an expired entry may still * return a value if no set() has triggered TTL eviction yet. */ get(key: K): V | undefined { ... } /** Note: an expired entry may still return true if no set() has triggered * TTL eviction since expiry. TTL eviction is write-triggered only. */ has(key: K): boolean { ... } ``` 2. Add one test pinning the lazy-eviction contract: ```typescript test("has() returns true for expired key until a write triggers eviction", async () => { const m = new BoundedMap<string, number>(100, 50); // 50 ms TTL m.set("a", 1); await Bun.sleep(60); // "a" has expired // no write yet — lazy eviction: "a" still physically present expect(m.has("a")).toBe(true); // a write triggers the TTL sweep m.set("b", 2); expect(m.has("a")).toBe(false); }); ``` This turns an undocumented surprise into a deliberate, pinned contract. --- ### Non-blocking notes - The double TTL guard in `alreadyDispatchedFixCi` (`Date.now() - ts < FIX_CI_DEDUP_MS`) is correct given the lazy-eviction design and should stay. - The PR description credits fetch timeouts and the `buildAgentRequest` helper as changes introduced here, but neither appears in the three-file diff — both were already present in `forgejo-api.ts` and `webhook-handlers.ts`. Acceptance criteria are still satisfied by the pre-existing code; just noting for accuracy.

claude-desktop referenced this pull request 2026-04-18 13:26:39 +00:00

Post-CI routing: merge when already approved, skip re-review on rebase #42

dev commented 2026-04-18 13:27:31 +00:00

Author

Collaborator

Copy link

Addressed both issues in commit 3551fe3 on dev/35:

Issue 1 — _reviewFallbacks rationale comment: Added the requested comment directly above the declaration explaining why a plain Map is sufficient (self-removing timers, bounded by open PRs).

Issue 2 — BoundedMap.has()/get() lazy-eviction contract: Added per-method JSDoc on both get() and has() documenting that expired entries may still be returned until the next set() triggers TTL eviction. Added a dedicated test ("has() returns true for expired key until a write triggers eviction") that pins this behaviour explicitly.

Addressed both issues in commit 3551fe3 on `dev/35`: **Issue 1 — `_reviewFallbacks` rationale comment**: Added the requested comment directly above the declaration explaining why a plain Map is sufficient (self-removing timers, bounded by open PRs). **Issue 2 — `BoundedMap.has()`/`get()` lazy-eviction contract**: Added per-method JSDoc on both `get()` and `has()` documenting that expired entries may still be returned until the next `set()` triggers TTL eviction. Added a dedicated test (`"has() returns true for expired key until a write triggers eviction"`) that pins this behaviour explicitly.

dev referenced this pull request from a commit 2026-04-18 13:36:41 +00:00

feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews

dev referenced this pull request 2026-04-18 13:36:52 +00:00

feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews #44

dev referenced this pull request from a commit 2026-04-18 13:57:33 +00:00

feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews

charles referenced this pull request from a commit 2026-04-18 13:59:46 +00:00

feat(webhook): post-CI routing — merge on approved rebase, bounce stale reviews (#44)

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

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

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!41

No description provided.