refactor(idempotency): remove dead in-memory Map and isAlreadyProcessed from router

[chrisleekr]

Summary

Post-#202 idempotency cleanup. Three parts from issue #211:

• Part 3 (code): Removes the processed Map, cleanupStaleIdempotencyEntries, the setInterval cleanup, and the isAlreadyProcessed tracking-comment scan from src/webhook/router.ts and src/core/tracking-comment.ts. These only ever guarded the dev-test-only /api/test/webhook → processRequest path (404 in production), which mints a fresh synthetic delivery id per call, so the dedup was a no-op there. processRequest and the decideDispatch/dispatch tree are kept (the dev endpoint legitimately exercises the orchestrator→daemon flow), and deliveryMarker is kept (it now solely locates the bot's tracking comment for in-place updates).
• Part 2 (test + doc): The issue's premise was stale. The idx_workflow_runs_inflight collision catch already surfaces "in-flight" (status: refused), NOT failed; the markFailed branch is a distinct compensator for post-insert enqueue failures and is correct as-is. Added a regression assertion in test/workflows/dispatcher.test.ts that a collision does NOT call markFailed.
• Part 1 (audit + doc, no code): chat-thread's state-changing effects are already DB-guarded (execute-workflow → idx_workflow_runs_inflight via a comment-id-derived synthetic deliveryId; propose-* → idx_chat_proposals_one_awaiting). The only unguarded redelivery effect is a cosmetic duplicate answer/decline reply, gated on Valkey fail-open + a same-delivery_id retry — the same posture fix(idempotency): production webhook handlers bypass router dedup, redeliveries unguarded #202 accepted system-wide. Decision: no new durable schema (proportionality); documented on the issue.

Stale comments (src/app.ts, src/mcp/servers/comment.ts, src/webhook/idempotency.ts, src/core/tracking-comment.ts) and docs (CLAUDE.md, docs/build/architecture.md, docs/use/invoking.md, docs/use/safety.md) updated to describe the real two-layer model.

Closes #211

Production idempotency after this PR

flowchart LR
    WH["Webhook delivery<br/>X-GitHub-Delivery"]:::changed --> CD["claimDelivery<br/>Valkey SET NX, 3-day TTL<br/>fail-open"]:::keep --> IDX["idx_workflow_runs_inflight<br/>Postgres partial-unique index<br/>durable backstop"]:::keep
    classDef keep fill:#196f3d,color:#ffffff
    classDef changed fill:#2c3e50,color:#ffffff

Loading

The retired layers (in-memory Map + isAlreadyProcessed marker scan) sat only on the dev-test processRequest path and never participated in this production flow.

Test plan

• bun run typecheck — 0 errors
• bun run build — clean
• bun run lint — 0 errors
• bun run format — all green
• test/core/tracking-comment.test.ts — isAlreadyProcessed suite deleted (function removed); remaining assertions pass (21 pass)
• test/webhook/router.test.ts — cleanupStaleIdempotencyEntries suite deleted; processRequest tests replaced with allowlist-pass + allowlist-block assertions (15 pass)
• test/workflows/dispatcher.test.ts — new expect(mockMarkFailed).not.toHaveBeenCalled() collision assertion passes (7 pass)
• Full suite: 0 real failures (local "FAIL" markers are DB-test skips without DATABASE_URL; they run in CI with Postgres)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@chrisleekr, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 57 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9fa842c7-4e56-487c-a7c0-e7aa03f7b4ff

📥 Commits

Reviewing files that changed from the base of the PR and between 68dacdb and b25f2ba.

📒 Files selected for processing (12)

• CLAUDE.md
• docs/build/architecture.md
• docs/use/invoking.md
• docs/use/safety.md
• src/app.ts
• src/core/tracking-comment.ts
• src/mcp/servers/comment.ts
• src/webhook/idempotency.ts
• src/webhook/router.ts
• test/core/tracking-comment.test.ts
• test/webhook/router.test.ts
• test/workflows/dispatcher.test.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Removes the legacy idempotency layers (in-memory processed Map, periodic cleanup, and the isAlreadyProcessed tracking-comment scan) that only ever applied to the dev-only /api/test/webhook → processRequest path, and aligns code/tests/docs with the post-#202 production idempotency model (claimDelivery + idx_workflow_runs_inflight).

Changes:

• Deleted dead dev-path idempotency mechanisms from src/webhook/router.ts and src/core/tracking-comment.ts.
• Updated tests to remove coverage for the deleted helpers and add/adjust assertions around correct collision handling behavior.
• Updated docs/comments to describe the current two-layer production idempotency model and the tracking-comment marker’s remaining purpose.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/webhook/router.ts	Removes in-memory dedup/cleanup + durable marker scan from processRequest; adds clarifying docstring that this path is dev-only.
src/core/tracking-comment.ts	Removes isAlreadyProcessed; updates marker docs to reflect marker’s role in locating/updating the tracking comment rather than idempotency.
src/webhook/idempotency.ts	Updates module docs to remove references to the retired marker-scan layer.
src/mcp/servers/comment.ts	Updates comments to reflect that the delivery marker is preserved for tracking-comment consistency, not idempotency.
src/app.ts	Updates dev test-webhook comment to reflect the router path’s remaining behaviors.
test/webhook/router.test.ts	Removes tests/mocks for the deleted idempotency utilities; replaces with allowlist wiring assertions for processRequest.
test/core/tracking-comment.test.ts	Removes the isAlreadyProcessed test suite now that the function is removed.
test/workflows/dispatcher.test.ts	Adds regression assertion that in-flight collision refusal must not call markFailed.
docs/use/safety.md	Updates idempotency docs to match claimDelivery + idx_workflow_runs_inflight.
docs/use/invoking.md	Updates idempotency section to the current two-layer production model; notes legacy layers retired.
docs/build/architecture.md	Updates architecture text to clarify production handler path vs dev-only router path and removes legacy idempotency references.
CLAUDE.md	Updates “How it runs / key concepts” router + idempotency descriptions to match current behavior.