Skip to content

fix(responses): keep usable fallback upstream error messages - #6438

Closed
oocheol wants to merge 1 commit into
lidge-jun:devfrom
oocheol:codex/upstream-error-message-fallback
Closed

oocheol wants to merge 1 commit into
lidge-jun:devfrom
oocheol:codex/upstream-error-message-fallback

Conversation

@oocheol

@oocheol oocheol commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • A malformed or empty higher-priority error.message currently masks valid last_error or response.error text. Select the first nonblank string in the existing precedence order so provider diagnostics remain usable without inventing an error.
  • Add focused regression coverage and update the source-owner contract plus English/Korean user documentation.
  • Independent branch from dev 8b23fe340a53799aac5ad7654582a75df7ade7c8; this PR contains only this change.

Verification

Validated on Windows with Bun 1.4.0 at current head cdb83c865866e2e0b8818f103b43991fd0f2dd1e. The layout and file-size guards run from a fresh worktree with the default test budget; no guard, fixture directory, baseline cap, or timeout was changed.

  • bun run scripts/test.ts tests/lib/upstream-error-message-fallback.test.ts tests/providers/cyber-policy-error-fidelity.test.ts tests/responses/chat-refusal.test.ts tests/responses/opaque-blob-wrapped-rejection.test.ts: PASS, 103 pass / 0 fail.
  • bun node_modules/typescript/bin/tsc --noEmit: PASS, exit 0.
  • bun scripts/structure-ssot.ts: PASS, exit 0.
  • bun scripts/privacy-scan.ts: PASS, exit 0.
  • bun run build (in docs-site): PASS, exit 0.
  • bun run scripts/test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: PASS, 27 pass / 0 fail.

The first validation from the original Windows checkout failed on a pre-existing EPERM fixture directory and, in some runs, a five-second subprocess timeout. Those attempts remain historical failures; the fresh-worktree current-head guard result above replaces neither the failure record nor a required CI run. The first cold file-size scan for #6429 also exceeded five seconds, then passed at the same head on the same default budget. No inaccessible fixture was deleted and no test budget was raised.

Integration validation used actual local merges of all ten PR heads onto dev 03ed9a3b1c1535d25f8f47c3d6bf38db8ffc1fc9 (local-only assembled commit dd7d1fe254c3fc86144a2c3cce0ae483332b55bb). Owner-contract edits in runtime/config/usage now merge independently; shared observe.ts consumers merge without a source conflict. Additive EN/KO document-append conflicts and test-map insertion conflicts were reconciled by retaining both sections/registrations, not choosing one PR over another.

  • The combined 24-file focused regression set: 331 pass / 3 platform skips / 0 fail.
  • Combined layout and file-size guards: 27 pass / 0 fail with the default budget.
  • Combined typecheck, structure SSOT, privacy scan, generated operating surface check, and docs-site build: PASS.

Full bun run test and bun run test:changed remain unrun: nine small independently validated revisions plus a combined 24-file run share one Windows machine, so repeated full repository runs on each draft would be disproportionate. The explicit regression files cover the touched behavior and source-oracle consumers; broader platform/integration coverage remains for exact-head CI. Updated-head CI and automated review must be confirmed separately; older-head results, queued jobs, skipped jobs, and approval-pending runs are not passing evidence.

Review response: CodeRabbit raised no actionable comments and reported 100% docstring coverage. Source is unchanged; only the description and current validation evidence are refreshed. Added function documentation addresses the warning in source; the remote coverage percentage is not claimed as rerun. All descriptions retain only the gate-managed canonical readiness checklist.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No credential values, authentication policy, dependency installation, or workflow permissions are changed.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Upstream error reporting now skips blank or malformed message fields and uses the first valid diagnostic in the established priority order. Selected messages retain their original text.
  • Documentation
    • Clarified how upstream error details are selected when responses contain multiple message fields.
  • Tests
    • Added coverage for fallback behavior, message precedence, and responses without a usable diagnostic.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 8602bca0-b936-4987-b9db-9c31de878d89

📥 Commits

Reviewing files that changed from the base of the PR and between 3d77e3d and cdb83c8.

📒 Files selected for processing (7)
  • docs-site/src/content/docs/ko/reference/cli/agents.md
  • docs-site/src/content/docs/reference/cli/agents.md
  • scripts/test-layout/layout.json
  • src/lib/errors.ts
  • structure/transports/responses-wire-shapes.md
  • tests/fixtures/test-layout-expected.json
  • tests/lib/upstream-error-message-fallback.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The upstream error-message selector now checks supported message paths in priority order and returns the first nonblank string. Tests and documentation describe fallback behavior and preservation of the selected text.

Changes

Upstream error fallback

Layer / File(s) Summary
Select the first usable diagnostic
src/lib/errors.ts, tests/lib/upstream-error-message-fallback.test.ts, scripts/test-layout/layout.json, tests/fixtures/test-layout-expected.json, structure/transports/responses-wire-shapes.md, docs-site/src/content/docs/reference/cli/agents.md, docs-site/src/content/docs/ko/reference/cli/agents.md
The selector skips blank and non-string candidates while checking message paths in priority order. Tests cover fallback, precedence, event handling, and preservation of the selected text. Test mappings and documentation are updated to describe the behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to cdb83

No material issue is established for this change. Focused regression checks and the build were reported passing; the optional documentation-placement suggestion is unsupported by a concrete usability problem or project rule.

Architecture Summary

Architecture risk: 🔵 Low · up to cdb83

The change affects 5 systems.

Changed systems: src, docs-site, tests, scripts, structure

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.
  • observed — docs-site (service) was modified; 2 changed files map to changed impact.
  • observed — tests (service) was modified; 2 changed files map to changed impact.
  • observed — scripts (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in docs-site/src/content/docs/ko/reference/cli/agents.md: Adds a section documenting upstream error-message selection: the existing priority order chooses the first non-empty string, so empty or malformed fields do not prevent a valid later diagnostic from being used.
  • observed — Modified behavior in docs-site/src/content/docs/reference/cli/agents.md: Added an “Upstream error details” section documenting the message-selection rule for upstream error envelopes with multiple fields.
  • observed — Modified behavior in scripts/test-layout/layout.json: Added the upstream-error-message-fallback.test.ts entry, mapped to the lib suite.
  • observed — Modified behavior in src/lib/errors.ts: The comment now describes selecting the first nonblank string across the upstream error paths, replacing the description of canonical message paths.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (5 skipped: 5 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving usable fallback upstream error messages when higher-priority messages are blank or malformed.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@oocheol

oocheol commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Please review the current head cdb83c865866e2e0b8818f103b43991fd0f2dd1e. The initial automatic review was skipped while the PR is in draft.

@coderabbitai review

@codex review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

lidge-jun added a commit that referenced this pull request Oct 2, 2026
…6431 #6432 #6433 #6434 #6435 #6437 #6438) (#6447)

* fix(cli): reject ignored capabilities arguments (carry #6429)

Carries #6429 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): reject ambiguous and unsafe integer options (carry #6431)

Carries #6431 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): accept UTF-8 BOM in imported config files (carry #6432)

Carries #6432 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): validate setup ports before saving config (carry #6433)

Carries #6433 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): support JSON output on the default alias command (carry #6434)

Carries #6434 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* feat(cli): show request IDs in human log output (carry #6435)

Carries #6435 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): return not-found for missing routing profiles (carry #6437)

Carries #6437 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(responses): keep usable fallback upstream error messages (carry #6438)

Carries #6438 by @oocheol onto current dev as part of the oocheol CLI batch.

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(cli): ask for the setup port again instead of abandoning ocx init

Amends the #6433 carry: an invalid interactive port now reports the error and re-asks, so a typo no longer discards every earlier answer. EOF and SIGINT at the repeated prompt still cancel without saving (exit 1 / 130); the wizard tests cover all three paths.

Co-authored-by: 정우철 <oocheol@naver.com>

* test(responses): pin recovery consumers of the upstream message fallback

Follow-up to the #6438 carry: encrypted-output resend and quota alternate-account detection now see a fallback message hidden behind a blank primary one; a nonblank primary still wins.

Co-authored-by: 정우철 <oocheol@naver.com>

* docs(cli): separate the carried reference sections

Co-authored-by: 정우철 <oocheol@naver.com>

* chore: keep the lane plan record out of the carry

---------

Co-authored-by: 정우철 <oocheol@naver.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Thanks @oocheol — this landed on dev through the batch carry #6447 (one commit per source PR, Co-authored-by credit). Closing as superseded.

@lidge-jun lidge-jun closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants