fix: A2A send reports HTTP errors as failures, trade parser rejects zero/infinite quantities

[groupthinking]

No description provided.

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1eece840-2d47-4216-a50e-f2c275fd4126

📥 Commits

Reviewing files that changed from the base of the PR and between 8005bb6 and 833f189.

📒 Files selected for processing (24)

• Makefile
• README.md
• a2a_store.py
• agents/__init__.py
• agents/base.py
• agents/broker.py
• agents/registry.py
• agents/router.py
• agents/team/__init__.py
• agents/team/general.py
• agents/team/research.py
• agents/team/shopping.py
• agents/team/tickerbot.py
• agents/team/tradedesk.py
• docs/AGENTS.md
• env.example
• listener.py
• mcp_dispatcher.py
• pytest.ini
• requirements-dev.txt
• tests/test_broker.py
• tests/test_router.py
• tests/test_tradedesk.py
• timeline_server.py

 _____________________________________
< No code too small, no bug too tall. >
 -------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• 🔄 Committing to branch...

🧪 Generate unit tests (beta)

✅ Unit Tests committed locally.

• Create PR with unit tests
• Commit unit tests in branch claude/myxstack-agent-merge-myaock

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/myxstack-agent-merge-myaock

---

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

[coderabbitai]

Note

Docstrings generation - SUCCESS
Generated docstrings and committed to branch claude/myxstack-agent-merge-myaock (commit: 5635c781c66d5178ff61f49c620bacb20c72529c)

[coderabbitai]

Note

Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[Copilot]

Pull request overview

This PR introduces an @handle-routed “agent team” architecture for the Python listener/dispatcher pipeline, adds a paper-trading TradeDesk agent (with stricter trade parsing), and hardens approval-gated workflows so HTTP failures don’t silently drop safety-critical timeline artifacts.

Changes:

• Add a team registry/router and multiple members (TradeDesk/Research/Shopping/TickerBot + fallback GeneralAgent) with kind classification (agent vs bot).
• Make mention processing safety-first by pushing timeline cards before X replies and surfacing timeline HTTP failures as hard failures (retry by holding watermark).
• Add paper broker + TradeDesk parsing/execution semantics and a pytest-based Python test suite.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
timeline_server.py	Adds kind to agent registration payload validation (agent vs bot).
a2a_store.py	Normalizes/persists kind and updates existing agent records on re-registration.
mcp_dispatcher.py	Routes approved actions to owning team members; adds processed-action tracking and timeline item lookup.
listener.py	Routes mentions to team members; makes timeline card push fail-fast; processes mentions oldest-first with watermark semantics.
agents/base.py	Introduces core team types (profiles/replies), Grok helper, and A2A send helper with HTTP error raising.
agents/registry.py	Defines team roster, routing helpers, and timeline A2A registration with retry/backoff.
agents/router.py	Implements earliest-@handle routing with case-insensitive, word-bounded matching.
agents/broker.py	Adds a paper broker with idempotent fills + ledger locking and recovery of corrupt ledgers.
agents/team/tradedesk.py	Adds deterministic trade parsing (rejects 0/inf qty), approval card creation, and approval execution via PaperBroker.
agents/team/tickerbot.py	Adds deterministic “bot” member for cashtag lookup.
agents/team/research.py	Adds research member that replies concisely + logs full brief to timeline.
agents/team/shopping.py	Adds shopping member that logs approval-gated purchase intents (no executor).
agents/team/general.py	Adds fallback general Grok-backed behavior for untagged mentions.
agents/init.py	Initializes agents package.
agents/team/init.py	Initializes agents.team package.
tests/test_tradedesk.py	Adds unit tests for TradeDesk parsing, card creation, and execution idempotency.
tests/test_router.py	Adds routing tests (case-insensitive, word boundary, earliest tag wins, kind classification).
tests/test_broker.py	Adds tests for PaperBroker fills, positions aggregation, idempotency keying, and corrupt-ledger preservation.
requirements-dev.txt	Adds dev requirements for running pytest.
pytest.ini	Configures pytest discovery and python path.
Makefile	Adds make test target to provision venv and run pytest.
README.md	Documents the agent team concept and routing examples.
env.example	Adds agent team configuration env vars (handles, paper ledger path, Grok toggle).
docs/AGENTS.md	Adds detailed documentation for team members, classification, flow, and safety defaults.

