Skip to content

fix(runtime): classify failed codex login and model check output without logging it - #2959

Merged
freeqaz merged 4 commits into
mainfrom
diag-2017
Oct 19, 2026
Merged

freeqaz merged 4 commits into
mainfrom
diag-2017

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

Finding 2017: after an OpenShell gateway outage, the redeployed Harness's first codex login (access-token exchange) or its startup codex.model_probe failed and was held at once (RUNTIME_LOGIN_FAILED, or RUNTIME_MODEL_PROBE_FAILED with PROCESS_EXIT exit-1 and no events). The wrapper never captures codex's stderr, so a sighting can't tell a transport failure from a provider 5xx or a credential rejection. That is the evidence needed to choose between a one-time retry and an upstream report.

Change

  • A failed native login (runtime.startup_phase, phase codex-login) and each failed codex.model_probe attempt now carry failureClass. It is one of a closed contract list (CODEX_FAILURE_OUTPUT_CLASSES): network, tls, http_401_403, http_5xx, http_other, timeout, empty, unknown.
  • The class is computed in-process from codex's stderr (and from the wrapper's own ETIMEDOUT). The classifier reads at most the last 8 KiB and only uses indexOf/includes and fixed-width digit reads, with no regex over the output, so its cost is linear in that window. A status/http word counts only at a word start, so a URL's ?status=500 does not.
  • Precedence:
    • A status means the provider or a proxy answered, so 401/403, then 5xx, then another 4xx win first.
    • Then TLS text, then transport text.
    • Then the wrapper's own timeout.
    • Then empty or unknown.
  • The output itself is never stored, logged or emitted. The held runtime failure status and its cause are unchanged, so there is no contract/API/DB change and no new transport.
  • OCC's runtime log sanitizer keeps failureClass only on those two events and only for a value in the list (occ agent logs, console Logs tab, and the QA probe-log line).
  • Docs: harness-execution.md and security.md name the field.

How the next sighting will read: the QA failure line's probe log (or occ agent logs) shows, for example, codex.model_probe … exitCode: 1, causeKind: PROCESS_EXIT, causeDetail: exit-1, failureClass: network. A failed login reads runtime.startup_phase … phase: codex-login, outcome: failed, failureClass: http_5xx.

Out of scope: retry policy (option A), the QA redeploy step (option C), and finding 2040.

Verification

  • tests/conformance/codex-native-startup.test.mjs runs the real generated entrypoint:
    • Login and probe stderr samples carry a fake API key, a JWT, a bearer header, the fixture access token and a URL with a query string.
    • Each maps to the expected class: network/DNS, 5xx, 401, 403, 429, TLS, three timeouts, empty, unknown, and the 2017 shape (probe exits 1 before any event).
    • None of that text reaches the held status or any log line, and the status has no failureClass.
    • Every failed probe and login logs an in-list class; a READY probe logs none.
  • Classifier table: near misses name no status (exit status 127, status 4011, ?status=500, httpstatus, a URL path).
  • Linearity: adversarial repeated and near-miss markers at 8 KiB, 256 KiB and 1 MiB. Every substring search in the classifier's own realm is counted and stays under 32 × the window, and the result equals the result for the last window alone.
  • tests/conformance/runtime-logs-content.test.mjs: the sanitizer keeps in-list values on the two events and drops token-shaped, URL, object and upper-case values. It also drops the field on openclaw.model_probe.
  • Both touched files pass 20/20 at 2 CPUs. Related conformance files pass (892 tests): contracts, kubernetes-compute, runtime-logs-replay, codex-stderr-filter, openshell-harness-status, kubernetes-runtime-credentials, plugin-compute, runtime-probe-cpu-counter. tsc --build, eslint and prettier pass on touched files.
  • Internal security review (leakage, linearity, sanitizer type confusion, side channel): approved with nits. Applied: status words must start a word, the contract text says when timeout applies, and a comment covers the startup-phase helper.

Risks

  • Diagnostics only: no retry, hold or status behaviour changes. A misclassification only skews the log field.
  • The Collector does not export failureClass (its attributes are unchanged), so it shows in pod logs and OCC reads only.
  • Open PRs touching these files (3-way merge-tree against main vs this head):
PR conflicted files / hunks vs main vs this head
#2633 5 / 6 5 / 6 (no change)
#2543 28 / 70 28 / 71: +1 hunk in runtime-entrypoints.ts (already conflicted)
#2487 12 / 20 13 / 21: +1 hunk in runtime-entrypoints.ts
#2695, #2520, #2518, #2445, #2398, #2259, #2258, #914, #906, #884, #115 no change

The one new hunk in #2543/#2487 is the codex-login logStartupPhase call. Those PRs wrap it in if (!noInference) and this PR adds the class argument, so the resolution is to keep both. The helper include was moved to avoid a second hunk. #2599 head is not fetched locally (fork).

…out logging it

