CodeRabbit's fourth review pass on PR #6049 read the API, SDK and runner, 181 files that no earlier pass had reached, and raised 18 actionable comments. All 18 arrived marked Major. Graded against the code at 83d086a9d2: seven are real defects worth a decision and were put in front of the release owner, two are not defects, two concern trust boundaries and are filed separately as issue 6945, and these are the remaining seven.
Each is real. None blocks. Two of them, marked below, touch credentials or transport and are here rather than in 6945 for the reason given in each.
The seven
-
api/oss/src/core/gateways/mcps/oauth/registration.py — the check for whether a stored OAuth client registration still names our callback drops a trailing slash from both sides before comparing. Our own callback is built from a fixed path and never carries one, so the looseness only matters when an authorization server echoes back a registration whose URI differs from the one we sent by exactly that slash. We would then treat the registration as covering us and send an authorization request the server may refuse. The function's own docstring argues the tolerance and a test pins it deliberately, so this is a tension in a documented choice rather than an oversight: the same docstring says a comparison looser than the server's is the defect it exists to prevent.
-
sdks/python/agenta/sdk/agents/capabilities.py — the harness and provider pairing check returns true for any mock deployment before it consults the provider, so a harness can be paired with a provider it cannot use whenever the deployment is the mock one. The mock deployment is flag-gated and exposes models from more than one provider family, which is what makes the pairing reachable. Removing the early return is one line, but it may be there so the mock works with any harness, so the fix needs a decision about what the mock is for rather than just the edit.
-
sdks/python/agenta/sdk/agents/platform/resolve.py — the MCP resolver's direct fallback builds its secret provider with no arguments, which constructs a fresh connection from ambient configuration, while the surrounding function resolved an explicit connection for its gateway calls. A caller who passes a connection therefore has it honoured for gateway state and silently ignored for secrets, so the two can come from different backends, or the secret read can fail when nothing ambient is set. The line that constructs the default predates this branch; the parameter that makes ignoring it wrong is new. One line: pass the resolved connection through.
-
sdks/python/agenta/sdk/evaluations/runtime/adapters.py — a batch response with a populated status and no data is classified as an unsupported streaming response, and its real error code and message are discarded. The underlying gap predates this branch, which read the same two conditions together and gave both a generic message. What is new is splitting them and attaching a specific and now misleading sentence to the no-data case. Check the status first, and use the streaming sentence only when no failure status is present.
-
services/runner/src/engines/sandbox_agent/mount.ts — touches credentials. Tunnel discovery prefers an HTTPS listing and falls back to any listing when none matches, so a plain HTTP endpoint can be handed to the mount helper while AWS credentials are supplied to the same process through its environment. No scheme check exists anywhere on that path. It is here rather than in 6945 because the tunnel list comes from a local control API belonging to a development sidecar, not from user input or a remote service. Reject a non-HTTPS result before mounting.
-
services/runner/src/extensions/model-provider-override.ts — touches transport. The override validator checks the scheme, the absence of embedded credentials, and the absence of a query or fragment, but never the host, so an override may name any origin while a gateway credential header is attached. The missing origin check predates this branch and is unchanged from main; what the branch added is an allowance for plain HTTP when a gateway-shaped credential header is present. The runner trusts the holder of its shared token by design, which makes this defence in depth rather than an open door, and is why it is here rather than in 6945.
-
services/runner/src/tools/relay.ts — after a turn is aborted, the activity source reports closed from its wait while still reporting itself healthy, because the abort sets neither the loop's own flag nor the source's closed state. The loop therefore keeps selecting the waiting branch, each wait returns immediately, and the directory listing runs back to back until a separate stop call arrives. The mismatch is new on this branch: an earlier fix taught the wait to honour the abort signal without teaching the health check the same thing. One line: end the loop when the signal is aborted.
Not included
Seven findings were graded P2 and given to the release owner for a decision rather than fixed on the branch: an OAuth grant that can be stored readable and read back by a view-only caller, a tool-prefix collision between custom and brokered connections that silently drops one connection's tools, a credential-bearing request with no transport check, a gateway refusal whose status and failure code are dropped, a published alias removed without deprecation, a gateway credential that never reaches one sandbox provider's daemon, and a typed error detail that reaches the live stream but not the persisted record.
Two were graded as not defects. The layering objection misreads the code: the adapter takes its router injected and untyped from the entrypoint, which is what the convention prescribes, and the convention forbids raising HTTP exceptions from the core layer rather than catching them. The claim that a gateway credential can be sent without an endpoint is wrong about its consequence: the validator's transport check already rejects a missing base URL, so the record shape it describes cannot be constructed. What survives is that the invariant holds incidentally rather than locally.
Two are in issue 6945.
CodeRabbit's fourth review pass on PR #6049 read the API, SDK and runner, 181 files that no earlier pass had reached, and raised 18 actionable comments. All 18 arrived marked Major. Graded against the code at
83d086a9d2: seven are real defects worth a decision and were put in front of the release owner, two are not defects, two concern trust boundaries and are filed separately as issue 6945, and these are the remaining seven.Each is real. None blocks. Two of them, marked below, touch credentials or transport and are here rather than in 6945 for the reason given in each.
The seven
api/oss/src/core/gateways/mcps/oauth/registration.py— the check for whether a stored OAuth client registration still names our callback drops a trailing slash from both sides before comparing. Our own callback is built from a fixed path and never carries one, so the looseness only matters when an authorization server echoes back a registration whose URI differs from the one we sent by exactly that slash. We would then treat the registration as covering us and send an authorization request the server may refuse. The function's own docstring argues the tolerance and a test pins it deliberately, so this is a tension in a documented choice rather than an oversight: the same docstring says a comparison looser than the server's is the defect it exists to prevent.sdks/python/agenta/sdk/agents/capabilities.py— the harness and provider pairing check returns true for any mock deployment before it consults the provider, so a harness can be paired with a provider it cannot use whenever the deployment is the mock one. The mock deployment is flag-gated and exposes models from more than one provider family, which is what makes the pairing reachable. Removing the early return is one line, but it may be there so the mock works with any harness, so the fix needs a decision about what the mock is for rather than just the edit.sdks/python/agenta/sdk/agents/platform/resolve.py— the MCP resolver's direct fallback builds its secret provider with no arguments, which constructs a fresh connection from ambient configuration, while the surrounding function resolved an explicit connection for its gateway calls. A caller who passes a connection therefore has it honoured for gateway state and silently ignored for secrets, so the two can come from different backends, or the secret read can fail when nothing ambient is set. The line that constructs the default predates this branch; the parameter that makes ignoring it wrong is new. One line: pass the resolved connection through.sdks/python/agenta/sdk/evaluations/runtime/adapters.py— a batch response with a populated status and no data is classified as an unsupported streaming response, and its real error code and message are discarded. The underlying gap predates this branch, which read the same two conditions together and gave both a generic message. What is new is splitting them and attaching a specific and now misleading sentence to the no-data case. Check the status first, and use the streaming sentence only when no failure status is present.services/runner/src/engines/sandbox_agent/mount.ts— touches credentials. Tunnel discovery prefers an HTTPS listing and falls back to any listing when none matches, so a plain HTTP endpoint can be handed to the mount helper while AWS credentials are supplied to the same process through its environment. No scheme check exists anywhere on that path. It is here rather than in 6945 because the tunnel list comes from a local control API belonging to a development sidecar, not from user input or a remote service. Reject a non-HTTPS result before mounting.services/runner/src/extensions/model-provider-override.ts— touches transport. The override validator checks the scheme, the absence of embedded credentials, and the absence of a query or fragment, but never the host, so an override may name any origin while a gateway credential header is attached. The missing origin check predates this branch and is unchanged from main; what the branch added is an allowance for plain HTTP when a gateway-shaped credential header is present. The runner trusts the holder of its shared token by design, which makes this defence in depth rather than an open door, and is why it is here rather than in 6945.services/runner/src/tools/relay.ts— after a turn is aborted, the activity source reports closed from its wait while still reporting itself healthy, because the abort sets neither the loop's own flag nor the source's closed state. The loop therefore keeps selecting the waiting branch, each wait returns immediately, and the directory listing runs back to back until a separate stop call arrives. The mismatch is new on this branch: an earlier fix taught the wait to honour the abort signal without teaching the health check the same thing. One line: end the loop when the signal is aborted.Not included
Seven findings were graded P2 and given to the release owner for a decision rather than fixed on the branch: an OAuth grant that can be stored readable and read back by a view-only caller, a tool-prefix collision between custom and brokered connections that silently drops one connection's tools, a credential-bearing request with no transport check, a gateway refusal whose status and failure code are dropped, a published alias removed without deprecation, a gateway credential that never reaches one sandbox provider's daemon, and a typed error detail that reaches the live stream but not the persisted record.
Two were graded as not defects. The layering objection misreads the code: the adapter takes its router injected and untyped from the entrypoint, which is what the convention prescribes, and the convention forbids raising HTTP exceptions from the core layer rather than catching them. The claim that a gateway credential can be sent without an endpoint is wrong about its consequence: the validator's transport check already rejects a missing base URL, so the record shape it describes cannot be constructed. What survives is that the invariant holds incidentally rather than locally.
Two are in issue 6945.