Skip to content

fix(responses): reject malformed retained compact input before routing - #8433

Merged
lidge-jun merged 10 commits into
lidge-jun:devfrom
luvs01:codex/fix-ocx2-input-validation-error
Nov 10, 2026
Merged

lidge-jun merged 10 commits into
lidge-jun:devfrom
luvs01:codex/fix-ocx2-input-validation-error

Conversation

@luvs01

@luvs01 luvs01 commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Reject malformed OpenCodex retained-compaction (ocx2:) input with HTTP 400 invalid_request_error before routing, account selection, credential refresh or upstream dispatch. Valid envelopes keep their existing send-time expansion. Add pooled-account and no-dispatch regressions, plus the canonical and affected translated compact references.

Carries luvs01/opencodex#813 at 16e55c7df92643fca035437cdbb6186d113a1608, preserving its commit history and authorship.

Verification

  • Parent head 2a1f907c04f622f2354f8a68010c6fa1bac73f29: Cross-platform CI 42570570874 passed its executed gates, changed-test suite, docs build, API/storage checks and smoke jobs. The full platform matrix was skipped; this is not Windows/macOS acceptance.
  • The same parent head: CodeQL 42570566921 passed.
  • The final 16e55c7 commit adds only a five-line JSDoc comment to the compact entrypoint; executable code and tests are unchanged from that passing parent. New-head checks are not yet claimed as passing.
  • Reviewed the final source diff and prior review fixes, including validation before credential refresh and translated references. No source code was changed for this transfer, so passing unchanged checks were not duplicated locally.
  • Remains draft pending checks/review in the upstream repository. No merge or deployment is requested by this publication.

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.

Co-authored-by: Epinephrine luvs01@hanmail.net
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Summary by CodeRabbit

  • Bug Fixes

    • Malformed retained-compaction input now returns HTTP 400 with an invalid_request_error before routing, credential refresh, or upstream requests.
    • These validation errors do not trigger upstream retries or affect pooled account health. Valid requests continue to work.
  • Documentation

    • Updated the compact-endpoint reference in English, Japanese, Korean, Russian, and Simplified Chinese to describe the error response and validation behavior.

@coderabbitai

coderabbitai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Walkthrough

The compact endpoint now rejects malformed proxy-owned ocx2: retained-compaction input with HTTP 400 and invalid_request_error. Validation occurs before routing and credential resolution. Tests cover upstream requests, pooled account health, and credential refresh; the reference documentation describes the behavior in five languages.

Changes: Retained-compaction validation

Layer / File(s) Summary
Envelope detection
src/responses/retained-compaction.ts
Adds a specific error type and a finder for marked retained-compaction items that fail decoding. Expansion uses the shared type guard and throws the specific error for invalid envelopes.
Compact endpoint rejection and coverage
src/server/responses/compact.ts, tests/responses/compact-transient-alternate.test.ts, tests/responses/responses-native-compact-ocx-envelope.test.ts, docs-site/src/content/docs/reference/proxy-formats.md, docs-site/src/content/docs/ja/reference/proxy-formats.md, docs-site/src/content/docs/ko/reference/proxy-formats.md, docs-site/src/content/docs/ru/reference/proxy-formats.md, docs-site/src/content/docs/zh-cn/reference/proxy-formats.md
The handler rejects malformed input before the body-read callback and routing, and maps the validation error to HTTP 400. Tests check that invalid input causes no upstream fetch or credential-refresh fetch and does not change pooled account health. The documentation describes the response and validation timing.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

  • lidge-jun/opencodex#8151: Changes how valid retained-compaction envelopes are lowered before native compact requests are sent; this PR adds validation for malformed envelopes.
  • lidge-jun/opencodex#7774: Changes native-main credential discovery and refresh behavior in the compact path; this PR validates malformed input before credential resolution.
Merge Risk: Minimal · up to 16e55

The reviewed change has no established issue that needs correction before merge. Normal checks remain appropriate for the final commit.

Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the main change: rejecting malformed retained-compaction input before routing.
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 with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 4 files. (5 skipped: 5 unsupported.)

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Nov 9, 2026
@luvs01
luvs01 marked this pull request as ready for review November 10, 2026 01:43
@chatgpt-codex-connector

Copy link
Copy Markdown
Codex Review Summary: ❌ Failed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review ❌ Failed 2026-11-10T01:43:16.236609Z
We couldn’t complete this review. Comment @codex security review to try again.
16e55c7 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments: 1

src/server/responses/compact.ts-1318-1320: 1

1318-1320: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Remove or justify the unreachable InvalidRetainedCompactionEnvelopeError mapping in localDispatchRefusal.

handleResponsesCompact now rejects malformed envelopes at Line 711, before any send. The only InvalidRetainedCompactionEnvelopeError source in the send path is expandRetainedCompactionInput. buildCompactSendBody does not call that function directly. scrubOcxCompactionItems may call it. Verify whether it can still throw after pre-validation. If it cannot, this branch is dead code. Dead code on a security-adjacent error path invites confusion.

If the branch is a deliberate defense-in-depth measure, add a comment that says so.

Review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between f0f34d0 and 16e55c7.

Files selected for processing: 9

  • docs-site/src/content/docs/ja/reference/proxy-formats.md
  • docs-site/src/content/docs/ko/reference/proxy-formats.md
  • docs-site/src/content/docs/reference/proxy-formats.md
  • docs-site/src/content/docs/ru/reference/proxy-formats.md
  • docs-site/src/content/docs/zh-cn/reference/proxy-formats.md
  • src/responses/retained-compaction.ts
  • src/server/responses/compact.ts
  • tests/responses/compact-transient-alternate.test.ts
  • tests/responses/responses-native-compact-ocx-envelope.test.ts

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

@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer judgment

Integrate at 16e55c7df92643fca035437cdbb6186d113a1608, subject to an unchanged head and a mergeable PR. This fixes a real user problem: malformed OpenCodex retained-compaction input now gets a useful 400 before routing or credential refresh instead of a late failure that can affect an account. The intended API change applies only to malformed proxy-owned envelopes; valid envelopes retain their existing expansion. There is no GUI or CLI layout change and no settings migration.

Existing users benefit from earlier rejection of requests that could not succeed. The affected code uses JSON and Base64 rather than platform-specific APIs; Windows and macOS were not independently exercised on this head. Documentation and regression coverage accompany the change; structure, file-size, Lab boundary and privacy were reviewed.

A fresh independent reviewer passed this exact head and reported 20 focused tests and 827 broader compaction tests passing, typecheck and repository gates passing, plus a base/head reproduction showing malformed requests no longer refresh credentials or mark an account unhealthy. I independently inspected the diff and verified the exact head and successful gates and test changed in CI run 42575482954. The CI aggregate succeeded. No actual change-caused CI failure was identified.

NOT RUN by maintainer: local tests, full suite, Windows/macOS matrix, live provider request. The reviewer's local commands were not independently rerun. The hosted security reviewer reported a usage limit, not a security approval.

Maintainer integration decision: I intend to integrate under the dev-only maintainer policy if GitHub requires an approval bypass; this does not substitute for the independent review above.

@lidge-jun
lidge-jun merged commit 550d0d6 into lidge-jun:dev Nov 10, 2026
35 checks passed
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