Comments suppressed due to low confidence (2)

mcp_dispatcher.py:201

• The dispatcher records processed_action via update_timeline_item(), but requests.patch() does not raise on 4xx/5xx. If the timeline server returns an HTTP error, this code will still advance the last-seen watermark and the action may be effectively dropped without any signal. Consider performing the PATCH here with raise_for_status() (or updating update_timeline_item() to do so) and at least logging failures.

                    try:
                        result = owner.execute_action(item, action)
                    except Exception as exc:
                        execution_failed = True

mcp_dispatcher.py:207

• send_message() posts the A2A result but does not check for HTTP errors. Since requests.post() won’t raise on 4xx/5xx, delivery failures can be silently ignored while the dispatcher advances its watermark. Consider issuing the POST with raise_for_status() (or updating send_message() accordingly) so failed deliveries are at least logged.

                # Fail closed: a card owned by a member must never fall
                # through to the generic Grok executor, or the member's
                # safety policy (paper-only trades, intent-only purchases)
                # would be bypassed.
                if result is None:

[coderabbitai]

Actionable comments posted: 19

🤖 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 `@a2a_store.py`:
- Around line 91-107: Update DEFAULT_AGENTS to include kind: "agent" for every
seeded record, and modify _read_store to normalize existing persisted agents by
assigning "agent" when kind is missing or invalid, then persist the normalized
data before returning it. Ensure all registry reads return agents with a valid
kind even when they never re-register.

In `@agents/broker.py`:
- Around line 20-37: Update _ledger_file_lock so it retains cross-process
serialization when fcntl is unavailable; replace the current unlocked yield with
a portable inter-process locking mechanism, or explicitly enforce single-process
broker operation on those platforms. Preserve the existing lock-file lifecycle
and exclusive critical-section behavior.
- Around line 44-58: Update _read() to validate that parsed JSON is a list of
valid fill objects with the required schema before returning it. Route non-list
values and malformed list entries through the same corrupt-ledger backup path
used for JSONDecodeError, preserving the original content and returning an empty
ledger; ensure valid fill lists remain unchanged.

In `@agents/team/general.py`:
- Around line 63-69: Update the prompt passed to grok_chat in the approval
execution flow to include item["body"], which contains the approved mention
stored earlier. Wrap the body with wrap_untrusted() before inserting it into the
executor prompt, while preserving the existing action, title, and concise status
instructions.

In `@agents/team/tradedesk.py`:
- Around line 27-48: Update _TICKER_FIRST and _SIDE_FIRST so the optional
quantity captures the complete numeric token, including exponent notation and
trailing non-whitespace characters, rather than partially matching prefixes. In
parse_trade_command, convert the captured token inside error handling and return
None on conversion failure before applying the existing math.isfinite and
positive checks, ensuring malformed quantities such as 1e309 and 10abc are
rejected.
- Around line 77-83: In handle_mention, wrap the optional grok_chat enrichment
in a targeted exception handler so failures are logged and context falls back to
an empty value; preserve the valid trade proposal and continue the approval flow
instead of propagating the error to listener.py.

In `@listener.py`:
- Around line 155-158: Bound the retry path around the `if not reply.card` check
so an ambiguously failed mention cannot block watermark advancement
indefinitely. Track attempts by mention ID and, after the configured maximum,
treat the mention as handled and allow the caller to advance the watermark;
preserve retries for attempts below the limit and use the existing
duplicate-content success handling if available.
- Around line 50-64: Update push_timeline_card to accept a dedupe_key argument
and include that deterministic key in the POST request so the timeline API can
upsert repeated deliveries. Ensure all callers pass stable mention-based keys,
including the “-error” and “-failed-reply” suffixes for recovery cards, while
preserving the existing status-error handling.
- Around line 76-88: Update the mention handling flow around MentionContext and
route_mention to enforce authorization before invoking the selected member:
allow only the configured owner author ID, or otherwise mark non-owner authors
in the proposal metadata so the UI suppresses the Approve action. Ensure
unauthorized mentions cannot produce an approval-gated trade card, and use the
existing author_id captured in MentionContext.
- Around line 204-211: Update the watermark persistence in the mention polling
flow around process_mention so the saved cursor is exclusive of the last
successfully processed mention, preventing it from being replayed on the next
poll. Preserve oldest-first ordering and stop processing after a failed
process_mention; use the mention ID for deduplication if that is the existing
cursor mechanism.

In `@mcp_dispatcher.py`:
- Around line 157-164: The replay-skip branch in the dispatch loop currently
continues without notifying the requester. Before the continue after logging the
already-processed action, use send_message to return an already-processed status
to the originating UI, while preserving the existing last_seen update and skip
behavior.
- Around line 146-151: Update the timeline action guard around get_timeline_item
so any message without a valid timeline_item_id fails closed before reaching the
legacy Grok fallback or MCP tool execution. Treat missing identifiers and
unloadable identified items as execution failures, while preserving the existing
ownership path for successfully loaded timeline items.
- Around line 195-201: Update the message-processing flow around
execution_failed and the unconditional last_seen persistence so failed
executions remain retryable instead of being filtered out by the watermark. Do
not advance or persist the watermark for failures, but add a bounded
retry/attempt cap so permanently missing timeline items cannot be retried
indefinitely; preserve normal watermark advancement for successful executions.
- Around line 165-182: Update the owned-agent execution flow around find_member
and owner.execute_action so an unresolved owned_agent_id is treated as a
retryable configuration failure: set execution_failed and return an appropriate
failure result when owner is None, preventing processed_action from being
written. Preserve the existing terminal “not handled” result only when a
resolved owner executes and returns None, and keep owned cards from reaching the
generic fallback.
- Around line 35-44: Update get_timeline_item to validate or URL-encode item_id
as a single path segment before constructing the request URL, preventing
path/query/fragment injection. Preserve the existing 404/missing-item behavior,
but propagate or distinguish other non-200 responses so callers and the retry
loop can treat transient failures differently from permanently missing items.

In `@pytest.ini`:
- Around line 1-3: Update the pytest configuration/workflow setup associated
with the [pytest] configuration to install requirements-dev.txt in addition to
requirements.txt, and remove the `|| true` suffix from the pytest command in
agent-ci.yml so test failures correctly fail the workflow.

In `@README.md`:
- Around line 18-29: Update the mention-routing flow immediately before the
“Agent Team” section to match the documented behavior: route recognized handles
to their configured interactive agents or API bots, and send untagged mentions
to the generic Grok fallback without claiming every mention creates a timeline
card. Keep the examples and classifications in the “Agent Team” section
unchanged.

In `@tests/test_broker.py`:
- Around line 30-38: Extend test_corrupt_ledger_is_preserved_not_wiped with a
regression case using valid JSON whose root is not a list, such as an object.
Verify PaperBroker backs up the original content under trades.corrupt-*,
preserves it unchanged, and continues with an empty ledger so execute() produces
the expected position; update _read() accordingly to treat every non-list root
as corrupt rather than replacing it with an empty list silently.

In `@tests/test_router.py`:
- Around line 6-8: Update the routing tests around
test_route_to_tradedesk_by_handle and the referenced tests to avoid hardcoded
production handles. Before constructing the roster, set deterministic values for
TRADEDESK_HANDLE, RESEARCH_HANDLE, and TICKERBOT_HANDLE, or inject a test roster
with explicit handles, then build each mention using those configured values
while preserving the existing route assertions.

🪄 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: 1eece840-2d47-4216-a50e-f2c275fd4126

📥 Commits

Reviewing files that changed from the base of the PR and between 8005bb6 and 833f189.

📒 Files selected for processing (24)

• Makefile
• README.md
• a2a_store.py
• agents/__init__.py
• agents/base.py
• agents/broker.py
• agents/registry.py
• agents/router.py
• agents/team/__init__.py
• agents/team/general.py
• agents/team/research.py
• agents/team/shopping.py
• agents/team/tickerbot.py
• agents/team/tradedesk.py
• docs/AGENTS.md
• env.example
• listener.py
• mcp_dispatcher.py
• pytest.ini
• requirements-dev.txt
• tests/test_broker.py
• tests/test_router.py
• tests/test_tradedesk.py
• timeline_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

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: groupthinking/MyXstack