Finding 2017: a fresh Harness's first codex login or startup model check
failed after an OpenShell gateway restart, and codex's stderr is never
captured, so the sighting could not tell a transport failure from a
provider 5xx or a credential rejection.

A failed native login (runtime.startup_phase, phase codex-login) and a
failed codex.model_probe attempt now add failureClass, one of a closed
contract list: network, tls, http_401_403, http_5xx, http_other, timeout,
empty, unknown. The wrapper reads at most the last 8 KiB of codex's
stderr and matches it with indexOf and fixed-width digit reads only, so
the work is linear in that window; the output itself is never logged.
OCC's runtime log sanitizer keeps the field only on those two events and
only for a value in the list. The held runtime failure status and its
cause are unchanged.
…out applies

Internal security review nits: a URL's ?status=500 no longer names a
status, the contract says timeout applies only when the output names no
other class, and logStartupPhase notes its callers pass only the
classifier's literals.
@freeqaz
freeqaz requested a review from a team as a code owner October 19, 2026 18:58
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR adds a bounded, closed-vocabulary failure class to failed Codex login and startup model-check logs.

Example: Codex exits 1 before producing probe events and stderr reports an error sending a request.

  • Before: The log shows MODEL_PROBE_FAILED with PROCESS_EXIT exit-1.
  • After: The log also shows failureClass=network without exposing stderr.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused diagnostic improvement with no concrete defect found; evidence supports the launcher and sanitizer paths but does not demonstrate native end-to-end behavior.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored PR is outside the ordinary contributor proof gate; supplied validation exercises the generated launcher with substituted Codex results and the sanitizer separately, while inspected screenshots show unrelated UI, so the previous optional native-failure capture remains useful.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Operators cannot distinguish transport failures, provider errors, and credential rejection when Codex exits before emitting probe events.
Reason: The maintainer-authored direction addresses an observed diagnostic gap through existing owners without adding configuration or changing startup policy.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it addresses a diagnostic gap still present on main, follows the maintainer-authored direction, and has no actionable correctness or security findings.

Priority: P2
Reviewed head: a6124354964f5a1fd6501cb6f955224acdd6e9f9

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No supplied native Codex failure capture demonstrates failureClass through OCC log reads, and no failing-base/passing-head regression run is documented.
Agent review details

How this fits together

The Compute runtime wrapper runs Codex login and model checks, emits structured diagnostics, and OCC sanitizes those records for authorized CLI and console log reads.

flowchart LR
  A[Codex stderr] --> B[Runtime wrapper]
  B --> C[Bounded classifier]
  C --> D[Structured runtime logs]
  D --> E[OCC sanitizer]
  E --> F[CLI and console logs]
Loading

Technical review

Best possible solution:

Keep classification in the existing runtime wrapper and sanitizer while preserving authentication, retry, readiness, and persisted failure semantics.

Do we have a high-confidence way to reproduce the issue?

Current-main source shows that a nonzero Codex exit without protocol events loses stderr classification; the reported outage was not independently reproduced.

Is this the best way to solve the issue?

The change uses existing diagnostic owners and a closed vocabulary to expose useful evidence without guessing retry policy or publishing provider text.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 141515257737.

Provenance checked

  • Startup phase diagnostics keeps the original intent (f088251: Record startup phase timing and outcomes without provider, credential, model, or path values; the helper later moved to the shared runtime-startup module.)
  • Probe failure causes keeps the original intent (987e80e: Expose classified startup failures while keeping native output and credentials out of published evidence.)
  • Sanitized diagnostic propagation keeps the original intent (7b11528: Preserve closed-vocabulary causes through OCC sanitization so CLI and console readers can diagnose failures.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • freeqaz: Raw commit 7b11528 adds packages/occ/src/runtime-logs/sanitize.ts:240 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 7b11528c93eb; files: packages/occ/src/runtime-logs/sanitize.ts)
  • kevinlin-openai: Raw commit a97cb35 adds apps/controller/src/drivers/compute/kubernetes/runtime-entrypoints.ts:1929 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: a97cb35ed604; files: apps/controller/src/drivers/compute/kubernetes/runtime-entrypoints.ts)

Review metrics

Metric Value Why it matters
Production and test scope Production +110/-3; tests and fixtures +373/-0 Most growth covers classification, bounded scanning, secret non-disclosure, unchanged status, and sanitizer rejection rather than additional runtime machinery.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: The change improves diagnosis of observed startup failures without changing availability or recovery behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Optionally capture a native failed login or probe and its sanitized OCC log output, redacting credentials and private endpoints.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-19T19:11:40.192Z sha d5a68ad :: needs maintainer review before merge. :: none

Reviewed October 19, 2026, 3:17 PM ET / 19:17 UTC (Revision 2).

@freeqaz

freeqaz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@freeqaz
freeqaz merged commit 491f1b6 into main Oct 19, 2026
65 checks passed
@freeqaz
freeqaz deleted the diag-2017 branch October 19, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant