Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Round 1 — reviewed head 30c87eac — reviewer summarizer:hermes/gpt-5.6-terra over coverage+credentials+deployment+general+lifecycle+prose.
terra
Advisory findings from outerloop — the code owner decides. Reply to disagree; the outerloop:no-review label opts this PR out.
Verdict: 3 blocking, 5 advisory.
8 findings attached to the lines below.
Blocking: authenticate the per-run gateway attribution, make fixed gateway URLs participate in outage waiting, and add the gateway to the upgrade/pin lifecycle. Advisory: correct the direct-endpoint credential claim, include the gateway version in conformance coverage, and apply the three prose rewrites. Rejected as duplicates: the credentials, general, and prose accounting findings all describe the same client-forgeable run header; the deployment and lifecycle fixed-URL findings describe the same missing readiness probe and endpoint-wait bypass.
| may use. As today, a session never sees an upstream credential; author and | ||
| judge keys stay distinct, so the existing credential-separation preflight | ||
| keeps its meaning. | ||
| - **Accounting**: the gateway logs tokens per model, role and run (from a |
There was a problem hiding this comment.
Blocking. Run accounting can be forged by a session. [coverage+credentials+general+prose] Sessions receive a reusable gateway role key and can send requests bearing another run ID, so a client-controlled attribution header can charge token use to the wrong run; bind the run ID to a per-run credential or authenticate the attribution header.
(high confidence)
| request header the kernel sets), as a second source next to session | ||
| capture. | ||
| - **Lifecycle**: an operator service with a health endpoint, pinned like the | ||
| harnesses (`harnesses.toml`), upgraded through `outerloop harness upgrade`. |
There was a problem hiding this comment.
Blocking. A dead fixed-URL gateway will fail the session instead of parking the run. [coverage+deployment+lifecycle] Fixed URLs return from EndpointProfile.session_url() without a health probe; only URL_FILE profiles issue the models probe that raises EndpointUnavailable, so an unreachable gateway becomes a session error rather than entering endpoint-wait handling.
(high confidence)
| request header the kernel sets), as a second source next to session | ||
| capture. | ||
| - **Lifecycle**: an operator service with a health endpoint, pinned like the | ||
| harnesses (`harnesses.toml`), upgraded through `outerloop harness upgrade`. |
There was a problem hiding this comment.
Blocking. The proposed gateway cannot be installed or upgraded through the named command. [deployment] harness upgrade and its pin reader accept only claude, codex, hermes, and bridge, so the rollout command cannot select a gateway installer or pin.
(high confidence)
| - **Credentials**: the gateway holds the upstream credentials (provider keys, | ||
| self-hosted endpoint keys, cloud service accounts). Sessions receive a | ||
| gateway key scoped to their role (author or judge) and to the models that role | ||
| may use. As today, a session never sees an upstream credential; author and |
There was a problem hiding this comment.
The credential claim describes current direct endpoints incorrectly. [general] Current endpoint profiles pass profile.key() into the session in role_runner.py:120-128, so “As today” is false and hides the security change; rewrite it as “With the gateway, a session never sees an upstream credential.”
(high confidence)
| The two paths diverge. When Codex 0.160 added a sub-agent tool and a hosted | ||
| web-search tool, the per-session bridge rejected every request ("unsupported | ||
| bridge request") while a shared proxy on another deployment let the same | ||
| requests through. One harness upgrade, two behaviors, tested on one. |
There was a problem hiding this comment.
Suggestion. The incident conclusion is unclear. [prose] The same harness upgrade produced two behaviors, but the test covered only one deployment.
(high confidence)
|
|
||
| 1. Ship the gateway as an optional service, with profiles pointing at it, while | ||
| the per-session bridge stays the default for `API=chat`. | ||
| 2. Run one deployment on the gateway through a soak; compare session outcomes |
There was a problem hiding this comment.
Suggestion. The migration step uses unexplained jargon. [prose] Run the gateway in one deployment for a trial period.
(high confidence)
|
|
||
| A scripted session per (harness version, upstream server) pair: tool calls, | ||
| freeform `apply_patch`, resume, streaming, and a refused unsupported part. Run | ||
| when either pin changes. It would have caught the Codex 0.160 tool types |
There was a problem hiding this comment.
Suggestion. The conformance-check claim uses unclear wording. [prose] before it reached all deployments.
(high confidence)
|
|
||
| ## Conformance check | ||
|
|
||
| A scripted session per (harness version, upstream server) pair: tool calls, |
There was a problem hiding this comment.
The conformance matrix omits the gateway version. [general] The check is keyed only by harness version and upstream server, so a gateway-pin change can introduce a translation regression without changing either matrix input; include the gateway version and run the check before its pin moves.
(medium confidence)
This branch has not been deployed
1 participant
Draft design note for review (tracking issue #464): replace the per-session Codex chat bridge with one pinned model gateway per deployment, keeping the property that sessions never hold upstream credentials and moving the faithfulness check into the gateway. Includes trade-offs, migration and open questions.
Docs only. Written by Claude; awaiting review before any build.