fix(runtime-host): retain actionable compatibility handshake details for remote profiles - #3246
Conversation
Generated-by: Codex
Generated-by: Codex
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR gives remote Runtime Host compatibility failures a typed, stable error path. It preserves profile identity, compatibility epoch, protocol range, composition identity, and revision in structured diagnostics. CLI, TUI, and Desktop receive consistent diagnostics and recovery guidance. The change extends the existing compatibility gate and permanent reconnect-error path. It does not create a parallel retry or recovery path. Incompatible connections remain non-retryable and do not replay commands, change profiles, or update the Host automatically. The solution is focused and coherent. The new error module centralizes formatting, protocol-range checks, sanitization, and recovery guidance. Unicode Tests cover epoch, protocol-range, and composition mismatches, sensitive-data exclusion, Unicode sanitization, unavailable Desktop profiles, rejected queued queries, no replay, and a single connection attempt. Reported validation includes 1,010/1,010 Runtime Host tests, typecheck, lint, and format checks. English and Chinese documentation describe compatible builds and required Host service restarts. Complexity delta
Review-relevant risks
These user-visible, public-contract, security-sensitive, and operational-recovery changes require independent human review under repository policy. No licensing, release, or governance effect was identified in the current diff. Required-check results are based on reported verification and remain unverified here. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughThe PR adds a typed remote compatibility error that preserves handshake details, prevents incompatible reconnects, projects unavailable states in Desktop, and provides consistent CLI/TUI diagnostics and documentation. ChangesRemote compatibility handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to Compatibility errors may still render remote Host composition data containing control characters, making CLI, TUI, or Desktop guidance misleading or unsafe to read. The PR is otherwise mergeable with explicit owner awareness and follow-up on sanitizing this diagnostic output. Sequence Diagram(s)sequenceDiagram
participant RemoteHost
participant host_profile
participant ClientSurface
RemoteHost->>host_profile: return HostIncompatible handshake
host_profile->>ClientSurface: raise RuntimeHostRemoteCompatibilityError
ClientSurface-->>RemoteHost: show diagnostics or unavailable profile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoRetain remote Runtime Host compatibility diagnostics
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 3d417486fb5a55d4b3e5ee4de1ee9d02cfe923de.
The typed compatibility error belongs at the Runtime Host client boundary, and the permanent/no-retry/no-replay behavior remains coherent across CLI, TUI, and Desktop. Current CI is green, and the PR body plus both commits carry complete Codex provenance disclosure.
Two items remain before approval:
- The existing Qodo inline finding is a valid P2; I confirmed the failure path and replied in that thread with the smallest projection-boundary fix.
- This changes user-visible compatibility errors in CLI, TUI, and Desktop. Please add representative screenshots of a terminal surface and the Desktop recovery dialog; before/after captures are preferred.
No additional P0-P2 findings were found. Because this changes a public client contract and security-sensitive diagnostics, independent human review is still required before merge.
AI-assisted review disclosure: Codex reviewed the exact-head diff, existing feedback, projection paths, tests, live CI, thread state, UI evidence, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
整体职责边界、永久失败语义和跨端共用投影都合理，当前 CI 全绿，AI disclosure 也完整。
合并前仍有两项：现有 Qodo 评论属于有效 P2，需要在可读消息投影处清理换行分隔符和 bidi controls，同时保留结构化 details 原值；此外该 PR 改变 CLI、TUI、Desktop 的用户可见错误，请补 terminal 和 Desktop recovery dialog 截图，最好包含前后对比。
public client contract 与安全相关诊断仍需独立人工审查。
Replace Unicode control and formatting characters only in the human-readable Host revision projection while retaining the raw typed detail. Generated-by: Codex
Review evidenceFixed the inline security finding in Terminal surfaceRepresentative output produced through the real 
Desktop recovery surfaceProduction-backed Settings page after enabling the incompatible remote profile; Local remains available, the remote Host becomes unavailable, and the shared recovery message is shown: 
Verification after the fix:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-host/src/client/remote-compatibility-error.ts (1)
69-73: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize the Host composition ID before formatting.
Line 72 interpolates
details.host.compositionIdwithout the existing sanitizer. TheHostIncompatible.compositionIdvalue comes from the remote handshake. A remote Host can therefore inject ANSI or other control characters into CLI, TUI, or log output.Apply
formatRuntimeHostDiagnosticValueto the Host composition ID. Keep the structured detail unchanged.Disposition: fix-now.
Proposed fix
- `Host composition id ${details.host.compositionId}`, + `Host composition id ${formatRuntimeHostDiagnosticValue(details.host.compositionId)}`,As per path instructions, reuse the closest existing seam before adding new behavior; this fix reuses
formatRuntimeHostDiagnosticValue.Source: Path instructions
🧹 Nitpick comments (1)
packages/runtime-host/src/__tests__/host-profile.test.ts (1)
721-726: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
as nevercast from the fixture.The cast disables TypeScript validation for the
activitypayload. The fixture can continue to compile afterHostActivitySnapshotchanges even when its shape is invalid. Use the actualHostActivitySnapshottype, or omitactivitywhen the tests do not need it.Disposition: optional.
As per path instructions, choose the smallest coherent solution and flag concrete test-maintenance risks.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97fb1eda-f493-4607-a482-b905c6fc60fb
📒 Files selected for processing (2)
packages/runtime-host/src/__tests__/host-profile.test.tspackages/runtime-host/src/client/remote-compatibility-error.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Preserve upstream lifecycle-aware CLI conflict coverage alongside remote compatibility projections. Generated-by: Codex
Keep structured compatibility details exact while replacing control characters in the human-readable Host composition id. Use the typed Host activity fixture shape in coverage. Generated-by: Codex
|
Addressed the latest CodeRabbit review in dafb60f: the human-readable Host composition id now uses the same Unicode control-character sanitizer as composition revision, while structured details retain the exact remote value. The regression test was verified red then green. I also removed the optional as-never fixture cast by using the real HostActivitySnapshot shape. Fresh verification: Runtime Host 1010/1010, typecheck, lint, and format:check all pass. |
|
@Astro-Han the requested sanitizer fix and representative screenshots are now present, and the branch has been merged with current main. When convenient, please re-review exact head dafb60f. GitHub did not allow this fork author to create a formal review request through the API. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head dafb60f29244248bcafad7dc8475c8794e94d7d3.
The earlier diagnostic-injection finding is fully addressed: both Host-controlled composition fields are sanitized at the human-readable projection boundary, while typed structured details preserve their exact values. The regression covers control characters, bidirectional formatting, and Unicode line/paragraph separators.
I also verified the requested terminal and Desktop screenshots. The existing thread is resolved, all checks are green, all five commits carry Generated-by: Codex, and I found no additional P0–P3 issues. Because this changes a public contract, security diagnostics, and user-visible behavior, please retain the required independent human review before merge.
AI-assisted review disclosure: Codex reviewed the exact-head diff, prior feedback and remediation, tests, live threads and CI, screenshots, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
原诊断注入问题已完整修复：两个 Host 控制字段只在可读消息投影时清理危险 Unicode 字符，结构化 details 仍保留原值；测试覆盖充分。Terminal 与 Desktop 截图、线程、CI 和 AI trailer 均已核实，没有新的 P0–P3。该 PR 涉及公开契约、安全诊断及用户可见行为，合并前仍需独立人工审查。
2 participants
Summary
Fixes #3203
Verification
Root npm test was also run but is not fully green because of two failures outside this diff that reproduce independently on this macOS environment:
Review focus
This adds a public Runtime Host client error contract and therefore requires the independent human review described in CONTRIBUTING.md. Please pay particular attention to the details allowlist and the permanent reconnect classification.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the typed compatibility error, formatter, projections, tests, and documentation. Both commits contain Generated-by: Codex trailers.
Checklist
Does this PR entail a change in behavior?