feat(events): emit structured events for engine to fan out to Slack#164
feat(events): emit structured events for engine to fan out to Slack#164jacsamell wants to merge 1 commit into
Conversation
WalkthroughThis PR adds a new event system for communicating peer review outcomes and human decisions from Cube to Aetheron Engine. It introduces typed event dataclasses, two transport mechanisms (GitHub PR footer comments and webhook delivery), and integrates event creation into the peer review workflow. ChangesEngine Events System for Cube→Aetheron Communication
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cube/integrations/engine_events.py`:
- Around line 130-153: The parser parse_events_footer currently returns any JSON
with an "events" list; update it to validate the footer schema version too:
retrieve payload.get("schema") and return [] unless it exactly equals the
expected footer schema constant (introduce or use a module-level constant like
_FOOTER_SCHEMA_VERSION), so only when schema is present and matches do you
proceed to retrieve "events" and filter dict entries; keep all other error paths
returning [] and continue using _FOOTER_OPEN/_FOOTER_CLOSE to locate the
payload.
- Around line 175-196: The code currently passes the value returned by
_webhook_url() directly to urllib.request.Request / urlopen; add an explicit
scheme allowlist by parsing the URL (e.g., via urllib.parse.urlparse) and
verifying parsed.scheme is either "http" or "https" before proceeding. If the
scheme is missing or not in {"http","https"}, return False (same behavior as
when url is falsy) or log and abort; perform this check right after calling
_webhook_url() and before building the payload/Request and before the urlopen
loop in send (or the function containing the for-attempt loop) so only
http/https requests are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66a1f50c-f7f4-49cb-94e0-77d3254878b6
📒 Files selected for processing (2)
python/cube/commands/peer_review.pypython/cube/integrations/engine_events.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
python/cube/integrations/engine_events.py
[error] 180-188: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 195-195: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🔇 Additional comments (2)
python/cube/commands/peer_review.py (1)
506-519: LGTM!python/cube/integrations/engine_events.py (1)
44-127: LGTM!Also applies to: 216-258
| def parse_events_footer(body: str) -> list[dict[str, Any]]: | ||
| """Inverse of `build_events_footer`. Returns raw event dicts. | ||
|
|
||
| Used by tests and (optionally) by engine implementations that want to | ||
| consume cube's emitted events without duplicating the marker constants. | ||
|
|
||
| Returns an empty list when no footer is present or the JSON is malformed. | ||
| """ | ||
| start = body.find(_FOOTER_OPEN) | ||
| if start < 0: | ||
| return [] | ||
| end = body.find(_FOOTER_CLOSE, start + len(_FOOTER_OPEN)) | ||
| if end < 0: | ||
| return [] | ||
| raw = body[start + len(_FOOTER_OPEN) : end].strip() | ||
| try: | ||
| payload = json.loads(raw) | ||
| except json.JSONDecodeError: | ||
| return [] | ||
| events = payload.get("events") | ||
| if not isinstance(events, list): | ||
| return [] | ||
| return [e for e in events if isinstance(e, dict)] | ||
|
|
There was a problem hiding this comment.
Validate schema version in footer parser.
At Line 146 onwards, the parser accepts any JSON containing an events list, even when schema is missing or incompatible. That weakens the versioned contract and can produce silent misreads across schema upgrades.
Suggested fix
def parse_events_footer(body: str) -> list[dict[str, Any]]:
@@
try:
payload = json.loads(raw)
except json.JSONDecodeError:
return []
+ if payload.get("schema") != EVENT_SCHEMA_VERSION:
+ return []
events = payload.get("events")
if not isinstance(events, list):
return []
return [e for e in events if isinstance(e, dict)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cube/integrations/engine_events.py` around lines 130 - 153, The parser
parse_events_footer currently returns any JSON with an "events" list; update it
to validate the footer schema version too: retrieve payload.get("schema") and
return [] unless it exactly equals the expected footer schema constant
(introduce or use a module-level constant like _FOOTER_SCHEMA_VERSION), so only
when schema is present and matches do you proceed to retrieve "events" and
filter dict entries; keep all other error paths returning [] and continue using
_FOOTER_OPEN/_FOOTER_CLOSE to locate the payload.
| url = _webhook_url() | ||
| if not url: | ||
| return False | ||
|
|
||
| payload = json.dumps({"schema": EVENT_SCHEMA_VERSION, "event": asdict(event)}).encode() | ||
| req = urllib.request.Request( | ||
| url, | ||
| data=payload, | ||
| method="POST", | ||
| headers={ | ||
| "Content-Type": "application/json", | ||
| "User-Agent": "agent-cube", | ||
| }, | ||
| ) | ||
|
|
||
| # Track last error for debugging; not surfaced upstream — engine is the | ||
| # source of truth for human-facing comms. | ||
| _last_err: Optional[Exception] = None | ||
| for attempt in range(2): | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=timeout) as resp: | ||
| return 200 <= resp.status < 300 |
There was a problem hiding this comment.
Restrict webhook URL schemes to http/https before opening.
At Line 175 and Line 195, CUBE_ENGINE_WEBHOOK_URL is used directly with urlopen. Add an explicit scheme allowlist to avoid unexpected scheme handling (file:, custom handlers) and tighten outbound request safety.
Suggested fix
import json
import os
import urllib.error
+import urllib.parse
import urllib.request
@@
def post_webhook_event(event: EngineEvent, *, timeout: int = 10) -> bool:
@@
url = _webhook_url()
if not url:
return False
+ parsed = urllib.parse.urlparse(url)
+ if parsed.scheme not in {"http", "https"}:
+ return False
payload = json.dumps({"schema": EVENT_SCHEMA_VERSION, "event": asdict(event)}).encode()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url = _webhook_url() | |
| if not url: | |
| return False | |
| payload = json.dumps({"schema": EVENT_SCHEMA_VERSION, "event": asdict(event)}).encode() | |
| req = urllib.request.Request( | |
| url, | |
| data=payload, | |
| method="POST", | |
| headers={ | |
| "Content-Type": "application/json", | |
| "User-Agent": "agent-cube", | |
| }, | |
| ) | |
| # Track last error for debugging; not surfaced upstream — engine is the | |
| # source of truth for human-facing comms. | |
| _last_err: Optional[Exception] = None | |
| for attempt in range(2): | |
| try: | |
| with urllib.request.urlopen(req, timeout=timeout) as resp: | |
| return 200 <= resp.status < 300 | |
| url = _webhook_url() | |
| if not url: | |
| return False | |
| parsed = urllib.parse.urlparse(url) | |
| if parsed.scheme not in {"http", "https"}: | |
| return False | |
| payload = json.dumps({"schema": EVENT_SCHEMA_VERSION, "event": asdict(event)}).encode() | |
| req = urllib.request.Request( | |
| url, | |
| data=payload, | |
| method="POST", | |
| headers={ | |
| "Content-Type": "application/json", | |
| "User-Agent": "agent-cube", | |
| }, | |
| ) | |
| # Track last error for debugging; not surfaced upstream — engine is the | |
| # source of truth for human-facing comms. | |
| _last_err: Optional[Exception] = None | |
| for attempt in range(2): | |
| try: | |
| with urllib.request.urlopen(req, timeout=timeout) as resp: | |
| return 200 <= resp.status < 300 |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 180-188: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 195-195: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cube/integrations/engine_events.py` around lines 175 - 196, The code
currently passes the value returned by _webhook_url() directly to
urllib.request.Request / urlopen; add an explicit scheme allowlist by parsing
the URL (e.g., via urllib.parse.urlparse) and verifying parsed.scheme is either
"http" or "https" before proceeding. If the scheme is missing or not in
{"http","https"}, return False (same behavior as when url is falsy) or log and
abort; perform this check right after calling _webhook_url() and before building
the payload/Request and before the urlopen loop in send (or the function
containing the for-attempt loop) so only http/https requests are allowed.
leobaldock
left a comment
leobaldock
left a comment
There was a problem hiding this comment.
Direction looks right to me. This PR is a good narrow slice: Cube emits structured review events and Engine owns Slack routing/fanout via a GitHub review trigger. I would keep this scoped to removing the Slack chatter path rather than expanding it into the full runner/conformance work.
To get this ready, I think the remaining work is:
-
Rebase onto current main and resolve the conflict in
python/cube/commands/peer_review.py. Keep the current main reconciliation /print_panel_summaryflow, then append the Engine footer after the gate has been computed and before the review body is printed/posted. Use the current expected judge count, not the stale oldtotalvariable. -
Keep the v1 event surface to the two wired event types for now:
human_callpr_ready_to_merge
I would drop
pr_blockedandsetup_neededfrom this PR unless there are actual emitters. They are reasonable future events, but adding unwired contract surface now creates support burden without moving this slice forward. -
Address the two CodeRabbit comments in
engine_events.py:parse_events_footer()should reject missing/wrong schema versions.post_webhook_event()should only allowhttp/httpswebhook URLs before passing anything tourlopen.
-
Add focused tests for the event module. I would cover footer round-trip, malformed/missing/wrong-schema footers returning
[], human-call event construction, ready-to-merge only onAPPROVE, invalid human-call severity normalization, unset webhook env returningFalse, and non-http webhook URLs being rejected. -
Update the PR description to make the integration point explicit: Engine should subscribe to
pull_request_review.submitted, filter reviews fromthe-agent-cube[bot], parse the hiddenagent-cube-eventsfooter, then routehuman_call/pr_ready_to_mergethrough the native Engine trigger/session path.
After that: rerun mypy + the targeted tests + CI. I would leave the separate “does this implementation match the approved plan?” judge as a follow-up PR, because that changes approval semantics. This PR can land the transport/event boundary cleanly.
2 participants
Cube emits structured events; Engine handles Slack delivery. Splits responsibilities cleanly.
Two emit channels
1. PR-context events ride the review body (the common case)
Every
cube prvreview body now ends in a hidden HTML-comment footer:pull_request_review.submittedfor reviews fromthe-agent-cube[bot]2. Non-PR events POST to
$CUBE_ENGINE_WEBHOOK_URL(rare path:setup_needed, scheduled sweeps). Fire-and-forget + one retry. No-op when env-var unset.Event types (schema v1)
human_call(severity: must-decide | worth-asking) — wiredpr_ready_to_merge— wiredpr_blocked— defined, detection path follow-upsetup_needed— defined, detection path follow-upWhat Engine needs to do
pull_request_review.submittedfromthe-agent-cube[bot]<!-- agent-cube-events ... -->marker<!-- agent-cube-resolution {...} -->comment, cube picks up on resume (via PR feat(auto): persist -p feedback to task spec so judges see it #156's-ppersistence) — follow-up PROpen to your preference on resolution pathway (GitHub PR comment vs
.cube/resolutions/<request_id>.jsonon the cube host).8 module smoke tests + 192 existing tests pass. Not admin-merging — architecture choice worth eyeballs.
🤖 Generated with Claude Code
Overview
This PR introduces structured event emission from Cube to delegate Slack delivery responsibilities to the Engine. It establishes a clear event-driven boundary between the two systems via two communication channels.
Key Changes
New module:
engine_events.py(+258 lines)HumanCallEvent,PrReadyToMergeEvent,PrBlockedEvent,SetupNeededEvent<!-- agent-cube-events { ... } -->) appended to review bodies$CUBE_ENGINE_WEBHOOK_URLwith one retry; no-op if env var unsetUpdated:
peer_review.py(+14 lines)Design
pull_request_review.submittedand extracts events from bot review footers—no new Cube infrastructure neededmust-decideandworth-askingseverity levels for human calls, plus merge readiness and blocking statesTest Coverage
8 new module smoke tests + 192 existing tests pass