Repository navigation
feat(shell): support detaching from remote shell - #2609
nborges-aws wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice, focused change. The detach handling in InteractiveTerminal.run is coherent:
onDatasetsdidDetachbefore enqueueing to ensure further input (and post-escape bytes in the same chunk) are intentionally dropped.- Pre-escape bytes are still forwarded; the detach callback and
resolveDetach()are serialized through the samependingqueue so ordering is correct. shell.tscorrectly skips the finally-blocksession.close()whenonDetachalready closed the peer, avoiding a double-close, and the test assertscloses === 1.- Removing
reconnectedfromRuntimeShellSdkSessionis safe — it was never read; reconnection status is already delivered via metadata/onReconnectinharnessShell.ts.
A couple of minor observations that aren't blockers:
- When
onDetachis set, Ctrl+] can no longer be forwarded to the remote shell (e.g., for tmux prefix). This is intentional given the UX, but worth keeping in mind if someone later asks for an escape to send a literal0x1d. - If
onDetachitself were to throw,resolveDetachisn't called and the operation surfaces the queued failure instead of returning"detached". That's reasonable, though in practicesession.close()is unlikely to reject.
No telemetry gap introduced — the shell handler doesn't currently emit telemetry, so this change isn't a regression there.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## shell-resume-command #2609 +/- ##
========================================================
- Coverage 96.60% 96.60% -0.01%
========================================================
Files 700 700
Lines 57681 57709 +28
========================================================
+ Hits 55723 55750 +27
- Misses 1958 1959 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
notgitika
left a comment
There was a problem hiding this comment.
One thing to address with the bedrock-agentcore version bump: the SDK change that makes close() detach also changes reconnectConfig.onReconnect from (reconnected: boolean) => ... to () => ....
The CLI still passes the boolean callback through in src/core/runtimeShell.ts and src/core/harnessShell.ts, so using the new SDK types currently fails typecheck. Could you update RuntimeShellRequest/ShellOperation to use the no-argument callback and adjust the reconnect message and tests as part of the version bump?
Description
Add
Ctrl+]as keybind to detach from a runtime or harness shell, keeping the remote shell alive. The local terminal is restored and a reconnect command with session and shell id is printed, allowing for reconnection. Normal shell exit behavior is unaffected by this change.This change is dependent on: aws/bedrock-agentcore-sdk-typescript#249, which enables the remote shell to stay open when detach is sent to the SDK. For detach to be fully functional in the CLI, this change will need to be released in TS SDK and the pinned SDK version will need to be updated in the CLI.
Type of Change
Testing
How have you tested the change?
bun test(x pass, 0 fail)bun run test:e2e, or explained why they are not applicablebun run typecheckbun run lint:checkbun run format:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.
Stack created with GitHub Stacks CLI • Give Feedback 💬