Timestamp: 2026-07-24T22:41:50.637Z
Learning: Member handles are configurable through `TRADEDESK_HANDLE`, `RESEARCH_HANDLE`, `SHOPPING_HANDLE`, and `TICKERBOT_HANDLE` environment variables.

Learnt from: CR
Repo: groupthinking/MyXstack

Timestamp: 2026-07-24T22:41:50.637Z
Learning: Until separate X handles are available, all members share the listener's X account and are selected by tagged member handles.

🪛 ast-grep (0.44.1)

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/registry.py

[warning] 74-76: 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/broker.py

[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)

---

[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)

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)

agents/base.py

[warning] 135-145: 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)

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)

🪛 Ruff (0.15.21)

agents/router.py

[warning] 4-4: typing.List is deprecated, use list instead

(UP035)

agents/team/research.py

[warning] 24-24: 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/team/general.py

[warning] 8-8: typing.Dict is deprecated, use dict instead

(UP035)

---

[warning] 22-22: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

agents/team/shopping.py

[warning] 12-12: typing.Dict is deprecated, use dict instead

(UP035)

---

[warning] 29-29: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

agents/registry.py

[warning] 6-6: typing.List is deprecated, use list instead

(UP035)

---

[warning] 30-30: Using the global statement to update _TEAM is discouraged

(PLW0603)

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)

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)

---

[warning] 40-40: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

mcp_dispatcher.py

[warning] 171-171: Do not catch blind exception: Exception

(BLE001)

agents/team/tradedesk.py

[warning] 14-14: typing.Dict is deprecated, use dict instead

(UP035)

---

[warning] 57-57: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

[warning] 109-109: Too many return statements (9 > 6)

(PLR0911)

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)

---

[warning] 149-149: Consider moving this statement to an else block

(TRY300)

listener.py

[warning] 89-89: Do not catch blind exception: Exception

(BLE001)

---

[warning] 112-112: Consider moving this statement to an else block

(TRY300)

---

[warning] 113-113: Do not catch blind exception: Exception

(BLE001)

---

[warning] 119-119: Do not catch blind exception: Exception

(BLE001)

---

[warning] 131-131: Do not catch blind exception: Exception

(BLE001)

---

[warning] 150-150: Do not catch blind exception: Exception

(BLE001)

🔍 Remote MCP GitHub Copilot

• PR #106 is open and non-draft, with mergeable_state: dirty, 24 changed files, 10 commits, and a net +1301/−89 lines; the head branch is claude/myxstack-agent-merge-myaock and review was requested from Copilot.
• Current PR activity shows only the autogenerated CodeRabbit “review in progress” comment; get_reviews returned no reviews, validate passed, and copilot-pull-request-reviewer is still in progress.
• The core refactor introduces a routed agent team: listener.py now registers the team, routes mentions by handle, writes timeline cards with fail-fast error handling, and processes mentions oldest-first; mcp_dispatcher.py now fetches the referenced timeline item, fails closed if it can’t load it, and only marks actions processed after successful execution.
• Trade-related behavior is tightened: agents/team/tradedesk.py rejects zero/non-finite quantities and uses PaperBroker with duplicate-key idempotency; agents/broker.py atomically writes the paper ledger, preserves corrupted ledgers as timestamped backups, and aggregates positions from fills.
• Classification plumbing was added across a2a_store.py and timeline_server.py so member kind is normalized/required as agent or bot, and new tests cover broker behavior, router matching/fallback, and trade parsing/approval/rejection flows.

🔇 Additional comments (22)

Makefile (1)

1-1: LGTM!

Also applies to: 30-32

requirements-dev.txt (1)

1-2: LGTM!

agents/team/__init__.py (1)

1-1: LGTM!

tests/test_router.py (1)

16-19: LGTM!

Also applies to: 22-24, 33-39, 49-52

agents/__init__.py (1)

1-1: LGTM!

agents/base.py (1)

1-152: LGTM!

agents/team/general.py (1)

21-55: LGTM!

agents/team/research.py (1)

1-56: LGTM!

