[poshinchen]

Replica of https://github.com/strands-agents/harness-sdk/pull/5041 (backticked: no cross-reference). Below: the v4-reason optimized review and a sanitized copy of live Strandly's actual comments.

[poshinchen]

Strandly v4-reason (optimized) review — run af1746, Opus 4.8

Assessment: approve

Assessment: Approve

Clean, well-scoped TypeScript port of the Python python_repl tool with thorough tests (shim behavior, local execution, timeout, cancellation, validation) and complete docs/README.

Review Categories

• Error handling: One important note — the timeout path mutates err.message to embed JSON partial output, forcing consumers (and the test) to parse the message string; a structured field would be more robust.
• Input validation: Minor edge cases — empty-string description bypasses the default, and whitespace-only name passes the non-empty check.
• Typing: The PythonReplOutput index signature widens the public type; a note or narrower boundary cast would preserve type safety.

Overall a solid contribution with strong test coverage and clear documentation.

[poshinchen]

Suggestion
Issue: description uses ??, so an explicitly-provided empty string (description: '') silently bypasses the default and ships an empty description to the model. name is guarded against empty, but description is not.
Suggestion: Treat an empty description as unset (or validate it), e.g. options.description || PYTHON_REPL_DESCRIPTION, to mirror the non-empty intent applied to name.

[poshinchen]

Important
Issue: The SandboxTimeoutError branch mutates err.message in place by appending the JSON partial output. If a caller inspects or logs this same error object elsewhere, they now see a message with embedded JSON. Mutating a caught error is a surprising side effect and makes the message harder to parse programmatically (the partial output is only recoverable via .split('\n')[1], as the test shows).
Suggestion: Consider attaching the partial output as a structured property on the error (or on PythonReplError-style wrapper) rather than mutating message, so consumers can read it without string parsing.

[poshinchen]

Suggestion
Issue: name is only validated as non-empty via !name, so a whitespace-only name (e.g. ' ') passes construction but is effectively invalid as a tool identifier.
Suggestion: Consider trimming before the check (e.g. if (!name.trim())) if whitespace-only names should be rejected, matching the "non-empty string" intent in the error message.

[poshinchen]

Suggestion
Issue: The timeout test asserts the partial payload by splitting the error message on \n and parsing index [1]. This couples the test to the exact message layout, so any formatting change (even an extra newline in the base message) breaks it in a non-obvious way.
Suggestion: If the partial output were exposed as a structured field on the error (see comment in python-repl.ts), this assertion could read that field directly instead of parsing the message string.

[poshinchen]

Suggestion
Issue: The PythonReplOutput interface includes an index signature [key: string]: string | number solely for JSONValue compatibility. This weakens type safety — any arbitrary string key is now accepted as valid, so typos like result.exitcode won't be caught at compile time.
Suggestion: If the index signature is required by a JSONValue constraint, a brief inline note on why it's unavoidable would help; otherwise consider a narrower cast at the boundary where JSONValue is required rather than widening the public return type.

[poshinchen]

Live Strandly's actual comments on the original PR 5041 (sanitized copy: mentions escaped, links backticked)

Review (COMMENTED):

Faithful port — nothing blocking. PYTHON_REPL_DESCRIPTION is byte-identical to Python (429 chars, compared programmatically), and timeout handling, the exit_code: 124 partial-output payload, LANGUAGE_PATTERN-at-construction, and the python3/120s defaults all match strands-py. The tests run real python3 through NotASandboxLocalEnvironment rather than echoing mocks — 19/19 pass at 174ad62, tsc/eslint/prettier clean.

Two 🟡s, both in TS-only cancellation/error plumbing that has no Python counterpart:

• context.cancelSignal isn't forwarded to executeCode, so agent.cancel() leaves the child python running (up to the 120s default). Python kills the process tree for free.
• err.message mutation leaves err.stack stale and gives TS consumers no structured access to the partial output.

One process note: this adds a public subpath export plus 5 public symbols, but carries no needs-api-review / completed-api-review label. The overall shape was settled in strands-agents#4384; the TS-only bits (MakePythonReplOptions, the PythonReplError/timeout error design, dropping shell's inputSchema option) weren't.

✅ What I verified

At 174ad625ba24ea2d920243eb7a95c88e0ff63251, base main (merge-base a2524a55):

• npx vitest run src/vended-tools/python-repl → 19/19 pass, no type errors
• npx tsc --noEmit -p src/tsconfig.json → clean; npx eslint src/vended-tools/python-repl → clean; npx prettier --check 'src/vended-tools/python-repl/**/*.ts' → clean
• Wrote an adversarial probe (uploaded as repro.ts / repro-output.txt) covering: non-positive timeout, timeout message-vs-stack, missing interpreter, LANGUAGE_PATTERN anchoring, non-Error throws, 2 MB stdout, non-ASCII source round-trip, abort signal

Port fidelity — matches:

• Description text byte-identical (429 chars both sides)
• DEFAULT_LANGUAGE = 'python3', DEFAULT_TIMEOUT = 120 ↔ _DEFAULT_LANGUAGE/_DEFAULT_TIMEOUT (python_repl.py:26-27)
• LANGUAGE_PATTERN.test() is correct here — the pattern is anchored and non-global (sandbox/constants.ts:6), which is exactly the semantics Python needed fullmatch for. Verified rejection of 'python3\n', 'python3\nrm -rf /', '../../bin/python3', 'python3;id'
• Timeout → SandboxTimeoutError with {output, error, exit_code: 124} appended as JSON. Verified the model actually receives it: tools/tool.ts:204 renders `Error: ${errorObject.message}`
• Sandbox resolution sits outside the try (python-repl.ts:102), so DefaultNotConfiguredError isn't wrapped — same as python_repl.py:81-82 and make-shell.ts:60-61
• Missing interpreter returns {output:'', error:'sh: 1: python4: not found\n', exit_code:127} rather than throwing — matches Python
• getTools() parity: neither SDK vends python_repl from a sandbox (docker.ts:83/ssh.ts:157 and docker.py:99/ssh.py:182 vend only file-editor + shell). No gap.
• Dropping shell's inputSchema option

Review (APPROVED):

Approving at 547ca72f. Follow-up on my two earlier findings:

• cancelSignal — fixed. signal: context.cancelSignal is forwarded and SandboxAbortError is rethrown before the PythonReplError wrap (python-repl.ts:105-112). Re-ran my repro: abort now rejects in ~1 ms with SandboxAbortError instead of running 3 s to completion. One caveat below the fold — it's a sandbox-layer gap, not this PR's.
• err.message mutation — withdrawn. Deliberate for Python parity; agreed.

One inline nit on the new README line, non-blocking.

✅ Verified / caveat

• npx vitest run src/vended-tools/python-repl → 15/15 pass; tsc --noEmit, eslint, prettier --check clean.
• New cancellation kills the interpreter and propagates unwrapped test (__tests__/python-repl.test.node.ts:126-133) pins the rejection type and latency — correct for the shim.
• Caveat: on NotASandboxLocalEnvironment/PosixShellSandbox the python child is not actually killed — streamProcess SIGTERMs the sh -c wrapper only, and the base64 -d | python3 pipeline survives (1.5 s after abort the pid is still alive; same on the timeout path, so this predates the PR and also affects shell). Python's stream_process.py uses start_new_session + killpg. Filed as [BUG] (ts) sandbox timeout/abort kills only the sh wrapper; the python3/pipeline child keeps running strands-agents/harness-sdk#5044; nothing to do here beyond the README wording.

Inline at strands-ts/src/vended-tools/python-repl/python-repl.ts:104:

Cancelling the agent leaves the python child running. executeCode forwards options to executeCodeStreaming (sandbox/base.ts:155-156), and ExecuteOptions.signal is honoured all the way down — stream-process.ts:128-135 calls terminate(new SandboxAbortError()) on abort — but no signal is passed here. Agent.cancel() spells out the tool's half of the contract: "If a tool is already executing, it will run to completion unless the tool checks context.cancelSignal internally" (agent/agent.ts:1065). Four sibling vended tools do (http-request, sleep, subagent, web-fetch).

The Python original gets this for free: strands-py/src/strands/sandbox/stream_process.py:9-10 — "cancelling the consuming task … runs the cleanup in finally, which kills the process" (_kill_tree, :110/:129). So the port loses behaviour Python has.

Verified with cancelSignal: AbortSignal.abort(): the tool ran to completion in 3022 ms → {"output":"ran to completion\n","error":"","exit_code":0}.

Suggestion:

        const result = await sandbox.executeCode(input.code, language, {
          timeout: input.timeout ?? DEFAULT_TIMEOUT,
          signal: context.cancelSignal,
        })

One subtlety if you take this: add if (err instanceof SandboxAbortError) throw err at the top of the catch, or cancellation gets re-wrapped as PythonReplError. Python doesn't need the guard because CancelledError is a BaseException and slips past its except Exception.

make-shell.ts:62 has the same gap — I'd leave that to a follow-up rather than widen this PR.

Inline at strands-ts/src/vended-tools/python-repl/python-repl.ts:120:

Mutating the caught error leaves stack inconsistent, and the payload is string-only. V8 snapshots stack at construction, so after the mutation the two disagree — verified:

message : Execution timed out after 0.4 seconds\n{"output":"partial\n","error":"","exit_code":124}
stack[0]: SandboxTimeoutError: Execution timed out after 0.4 seconds

Anything logging err.stack (the usual reflex) loses the partial output. Python doesn't have this problem: the traceback renders str(e) at format time, so e.args = (…) does show up in Python logs. The model path is fine — tools/tool.ts:204 uses .message, verified end to end.

A TS consumer also gets no structured access: the test has to do JSON.parse(err.message.split('\n')[1]) (__tests__/python-repl.test.node.ts:116), where the shell sibling exposes a typed ShellTimeoutError.partial (shell/types.ts:21-29).

I can see the constraint — SandboxTimeoutError builds its message from seconds (sandbox/errors.ts:14-21), so you can't construct one with an augmented message, and preserving instanceof SandboxTimeoutError is arguably better than shell (whose ShellTimeoutError isn't one). So: deliberate? If the message mutation stays, Object.assign(err, { partial }) would at least hand consumers the data without a second error class; a PythonReplTimeoutError extends SandboxTimeoutError would be the fuller fix at the cost of Python symmetry.

Inline at strands-ts/src/vended-tools/python-repl/README.md:63:

⚪ "the interpreter is killed" isn't true on the local/posix sandboxes today (streamProcess kills the sh wrapper, the python3 child survives — strands-agents#5044). The tool only hands the signal to the sandbox; worth not over-promising.

- `SandboxAbortError`: the agent cancelled the call (`context.cancelSignal`); the sandbox is told to stop the interpreter and the error propagates unwrapped.

Inline at strands-ts/src/vended-tools/python-repl/python-repl.ts:104:

Verified fixed at 547ca72f (abort rejects in ~1 ms with SandboxAbortError). The child not actually dying on local sandboxes is a pre-existing streamProcess gap — filed strands-agents#5044.

[github-actions]

Documentation Preview Failed

The documentation deployment encountered an error. Please check the deployment logs for more details.