Conversation
![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: 3c33ab64e8
ℹ️ 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".
| "error": { | ||
| "code": "upstream_rate_limited", | ||
| "message": "upstream returned 429", | ||
| "retryable": true, |
There was a problem hiding this comment.
When a plugin author follows this example and sets retryable, the host silently discards that value: decodeEnvelopeResult copies only Code, Message, and HTTPStatus into rpcError, which exposes no retryability signal to the executor retry logic. This newly documents retryable as part of the failure contract even though toggling it cannot affect CPA behavior; either preserve it through the RPC error path or explicitly state that the field is currently informational/ignored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7f62d618 after local review. The JSON and Go examples no longer set retryable, and both guides now state that the current host treats error.retryable as informational and does not use it for executor retry decisions. The pluginabi.Error.Retryable Go doc now states the same limitation.
3c33ab6 to
7f62d61
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f62d6187b
ℹ️ 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".
| CPA uses the status to produce the client-facing error classification. On its | ||
| OpenAI-compatible handlers, for example, `403` becomes `permission_error` / | ||
| `insufficient_quota`, `429` becomes `rate_limit_error` / | ||
| `rate_limit_exceeded`, and `404` becomes `invalid_request_error` / | ||
| `model_not_found`. If `http_status` is omitted or zero, an executor failure |
There was a problem hiding this comment.
When a plugin forwards an upstream JSON error body in error.message, these mappings do not occur: BuildErrorResponseBodyWithError returns any valid JSON message verbatim before consulting the status-to-type/code switch. Thus a 429 envelope can still expose whatever type/code the JSON contains—or none at all—rather than rate_limit_error / rate_limit_exceeded; document this structured-message exception or change the host to apply the promised classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0ce20831 after local review. Both guides now state that http_status sets the HTTP response status, while status-derived OpenAI error type/code mappings apply to plain-text messages. They also document that a valid JSON error.message is forwarded unchanged and must contain any desired structured fields. The pluginabi.Error Go doc was narrowed accordingly.
7f62d61 to
0ce2083
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ce2083149
ℹ️ 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".
| `executor.execute_stream` when the upstream rejects the request before the | ||
| stream is established. These are separate plugin methods, so handling only one | ||
| leaves the other path using CPA's default HTTP 500 response. After response | ||
| headers or stream chunks have reached the client, the HTTP status can no longer |
There was a problem hiding this comment.
For asynchronous executor.execute_stream plugins that return a success envelope before dialing upstream—as examples/plugin/claude-web-search-router/go/execute_stream.go does—an upstream 429 can occur while no headers or chunks have reached the client, yet http_status can no longer be supplied: host.stream.emit and host.stream.close accept only a string error, so executionErrorMessage falls back to 500. The guide should require plugins to wait for the upstream status and return a failed execute_stream envelope before returning success, rather than implying that the status remains changeable until client output begins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4e9a332b after local review. The guide now requires streaming plugins to wait for the initial upstream accept/reject result before returning success from executor.execute_stream. It also states that after success, host.stream.emit and host.stream.close can carry only string errors, so later failures default to HTTP 500 even before client output.
| `invalid_request_error` / `model_not_found`. If `error.message` is itself valid | ||
| JSON, CPA forwards that JSON unchanged, so the plugin must include any desired |
There was a problem hiding this comment.
For pre-frame failures on the streaming OpenAI Responses endpoint, valid JSON is not forwarded unchanged: handleStreamingResponse calls sanitizeResponsesInitialErrorMessage, and responsesStreamErrorText recursively redacts and truncates values and can discard top-level fields other than error and sequence_number before WriteErrorResponse. Qualify this promise so plugins do not rely on field or byte preservation in that path. The fresh evidence beyond the earlier review thread is this Responses-specific sanitizer, rather than BuildErrorResponseBodyWithError itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4e9a332b after local review. The guide no longer promises unchanged JSON passthrough. It now treats error-body handling as protocol-specific and explicitly warns that structured and streaming handlers may forward, normalize, or sanitize error.message; only http_status is documented as the machine-readable status contract.
0ce2083 to
4e9a332
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
4e9a332 to
98c3187
Compare
camy-x
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Follow-up to #5809 and upstream commit
6ba44455, which implemented HTTP-status propagation and closed the original issue while this PR was under review.This PR now contains only documentation accuracy fixes on top of that implementation:
type/codemappings to plain-text errors on OpenAI-compatible handlers;executor.execute_streamcutoff: plugins must wait for the initial upstream accept/reject result before returning success if they need to preservehttp_status;host.stream.emit/host.stream.closeerrors carry no status and default to HTTP 500, even before client output;error.retryableis currently informational and does not control executor retries.Validation:
go test -count=1 ./sdk/pluginabi ./internal/pluginhost ./sdk/api/handlersgo vet ./sdk/pluginabi ./sdk/api/handlersgo build -o test-output ./cmd/serverThe newly added
examples/plugin/executor/gotest currently fails on cleanorigin/devbecausemain_test.goreferences undefinederrorEnvelopeandenvelopesymbols. That upstream baseline issue is outside this documentation-only follow-up.Assistance disclosure: implementation, conflict resolution, self-review, and verification were AI-assisted and reviewed against the repository contribution rules.