Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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 GitHub
Your 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
|
|
||
| ### 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 agents
Check 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. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
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
1 participant
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.