fix(terminal): realign legacy residual output metadata - #9720
Conversation
📝 WalkthroughWalkthroughTerminal subscribe buffering now uses a shared helper to align 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f7bca9f-eaa1-4573-984f-55c930988b6f
📒 Files selected for processing (2)
src/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/terminal-subscribe-buffer.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/runtime/rpc/terminal-subscribe-transformed-residual.test.ts (1)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the partial-runtime double assertion.
as unknown as OrcaRuntimeServicedeliberately bypasses structural checking for this partial mock; add a concise rationale.Proposed change
+ // This test only exercises the runtime surface required by terminal.subscribe. } as unknown as OrcaRuntimeServiceAs per coding guidelines, “For non-obvious TypeScript code, add a concise comment explaining why the constraint exists, not how the code works.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ebc56ac-e366-474c-bfcc-fb9e6feedc7d
📒 Files selected for processing (1)
src/main/runtime/rpc/terminal-subscribe-transformed-residual.test.ts
132b81d to
392d794
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 剥离查询字节后，普通残余需按实际数据重对齐，避免被误编码为 OutputSpan。 | ||
| function realignResidualOutputMeta( |
There was a problem hiding this comment.
| // 剥离查询字节后，普通残余需按实际数据重对齐，避免被误编码为 OutputSpan。 | |
| function realignResidualOutputMeta( | |
| // After query bytes are stripped, plain residual output must be realigned to the actual data length to avoid being misencoded as an OutputSpan. | |
| function realignResidualOutputMeta( |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // 为什么：此用例只实现 terminal.subscribe 触及的 runtime 表面，避免构造无关方法。 | ||
| } as unknown as OrcaRuntimeService |
There was a problem hiding this comment.
| // 为什么：此用例只实现 terminal.subscribe 触及的 runtime 表面，避免构造无关方法。 | |
| } as unknown as OrcaRuntimeService | |
| // Why: this case only implements the runtime surface that terminal.subscribe touches, avoiding construction of unrelated methods. | |
| } as unknown as OrcaRuntimeService |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR fixes a metadata misalignment bug in the mobile/legacy terminal subscribe path: after a snapshot boundary clips a buffered chunk and query-suffix stripping further shortens it, the old code updated
Confidence Score: 4/5Safe to merge; the change is a narrow, well-reasoned metadata realignment that does not alter data bytes or control flow beyond the specific residual replay path. The core logic in Files Needing Attention: No files require special attention beyond the two inline comment suggestions.
|
| Filename | Overview |
|---|---|
| src/main/runtime/rpc/methods/terminal.ts | Adds realignResidualOutputMeta and calls it after query stripping rather than before; removes the pre-stripping uncoveredMeta update that had a race between slice length and strip length. Logic and math check out; Chinese-language inline comment is inconsistent with the rest of the file. |
| src/main/runtime/rpc/terminal-subscribe-buffer.test.ts | Renames the test to match its new coverage focus, changes snapshot seq from 4→2 and data seq from 4→9 to force a genuine cross-boundary slice, and updates expected output accordingly. Test now exercises the stripped + coalesced path the fix targets. |
| src/main/runtime/rpc/terminal-subscribe-transformed-residual.test.ts | New test verifying that transformed spans keep their original seq/rawLength/transformed metadata when replayed after a snapshot boundary trim. Comment at line 62 is in Chinese, inconsistent with the codebase style. |
Sequence Diagram
sequenceDiagram
participant D as dataListener
participant P as pendingOutput buffer
participant S as serializeTerminalBuffer
participant G as getOutputAfterSnapshotSeq
participant Strip as stripSnapshotBoundaryQuerySuffixes
participant R as realignResidualOutputMeta (NEW)
participant B as outputBatcher
D->>P: "push('\x1b[6nafter', {seq:9, rawLength:9})<br/>while buffering=true"
Note over D,P: query '\x1b[6n' added to pendingQuerySequences
S-->>G: "snapshot resolves {seq:2}"
Note over G: snapshotOutputSeq = 2
G->>G: "slice data.slice(2) → '6nafter'<br/>(uncoveredData ≠ item.data → enter strip block)"
G->>Strip: stripSnapshotBoundaryQuerySuffixes('6nafter', 2, 2, queries)
Strip-->>G: 'after' (removed '6n' bytes [2..4) of original stream)
Note over B: queryReplayData path
B->>B: push('\x1b[6n', no meta) — plain batch
G->>R: "realignResidualOutputMeta('after', {seq:9, rawLength:9})"
Note over R: !transformed → return {seq:9, rawLength:5}
R-->>B: "push('after', {seq:9, rawLength:5})<br/>rawLength==data.length → plain batch, lastSeq=9"
B->>B: "flush() → coalesce '\x1b[6nafter'<br/>meta: {seq:9, rawLength:9}"
B-->>Client: "Output frame '\x1b[6nafter'<br/>(NOT OutputSpan — no redundant resync)"
Reviews (1): Last reviewed commit: "test(terminal): cover transformed residu..." | Re-trigger Greptile
3 participants
Summary
OutputSpanand triggering a redundant resync.Fixes #9682
Screenshots
No visual change.
Testing
pnpm lint- scoped oxlint and the max-lines ratchet pass; the full command is blocked by a pre-existing non-exhaustive switch inskill-freshness-group.tsx:101.pnpm typecheckpnpm test- 33,490 passed and 56 skipped; 7 environment-dependent failures remain in unrelated submodule, zsh path lookup, and system SSH tests. The two directly affected replay suites pass 11/11.pnpm run build:desktopAI Review Report
Reviewed the post-strip ordering,
rawLengthand sequence semantics, preservation of genuinely transformed spans, output batching behavior, and regression coverage. The initial review strengthened the existing query replay harness with zero net line growth. Follow-up coverage for sparse transformed output lives in a dedicated test file because the existing buffering suite is at its effective line limit, without adding a max-lines bypass. Cross-platform compatibility was checked for macOS, Linux, and Windows: this change is platform-neutral protocol metadata logic and touches no shortcuts, labels, paths, shell behavior, or Electron platform branches. The same behavior applies to local, SSH, and remote terminal streams.Security Audit
Reviewed terminal data handling and frame metadata boundaries. The change only realigns an existing string-length field after existing query removal and does not execute or reinterpret terminal bytes. It adds no command execution, path handling, authentication, secrets, dependencies, permissions, or new IPC surface.
Notes
This is the mobile and legacy sibling of the multiplex residual metadata handling described in #8255.
X handle: N/A
ELI5
Legacy clients could get residual terminal text as the wrong span type and force a redundant full resync. Metadata is realigned after suffix stripping so plain residual output does not look like a structured span.