fix: classify adapter dispatch outcomes#75
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 3 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Bug in
|
![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: fb20c76c58
ℹ️ 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".
| dispatch_started=False, | ||
| dispatch_uncertain=_dispatch_is_uncertain(str(exc)), |
There was a problem hiding this comment.
When the provider returns a successful HTTP response but the adapter rejects the payload (for example _openai_completion raises OpenAI returned empty choices/empty content, or _anthropic_completion raises returned no text content), the request has already reached the provider, but this catch-all marks dispatch_started=False and only infers uncertainty from the error string. Any caller using the new flags to decide whether work was accepted can treat a confirmed provider response as not dispatched/pending and retry or release capacity incorrectly; distinguish pre-request failures from post-response parsing/content failures and set the dispatch state accordingly.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 324ec80afb
ℹ️ 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".
| except (urllib.error.URLError, OSError, ValueError) as exc: | ||
| raise LlmAdapterError(f"OpenAI network error: {exc}") from exc | ||
| raise LlmAdapterError( | ||
| f"OpenAI network error: {exc}", dispatch_uncertain=True | ||
| ) from exc |
There was a problem hiding this comment.
Fresh evidence in this patch is that ValueError from json.loads(resp.read().decode(...)) is still grouped with transport failures here, and the Anthropic path has the same catch. When the provider returns a 200 response with malformed/non-UTF8 JSON, the request has definitely reached the provider, but this reports dispatch_uncertain=True and dispatch_started=False; callers using these new flags can treat consumed provider work as only pending/ambiguous and retry or release capacity incorrectly. Split JSON/decode failures after urlopen succeeds into a dispatched parse error.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da24f81eb9
ℹ️ 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".
| with urllib.request.urlopen(req, timeout=timeout) as resp: | ||
| payload = json.loads(resp.read().decode("utf-8")) | ||
| raw = resp.read() |
There was a problem hiding this comment.
When the provider has already returned response headers but the socket times out or resets during resp.read(), this line is still inside the same try whose except (URLError, OSError) reports only dispatch_uncertain=True with dispatch_started=False. In that scenario the request has definitely reached the provider, so callers using these new flags can retry or release capacity as if no provider work was consumed; read failures after entering the response context should be classified as dispatched (the Anthropic path has the same pattern).
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f85bea24a
ℹ️ 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".
| return CodexResult( | ||
| success=False, | ||
| error=f"codex exec failed (rc={completed.returncode})" + (f":\n{detail}" if detail else ""), | ||
| dispatch_uncertain=True, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| payload = json.loads(resp.read().decode("utf-8")) | ||
| raw = resp.read() | ||
| except urllib.error.HTTPError as exc: | ||
| detail = _scrub_api_keys(exc.read().decode("utf-8", errors="replace")[:500]) |
There was a problem hiding this comment.
When the provider has already returned an HTTP error but the socket resets or times out while reading the error body, exc.read() raises before this block wraps the error with dispatch_started=True and status_code; _retry_with_backoff then sees only a raw OSError, reports dispatch_uncertain=True/dispatch_started=False, and also loses retryability for 429/5xx. The request definitely reached the provider once HTTPError exists, so the dispatch state and status should be preserved even if the error body is unreadable; the Anthropic handler has the same pattern.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 317c3152c1
ℹ️ 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".
| model=resolved_model, | ||
| output=output, | ||
| latency_seconds=time.time() - started, | ||
| dispatch_started=not dispatch_uncertain, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| payload = json.loads(resp.read().decode("utf-8")) | ||
| try: | ||
| raw = resp.read() | ||
| except (OSError, TimeoutError) as exc: |
There was a problem hiding this comment.
Fresh evidence in this version is that the new read handler still only catches OSError/TimeoutError; Python's HTTPResponse.read() can raise http.client.IncompleteRead when the provider has sent headers but the body is truncated, and that class is not an OSError. In that scenario (same pattern in the Anthropic read path) the raw exception bypasses _retry_with_backoff/LlmAdapterError, so /v1/ai/analyze can turn a confirmed provider dispatch into an internal error with no dispatch flags; catch IncompleteRead/HTTPException here and mark it dispatch_started=True.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb52d33343
ℹ️ 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".
| reason = exc.reason if isinstance(exc, urllib.error.URLError) else exc | ||
| if isinstance(reason, TimeoutError): | ||
| return True | ||
| if isinstance(reason, (socket.gaierror, ConnectionRefusedError, ssl.SSLError)): |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06549610dc
ℹ️ 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".
| def _http_error_detail(exc: urllib.error.HTTPError) -> str: | ||
| try: | ||
| return _scrub_api_keys(exc.read().decode("utf-8", errors="replace")[:500]) | ||
| except (OSError, TimeoutError): |
There was a problem hiding this comment.
Fresh evidence in this patch is that _http_error_detail still only catches OSError/TimeoutError; if a provider returns an HTTP 429/5xx but the error body is truncated, HTTPError.read() can raise http.client.IncompleteRead (HTTPException, not OSError). That escapes before the code wraps the response with dispatch_started=True and status_code, so /v1/ai/analyze can turn a confirmed provider dispatch into an internal error and skip the intended retry/backoff for the 429/5xx; catch HTTPException here and fall back to the placeholder detail.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa48af98e5
ℹ️ 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".
| job["error"] = result.error | ||
| job["failure_category"] = _classify_failure(result.error) | ||
| job["failure_category"] = ( | ||
| "dispatch_uncertain" if dispatch_state == "pending_uncertain" else _classify_failure(result.error) |
There was a problem hiding this comment.
When an async Codex job now fails with this new dispatch_uncertain category, downstream paths format it as [...] and then reuse existing category helpers, but those helpers only extract bracketed names ending in _failure and the monthly workflow's infra-category set also omits this value. In the non-timeout uncertain case, such as a nonzero Codex exit, scripts/run_monthly_codex_audit.py will classify Codex audit service job failed [dispatch_uncertain]: ... as unknown_failure and may run the auto-provider fallback, contradicting the new contract that uncertain dispatches are never retried automatically; either make the new category parseable/recognized by those helpers or use an existing _failure category name.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4132b17026
ℹ️ 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".
| job["failure_category"] = "stale_job_timeout" | ||
| if previous_status == "running" and str(job.get("dispatch_state") or "") != "not_dispatched": | ||
| _apply_dispatch_state(job, "pending_uncertain") | ||
| job["failure_category"] = "dispatch_uncertain" |
There was a problem hiding this comment.
Fresh evidence in this patch is this stale-running-job branch: it persists failure_category as dispatch_uncertain, while the normal async failure path uses dispatch_uncertain_failure. When a polling client receives this job it formats Codex audit service job failed [dispatch_uncertain]: ..., but scripts/run_monthly_codex_audit.service_failure_category only extracts bracketed categories ending in _failure, so the monthly auto-provider path can classify the stale uncertain dispatch as unknown_failure and run API fallback instead of forcing reconciliation/no automatic retry.
Useful? React with 👍 / 👎.
| job["failure_category"] = ( | ||
| "dispatch_uncertain_failure" if dispatch_state == "pending_uncertain" else _classify_failure(result.error) | ||
| ) |
There was a problem hiding this comment.
When a job reaches this new dispatch_uncertain_failure path, _record_job_automation_run records its task_state via job_task_state, which still maps this category to plain failed rather than blocked; decide_automation_execution only escalates after max_consecutive_failures failed states, so the very next control snapshot can still allow review_and_fix/auto actions after one pending-uncertain dispatch. Since this category is the signal that accepted work may already exist and must be reconciled, mark it as a blocked/human-review state before feeding it to the automation ledger/control decision.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c94bc8184
ℹ️ 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".
| raise LlmAdapterError( | ||
| f"OpenAI HTTP {exc.code}: {detail}", dispatch_started=True, status_code=exc.code | ||
| ) from exc | ||
| except (urllib.error.URLError, OSError) as exc: |
There was a problem hiding this comment.
When the provider or an intermediate proxy accepts the POST but returns a malformed status line/header or closes during response-header parsing, urllib.request.urlopen can surface an http.client.HTTPException such as BadStatusLine before the response context is entered. This handler only wraps URLError/OSError, so that exception bypasses LlmAdapterError and the new dispatch flags entirely, turning a pending-uncertain provider invocation into an unclassified internal error; the Anthropic path has the same gap.
Useful? React with 👍 / 👎.
| adapter_invoked = True | ||
| _apply_dispatch_state(job, "pending_uncertain") | ||
| job["updated_at"] = _now() | ||
| _write_job(job) |
There was a problem hiding this comment.
If _write_job fails inside this callback (for example a transient job-store write error), adapter_invoked has already been set even though subprocess.run has not been called yet. That exception propagates out of CodexAdapter.execute before launching Codex, and the outer handler then records pending_uncertain/dispatch_uncertain_failure for work that was provably never handed to the CLI; move this marker after the pending state is persisted or distinguish callback persistence failures from actual invocation.
Useful? React with 👍 / 👎.
| job["failure_category"] = ( | ||
| "dispatch_uncertain_failure" if dispatch_state == "pending_uncertain" else _classify_failure(result.error) | ||
| ) |
There was a problem hiding this comment.
When a Codex job fails with this new dispatch_uncertain_failure category and an error like codex exec failed, scripts/run_codex_pr_review._service_review_should_fallback still matches the codex exec failed fallback signal and immediately runs the direct API fallback. That automatically retries a pending-uncertain dispatch despite the new contract that these outcomes require reconciliation first, so this category needs to be excluded from client fallback before it is emitted here.
Useful? React with 👍 / 👎.
| self.assertFalse( | ||
| is_service_infrastructure_failure("Codex audit service job failed [dispatch_uncertain_failure]: timeout") |
There was a problem hiding this comment.
Fresh evidence in this version is this new expectation that dispatch_uncertain_failure is not a service infrastructure failure. In the monthly workflow, run_codex_service returns exit code 1 for non-infra categories and the provider == "auto" branch later falls through to run_auto_provider_fallback, so a job reported as pending-uncertain (for example a nonzero Codex exit after possible dispatch) is retried automatically via API fallback instead of requiring reconciliation as the new dispatch contract states.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d162dd741c
ℹ️ 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".
| process = subprocess.Popen( | ||
| command, | ||
| text=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, |
There was a problem hiding this comment.
With the switch to Popen, communicate(input=prompt) only sends the prompt when the child was created with stdin=subprocess.PIPE; otherwise Codex inherits the service stdin, and _codex_command still passes -, so normal service jobs and the /v1/ai/review Codex verifier read an empty/closed stdin instead of the requested prompt. Add stdin=subprocess.PIPE to preserve the previous subprocess.run(input=prompt) behavior.
Useful? React with 👍 / 👎.
| on_dispatch_start() | ||
| completed_stdout, completed_stderr = process.communicate(input=prompt, timeout=timeout) | ||
| except subprocess.TimeoutExpired as exc: | ||
| return CodexResult(success=False, error=f"codex exec timed out after {timeout}s: {exc}") | ||
| except FileNotFoundError as exc: | ||
| return CodexResult(success=False, error=f"codex command not found: {exc}") | ||
| try: | ||
| process.kill() |
There was a problem hiding this comment.
After Popen succeeds, an on_dispatch_start exception before communicate() returns through the broad handler without terminating the child at all, and the timeout path only sends kill() without waiting/draining. In the async service, a transient job-store write failure or timeout can leave a codex exec process still running or defunct while the job is marked failed/pending, consuming local/provider capacity outside service tracking; terminate and reap the child before returning on these paths.
Useful? React with 👍 / 👎.
|
|
||
| def _service_review_should_fallback(exc: ReviewError) -> bool: | ||
| message = str(exc).lower() | ||
| if "[dispatch_uncertain_failure]" in message: |
There was a problem hiding this comment.
This only suppresses fallback when the service returns a categorized dispatch_uncertain_failure, but once run_codex_service_review has accepted a job_id, any later polling URLError/OSError is caught by the next except in run_codex_review_with_fallback and immediately runs direct API fallback. In that scenario the service job may still be queued/running and can dispatch Codex, so the workflow automatically duplicates provider work instead of reconciling the accepted job; track that submission was accepted and do not fall back on post-submit polling failures.
Useful? React with 👍 / 👎.
| dispatch_started=bool(job.get("dispatch_started")), | ||
| dispatch_uncertain=True, | ||
| ) | ||
| job["failure_category"] = "dispatch_uncertain_failure" |
There was a problem hiding this comment.
When a duplicate submission hits a stale running job, this branch now persists dispatch_uncertain_failure, but _find_active_job_by_dedupe_key returns that failed job to _submit_job, whose reuse check only accepts active statuses before creating a fresh job. For a worker that timed out after Codex may have been invoked, a retrying client with the same dedupe key will automatically start a second Codex job despite the new pending-uncertain contract; return the blocked job or otherwise prevent resubmission until reconciliation.
Useful? React with 👍 / 👎.
1 participant
Scope
Vertical dispatch-outcome prerequisite for AI Budget Guard PR #73. It includes only Codex/LLM adapter outcomes plus the gateway persistence, triage, and review-consumption boundary required to make those outcomes effective. Budget ledgers, monthly caps, and settlement implementation remain in #73.
Stable contract
not_dispatched.dispatched.pending_uncertain; non-zero CLI exits and stderr text are not release evidence.pending_uncertainis persisted, classified non-retryable, and forces review escalation rather thanauto_prorauto_merge.Validation
python3 -m pytest -q— 674 passed, 1 skipped, 16 subtests passedpython3 -m ruff check service scripts testsactionlintpython3 -m compileall -q service scripts