agents/team/shopping.py (1)

1-78: LGTM!

docs/AGENTS.md (1)

1-88: LGTM!

env.example (1)

55-66: LGTM!

tests/test_broker.py (1)

4-27: LGTM!

tests/test_tradedesk.py (1)

1-132: LGTM!

agents/team/tickerbot.py (1)

15-38: LGTM!

agents/team/tradedesk.py (1)

109-140: LGTM!

agents/router.py (1)

9-28: LGTM!

agents/registry.py (2)

14-54: LGTM!

---

57-85: LGTM!

timeline_server.py (1)

2-2: LGTM!

Also applies to: 38-47

listener.py (2)

11-13: LGTM!

---

169-169: LGTM!

mcp_dispatcher.py (1)

13-13: LGTM!

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Normalize existing registry records too.

This only normalizes incoming registrations. The seeded agents at Lines 12-37 and old persisted records can still be returned without kind; several will never re-register. Normalize and persist kind: "agent" during store loading, and add it to DEFAULT_AGENTS.

🤖 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 91 - 107, Update DEFAULT_AGENTS to include kind:
"agent" for every seeded record, and modify _read_store to normalize existing
persisted agents by assigning "agent" when kind is missing or invalid, then
persist the normalized data before returning it. Ensure all registry reads
return agents with a valid kind even when they never re-register.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not drop the cross-process lock on non-Unix hosts.

When fcntl is unavailable, two dispatcher processes can both read the same ledger and race os.replace; the last writer wins and an approved fill disappears. Use a portable inter-process lock or explicitly restrict this broker to a single process on those platforms.

🧰 Tools

🪛 ast-grep (0.44.1)

[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)

🪛 Ruff (0.15.21)

[warning] 21-21: Missing return type annotation for private function _ledger_file_lock

(ANN202)

🤖 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 20 - 37, Update _ledger_file_lock so it
retains cross-process serialization when fcntl is unavailable; replace the
current unlocked yield with a portable inter-process locking mechanism, or
explicitly enforce single-process broker operation on those platforms. Preserve
the existing lock-file lifecycle and exclusive critical-section behavior.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve semantically corrupted ledgers instead of overwriting them.

Valid JSON that is not a fill list ({}, [1], malformed fill objects) bypasses the backup path. {} is silently replaced on the next execution, while [1] crashes at existing.get(...). Validate the list and fill schema in _read(), then preserve invalid content through the same corrupt-backup path.

🤖 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 44 - 58, Update _read() to validate that
parsed JSON is a list of valid fill objects with the required schema before
returning it. Route non-list values and malformed list entries through the same
corrupt-ledger backup path used for JSONDecodeError, preserving the original
content and returning an empty ledger; ensure valid fill lists remain unchanged.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Pass the approved request to the executor.

This prompt contains the action and title, but not item["body"], where Line 45 stored the mention. Approval actions are therefore context-free and can execute the wrong thing—or nothing. Include the card body with wrap_untrusted().

Proposed fix

+        request = wrap_untrusted(str(item.get("body", "")))
         result = grok_chat(
             f"You are a workflow agent. A user took the action '{action}' on "
             f"timeline item {item.get('id', '?')} titled "
             f"'{item.get('title', '')}'.\n"
             "Use MCP tools to execute any required external steps. "
-            "Return a concise status update."
+            "Return a concise status update.\n\n"
+            f"Approved request:\n{request}"
         )

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	result = grok_chat(
	f"You are a workflow agent. A user took the action '{action}' on "
	f"timeline item {item.get('id', '?')} titled "
	f"'{item.get('title', '')}'.\n"
	"Use MCP tools to execute any required external steps. "
	"Return a concise status update."
	)
	request = wrap_untrusted(str(item.get("body", "")))
	result = grok_chat(
	f"You are a workflow agent. A user took the action '{action}' on "
	f"timeline item {item.get('id', '?')} titled "
	f"'{item.get('title', '')}'.\n"
	"Use MCP tools to execute any required external steps. "
	"Return a concise status update.\n\n"
	f"Approved request:\n{request}"
	)

🤖 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/general.py` around lines 63 - 69, Update the prompt passed to
grok_chat in the approval execution flow to include item["body"], which contains
the approved mention stored earlier. Wrap the body with wrap_untrusted() before
inserting it into the executor prompt, while preserving the existing action,
title, and concise status instructions.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject malformed numeric quantities instead of silently changing the order.

$TSLA buy 1e309 matches only 1, so this creates a one-share proposal instead of being rejected as non-finite. Inputs such as 10abc have the same partial-match problem. Capture the full quantity token, convert it, and reject conversion failures before the finite/positive check.

🤖 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 27 - 48, Update _TICKER_FIRST and
_SIDE_FIRST so the optional quantity captures the complete numeric token,
including exponent notation and trailing non-whitespace characters, rather than
partially matching prefixes. In parse_trade_command, convert the captured token
inside error handling and return None on conversion failure before applying the
existing math.isfinite and positive checks, ensuring malformed quantities such
as 1e309 and 10abc are rejected.

[coderabbitai]

🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

"Failures stay retryable" is not what this code does — the message watermark advances anyway.

The comment claims a failed execution stays retryable because processed_action isn't written. It doesn't. At the end of this same iteration last_seen is advanced and persisted unconditionally, so the A2A message that carried the action is filtered out on every subsequent poll (created_at <= last_seen). Nothing re-reads it. Net effect of execution_failed: one missing metadata key. The approval is dropped, the user got a "Could not load timeline item" message, and the trade never executes — silently, permanently.

Either don't advance the watermark for failed executions, or persist a retry queue. If you go with the former, bound it — a deleted card would otherwise re-fail every 5 seconds forever:

Proposed change

             if item_id:
-                # A failed execution must stay retryable: record the result
-                # but don't mark the action processed.
                 update = {"mcp_result": result}
                 if not execution_failed:
                     update["processed_action"] = action
+                else:
+                    attempts = int(item_meta.get("execution_attempts", 0)) + 1
+                    update["execution_attempts"] = attempts
                 update_timeline_item(item_id, update)
+
+            if execution_failed and attempts < MAX_ACTION_ATTEMPTS:
+                # Hold the watermark so this action is re-read next poll.
+                send_message(...)
+                time.sleep(RETRY_BACKOFF_SECONDS)
+                break

(sketch — the point is that the watermark advance below has to be conditional, with an attempt cap so a permanently-missing card doesn't become a retry storm.)

🤖 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 195 - 201, Update the message-processing flow
around execution_failed and the unconditional last_seen persistence so failed
executions remain retryable instead of being filtered out by the watermark. Do
not advance or persist the watermark for failures, but add a bounded
retry/attempt cap so permanently missing timeline items cannot be retried
indefinitely; preserve normal watermark advancement for successful executions.

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== pytest.ini ==\n'
cat -n pytest.ini

printf '\n== workflow files mentioning pytest ==\n'
rg -n "pytest|requirements-dev|requirements.txt|\|\| true|make test" .github Makefile requirements*.txt pytest.ini -S || true

printf '\n== list relevant files ==\n'
fd -a 'agent-ci\.yml|Makefile|requirements-dev\.txt|requirements\.txt' . .github 2>/dev/null || true

Repository: groupthinking/MyXstack

Length of output: 1480

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== .github/workflows/agent-ci.yml ==\n'
cat -n .github/workflows/agent-ci.yml

printf '\n== requirements files ==\n'
for f in requirements.txt requirements-dev.txt; do
  echo "--- $f ---"
  cat -n "$f"
done

printf '\n== Makefile ==\n'
[ -f Makefile ] && cat -n Makefile || true

Repository: groupthinking/MyXstack

Length of output: 9957

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "Waiting for file inspection results."

Repository: groupthinking/MyXstack

Length of output: 197

---

Install the dev test deps and stop swallowing pytest failures
.github/workflows/agent-ci.yml only installs requirements.txt and then runs pytest ... || true, so a bad test run can still greenlight the workflow. Install requirements-dev.txt here and let pytest fail the job.

🤖 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 `@pytest.ini` around lines 1 - 3, Update the pytest configuration/workflow
setup associated with the [pytest] configuration to install requirements-dev.txt
in addition to requirements.txt, and remove the `|| true` suffix from the pytest
command in agent-ci.yml so test failures correctly fail the workflow.

Source: MCP tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the stale architecture flow above this section.

The new routing docs say TickerBot is deterministic and untagged mentions use Grok, but README.md Line 16 still claims every mention goes to Grok and creates a timeline card. Update that flow or users will configure against the wrong behavior.

🤖 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 18 - 29, Update the mention-routing flow immediately
before the “Agent Team” section to match the documented behavior: route
recognized handles to their configured interactive agents or API bots, and send
untagged mentions to the generic Grok fallback without claiming every mention
creates a timeline card. Keep the examples and classifications in the “Agent
Team” section unchanged.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve schema-invalid ledgers too.

This only covers malformed JSON. The current _read() also treats valid non-list JSON as [], then execute() overwrites it without a backup. Treat every non-list root as corrupt and add this regression case.

Regression test

+def test_non_list_ledger_is_preserved_not_wiped(tmp_path):
+    path = tmp_path / "trades.json"
+    path.write_text('{"legacy": "data"}', encoding="utf-8")
+    broker = PaperBroker(str(path))
+    broker.execute("TSLA", "buy", 1)
+    backups = list(tmp_path.glob("trades.corrupt-*"))
+    assert len(backups) == 1
+    assert backups[0].read_text(encoding="utf-8") == '{"legacy": "data"}'

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def test_corrupt_ledger_is_preserved_not_wiped(tmp_path):
	path = tmp_path / "trades.json"
	path.write_text("{not json", encoding="utf-8")
	broker = PaperBroker(str(path))
	broker.execute("TSLA", "buy", 1)
	backups = list(tmp_path.glob("trades.corrupt-*"))
	assert len(backups) == 1
	assert backups[0].read_text(encoding="utf-8") == "{not json"
	assert broker.positions() == {"TSLA": 1.0}
	def test_corrupt_ledger_is_preserved_not_wiped(tmp_path):
	path = tmp_path / "trades.json"
	path.write_text("{not json", encoding="utf-8")
	broker = PaperBroker(str(path))
	broker.execute("TSLA", "buy", 1)
	backups = list(tmp_path.glob("trades.corrupt-*"))
	assert len(backups) == 1
	assert backups[0].read_text(encoding="utf-8") == "{not json"
	assert broker.positions() == {"TSLA": 1.0}
	def test_non_list_ledger_is_preserved_not_wiped(tmp_path):
	path = tmp_path / "trades.json"
	path.write_text('{"legacy": "data"}', encoding="utf-8")
	broker = PaperBroker(str(path))
	broker.execute("TSLA", "buy", 1)
	backups = list(tmp_path.glob("trades.corrupt-*"))
	assert len(backups) == 1
	assert backups[0].read_text(encoding="utf-8") == '{"legacy": "data"}'

🤖 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 30 - 38, Extend
test_corrupt_ledger_is_preserved_not_wiped with a regression case using valid
JSON whose root is not a list, such as an object. Verify PaperBroker backs up
the original content under trades.corrupt-*, preserves it unchanged, and
continues with an empty ledger so execute() produces the expected position;
update _read() accordingly to treat every non-list root as corrupt rather than
replacing it with an empty list silently.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Stop hardcoding configurable handles in routing tests.

These tests assume Tradedesk, Research, and TickerBot are permanent handles, but production handles come from TRADEDESK_HANDLE, RESEARCH_HANDLE, and TICKERBOT_HANDLE. A configured deployment will make these tests fail or validate the wrong route. Set deterministic environment values before constructing the roster, or inject a test roster with explicit handles.

Also applies to: 11-13, 27-30, 41-46

🤖 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 6 - 8, Update the routing tests around
test_route_to_tradedesk_by_handle and the referenced tests to avoid hardcoded
production handles. Before constructing the roster, set deterministic values for
TRADEDESK_HANDLE, RESEARCH_HANDLE, and TICKERBOT_HANDLE, or inject a test roster
with explicit handles, then build each mention using those configured values
while preserving the existing route assertions.

Source: Learnings

[Copilot]

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

[coderabbitai]

✅ Unit tests committed locally. Commit: 361b33568599e7d721d026d58cd6713e6665b10b

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.