Skip to content

feat(signals): register Centaur callbacks on centaur_review watch#438

Open
dimakis wants to merge 1 commit into
mainfrom
feat/centaur-signal-callbacks
Open

feat(signals): register Centaur callbacks on centaur_review watch#438
dimakis wants to merge 1 commit into
mainfrom
feat/centaur-signal-callbacks

Conversation

@dimakis

@dimakis dimakis commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • SignalProcessor registers a callback URL with Centaur's signal bridge when watching a centaur_review gate
  • Deregisters on unwatch (best-effort)
  • Polling fallback continues alongside push — belt and suspenders
  • Gracefully handles Centaur being unavailable (fire-and-forget registration)
  • Race condition fix for pending registrations
  • Env var wiring for CENTAUR_URL and MITZO_URL

Supersedes #399 (cleaned up: removed mixed SSE commits, squashed into single commit).

Part of multi-agent orchestration Phase 2 (Centaur signal bridge). Companion PR: centaur side.

Test plan

  • vitest run server/__tests__/signal-processor.test.ts — all tests pass
  • Manual integration test with Centaur

🤖 Generated with Claude Code

SignalProcessor registers a callback URL with Centaur's signal bridge
when watching a centaur_review gate, enabling push-based resolution
alongside the existing 30s polling fallback. Deregisters on unwatch.
Gracefully handles Centaur being unavailable (fire-and-forget).

Includes race condition fix for pending registrations and env var
wiring for CENTAUR_URL and MITZO_URL.

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

dimakis commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Centaur Review

Found 5 issue(s) (3 warning).

server/signal-processor.ts

unwatchAll() doesn't deregister Centaur callbacks (dangling registrations on shutdown), and the polling path still hardcodes localhost:8642 while callback registration uses the new configurable URL — otherwise clean implementation with good error handling and test coverage.

  • 🟡 bugs (L96): unwatchAll() clears intervals and the watches map, but does not deregister Centaur callbacks for any centaur_review entries. This is called during server shutdown (signalProc.unwatchAll() in index.ts:993), leaving dangling callback registrations in Centaur. Should iterate entries and call deregisterCentaurCallback for centaur_review types before clearing, or delegate to unwatch() per entry. [fixable]
  • 🟡 unsafe_assumptions (L337): The standalone checkCentaurReview polling function (line 337) hardcodes http://localhost:8642, while the new callback registration uses the configurable this.centaurBaseUrl. If a non-default CENTAUR_URL is configured, callback registration hits the correct host but polling falls back to localhost:8642. Consider threading centaurBaseUrl through to checkGate/checkCentaurReview, or making poll() use the instance URL. [fixable]
  • 🔵 bugs (L89): deregisterCentaurCallback is async but called fire-and-forget from unwatch() (synchronous). The await pending.catch(() => {}) inside it correctly serializes against in-flight registrations, but if the DELETE fetch itself rejects, the error is silently swallowed (line 223 empty catch). This is documented as best-effort, which is fine, but the unhandled promise from the fire-and-forget call could surface as an unhandledRejection in edge cases. Consider adding .catch(() => {}) at the call site in unwatch(). [fixable]

server/__tests__/signal-processor.test.ts

unwatchAll() doesn't deregister Centaur callbacks (dangling registrations on shutdown), and the polling path still hardcodes localhost:8642 while callback registration uses the new configurable URL — otherwise clean implementation with good error handling and test coverage.

  • 🟡 missing_tests: No test for unwatchAll() with centaur_review entries — the existing unwatchAll test (line 146) uses human_approval gates only, so it doesn't verify that Centaur callbacks are deregistered on bulk cleanup (which they aren't — see the unwatchAll bug). [fixable]
  • 🔵 missing_tests: No test verifying the callback URL body content. The registration test checks that body contains task.id, but doesn't verify pr_url or callback_url are included correctly. A more precise assertion on the parsed body would catch serialization bugs. [fixable]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant