Skip to content

fix(runtime): update shell session wire protocol - #1968

Open
priyasingh2111 wants to merge 1 commit into
mainfrom
fix/shell-fixes
Open

fix(runtime): update shell session wire protocol#1968
priyasingh2111 wants to merge 1 commit into
mainfrom
fix/shell-fixes

Conversation

@priyasingh2111

Copy link
Copy Markdown

Description

Remove the 0x03 metadata frame dependency and 0xFF close frame send from the shell connection flow to align with the K8s wire protocol.

Changes:

  1. Removed encodeClose() method from ShellFramer — CLI already uses ws.close() for detach, this was dead code
  2. Removed confirmation frame blocking wait — connection is now ready immediately after WebSocket opens + shellId read from the 101 upgrade
    header (eliminates up to 10s wait)
  3. Removed reconnected / bytesDropped from ShellConnection — these were populated by the confirmation frame we no longer wait for
  4. Confirmation frames silently swallowed during iteration — forward-compatible if server still sends them

Exit codes, keepalive/reconnect, and Ctrl+] detach all continue to work unchanged.

Related Issue

Closes #

Documentation PR

N/A — no user-facing documentation changes needed.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Results:

  • 6,068 unit tests pass (422 test files)
  • 0 lint errors (33 pre-existing warnings unchanged)
  • Format check clean
  • Live integration tests: 10/10 against prod (connection in ~3s, no 10s wait)

Breaking changes:

  • ShellConnection.reconnected and ShellConnection.bytesDropped no longer exist
  • ShellReconnectOptions.onBytesDropped and onNewSession callbacks removed
  • ShellFramer.encodeClose() method removed
  • Old clients will hard-fail (10s timeout then error) when the server stops sending the 0x03 confirmation frame — this is an accepted breaking change
    per team decision (no protocol negotiation)

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

@priyasingh2111
priyasingh2111 requested review from a team and padmak30 August 11, 2026 03:01
@github-actions github-actions Bot added size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress labels Aug 11, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Aug 11, 2026
@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 11, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Aug 11, 2026

ws.on('message', (data: Buffer) => {
// Connection is ready immediately after WebSocket opens — no confirmation frame wait.
ws.on('open', () => {

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.

Could we update the close handling so only code 1000 is treated as a clean exit? Now that the connection resolves when the WebSocket opens, an abnormal close such as 1006 can happen before the shell is usable. Treating that as exit 0 reports a failed connection as successful.

settled = true;
resolve({
ws,
shellId: shellIdFromHeader ?? shellId ?? '',

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.

Could we return an error when a new connection does not include the shell ID header? Returning an empty shell ID means the user cannot reconnect to the session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants