Repository navigation
Add an AMQP 0.9.1 transport example - #3521
Kludex wants to merge 5 commits into
Conversation
Sorry, something went wrong.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbaffce9e8
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="examples/transports/README.md">
<violation number="1" location="examples/transports/README.md:144">
P2: Use the empty exchange name (`""`) here; `amq.default` is not RabbitMQ's default exchange, and the permission check actually calls `get_exchange("")`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
|
|
||
| ### AMQP authorization and limits | ||
|
|
||
| The fixture grants each client publication rights only to its request exchanges. Queue-name permissions alone do not constrain default-exchange routing, so client credentials cannot publish through `amq.default`. The live permission check also rejects writes to another principal's exchange. Server credentials manage both sides of the configured routes; messages cannot choose an arbitrary reply destination. |
There was a problem hiding this comment.
P2: Use the empty exchange name ("") here; amq.default is not RabbitMQ's default exchange, and the permission check actually calls get_exchange("").
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At examples/transports/README.md, line 144:
<comment>Use the empty exchange name (`""`) here; `amq.default` is not RabbitMQ's default exchange, and the permission check actually calls `get_exchange("")`.</comment>
<file context>
@@ -105,3 +105,44 @@ The fixture uses public test credentials, binds only to localhost, and disables
+
+### AMQP authorization and limits
+
+The fixture grants each client publication rights only to its request exchanges. Queue-name permissions alone do not constrain default-exchange routing, so client credentials cannot publish through `amq.default`. The live permission check also rejects writes to another principal's exchange. Server credentials manage both sides of the configured routes; messages cannot choose an arbitrary reply destination.
+
+The fixture uses public test credentials, listens only on localhost, and disables durable storage. Do not deploy it. Use TLS and broker authorization in production, and bind request state to verified, authority-qualified identity. Change the local port with `AMQP_TEST_PORT` (default 15672).
</file context>
| The fixture grants each client publication rights only to its request exchanges. Queue-name permissions alone do not constrain default-exchange routing, so client credentials cannot publish through `amq.default`. The live permission check also rejects writes to another principal's exchange. Server credentials manage both sides of the configured routes; messages cannot choose an arbitrary reply destination. | |
| The fixture grants each client publication rights only to its request exchanges. Queue-name permissions alone do not constrain default-exchange routing, so client credentials cannot publish through the default exchange (the empty exchange name, `""`). The live permission check also rejects writes to another principal's exchange. Server credentials manage both sides of the configured routes; messages cannot choose an arbitrary reply destination. |
Sorry, something went wrong.
There was a problem hiding this comment.
Code review found no issues
No high-confidence issues detected in this change.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Still open from earlier reviews (4):
- 🔴 examples/transports/mcp_transport_examples/amqp.py:59 — Users of the AMQP example never learn that the remote peer died: a client awaiting a tool result hangs forever, and a s…
- 🔴 examples/transports/mcp_transport_examples/amqp.py:134 — Users of this adapter get their borrowed channel closed by the broker at every normal session end, although the docstri…
- Also unresolved: 2 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Sorry, something went wrong.
Review scope
Stacked on #3520 for the shared broker harness and CI setup. This diff contains only the AMQP 0.9.1 adapter, RabbitMQ configuration, examples, configuration tests, and AMQP documentation; there are no MQTT or gRPC implementation changes.
The existing review correction for cross-principal injection is preserved: dedicated direct exchanges replace default-exchange publication. Broker recording/coverage, broader failure validation, and production TLS authorization remain open merge gates.
Validation
Six AMQP configuration tests and pre-commit pass after the split; the dedicated CI job also runs the live broker programs.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.