fix(responses): give routed models the Codex App visualization directive in ASCII - #6045
Conversation
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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 configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe parser now converts valid private-use visualization references in model-visible conversation text into Codex App directives. Raw request bodies, stored continuation history, and non-text content remain unchanged. Tests and documentation describe the conversion rules and request-parsing scope. ChangesVisualization Directive Normalization
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Parser as parseRequest
participant Normalizer as normalizeVisualizationContext
participant Adapter as Anthropic adapter
Parser->>Normalizer: assembled conversation context
Normalizer-->>Parser: context with valid directives normalized
Parser->>Adapter: parsed request context
Adapter-->>Adapter: serialize message text
Merge Risk: ⚪ Minimal · up to No concrete issue currently blocks merging. Confirm live rendering as planned after rebuilding the app. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects what routed models see, but the reviewed code preserves the original request and validates references before rewriting them. No introduced security vulnerability was established. The app’s rendering behavior was not independently verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
Summary
With a Claude-routed model, the Codex App Visualize plugin printed
visualize{"path":…}as plain text instead of drawing the visualization. The plugin asks the model to reply withU+E200 visualize U+E202 {json} U+E201, and every Claude route we checked (direct Anthropic and through Cursor) drops those private-use characters before the model sees them. The model reads the template without its markers and writes it back the same way. A code-point echo test shows the difference: GPT and Grok return all six characters of[A U+E200 B U+E202 C U+E201], while Claude returns onlyA B C. OpenCodex's own Anthropic request body still contains them.The app does not need the private-use form. Its renderer (app 26.924.22138) rewrites each span into the plain directive
::codex-inline-vis{path="…"}before parsing, and it renders that directive when it appears directly. This PR applies the same rewrite to the conversation text that routed models read:src/responses/visualization-directives.tsconverts eachU+E200visualize U+E202…U+E201span into::codex-inline-vis{…}(or::codex-live-vis{…}), using the app's own payload rules. Those rules cover JSON versus bare path, the.htmlbasename,..and quote rejection, absolute-path detection including Windows and UNC, the path/file attribute, title, and wide mode. A span the app would reject is left exactly as written. The scan matches the app's regex in linear time.<absolute-path>/<title>.htmlplaceholder is accepted, so the model still sees the template.parseRequestapplies the rewrite to the context it returns, so every context-built adapter gets it._rawBodyis untouched, which keeps native passthrough byte-identical and storedprevious_response_idhistory unchanged.The citation filter from #6040 is unchanged. Replies already saved in the bare form are not repaired.
Verification
bun test tests/responses/visualization-directives.test.ts: 18 pass. The three parser and Anthropic integration cases fail with theparser.tshook removed.bun run typecheck,bun run structure:check, andbun run privacy:scanpass.toAsciiDirectiveand the scanner against a port of the app'sf2over 132,715 generated cases and found no mismatch outside the two documented differences. It also checked that frozen inputs are not mutated and that_rawBodystays identical.bun test tests/responses tests/adapterslocally: 5885 pass and 134 fail. Every failing file either passes when run alone or fails identically with theparser.tshook removed, because this checkout sits under~/.codexand cross-file state leaks there. The full suite is left to CI.dev. Computer Use cannot inspect the Codex app, so a person has to confirm it.Checklist
Summary by CodeRabbit