feat: add @handle-routed agent team (interactive agents + API bots) - #91
Conversation
Mentions are now routed to a team of addressable members by @handle: - @tradedesk (agent): parses $TICKER buy/sell, logs approval-gated trade proposals, executes via paper broker on approval (no live orders) - @research (agent): Grok + MCP research briefs filed to the timeline - @shopping (agent): product picks with approval-gated purchase intents - @tickerbot (bot): deterministic cashtag lookup, reference API bot Members are classified kind=agent (interactive, LLM-backed, can delegate over A2A) vs kind=bot (deterministic function executor); the A2A registry now stores the classification. The dispatcher routes timeline approvals back to the owning member before falling back to generic Grok execution. Untagged mentions keep the original generic behavior via a fallback agent. Includes pytest suite (routing, trade parsing, paper broker), make test target, env.example entries, and docs/AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation
|
github-actions
Bot
added
the
documentation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds agent routing, built-in members, approval-gated trade execution, structured timeline actions, agent-kind support, and docs/tests/tooling for local runs. ChangesAgent Team Framework
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Listener
participant route_mention
participant TeamMember
participant TimelineServer
participant Dispatcher
participant find_member
participant PaperBroker
Listener->>route_mention: MentionContext(text)
route_mention->>TeamMember: select member
TeamMember-->>Listener: AgentReply(text, card)
Listener->>TimelineServer: push_timeline_card(card, posted_by)
Dispatcher->>TimelineServer: get_timeline_item(item_id)
Dispatcher->>find_member: agent_id
find_member-->>Dispatcher: TeamMember
Dispatcher->>TeamMember: execute_action(item, action)
TeamMember->>PaperBroker: execute(...)
TeamMember-->>Dispatcher: result
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
groupthinking
changed the title
🔍 PR Validation |
There was a problem hiding this comment.
Pull request overview
This PR introduces an @handle-routed “agent team” for the Python listener so mentions can be dispatched to specialized interactive agents (LLM-backed) or deterministic API bots, with approvals routed back to the owning member via timeline card metadata.
Changes:
- Add a registry/router layer (
agents/) and several built-in team members (TradeDesk, Research, Shopping, TickerBot, plus fallback GeneralAgent). - Update listener + dispatcher to route mentions and approval actions to the correct team member.
- Add a new pytest suite +
make testflow, plus docs/README/env updates describing the agent team.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| timeline_server.py | Adds kind to A2A agent create payloads (classification: agent vs bot). |
| a2a_store.py | Persists the new kind classification in the A2A registry store. |
| listener.py | Routes inbound mentions to a team member and posts member-owned timeline cards. |
| mcp_dispatcher.py | Routes Approve/Reject actions to the owning member’s execute_action() before falling back to Grok. |
| agents/init.py | Defines agents package for team routing. |
| agents/base.py | Introduces shared dataclasses (MentionContext, AgentReply, AgentProfile) and grok_chat(). |
| agents/broker.py | Adds PaperBroker to execute/record simulated fills in a local JSON ledger. |
| agents/router.py | Implements word-bounded, case-insensitive @handle matching. |
| agents/registry.py | Builds the team roster, provides routing helpers, and registers members into A2A on startup. |
| agents/team/init.py | Declares built-in team members package. |
| agents/team/general.py | Implements fallback “GeneralAgent” that preserves the legacy Grok behavior. |
| agents/team/tradedesk.py | Implements approval-gated trade proposal parsing + execution via PaperBroker. |
| agents/team/research.py | Implements research agent that replies briefly and files a full timeline brief. |
| agents/team/shopping.py | Implements shopping agent that posts picks + approval-gated purchase intents. |
| agents/team/tickerbot.py | Implements deterministic cashtag lookup bot (no LLM). |
| tests/test_router.py | Adds routing tests (case-insensitive, word boundaries, fallback, bot kind). |
| tests/test_tradedesk.py | Adds trade parsing + approval-card + approve/reject execution tests. |
| tests/test_broker.py | Adds paper broker fill recording + position aggregation tests. |
| requirements-dev.txt | Adds dev deps for tests (pytest). |
| pytest.ini | Configures pytest discovery and pythonpath. |
| Makefile | Adds make test target to install dev deps and run pytest. |
| README.md | Documents the agent team and agent vs bot classification. |
| env.example | Adds env vars for member handles and paper-ledger path. |
| docs/AGENTS.md | Adds full agent-team documentation: roster, flow, and how to add members. |
| summary = ( | ||
| f"{str(metadata.get('side', '')).upper()} " | ||
| f"{metadata.get('quantity', 0):g} ${metadata.get('ticker', '?')}" | ||
| ) |
| def get_timeline_item(item_id: str) -> Optional[Dict]: | ||
| timeline_url = os.getenv("TIMELINE_API_URL", "http://127.0.0.1:8080") | ||
| response = requests.get(f"{timeline_url}/v1/timeline/items/{item_id}", timeout=10) | ||
| if response.status_code != 200: | ||
| return None | ||
| return response.json() |
| description: Optional[str] = "" | ||
| status: Optional[str] = "offline" | ||
| endpoint: Optional[str] = "" | ||
| kind: Optional[str] = "agent" # "agent" (interactive) or "bot" (deterministic) |
Address Copilot review on #91: - get_timeline_item swallows request/JSON errors so the dispatcher loop survives timeline-server hiccups and falls back to generic execution - Tradedesk coerces ticker/side/quantity from card metadata before formatting or executing, rejecting non-numeric quantities safely - timeline_server constrains A2A 'kind' to Literal['agent','bot'] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_router.py (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the multi-handle-in-one-mention case.
Every test here tags exactly one handle. Given the priority-ordering bug flagged in
agents/router.py(find_target, lines 9-23), a test likeroute_mention(MentionContext(text="@research@tradedesk..."))would have caught it immediately. Add it once that fix lands.🤖 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 `@tests/test_router.py` around lines 1 - 45, Add coverage in test_route_to... around route_mention and/or find_target for the multi-handle mention case, since current tests only exercise one handle at a time. Create a test that passes a MentionContext like "`@Research` `@Tradedesk` ..." and assert the router picks the correct target according to the intended priority order in agents.router.find_target. Use the existing symbols route_mention, find_target, and build_team to keep the new test aligned with the current routing behavior.tests/test_broker.py (1)
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests only cover the happy path.
No test exercises a corrupted/truncated ledger file (silently returns
[], losing history) or callingexecute()twice for the same trade. Both are the exact failure modes flagged inagents/broker.py. Add one before this bites in prod.🤖 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 `@tests/test_broker.py` around lines 1 - 19, The broker tests only cover normal execution and miss the failure paths in PaperBroker. Add a test around PaperBroker’s ledger-loading/restore behavior for a corrupted or truncated trades file to verify it does not silently drop history, and add a test that calls PaperBroker.execute() twice with the same trade to ensure duplicate executions are handled as expected. Use the PaperBroker and execute identifiers so the new coverage targets the exact regression points.a2a_store.py (1)
84-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDuplicate IDs leave the A2A registry stale.
register_agent()returns early on an existingid, so the POST path never updates the seededx-agentrecord. SinceDEFAULT_AGENTSis written directly into the store withoutkind,GET /v1/a2a/agentskeeps returning that record without the classification field. Update mutable fields on match instead of no-oping, or stop re-registering the same id.🤖 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 `@a2a_store.py` around lines 84 - 102, register_agent() currently no-ops when an existing agent id is found, which leaves seeded entries like x-agent stale and missing updated fields such as kind. Update the existing record in the data["agents"] list when the id matches instead of returning early, so mutable fields from the incoming payload are merged into the stored agent before _write_store(data) is called. Use register_agent and the A2A_STORE_LOCK/_read_store/_write_store flow to locate the fix.
🤖 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 `@agents/base.py`:
- Around line 93-112: Wrap the requests.post call in send_a2a_message with
RequestException handling so timeline outages don’t bubble up to callers like
handle_mention; follow the same graceful-failure pattern used in register_team,
and make sure send_a2a_message logs or otherwise swallows the failure instead of
raising. If you choose to address auth instead, add the required authorization
header consistently in send_a2a_message and align it with the server-side
expectations for /v1/a2a/messages.
- Around line 71-90: grok_chat currently relies on the xai_sdk Client default
deadline, which can let a single xAI/MCP request stall the mention loop; update
grok_chat to enforce an explicit hard timeout around the chat request/stream
using the existing Client/chat.stream flow, and make it fail fast by returning
the empty fallback already used when XAI_API_KEY is missing. Keep the change
localized to grok_chat and its Client/chat.create/chat.stream path so the
listener’s higher-level handle_mention error handling can continue to work
unchanged.
In `@agents/broker.py`:
- Line 16: The ledger write path in broker.py is not crash-safe and the current
_LEDGER_LOCK only protects threads, not multiple processes. Update the
read/write flow around _read and the ledger persistence logic to use an atomic
write pattern so a mid-write crash cannot truncate the file, and stop swallowing
parse failures in _read so corruption is surfaced instead of silently returning
an empty list. If this ledger can be shared across workers, replace the
in-process lock with a cross-process guard or otherwise enforce single-writer
access, and thread through an idempotency key from the execute_action flow so
duplicate approvals do not double-fill.
In `@agents/registry.py`:
- Around line 33-35: The fallback in route_mention currently depends on
team[-1], which silently breaks if build_team() changes member ordering. Update
route_mention to select the general fallback by an explicit marker instead of
position, using the TeamMember returned by get_team() and the same target lookup
logic. Prefer identifying the fallback via a property such as an empty
profile.handle, so untagged mentions always route to the intended GeneralAgent
regardless of list order.
- Around line 47-64: register_team() currently swallows transient
timeline-server startup failures, so a member can miss A2A registration forever
on the first POST. Update register_team() to retry requests.post to
/v1/a2a/agents with a small backoff and only give up after several attempts,
using the existing timeline_url/profile payload logic. Keep the exception
handling around requests.RequestException, but add retry logging tied to
register_team() so startup races are handled instead of silently moving on.
- Around line 26-30: `get_team()` is not thread-safe: the lazy singleton check
on `_TEAM` can race and call `build_team()` twice. Add synchronization around
the first initialization in `agents.registry.get_team` (for example, a
module-level lock guarding the None check and assignment) so only one thread can
build and publish `_TEAM`, while preserving the existing cached return path.
In `@agents/router.py`:
- Around line 9-23: The routing in find_target currently returns the first
matching handle based on team roster order, which breaks multi-mention messages.
Update find_target so it scans all matched handles in the text and selects the
member whose `@handle` appears earliest in the input, while still ignoring empty
handles and preserving case-insensitive, word-bounded matching. Use the existing
find_target and TeamMember/profile.handle symbols to locate the logic and keep
the fallback behavior unchanged.
In `@agents/team/shopping.py`:
- Line 62: The reply truncation logic in shopping.py is duplicated in
research.py, so extract it into a shared helper in agents/base.py such as
truncate_for_reply(text, limit=270, suffix=...). Update the shopping reply
assignment to call that helper instead of inlining the one-liner, and reuse the
same helper from the matching code in research.py so the threshold and wording
stay consistent in one place.
- Around line 39-49: The prompt in handle_mention interpolates untrusted
mention.text directly into the Grok request, creating a prompt-injection path.
Update handle_mention in ShoppingAgent to wrap mention.text in a clearly
delimited data block and explicitly instruct grok_chat to treat it as untrusted
content, not instructions. Use the existing mention.text and grok_chat call
structure as the place to apply the fix, and mirror the same protection pattern
in the other team agents mentioned in the review.
In `@agents/team/tradedesk.py`:
- Around line 101-118: The approve path in execute_action currently calls
self.broker.execute() every time, which can double-fill the same trade if the
dispatcher replays the message before last_seen is persisted. Add an idempotency
guard by deriving a stable key from the item metadata and passing it through to
broker.execute, then have execute_action skip or no-op when the same approval
has already been processed. Use the existing execute_action method and the
broker.execute call site as the entry points for the change, and make the
returned approval message reflect the deduped behavior.
- Around line 101-118: The trade action path in execute_action currently
approves or rejects any matching trade item without verifying who requested it.
Update the PATCH /v1/timeline/items/{id} flow and TradeDeskAgent.execute_action
so the caller’s identity is passed in and compared against the card’s author_id
or an authorized approver list before any approve/reject dispatch; if it doesn’t
match, reject the action early and avoid calling broker.execute.
In `@listener.py`:
- Around line 107-143: main() (or the mention-processing block in listener.py)
is doing routing, reply handling, card pushing, and logging in one large
branch-heavy section. Extract this flow into a focused process_mention(client,
mention) helper, moving the MentionContext creation, route_mention call,
member.handle_mention error handling, client.create_tweet reply logic, and
push_timeline_card logic there so the top-level loop stays small and the
branch/statement count drops.
- Around line 107-146: The mention-processing flow in listener.py marks a
mention as seen even when push_timeline_card fails, which can permanently drop
approval-gated cards. Update the mention handling around member.handle_mention,
client.create_tweet, and push_timeline_card so a card-push failure does not
advance the watermark via save_last_seen/start_time. Either skip save_last_seen
for that mention until the card is successfully pushed, or persist the failed
card in a retry queue and only mark the mention seen after the card lands.
In `@Makefile`:
- Around line 30-31: The test target is not bootstrappable because it assumes
.venv/bin/activate already exists, so make test fails on a clean checkout before
pytest starts. Update the Makefile’s test rule to depend on setup or
create/activate the virtual environment inline before running pip install and
pytest, using the existing setup target and test target names so the workflow
works from scratch.
In `@mcp_dispatcher.py`:
- Around line 35-42: Wrap the network call in get_timeline_item with exception
handling so requests.get failures do not crash the dispatcher loop. Catch
request-related errors inside get_timeline_item and return None on failure, and
make sure the caller that polls timeline items continues gracefully, similar to
the try/except used around owner.execute_action. Keep the fix localized around
get_timeline_item and the polling path that uses it.
- Around line 138-156: Add an idempotency guard in the structured action path
before calling owner.execute_action in mcp_dispatcher.py so retries do not
replay the same timeline_action. Check and mark the timeline item as already
processed (or otherwise make the action idempotent) before the side effect
occurs, and ensure PaperBroker.execute is only reached once for a given action
even if the dispatcher crashes and reruns.
In `@README.md`:
- Around line 22-27: Two fenced Markdown blocks in README are missing a language
tag and will keep triggering markdownlint. Update the bare fences in the
affected examples to use a plain text tag such as text so the blocks remain
unchanged visually while satisfying the linter.
In `@tests/test_tradedesk.py`:
- Around line 54-70: Add a regression test for double-approval idempotency in
test_approve_executes_paper_trade by calling TradeDeskAgent.execute_action twice
with the same item and asserting the PaperBroker position stays at 10.0 instead
of doubling. Use the existing TradeDeskAgent and PaperBroker setup to verify the
duplicate-fill case is prevented once the execute_action/broker.execute fix is
in place.
In `@timeline_server.py`:
- Line 44: The TimelineServer model’s kind field is too permissive and allows
arbitrary strings into the registry. Tighten validation in the TimelineServer
schema by constraining kind to the existing allowed values used by downstream
routing, and keep the default aligned with the current agent behavior. Use the
kind field declaration in TimelineServer as the anchor and update any related
validation/type definition so only "agent" or "bot" can pass.
---
Outside diff comments:
In `@a2a_store.py`:
- Around line 84-102: register_agent() currently no-ops when an existing agent
id is found, which leaves seeded entries like x-agent stale and missing updated
fields such as kind. Update the existing record in the data["agents"] list when
the id matches instead of returning early, so mutable fields from the incoming
payload are merged into the stored agent before _write_store(data) is called.
Use register_agent and the A2A_STORE_LOCK/_read_store/_write_store flow to
locate the fix.
In `@tests/test_broker.py`:
- Around line 1-19: The broker tests only cover normal execution and miss the
failure paths in PaperBroker. Add a test around PaperBroker’s
ledger-loading/restore behavior for a corrupted or truncated trades file to
verify it does not silently drop history, and add a test that calls
PaperBroker.execute() twice with the same trade to ensure duplicate executions
are handled as expected. Use the PaperBroker and execute identifiers so the new
coverage targets the exact regression points.
In `@tests/test_router.py`:
- Around line 1-45: Add coverage in test_route_to... around route_mention and/or
find_target for the multi-handle mention case, since current tests only exercise
one handle at a time. Create a test that passes a MentionContext like "`@Research`
`@Tradedesk` ..." and assert the router picks the correct target according to the
intended priority order in agents.router.find_target. Use the existing symbols
route_mention, find_target, and build_team to keep the new test aligned with the
current routing behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 18dc353d-e5d3-4c34-8944-9a1aa2319033
📒 Files selected for processing (24)
MakefileREADME.mda2a_store.pyagents/__init__.pyagents/base.pyagents/broker.pyagents/registry.pyagents/router.pyagents/team/__init__.pyagents/team/general.pyagents/team/research.pyagents/team/shopping.pyagents/team/tickerbot.pyagents/team/tradedesk.pydocs/AGENTS.mdenv.examplelistener.pymcp_dispatcher.pypytest.inirequirements-dev.txttests/test_broker.pytests/test_router.pytests/test_tradedesk.pytimeline_server.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
⚠️ CI failures not shown inline (2)
GitHub Actions: 🤖 Autonomous Agent CI/CD / 2_🏷️ Auto-Label PRs.txt: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
GitHub Actions: 🤖 Autonomous Agent CI/CD / 🏷️ Auto-Label PRs: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
🧰 Additional context used
🪛 ast-grep (0.44.0)
agents/router.py
[warning] 20-20: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(r"@" + re.escape(handle) + r"\b", lowered)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
agents/registry.py
[warning] 61-61: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(f"{timeline_url}/v1/a2a/agents", json=payload, timeout=10)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
mcp_dispatcher.py
[warning] 36-36: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(f"{timeline_url}/v1/timeline/items/{item_id}", timeout=10)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
agents/base.py
[warning] 101-111: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{timeline_url}/v1/a2a/messages",
json={
"from": from_agent,
"to": to,
"type": message_type,
"content": content,
"metadata": metadata or {},
},
timeout=10,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
agents/broker.py
[info] 46-46: use jsonify instead of json.dumps for JSON output
Context: json.dumps(fills, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
listener.py
[warning] 60-60: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(f"{timeline_url}/v1/timeline/items", json=payload, timeout=10)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 markdownlint-cli2 (0.22.1)
docs/AGENTS.md
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.15.20)
agents/router.py
[warning] 4-4: typing.List is deprecated, use list instead
(UP035)
agents/team/research.py
[warning] 22-22: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
agents/registry.py
[warning] 4-4: typing.List is deprecated, use list instead
(UP035)
[warning] 27-27: Using the global statement to update _TEAM is discouraged
(PLW0603)
mcp_dispatcher.py
[warning] 147-147: Do not catch blind exception: Exception
(BLE001)
agents/base.py
[warning] 14-14: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 14-14: typing.List is deprecated, use list instead
(UP035)
[warning] 59-59: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 65-65: Unused method argument: item
(ARG002)
[warning] 65-65: Unused method argument: action
(ARG002)
agents/team/shopping.py
[warning] 12-12: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 27-27: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
agents/team/general.py
[warning] 19-19: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
agents/team/tradedesk.py
[warning] 13-13: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 51-51: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
agents/team/tickerbot.py
[warning] 19-19: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
agents/broker.py
[warning] 14-14: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 14-14: typing.List is deprecated, use list instead
(UP035)
[warning] 20-20: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
listener.py
[warning] 64-64: Too many branches (13 > 12)
(PLR0912)
[warning] 64-64: Too many statements (52 > 50)
(PLR0915)
[warning] 69-69: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 120-120: Do not catch blind exception: Exception
(BLE001)
[warning] 133-133: Do not catch blind exception: Exception
(BLE001)
[warning] 139-139: Do not catch blind exception: Exception
(BLE001)
🔍 Remote MCP GitHub Copilot
PR context
- The target PR is
#91ingroupthinking/MyXstack, titled “feat: add@handle-routedagent team (interactive agents + API bots)”. It is open on branchclaude/myxstack-agent-merge-myaockagainstmain. - CI is mostly green:
validatepassed,🧠 Copilot Auto-Reviewpassed,labeland🔍 Auto-Detect & Buildpassed;copilot-pull-request-revieweris still in progress, whileauto-mergeand🛡️ Security Scanwere skipped. - GitHub’s validation comment flags two review-process notes: the PR title should follow conventional commits, and the PR is large (1030 lines changed).
- The main code changes are an
agents/team system:agents/router.pyadds@handlematching;listener.pynow routes each mention to aTeamMember, postsreply.text, and optionally pushesreply.card;mcp_dispatcher.pynow resolves timeline approvals back to the owning member viaexecute_action()before falling back to legacy Grok execution;agents/broker.pyadds a local JSONPaperBroker. - Built-in members are
@Tradedesk(agent),@Research(agent),@Shopping(agent),@TickerBot(bot), plusGeneralAgentas fallback. The docs/env updates describe paper-only trades, approval-gated shopping intents, and configurable handles. Tests were added for routing, broker fills, and trade approval/rejection paths.
🔇 Additional comments (14)
docs/AGENTS.md (1)
1-88: LGTM!env.example (1)
55-67: LGTM!pytest.ini (1)
1-3: LGTM!requirements-dev.txt (1)
1-2: LGTM!agents/__init__.py (1)
1-1: LGTM!agents/base.py (1)
46-53: LGTM on the dataclasses themselves.agents/team/__init__.py (1)
1-1: LGTM!agents/team/research.py (2)
34-39: 🔒 Security & PrivacySame unsanitized-prompt problem as
shopping.py.
mention.textgoes straight into the research prompt. Already flagged with fix suggestion inagents/team/shopping.py.
43-57: LGTM!agents/team/general.py (2)
31-37: 🔒 Security & PrivacySame unsanitized-prompt problem as
shopping.py.Fallback agent gets the widest exposure since it handles every untagged mention — same fix applies.
40-52: LGTM!agents/team/shopping.py (1)
65-76: LGTM!agents/team/tickerbot.py (1)
18-38: LGTM!Deterministic, no LLM, no external I/O — the one member in this batch that can't be prompt-injected or double-fire a side effect.
listener.py (1)
64-71: LGTM!
| ``` | ||
| @MyXstack @Tradedesk $TSLA buy 100 → approval-gated trade proposal (paper broker) | ||
| @MyXstack @Research why is $NVDA down? → Grok research brief on the timeline | ||
| @MyXstack @Shopping shoes under $150 → product picks, purchase approval-gated | ||
| @MyXstack @TickerBot $BTC → deterministic cashtag lookup (API bot) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tag the two bare fences.
Markdownlint will keep flagging Lines 22 and 39 until those blocks get a language tag, e.g. text.
Also applies to: 39-53
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` around lines 22 - 27, Two fenced Markdown blocks in README are
missing a language tag and will keep triggering markdownlint. Update the bare
fences in the affected examples to use a plain text tag such as text so the
blocks remain unchanged visually while satisfying the linter.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@agents/team/tradedesk.py`:
- Around line 105-112: The coercion in tradedesk processing still allows invalid
trade values through: validate the normalized side in the trade-card flow before
calling broker.execute(), and reject anything outside buy/sell instead of
persisting it. Also add a positive-quantity check after the float conversion in
the same trade-handling path so zero or negative quantities return an error
before execution. Use the existing metadata parsing in the trade card logic and
the broker.execute() call site to place these guards.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: dd8fccb2-5436-435c-af1d-fe640f90b8b0
📒 Files selected for processing (3)
agents/team/tradedesk.pymcp_dispatcher.pytimeline_server.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: 🤖 Autonomous Agent CI/CD / 1_🏷️ Auto-Label PRs.txt: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
GitHub Actions: 🤖 Autonomous Agent CI/CD / 🏷️ Auto-Label PRs: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Classify each team member by `kind` in its profile and the A2A registry as either an `agent` (interactive, LLM-backed, can reason, use MCP tools, delegate over A2A, and propose approval-gated actions) or a `bot` (deterministic function executor with no LLM or autonomy).
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: `Tradedesk` must parse `$TICKER buy/sell [qty]`, log a trade proposal to the approval timeline, and only execute fills through `PaperBroker` after approval; no live broker should be used by default.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: `Shopping` must research product picks with Grok and treat purchases as approval-gated intents; no payment executor is wired in by default.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: `Research` must answer using Grok plus MCP tools for live X context, post a short reply, and file the full brief on the timeline.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: `TickerBot` must act as a deterministic cashtag lookup bot that returns live-search links for each `$TICKER`.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Members are configurable via `TRADEDESK_HANDLE`, `RESEARCH_HANDLE`, `SHOPPING_HANDLE`, and `TICKERBOT_HANDLE` in `.env`.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Interactive agents may message each other directly over the A2A bus via `POST /v1/a2a/messages` using `agents.base.send_a2a_message()`.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: When adding a team member, create a `TeamMember` subclass in `agents/team/mymember.py`, define a unique `AgentProfile` with `id`, `handle`, and `kind`, implement `handle_mention()` returning `AgentReply(text, card=None)`, and implement `execute_action()` when the member has Approve/Reject cards.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Register the new team member in `build_team()` in `agents/registry.py` before the fallback `GeneralAgent`.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Trades must be paper-only: `PaperBroker` writes simulated fills to a local ledger and must not touch a real exchange.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Purchases must remain intents only; approving a shopping card records the intent and no payment adapter is shipped by default.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: All side-effecting actions must be approval-gated on the timeline before the dispatcher executes them.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:22:51.655Z
Learning: Until each member gets its own real X handle, all members share the listener's X account and the router selects the member from the tagged handle in the text.
🪛 ast-grep (0.44.0)
mcp_dispatcher.py
[warning] 37-37: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(f"{timeline_url}/v1/timeline/items/{item_id}", timeout=10)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 Ruff (0.15.20)
timeline_server.py
[warning] 2-2: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 2-2: typing.List is deprecated, use list instead
(UP035)
🔍 Remote MCP GitHub Copilot
Relevant review context for PR #91
- PR is open on
claude/myxstack-agent-merge-myaockintomain, with 24 files changed, 957 additions / 81 deletions (GitHub later validation also flagged the PR as large). - CI status is mostly green, but GitHub validation noted conventional-commit title concerns and the PR size; CodeRabbit’s run also says
copilot-pull-request-reviewerwas COMMENTED/CHANGES_REQUESTED, not approved. - The latest review highlights these still-relevant issues:
- Prompt injection risk:
mention.textis passed directly into Grok prompts inagents/team/shopping.py,agents/team/research.py, andagents/team/general.py. - Routing edge case:
agents/router.pycurrently returns the first matching handle by roster order, and CodeRabbit wants multi-handle mentions to route by earliest handle in text. - Idempotency / duplicate-fills risk:
TradeDeskAgent.execute_action()can callPaperBroker.execute()again if an approval is replayed; reviewer wants a stable idempotency guard. - A2A registry staleness:
register_agent()may no-op on existing IDs, leaving seeded records likex-agentwithout updated fields such askind. - Unresolved lint/docs items: markdown fences in
README.md/docs/AGENTS.md, plus severalruffwarnings about type hints / exception handling / branch complexity.
- Prompt injection risk:
- Key implementation facts confirmed by the diff:
listener.pynow routes mentions throughMentionContext -> route_mention() -> handle_mention(), then postsreply.textand optionalreply.card.mcp_dispatcher.pynow fetches the timeline item, resolves the owning member viafind_member(agent_id), and callsexecute_action()before falling back to Grok.agents/broker.pystores simulated fills in a local JSON ledger;PaperBroker.execute()normalizes ticker/side andpositions()aggregates net quantity.
🔇 Additional comments (4)
mcp_dispatcher.py (2)
142-160: Still no idempotency guard on the approve/reject execution path.Fetch failures are handled now, fine. But
owner.execute_action(item, action)at line 150 can still be replayed on crash/retry before whatever marks the item "done" — same trade gets filled twice, same purchase intent double-logged. This was called out before and it's still not fixed. Ledger's a JSON file right now, blast radius is small, but the pattern's the same one that'll bite in prod with a real broker.
35-44: LGTM!agents/team/tradedesk.py (1)
101-119: Duplicate approvals still double-fill, and anyone can trigger approve/reject.Coercion on ticker/side/quantity is fine as far as it goes, but the two real problems from last round are untouched:
broker.execute()on line 115 fires unconditionally on approve with no idempotency key, and there's still no check that the approver matchesauthor_idfrom the card metadata. Retry/replay on that PATCH still means duplicate fills; unauthenticated approve still means anyone can execute someone else's trade proposal.timeline_server.py (1)
2-2: LGTM!Also applies to: 44-45
…injection-guarded prompts Address CodeRabbit review on #91: - PaperBroker: idempotency key (card id) so replayed/double approvals never double-fill; atomic ledger writes via os.replace; cross-process flock; corrupt ledgers preserved as .corrupt-* backups instead of silently discarded - Tradedesk: passes the card id as fill key, reports duplicate approvals; validates side in {buy,sell} and quantity > 0 before executing - Dispatcher: replay guard via processed_action metadata flag so a crash between execute and watermark save can't re-run a side effect - Listener: mention flow extracted to process_mention(); the approval card is pushed before the X reply, and a failed card push stops the watermark so the mention retries next poll instead of losing the card - Prompts: mention text wrapped in delimited untrusted-content blocks (shopping/research/general); Grok client gets a hard XAI_TIMEOUT_SECONDS deadline (default 120s) - Registry: thread-safe team singleton; fallback selected by empty-handle marker instead of roster position; A2A registration retries with backoff - Router: earliest @tag in the text wins for multi-handle mentions - send_a2a_message returns bool instead of raising on bus outages - a2a_store: re-registration updates mutable fields (stale seeded records now pick up kind) - Makefile: make test bootstraps the venv; README/docs fences tagged - Tests: double-approval, invalid side/quantity, duplicate fill key, corrupt-ledger preservation, multi-handle routing (23 total) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
| # "agent" (interactive, LLM-backed) or "bot" (deterministic executor) | ||
| kind: Optional[Literal["agent", "bot"]] = "agent" | ||
| tags: List[str] = Field(default_factory=list) |
| # Classification: "agent" (interactive, LLM-backed, autonomous) vs | ||
| # "bot" (deterministic function executor). | ||
| "kind": payload.get("kind", "agent"), | ||
| "tags": payload.get("tags", []), |
| "actions": card.get("actions", []), | ||
| "metadata": card.get("metadata", {}), | ||
| } | ||
| requests.post(f"{timeline_url}/v1/timeline/items", json=payload, timeout=10) |
| if len(text) <= limit: | ||
| return text | ||
| return text[:240].rsplit(" ", 1)[0] + suffix |
… null, truncation honors limit - push_timeline_card raises on 4xx/5xx so a failed card push holds the mention watermark instead of being treated as success - timeline_server: kind is non-Optional; explicit null is rejected - a2a_store: kind normalized to agent/bot with safe default at the store - truncate_for_reply uses its limit parameter instead of hardcoding 240 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mcp_dispatcher.py (2)
157-171: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed for owned cards instead of falling back to generic Grok.
If a card has
agent_idbutfind_member()fails orexecute_action()returnsNone, Line 171 sends the action to the legacy MCP workflow. That bypasses the member’s safety policy, including paper-only trades and shopping-intent-only behavior. Only use the legacy fallback for cards with noagent_id.Based on learnings: trades must remain paper-only, shopping approvals are intents only, and side-effecting actions must be approval-gated through the owning member before execution.
🤖 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 `@mcp_dispatcher.py` around lines 157 - 171, The fallback in the action execution path is too permissive for owned cards, since mcp_dispatcher’s owned-item branch can still drop into the legacy Grok workflow when find_member() fails or execute_action() returns None. Update the logic around owner.execute_action so that if item_meta has an agent_id, the code fails closed and does not call call_grok at all; reserve the legacy fallback only for items without an agent_id. Keep the handling inside the mcp_dispatcher action flow and the owner/find_member/execute_action branch so the member’s safety policy remains the only execution path for owned cards.Source: Learnings
138-163: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDon’t execute timeline approvals without an approver check.
timeline_server.pyemitstimeline_actionwith onlytimeline_item_idandaction, andmcp_dispatcher.pygoes straight toowner.execute_action()with no actor/ACL validation. Any client that can post to the A2A bus can approve someone else’s card. Require a signed approver identity or check the card author/ACL before dispatch.🤖 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 `@mcp_dispatcher.py` around lines 138 - 163, The structured timeline action handling in mcp_dispatcher.py currently dispatches directly from item_id and action to owner.execute_action without verifying who is allowed to approve it. Update the timeline_action path in the dispatcher to require and validate an approver identity or ACL before calling find_member() and execute_action(), and reject the message if the actor is missing or unauthorized. Use the existing metadata/item handling around get_timeline_item(), item_meta, and owner.execute_action() to locate the fix and enforce the check before any side effect runs.Source: MCP tools
agents/broker.py (1)
95-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
positions()skips the cross-process lock — corrupt-ledger recovery can race and crash.
execute()takes_LEDGER_LOCKand_ledger_file_lock(...).positions()only takes_LEDGER_LOCK. Fine for a single process — useless the moment two processes (dispatcher + whatever else callspositions()) hit a corrupt ledger at the same time._read()'s corruption path doesself.ledger_path.rename(backup)unguarded by the file lock here; second caller's rename target is already gone → unhandledFileNotFoundError, process dies. Cute way to turn "preserve corrupt ledger" into "crash on corrupt ledger" for whoever loses the race. The docstring on_ledger_file_lockliterally says this lock exists so "concurrent dispatchers can't corrupt the ledger" — apply it consistently or don't bother.🔧 Fix
def positions(self) -> Dict[str, float]: totals: Dict[str, float] = {} - with _LEDGER_LOCK: + with _LEDGER_LOCK, _ledger_file_lock(self.ledger_path): fills = self._read()🤖 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 `@agents/broker.py` around lines 95 - 103, The positions() method only uses _LEDGER_LOCK and misses the cross-process protection already used by execute(), so corrupt-ledger recovery can race during _read(). Update positions() to also wrap the ledger access in _ledger_file_lock(...) for the same ledger path, alongside the existing _LEDGER_LOCK, so the rename-and-backup recovery path is serialized consistently. Use the existing positions(), _read(), _LEDGER_LOCK, and _ledger_file_lock symbols to locate the change.
♻️ Duplicate comments (2)
mcp_dispatcher.py (1)
147-176: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThis is still not a pre-side-effect replay guard.
processed_actionis written afterowner.execute_action(), so a crash between Line 161 and Line 174 can replay the same approval. Use a durable idempotency key/state transition before execution, or require every executor path to be idempotent bytimeline_item_id:action.🤖 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 `@mcp_dispatcher.py` around lines 147 - 176, The replay guard in the action handling flow is still happening after the side effect, so a crash before update_timeline_item can re-execute the same approval. Move the idempotency/state transition ahead of owner.execute_action in the main dispatcher flow, or make both the owner.execute_action and fallback call_grok paths explicitly idempotent using a durable key based on item_id and action. Keep the guard logic around processed_action and the item/action handling branch in mcp_dispatcher.py.Source: MCP tools
agents/team/tradedesk.py (1)
103-132: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidation and dedup are fixed. Approver identity still isn't checked — anyone can fill anyone's trade.
Side/quantity validation and the idempotency
keyfix the two prior blockers. Still nothing here binds "who clicked Approve" tometadata["author_id"].execute_action(item, action)takes no approver identity at all, andmcp_dispatcher.py'sowner.execute_action(item, action)call site doesn't pass one either. Today it's paper trades, so worst case is a fake fill in a JSON file — but this is the exact spot that gets wired to a real broker per the module docstring, and right now anyone with timeline access approves anyone else's trade proposal. This was already flagged in a prior review and is still open.Plumb the approver through
execute_action(and the dispatcher call site) and reject on mismatch before touchingself.broker.execute.🤖 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 `@agents/team/tradedesk.py` around lines 103 - 132, The approver identity is not enforced in tradedesk approval flow, so anyone can execute another user’s trade proposal. Update execute_action in tradedesk.py to accept the approver identity, compare it against metadata["author_id"] before calling self.broker.execute, and reject mismatches early with a clear message. Also update the owner.execute_action call site in mcp_dispatcher.py to pass the clicked approver identity through so the authorization check is actually applied.
🤖 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 `@listener.py`:
- Around line 84-91: The mention handling in member.handle_mention currently
treats any exception as successfully processed, which causes the watermark to
advance and drops retryable mentions. Update the exception path in listener.py
around handle_mention so transient handler failures return False instead of
True, and only advance the watermark when processing actually succeeds. If you
need to preserve unrecoverable mentions, route them to a dead-letter path before
returning success.
- Around line 155-161: The watermark update in get_users_mentions processing is
not monotonic because the loop in listener.py saves each mention’s created_at as
it goes while iterating newest-to-oldest, which can move the stored last-seen
timestamp backward. Update the mention-handling flow so process_mention and the
save_last_seen call only persist the maximum successful created_at once after
all successful mentions are processed, or sort the mentions into ascending order
before iterating, so the stored watermark always advances forward.
---
Outside diff comments:
In `@agents/broker.py`:
- Around line 95-103: The positions() method only uses _LEDGER_LOCK and misses
the cross-process protection already used by execute(), so corrupt-ledger
recovery can race during _read(). Update positions() to also wrap the ledger
access in _ledger_file_lock(...) for the same ledger path, alongside the
existing _LEDGER_LOCK, so the rename-and-backup recovery path is serialized
consistently. Use the existing positions(), _read(), _LEDGER_LOCK, and
_ledger_file_lock symbols to locate the change.
In `@mcp_dispatcher.py`:
- Around line 157-171: The fallback in the action execution path is too
permissive for owned cards, since mcp_dispatcher’s owned-item branch can still
drop into the legacy Grok workflow when find_member() fails or execute_action()
returns None. Update the logic around owner.execute_action so that if item_meta
has an agent_id, the code fails closed and does not call call_grok at all;
reserve the legacy fallback only for items without an agent_id. Keep the
handling inside the mcp_dispatcher action flow and the
owner/find_member/execute_action branch so the member’s safety policy remains
the only execution path for owned cards.
- Around line 138-163: The structured timeline action handling in
mcp_dispatcher.py currently dispatches directly from item_id and action to
owner.execute_action without verifying who is allowed to approve it. Update the
timeline_action path in the dispatcher to require and validate an approver
identity or ACL before calling find_member() and execute_action(), and reject
the message if the actor is missing or unauthorized. Use the existing
metadata/item handling around get_timeline_item(), item_meta, and
owner.execute_action() to locate the fix and enforce the check before any side
effect runs.
---
Duplicate comments:
In `@agents/team/tradedesk.py`:
- Around line 103-132: The approver identity is not enforced in tradedesk
approval flow, so anyone can execute another user’s trade proposal. Update
execute_action in tradedesk.py to accept the approver identity, compare it
against metadata["author_id"] before calling self.broker.execute, and reject
mismatches early with a clear message. Also update the owner.execute_action call
site in mcp_dispatcher.py to pass the clicked approver identity through so the
authorization check is actually applied.
In `@mcp_dispatcher.py`:
- Around line 147-176: The replay guard in the action handling flow is still
happening after the side effect, so a crash before update_timeline_item can
re-execute the same approval. Move the idempotency/state transition ahead of
owner.execute_action in the main dispatcher flow, or make both the
owner.execute_action and fallback call_grok paths explicitly idempotent using a
durable key based on item_id and action. Keep the guard logic around
processed_action and the item/action handling branch in mcp_dispatcher.py.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2e055e15-ceef-41b8-9b6e-a0256024ea39
📒 Files selected for processing (17)
MakefileREADME.mda2a_store.pyagents/base.pyagents/broker.pyagents/registry.pyagents/router.pyagents/team/general.pyagents/team/research.pyagents/team/shopping.pyagents/team/tradedesk.pydocs/AGENTS.mdlistener.pymcp_dispatcher.pytests/test_broker.pytests/test_router.pytests/test_tradedesk.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
⚠️ CI failures not shown inline (2)
GitHub Actions: 🤖 Autonomous Agent CI/CD / 🏷️ Auto-Label PRs: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
GitHub Actions: 🤖 Autonomous Agent CI/CD / 1_🏷️ Auto-Label PRs.txt: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Run MyXstack as a team of handle-addressable members on the A2A layer, where tagging a member in a mention routes the request to that member via the listener/router.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Classify each member by `kind` in its profile and the A2A registry: `agent` for conversational, LLM-backed interactive agents; `bot` for deterministic API bots.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Members may message each other directly on the A2A bus using `POST /v1/a2a/messages` via `agents.base.send_a2a_message()`, allowing interactive agents to drive sub-agents and bots.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Trades must remain paper-only: `PaperBroker` records simulated fills locally and must not place real exchange orders.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Shopping approvals are intents only: approving a shopping card records the intent, but no payment adapter is wired in by default.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: All side-effecting actions must be approval-gated on the timeline before the dispatcher executes them.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:32:00.173Z
Learning: Configure member handles through `TRADEDESK_HANDLE`, `RESEARCH_HANDLE`, `SHOPPING_HANDLE`, and `TICKERBOT_HANDLE` in `.env`.
🪛 ast-grep (0.44.0)
agents/router.py
[warning] 24-24: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(r"@" + re.escape(handle) + r"\b", lowered)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
agents/broker.py
[warning] 30-30: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(lock_file, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 62-62: use jsonify instead of json.dumps for JSON output
Context: json.dumps(fills, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
agents/registry.py
[warning] 74-74: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(f"{timeline_url}/v1/a2a/agents", json=payload, timeout=10)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
agents/base.py
[warning] 131-141: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{timeline_url}/v1/a2a/messages",
json={
"from": from_agent,
"to": to,
"type": message_type,
"content": content,
"metadata": metadata or {},
},
timeout=10,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 Ruff (0.15.20)
agents/broker.py
[warning] 15-15: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 15-15: typing.List is deprecated, use list instead
(UP035)
[warning] 21-21: Missing return type annotation for private function _ledger_file_lock
(ANN202)
agents/registry.py
[warning] 30-30: Using the global statement to update _TEAM is discouraged
(PLW0603)
agents/base.py
[warning] 143-143: Consider moving this statement to an else block
(TRY300)
listener.py
[warning] 86-86: Do not catch blind exception: Exception
(BLE001)
[warning] 96-96: Do not catch blind exception: Exception
(BLE001)
[warning] 108-108: Do not catch blind exception: Exception
(BLE001)
🔍 Remote MCP GitHub Copilot
Additional review context for PR #91
- The PR is open on
groupthinking/MyXstackintomain, with 24 changed files, 1177 additions / 86 deletions, andmergeable_state: unstable. - GitHub validation flagged the PR title format and the PR size as large.
- Copilot review ultimately marked the PR CHANGES_REQUESTED on the latest run.
Still-relevant review risks
- Prompt injection remains in
agents/team/shopping.py,agents/team/research.py, andagents/team/general.py:mention.textis still interpolated directly into Grok prompts; CodeRabbit explicitly called this out. - Trade approval replay/idempotency is still unresolved: Copilot’s latest review says
TradeDeskAgent.execute_action()can still double-fill on repeated approvals, and it also notes there is still no check that the approver matchesauthor_id. - Dispatcher replay protection is still incomplete:
mcp_dispatcher.pynow fetches timeline items and marksprocessed_action, but Copilot says there is still no idempotency guard beforeowner.execute_action(). - Routing behavior is now earliest-tag-wins:
agents/router.pyscans all handles and selects the one whose@handleappears earliest in the text, case-insensitively and word-bounded. - A2A registry updates now merge existing records:
register_agent()updates mutable fields, includingkind, instead of returning early on duplicate IDs. kindis constrained toagent | botintimeline_server.py.
Review/supporting test coverage
- Added tests cover routing, broker fill recording, duplicate key handling, corrupt-ledger recovery, trade parsing, approve/reject execution, and
kindclassification. - Copilot’s comments still ask for a multi-handle routing test and stronger duplicate-execution coverage at the PR-review level.
🔇 Additional comments (15)
Makefile (1)
30-33: Venv bootstrap fixed, past complaint resolved.
test -d .venv || python3 -m venv .venvfinally stops this from face-planting on a clean checkout. Previous review flagged exactly this. Good, ship it.One nit nobody will fix:
pip install -qswallows install failures into silence — if a dep is broken you get a crypticModuleNotFoundErrorfrom pytest instead of a clear pip error. Not blocking.docs/AGENTS.md (1)
1-88: LGTM!tests/test_router.py (1)
41-47: LGTM!agents/base.py (2)
119-146: 🔒 Security & PrivacyTry/except fixed, auth gap from the earlier review is still open.
Graceful-failure handling landed — good,
handle_mentioncallers won't die on a bus hiccup anymore. But the second half of the earlier comment is untouched: this still POSTsfrom/to/content/metadatato/v1/a2a/messageswith zero credentials. Same story inregister_team(). IfTIMELINE_API_URLis reachable by anything besides localhost, any client can forge messages and impersonate a team member on the A2A bus. Needs confirmation the timeline server enforces auth server-side, because nothing client-side is doing it.
71-116: LGTM!agents/registry.py (3)
57-81: 🔒 Security & PrivacyRetry/backoff lands, auth gap doesn't.
3x retry with backoff before giving up fixes the startup-ordering race — that's the fix that was asked for. Same unresolved auth gap as
send_a2a_messageinagents/base.pythough: this POSTsid/name/description/kindto/v1/a2a/agentswith no credentials. One root cause, two call sites — see the comment onagents/base.py.
26-35: LGTM!
38-45: LGTM!agents/router.py (1)
9-28: LGTM!agents/team/general.py (1)
19-53: LGTM!a2a_store.py (1)
84-109: LGTM!agents/broker.py (1)
1-93: LGTM!tests/test_broker.py (1)
21-38: LGTM!agents/team/tradedesk.py (1)
64-102: LGTM!tests/test_tradedesk.py (1)
72-106: LGTM!
…atch Address CodeRabbit re-review: - listener processes mentions oldest-first (X returns newest-first), so the last-seen watermark can never move backwards and replay mentions - a handler crash now files a dead-letter card on the timeline instead of silently dropping the mention; if even that card can't land, the watermark holds and the mention retries next poll - dispatcher fails closed for member-owned cards: an unhandled action on an owned card never falls through to the generic Grok executor - PaperBroker.positions() takes the cross-process file lock like execute(), so corrupt-ledger recovery can't race between processes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
| if item and action and item_meta.get("processed_action") == action: | ||
| # Replay guard: a crash between execute and save_last_seen | ||
| # re-delivers the message; don't run the side effect twice. | ||
| print( | ||
| f"Skipping already-processed '{action}' on item {item_id}", | ||
| flush=True, | ||
| ) | ||
| last_seen = created_at or datetime.now(timezone.utc) | ||
| save_last_seen(last_seen.isoformat()) | ||
| continue |
| for attempt in range(3): | ||
| try: | ||
| requests.post(f"{timeline_url}/v1/a2a/agents", json=payload, timeout=10) | ||
| break | ||
| except requests.RequestException as exc: | ||
| if attempt == 2: | ||
| print(f"Could not register agent {profile.id}: {exc}", flush=True) | ||
| else: | ||
| time.sleep(2**attempt) |
| ticker = str(metadata.get("ticker", "?")).upper() | ||
| side = str(metadata.get("side", "buy")).lower() | ||
| if side not in ("buy", "sell"): | ||
| return f"⚠️ Invalid side '{side}' on trade card {item.get('id', '?')}; nothing executed." |
| try: | ||
| client.create_tweet( | ||
| text=reply.text[:280], | ||
| in_reply_to_tweet_id=mention.id, | ||
| ) | ||
| except Exception as exc: | ||
| print(f"Error replying to mention {mention.id}: {exc}", flush=True) | ||
| return True |
…er format check, surfaced dropped replies
Address Copilot re-review round 2:
- dispatcher treats any processed_action as terminal, so a late Reject
after an executed Approve (or vice versa) is ignored instead of run
- register_team raises on 4xx/5xx so registration retries during
timeline-server boot instead of silently accepting an error response
- Tradedesk validates ticker against ^[A-Z]{1,10}$ before executing —
a missing/None ticker can no longer reach the broker
- a failed reply tweet is surfaced as a timeline card so operators can
recover it (the mention itself is not retried; its card already landed)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agents/base.py (1)
86-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStop hard-coding the suffix budget.
Line 92 subtracts
30instead oflen(suffix), so the default suffix can already overshootlimit, and custom suffixes make this helper lie about its cap.Fix the budget calculation
- return text[: max(0, limit - 30)].rsplit(" ", 1)[0] + suffix + if len(suffix) >= limit: + return suffix[:limit] + budget = limit - len(suffix) + prefix = text[:budget].rsplit(" ", 1)[0] or text[:budget] + return prefix + suffix🤖 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 `@agents/base.py` around lines 86 - 92, The truncate_for_reply helper is hard-coding the suffix budget instead of using the actual suffix length, so fix the truncation calculation to subtract len(suffix) in the return path rather than 30. Update the logic in truncate_for_reply to keep the final string within limit for both the default suffix and any custom suffix values.mcp_dispatcher.py (1)
145-176: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when the timeline item can’t be loaded.
If
item_idis present butget_timeline_item()returnsNone, ownership is unknown and Line 176 falls into the generic Grok executor. That bypasses the member policy exactly when the dispatcher has lost the card context.Block generic fallback when item context is missing
result = None item = get_timeline_item(item_id) if item_id else None + if item_id and item is None: + result = f"Could not load timeline item {item_id}; nothing executed." item_meta = (item.get("metadata") or {}) if item else {}Based on learnings, trades must remain paper-only through
PaperBrokerand purchases are intents only, so owned-card actions must not fall back to generic execution.🤖 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 `@mcp_dispatcher.py` around lines 145 - 176, The dispatcher in mcp_dispatcher.py should fail closed when an item_id is present but get_timeline_item() returns None, instead of allowing the generic Grok fallback. Update the action handling around item, item_meta, owned_agent_id, and the result flow so missing item context blocks execution for owned cards, matching the existing fail-closed behavior used when owner.execute_action() returns None. Preserve the current processed_action replay guard, but ensure fallback to the generic executor only happens when there is no item_id/ownership context at all.Source: Learnings
🤖 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 `@mcp_dispatcher.py`:
- Around line 161-164: The member execution path in mcp_dispatcher.py is
incorrectly treating execute_action() exceptions as a normal result, which
allows failed actions to be persisted as processed. Update the logic around
owner.execute_action() so only successful executions reach the processed_action
persistence in the later block, and ensure failures from the exception path do
not record or advance the action state. Use the execute_action and
processed_action flow in the same dispatcher branch to keep failed approvals
retryable instead of marking them done.
---
Outside diff comments:
In `@agents/base.py`:
- Around line 86-92: The truncate_for_reply helper is hard-coding the suffix
budget instead of using the actual suffix length, so fix the truncation
calculation to subtract len(suffix) in the return path rather than 30. Update
the logic in truncate_for_reply to keep the final string within limit for both
the default suffix and any custom suffix values.
In `@mcp_dispatcher.py`:
- Around line 145-176: The dispatcher in mcp_dispatcher.py should fail closed
when an item_id is present but get_timeline_item() returns None, instead of
allowing the generic Grok fallback. Update the action handling around item,
item_meta, owned_agent_id, and the result flow so missing item context blocks
execution for owned cards, matching the existing fail-closed behavior used when
owner.execute_action() returns None. Preserve the current processed_action
replay guard, but ensure fallback to the generic executor only happens when
there is no item_id/ownership context at all.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 45cafbfa-a288-432c-913b-e0a38637cdf8
📒 Files selected for processing (6)
a2a_store.pyagents/base.pyagents/broker.pylistener.pymcp_dispatcher.pytimeline_server.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
⚠️ CI failures not shown inline (2)
GitHub Actions: 🤖 Autonomous Agent CI/CD / 3_🏷️ Auto-Label PRs.txt: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
GitHub Actions: 🤖 Autonomous Agent CI/CD / 🏷️ Auto-Label PRs: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: Tag the intended member handle in a mention so the listener can route the request to that member over A2A.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: Classify each member by `kind` in its profile and in the A2A registry, using `agent` for interactive LLM-backed members and `bot` for deterministic API executors.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: Use the A2A bus (`POST /v1/a2a/messages`) and `agents.base.send_a2a_message()` for member-to-member communication and delegation.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: Trades must remain paper-only; executions should go through `PaperBroker`, which records simulated fills locally and never sends live orders.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: Purchases are intents only, and side-effecting actions must be approval-gated before the dispatcher executes them.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:45:16.987Z
Learning: If members should run under separate real X handles in the future, account for the configurable handle environment variables (`TRADEDESK_HANDLE`, `RESEARCH_HANDLE`, `SHOPPING_HANDLE`, `TICKERBOT_HANDLE`) in `.env`.
🪛 Ruff (0.15.20)
mcp_dispatcher.py
[warning] 163-163: Do not catch blind exception: Exception
(BLE001)
listener.py
[warning] 112-112: Consider moving this statement to an else block
(TRY300)
[warning] 113-113: Do not catch blind exception: Exception
(BLE001)
🔍 Remote MCP GitHub Copilot
Additional PR review context
- The latest Copilot review is still pending on commit
93f717c..., so the review is not finished yet. The PR also has a pending CodeRabbit status. - New unresolved review comments flag:
timeline_server.py:kindcan still becomenull/ invalid unless it is strictly constrained and normalized.a2a_store.py:register_agent()still storeskinddirectly from payload, soNoneor arbitrary strings can be persisted.listener.py:push_timeline_card()failure handling may miss non-2xx responses becauserequests.post()does not raise by default, which can advance the watermark and drop a card.agents/base.py:truncate_for_reply()is reported to ignore itslimitparameter.
- A still-open functional risk remains in the trade path:
TradeDeskAgent.execute_action()is still described as needing an idempotency guard so repeated approvals don’t double-fill. - The earlier batch of CodeRabbit findings shows several items were addressed in commit
2380fea, includingget_timeline_item()exception handling,send_a2a_message()failure handling, routing by earliest handle occurrence, thread-safeget_team(), and the duplicate-fill regression test.
🔇 Additional comments (4)
timeline_server.py (1)
44-46: LGTM!a2a_store.py (1)
91-107: LGTM!agents/broker.py (1)
97-100: LGTM!listener.py (1)
61-64: LGTM!Also applies to: 94-114, 178-185
…reply budget Address CodeRabbit review of 93f717c: - dispatcher no longer marks processed_action when execute_action raised or the timeline item couldn't be loaded, so a transient broker/ledger failure can't consume an approval; missing item context also fails closed instead of reaching the generic executor - truncate_for_reply budgets by len(suffix) instead of a hardcoded 30, so replies never exceed the requested limit for any suffix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
| if result is None: | ||
| result = ( | ||
| f"Action '{action}' not handled by agent {owned_agent_id}; " | ||
| "nothing executed (owned cards never use the generic fallback)." | ||
| ) |
The dispatcher fails closed for member-owned cards, which silently no-op'd Approve/Reject/Snooze on the general agent's mention cards (agent_id x-agent, no execute_action). GeneralAgent now runs the legacy generic Grok workflow itself, restoring pre-team behavior without weakening fail-closed for trade/purchase cards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcp_dispatcher.py (1)
184-198: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t consume the action when Grok never ran.
call_grok()returns"Missing XAI_API_KEY."without executing anything, but Line 197 still writesprocessed_action. That burns the timeline action even though the legacy executor did nothing. Setexecution_failed = Truefor that failure path before updating the item.Fix the failure marker before persisting action state
- result = call_grok(prompt) + result = call_grok(prompt) + if result == "Missing XAI_API_KEY.": + execution_failed = True🤖 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 `@mcp_dispatcher.py` around lines 184 - 198, In the legacy Grok fallback inside the MCP dispatcher flow, `call_grok()` can return the “Missing XAI_API_KEY.” failure without actually running anything, but `update_timeline_item()` still records `processed_action`. Update the `result is None` fallback path in `mcp_dispatcher.py` to detect that missing-key response and set `execution_failed = True` before building the `update` dict, so the `processed_action` field is not persisted when Grok never executed.agents/team/tradedesk.py (1)
116-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject non-finite quantities in
agents/team/tradedesk.py:116-120.float()acceptsnanandinf, andquantity <= 0still lets bothnanand+infthrough.PaperBroker.execute()writes that value straight into the JSON ledger, andpositions()later sums it, so one bad card poisons local fills and position totals. Addmath.isfinite(quantity)beforebroker.execute().🤖 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 `@agents/team/tradedesk.py` around lines 116 - 120, The trade quantity validation in tradedesk should reject non-finite values, because float(metadata.get("quantity", 1)) can produce nan or inf and the existing quantity <= 0 check still allows them through. Update the validation in the quantity parsing flow before PaperBroker.execute() is called so it also checks math.isfinite(quantity), alongside the existing TypeError/ValueError handling and positive-quantity check, using the nearby symbols execute(), positions(), and the quantity assignment block to locate the fix.Source: Learnings
♻️ Duplicate comments (1)
agents/team/tradedesk.py (1)
122-128: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApproval still has no actor check before the fill.
author_idis stored on the card, butexecute_action()never receives or verifies the user who clicked Approve. Anytimeline_actionthat reaches this path can executebroker.execute(). Pass the approver identity from the timeline action and reject anything outside the requester/authorized approver set before Line 128.#!/bin/bash set -euo pipefail rg -n -C3 'author_id|approver|actor|timeline_action|execute_action|broker\.execute' \ timeline_server.py mcp_dispatcher.py agents/team/tradedesk.py agents 2>/dev/null || true🤖 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 `@agents/team/tradedesk.py` around lines 122 - 128, Approval handling in execute_action() and the broker.execute() path currently lacks an actor authorization check, so any timeline_action can trigger a fill. Update the tradedesk approval flow to accept the approver identity from the timeline action, compare it against the stored author_id/requester or other authorized approver set before the fill is submitted, and reject unauthorized approvals before calling self.broker.execute(). Use the execute_action() method and the broker.execute() callsite as the main anchors when updating the approval logic.Source: MCP tools
🤖 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 `@listener.py`:
- Around line 136-151: The recovery path in listener.py is swallowing failures
from push_timeline_card, which can let create_tweet() failures fall through and
be treated as handled. Update the failure handling around the push_timeline_card
call in the reply-posting flow so that if the safety card cannot be created and
no earlier safety card was successfully pushed, the code logs the exception and
returns False instead of continuing to the final success path in the mention
handling logic. Use the existing create_tweet(), push_timeline_card(), and
mention/reply handling branch to locate the fix.
---
Outside diff comments:
In `@agents/team/tradedesk.py`:
- Around line 116-120: The trade quantity validation in tradedesk should reject
non-finite values, because float(metadata.get("quantity", 1)) can produce nan or
inf and the existing quantity <= 0 check still allows them through. Update the
validation in the quantity parsing flow before PaperBroker.execute() is called
so it also checks math.isfinite(quantity), alongside the existing
TypeError/ValueError handling and positive-quantity check, using the nearby
symbols execute(), positions(), and the quantity assignment block to locate the
fix.
In `@mcp_dispatcher.py`:
- Around line 184-198: In the legacy Grok fallback inside the MCP dispatcher
flow, `call_grok()` can return the “Missing XAI_API_KEY.” failure without
actually running anything, but `update_timeline_item()` still records
`processed_action`. Update the `result is None` fallback path in
`mcp_dispatcher.py` to detect that missing-key response and set
`execution_failed = True` before building the `update` dict, so the
`processed_action` field is not persisted when Grok never executed.
---
Duplicate comments:
In `@agents/team/tradedesk.py`:
- Around line 122-128: Approval handling in execute_action() and the
broker.execute() path currently lacks an actor authorization check, so any
timeline_action can trigger a fill. Update the tradedesk approval flow to accept
the approver identity from the timeline action, compare it against the stored
author_id/requester or other authorized approver set before the fill is
submitted, and reject unauthorized approvals before calling
self.broker.execute(). Use the execute_action() method and the broker.execute()
callsite as the main anchors when updating the approval logic.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9dead3f6-3d47-4854-ba58-b81c44fdd21d
📒 Files selected for processing (5)
agents/base.pyagents/registry.pyagents/team/tradedesk.pylistener.pymcp_dispatcher.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
⚠️ CI failures not shown inline (2)
GitHub Actions: 🤖 Autonomous Agent CI/CD / 🏷️ Auto-Label PRs: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
GitHub Actions: 🤖 Autonomous Agent CI/CD / 2_🏷️ Auto-Label PRs.txt: feat: add @handle-routed agent team (interactive agents + API bots)
Conclusion: failure
##[group]Run actions/labeler@v5
with:
repo-***REDACTED***
configuration-path: .github/labeler.yml
sync-labels: false
dot: true
##[endgroup]
The configuration file (path: .github/labeler.yml) was not found locally, fetching via the api
##[warning]The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
##[error]HttpError: Not Found
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:54:18.822Z
Learning: Trades are paper-only: `PaperBroker` must record simulated fills locally and must not send real exchange orders.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:54:18.822Z
Learning: Purchases are approval-gated intents only; no payment adapter is wired in by default.
Learnt from: CR
Repo: groupthinking/MyXstack
Timestamp: 2026-07-01T23:54:18.822Z
Learning: All side-effecting actions must be approval-gated before the dispatcher executes them.
🪛 Ruff (0.15.20)
listener.py
[warning] 136-151: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 150-151: try-except-pass detected, consider logging the exception
(S110)
[warning] 150-150: Do not catch blind exception: Exception
(BLE001)
🔍 Remote MCP GitHub Copilot
Relevant PR context
- PR
#91ingroupthinking/MyXstackis open onclaude/myxstack-agent-merge-myaockintomain, with 24 files changed andmergeable_state: unstable. - Copilot review status is not clean: the PR has multiple
CHANGES_REQUESTEDreviews and the latest review on commit93f717c...is still pending/active.
Open review risks still visible from the tool output
agents/team/tradedesk.py: trade approval still needs stronger safety checks; Copilot flags unresolved concerns around duplicate execution/idempotency, approver identity, and validation ofticker/side/quantity.mcp_dispatcher.py: review comments still flag replay/idempotency issues and a fail-closed concern when timeline item context is missing.listener.py: review comments still flag watermark/mention-loss risks when reply or card posting fails.agents/base.py:truncate_for_reply()is still flagged for not honoring itslimitcorrectly.timeline_server.pyanda2a_store.py:kindhandling is still flagged as needing stricter normalization/validation to preventnullor arbitrary strings.
What appears addressed already
- Earlier comments marked addressed in commit
2380feainclude: earliest-handle routing inagents/router.py, thread-safeget_team(), retries inregister_team(),send_a2a_message()failure handling,get_timeline_item()exception handling, and duplicate-fill regression coverage.
🔇 Additional comments (2)
agents/base.py (1)
92-96: LGTM!agents/registry.py (1)
75-79: LGTM!
…nite quantities Address CodeRabbit review of 411647f: - listener logs recovery-card failures and holds the watermark for reply-only mentions whose reply AND recovery card both failed, so they retry instead of vanishing - dispatcher doesn't mark processed_action when the legacy Grok fallback returned its missing-API-key sentinel without executing anything - Tradedesk rejects non-finite quantities (nan/inf) so a bad card can't poison the paper ledger or position totals Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
| timeline_url = os.getenv("TIMELINE_API_URL", "http://127.0.0.1:8080") | ||
| try: | ||
| requests.post( | ||
| f"{timeline_url}/v1/a2a/messages", | ||
| json={ | ||
| "from": from_agent, | ||
| "to": to, | ||
| "type": message_type, | ||
| "content": content, | ||
| "metadata": metadata or {}, | ||
| }, | ||
| timeout=10, | ||
| ) | ||
| return True | ||
| except requests.RequestException as exc: | ||
| print(f"Could not send A2A message to {to}: {exc}", flush=True) | ||
| return False |
| def parse_trade_command(text: str) -> Optional[Dict[str, Any]]: | ||
| match = _TICKER_FIRST.search(text) or _SIDE_FIRST.search(text) | ||
| if not match: | ||
| return None | ||
| return { | ||
| "ticker": match.group("ticker").upper(), | ||
| "side": match.group("side").lower(), | ||
| "quantity": float(match.group("qty")) if match.group("qty") else 1.0, | ||
| } |
…ero/infinite quantities Address Copilot re-review: - send_a2a_message raises on 4xx/5xx so callers can't mistake a rejected message for a delivered one - parse_trade_command rejects quantities that are zero or float to inf (huge digit strings), which would otherwise break JSON card serialization or produce invalid proposals Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
🔍 PR Validation |
Summary
Mentions are now routed to a team of @handle-addressable members. Tag a member anywhere in a mention and the listener dispatches to it:
Classification: interactive agents vs API bots
Every member carries a
kindin its profile and in the A2A registry:agent(interactive agent) — conversational, LLM-backed via Grok + MCP tools, can run sub-steps, delegate to other members over the A2A bus, and propose approval-gated actions.bot(API bot) — deterministic function executor: input → function → output. No LLM, no autonomy.@TickerBotis the reference implementation.How it works
agents/router.pymatches registered @Handles in mention text (case-insensitive, word-bounded); untagged mentions fall back to aGeneralAgentthat preserves the original generic Grok behavior.listener.pyroutes each mention to a member, posts its reply, and pushes its timeline card (cards carryagent_idin metadata).mcp_dispatcher.pynow looks up the card's owning member on Approve/Reject and calls itsexecute_action()before falling back to the legacy generic Grok path.a2a_store.py/timeline_server.pystore the newkindclassification, and all team members self-register in the A2A registry at listener startup.Safety defaults
PaperBrokerrecords simulated fills in a local JSON ledger; no live orders. A real broker is a pluggable adapter behind the sameexecute()interface.Testing
make testtarget +requirements-dev.txt+pytest.iniadded.requirements.txt.Docs
docs/AGENTS.md: roster, agent-vs-bot classification, request flow diagram, how to add a member, X API Exhibit note.env.example: configurable member handles and paper-ledger path.🤖 Generated with Claude Code
https://claude.ai/code/session_01FTLFzAjoSosiPfHGaXSZkP
Generated by Claude Code