Repository navigation
feat(shell): support detaching from remote shell - #2609
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 @@
## refactor #2609 +/- ##
=========================================
Coverage 96.61% 96.61%
=========================================
Files 702 702
Lines 58318 58344 +26
=========================================
+ Hits 56343 56370 +27
+ Misses 1975 1974 -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?
9acc92b to
33a5f52
Compare
33a5f52 to
0350a63
Compare
0350a63 to
b73b855
Compare
|
Claude Security Review: no high-confidence findings. (run) |
b73b855 to
a91e23a
Compare
|
Claude Security Review: no high-confidence findings. (run) |
a91e23a to
bdcc519
Compare
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
Minor, non-blocking: a resize event during the narrow detach window can cause the CLI to report an error even though the remote shell was detached successfully. This can be handled as a follow-up.
Otherwise LGTM!
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 💬