[poshinchen]

Replica of https://github.com/strands-agents/harness-sdk/pull/4971 (URL in backticks to avoid a cross-reference on the original PR) for reviewer evaluation. Branches mirror the original PR's base and head commits.

The review below was produced by the optimized Strandly reviewer (variant v3-reason, optimization run aeb3ee, agent model Opus 4.8) during validation on PRs it never saw in training. Compare with the human and Strandly reviews on the original PR.

[poshinchen]

Strandly v3-reason (optimized) review

Assessment: comment

Assessment: Comment

This is a well-structured, thoroughly documented port that mirrors the Python feature closely, with strong test coverage (depth capping, context forking, interrupts, schema derivation). My feedback centers on one recurring theme plus an API-process note.

Review Categories

• Maintainability: The default builder and its tests reach into Agent's private fields (_sandbox, _printer, _tracer._traceAttributes) via as unknown as casts. This bypasses type checking, so a field rename breaks propagation silently. Prefer public/internal accessors.
• Behavior correctness: printer: printer !== undefined collapses a possibly-custom Printer to a boolean; please verify this matches the "built the way the parent was" intent and Python's behavior for string/router models.
• Testing: Where propagated child config is deterministic, prefer a single object-shape assertion over per-field checks that can miss regressed fields.
• API process: New public subpath — please confirm it carries api/needs-review.

Nice work bringing TypeScript to parity with the Python subagent tool, including the matching docs and README.

File-level comments:

• strands-ts/src/vended-tools/subagent/index.ts: Important

Issue: This PR introduces a new public subpath export @strands-agents/sdk/vended-tools/subagent with new customer-facing APIs (makeSubagent, MakeSubagentOptions, SubagentInput, etc.). I can't confirm from the diff whether the PR carries the api/needs-review label.
Suggestion: Please ensure this change goes through API bar-raising review and carries the api/needs-review label, since it adds public primitives customers will depend on. The PR description's API section is thorough, which should make that review straightforward.

Inline comments (could not anchor to the diff):

• strands-ts/src/multiagent/spec.ts:410: Important

Issue: The default builder reaches into the parent's private fields via as unknown as casts (_sandbox, _printer, _tracer, _traceAttributes) to propagate sandbox, printer, and trace attributes. This couples the builder to Agent's internal representation, so a rename or refactor of those fields silently breaks propagation with no compile-time signal.
Suggestion: Please consider exposing these via public/internal getters on Agent (e.g. agent.sandbox, agent.traceAttributes) and reading those instead, so the contract is checked by the type system. If public accessors are intentionally avoided, please document why here.

• strands-ts/src/multiagent/spec.ts:441: Important

Issue: printer: printer !== undefined collapses the parent's printer configuration to a boolean. If the parent was constructed with a custom Printer instance (not just true/false), the child receives a default printer rather than the parent's, which differs from "built the way the parent was."
Suggestion: Please verify the intended behavior — if the goal is to mirror the parent's printer, consider passing the actual printer instance through (when available) rather than coercing to a boolean. If a boolean is deliberate for isolation, a brief comment explaining the tradeoff would help.

• strands-ts/src/multiagent/spec.ts:436: Suggestion

Issue: The stateful check typeof childModel === 'object' && 'stateful' in childModel && childModel.stateful can match on a string model name that was resolved from a Choice only via the typeof === 'object' guard, which is fine, but when childModel is a bare model string the 'auto' context manager is always applied regardless of statefulness.
Suggestion: Please verify this matches Python's behavior for string/router models. If statefulness can only be known for Model objects, a short comment noting that string/router models always get 'auto' would clarify intent for future readers.

• strands-ts/src/multiagent/spec.ts:440: Suggestion

Issue: traceAttributes: { ...traceAttributes } is spread from a value read via unchecked private-field casts (_tracer._traceAttributes). If _tracer is ever undefined at build time, this throws; the cast asserts it is always present.
Suggestion: Please verify _tracer is guaranteed to be initialized on the parent at the point _defaultBuilder runs. A defensive ?? {} on the attributes (or reading from a public accessor) would avoid a runtime crash if that invariant ever changes.

• strands-ts/src/multiagent/__tests__/spec.test.ts:33: Suggestion

Issue: internals() reaches into _printer and _tracer._traceAttributes via as unknown casts, mirroring the production code's private-field access. These tests will pass even if the public contract regresses, and break on an internal rename unrelated to behavior.
Suggestion: If Agent exposes (or can expose) public getters for sandbox/trace attributes/printer, please assert against those instead so the test validates observable behavior rather than internal field names.

• strands-ts/src/multiagent/__tests__/spec.test.ts:268: Suggestion

Issue: This asserts statefulness/context-manager behavior only via a boolean (contextManager !== undefined), while the sandbox/printer test (lines 265-278) uses three separate expect calls on child.sandbox, trace attributes, and printer. Where the full set of propagated fields is deterministic, a single object-shape assertion would catch an unexpected or regressed field that per-field checks silently miss.
Suggestion: Please consider asserting the relevant child configuration as one object equality where feasible, reserving per-field checks for genuinely non-deterministic values.

• strands-ts/src/vended-tools/subagent/subagent.ts:105: Suggestion

Issue: presets is shallow-copied with { ...options.presets }, but each Preset value is shared by reference. If a Preset instance is mutable and reused across multiple makeSubagent calls, state could leak between tools. Please verify Preset is immutable after construction.
Suggestion: If Preset is immutable, no change needed — a one-line note would confirm intent. If it is mutable, consider documenting that callers must not mutate presets after passing them.

• site/src/content/docs/user-guide/sdk/tools/vended-tools.ts:303: Suggestion

Issue: In this example the local const subagent = makeSubagent({...}) shadows the imported subagent default tool. Since the custom import block only imports makeSubagent, this works, but naming the custom tool subagent (same as the exported default) in docs may confuse readers into thinking they must reuse that name.
Suggestion: Consider naming the local variable something role-specific (e.g. delegate) in the doc example to make clear it's an independent tool instance, matching the PR description's own delegate naming.

[github-actions]

Documentation Preview Failed

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

[poshinchen]

Strandly v4-reason (optimized) review — run af1746, Opus 4.8, upgraded harness (read_file + suggestion-aware reward)

Assessment: approve

Assessment: Approve

This is a well-structured, thorough port that mirrors the Python behavior and ships with strong test coverage (metadata, validation, resolution, context modes, depth, interrupts, and real-agent integration) plus clear README/docs.

Review Categories

• Correctness: Logic reads cleanly; depth tracking, interrupt namespacing, and context forking are well tested with full-object assertions.
• Maintainability: The default builder reaches into Agent private fields (_sandbox, _printer, _tracer) via as unknown casts, which is the main coupling concern.
• Behavior nuances: Flagged two things to confirm — propagating sandbox: false to children, and collapsing a custom parent printer to a boolean.
• API surface: description option is implemented/documented but missing from the PR's public API summary.

None of the comments are blocking; nice attention to parity with Python and to edge-case testing.

[poshinchen]

Suggestion
Issue: The child's printer is collapsed to a boolean (printer: printer !== undefined). If the parent was constructed with a custom Printer instance, the child silently gets the default printer rather than the parent's configured one, which differs from how sandbox/trace attributes are passed through by reference.
Suggestion: Consider passing the parent's actual printer through when present (e.g. ...(printer !== undefined && { printer })) so custom printers propagate like the other inherited attributes — or document explicitly why only the on/off state is intended to be inherited.

[poshinchen]

Important
Issue: _sandbox can legitimately be false (sandbox explicitly disabled), and the spread ...(sandbox !== undefined && { sandbox }) will forward sandbox: false to the child. If the Agent constructor treats false differently from "unset", a parent that explicitly disabled its sandbox would propagate that disabled state — which may or may not be intended.
Suggestion: Please confirm sandbox: false is the desired value to pass to children, and if so add a brief comment; otherwise narrow the condition to only forward truthy sandbox instances.

[poshinchen]

Suggestion
Issue: The private-field access via as unknown as { _sandbox?; _printer?; _tracer } reaches into Agent internals in three places. This couples the default builder tightly to the Agent class's private field names, so an internal rename would silently break propagation (the as unknown cast suppresses any compiler signal).
Suggestion: If Agent exposes (or could expose) public getters for sandbox, printer, and traceAttributes, prefer those. Otherwise, consider a single typed internals helper (as the test file already does) to centralize the coupling and make the dependency explicit.

[poshinchen]

Suggestion
Issue: MakeSubagentOptions exposes a public description option (and makeSubagent validates/uses it), but it isn't listed in the PR description's "Also exported" / public API summary even though the README documents it. Minor inconsistency in the surface being advertised for API review.
Suggestion: Add description to the PR's Public API Changes section so API reviewers evaluate the full exported option set.

[poshinchen]

Suggestion
Issue: _resolveContext parses last_messages for every mode, but a non-numeric string like 'bogus' passes typeof rawLast === 'string', goes through Math.trunc(Number('bogus')) → NaN, and is then discarded by the Number.isFinite guard. This is correct but relies on the reader tracing two conversions; the test last_messages: 'bogus' covers it well.
Suggestion: Optional: a brief comment noting that string inputs are coerced and invalid/<1 values fall back to "share all" would make the intent clearer at a glance.

[poshinchen]

Live Strandly's actual comments on the original PR 4971 (copied for comparison; mentions escaped and links de-linked so nothing is pinged or cross-referenced)

Review (CHANGES_REQUESTED):

Changes requested — two reachable bugs in the default path, both with running repros. The port itself is faithful: I diffed the model-facing toolSpec, error strings and <parent_context> framing against Python and they are identical.

🔴 Nested delegation (the default maxDepth: 2) and any parent with contextManager: 'auto' | 'agentic' always fail with Subagent error: Tool with name 'retrieve_context' already registered — _defaultBuilder inherits the parent's retrieve_context and also gives the child contextManager: 'auto'. The PR's only real-agent test is depth 1 on a parent without a context manager, so it never hits this.

🔴 A child that interrupts twice (answer A → child continues → interrupts B → answer B) fails on the second resume with Subagent error: interrupt_id=<tool:inner-1:confirmA> | no interrupt found; the user's second answer is lost. _prepareResume replays every answered interrupt in parentState.interrupts, including the stale one from round 1. The sibling agent-as-tool.ts:350 reads parentState.resumeResponses instead.

🟡 AgentBuilder is referenced by MakeSubagentOptions.builder but not exported from ``@strands-agents/sdk/multiagent; `defaultPreset` isn't validated against `presets`.

No api/needs-review label — this is a port of the shape reviewed in strands-agents#4841, so probably fine, but a maintainer should say so.

✅ Verified, questions, reading order, appendix

Verified at 7e881807b (base 1556ede43): tsc --noEmit -p src/tsconfig.json clean; vitest --project unit-node 49 subagent + 24 spec tests green; eslint, prettier, check:browser-bundle, test:package all pass. Repros for both 🔴s ran against the PR head and fail; with the two suggested fixes applied, the repros pass and the PR's 49 + 24 tests stay green (both fixes are ~5 lines). toolSpec JSON for the default tool and a fully-configured tool dumped from both SDKs and diffed: identical.

Questions (non-blocking)

• MakeSubagentOptions has no description? while every other TS Make*Options does (file-editor.ts:78 pattern), yet DEFAULT_SUBAGENT_DESCRIPTION is exported. Python lacks it too — deliberate parity, or add to both?
• defaultPreset/context validation gaps exist in Python as well — fix here and back-port, or track once?

Reading order: multiagent/spec.ts:379-440 (_defaultBuilder) → subagent/subagent.ts:286-328 (stream) → :395-471 (resume/finalize) → :480-578 (context forking/rendering) → tests.

Appendix — non-blocking (6)

• ⚪ Factory error strings are Python syntax: tools=Choice([]), multiple=true (subagent.ts:117,120,127,130) — in TS it is new Choice(options, true).
• ⚪ Depth-limit message hardcodes calling subagent again (subagent.ts:354) even when name is customised; use this.name. Same in Python.
• ⚪ context: new Fixed('al') (typo) is accepted and behaves as 'none' (subagent.ts:472-480, branches only on 'all'/'no_tools'). Same in Python.
• ⚪ Model sending a non-string instructions surfaces as Subagent error: data.map is not a function; unhelpful to the model, but the schema says string so providers mostly prevent it.
• ⚪ GENERALIST lands un-namespaced on the vended-tools barrel (vended-tools/index.ts:32); siblings use DEFAULT_SUBAGENT_*. Name-only.
• ⚪ Attacks that found nothing: concurrent children both interrupting, toolUseId with :/special chars, resume after child gone, maxDepth: 1, user-preset appState depth values, non-object / array / numeric inputs, cancelSignal mid-child-tool, child model throwing mid-stream, 'all' forking with in-flight + parallel toolUses, last_messages trimming, </parent_context> sentinel escaping (matches Python).

Evidence: strands-agents/harness-sdk/pr/4971/repro-subagent.test.ts, repro-results.txt, vitest-with-fixes.log, api-spec-parity.txt in the artifacts bucket.

Review (APPROVED):

Approve — all four findings fixed at 19cbbdcd, verified against my repros, and the fixes hold up under attack.

• 🔴 retrieve_context clash → fixed (spec.ts skips parent.contextManager.getTools() by identity; new tests for depth-2 and an 'auto' parent; docs/README updated).
• 🔴 stale interrupt replay → fixed (_prepareResume now reads parentState.resumeResponses; new consecutive-interrupt test).
• 🟡 AgentBuilder export, defaultPreset validation → fixed; appendix items (TS-syntax error strings, this.name in depth message, context-mode validation, non-string instructions) also addressed.

Still open for a maintainer, not for the author: whether this port needs the api/needs-review label, and the description? option question from the first review.

✅ Verified

• tsc --noEmit -p src/tsconfig.json clean (PR files); eslint clean on changed files.
• vitest --project unit-node: subagent 55 passed (was 49), spec 25 passed (was 24).
• My original repros (contextManager: 'auto' parent, default-parent grandchild, two sequential child interrupts) all pass on this head.
• Attacked the new re-raise path in _prepareResume (it re-raises all own interrupts, unlike agent-as-tool.ts:361 which filters answered ones): two concurrent children, c1 interrupts A→B and c2 interrupts X, answered one at a time across four rounds — pending sets were exactly {A,X} → {B,X} → {B} → done, no answered interrupt resurfaced, both children completed. The parent de-dupes, so no change needed.
• 'agentic' parent: child now builds and inherits summarize/truncate/pin_context, which operate on toolContext.agent.messages and so work on the child; nothing to raise.

Review (APPROVED):

Approve at c092add0 — the delta since my last approval is small and checks out.

• description? option added to makeSubagent and make_subagent (empty string rejected, presets still appended) — closes the open question from round 1; consistent with the other Make*Options.
• Context-manager tool skip ported to Python _default_builder by identity (id(t) in parent.context_manager.tools), with a test that a stateful child no longer inherits the parent's retrieve_context.
• Test-helper cleanup in spec.test.ts is behaviour-neutral.

✅ Verified

• TS: tsc clean, eslint + prettier clean on changed files; subagent.test.ts 57 passed, spec.test.ts 25 passed; my earlier repros (auto-parent, depth-2, consecutive interrupts) still pass.
• Python: pytest tests/strands/multiagent/test_spec.py tests/strands/vended_tools/test_subagent.py → 59 passed. Live check: Agent(context_manager="auto") → _default_builder child → grandchild, each ends up with exactly one retrieve_context (its own).

Inline at strands-ts/src/multiagent/spec.ts:439:

🔴 Child init throws Tool with name 'retrieve_context' already registered whenever the parent already owns a context manager — which includes every child built by this builder.

parent.toolRegistry.list() (line 383) includes the retrieve_context tool the parent's ContextManager vended, so it's copied into childTools; then this line gives the child its own contextManager: 'auto', whose getTools() registers retrieve_context again and ToolRegistry.add throws (registry/tool-registry.ts:35). Reached by:

• new Agent({ tools: [subagent], contextManager: 'auto' }) → every delegation errors.
• new Agent({ tools: [subagent] }) with the default maxDepth: 2 → the child's own subagent call always errors, so the documented second level never works.

Repro (parent default, grandchild built via _defaultBuilder(child) and initialize()d): default parent: child init -> ok / grandchild init -> Tool with name 'retrieve_context' already registered; contextManager auto: child init -> Tool with name 'retrieve_context' already registered.

Suggestion: skip the tools the parent's context manager vends, since the child's own manager re-registers them:

const managed = new Set((parent.contextManager?.getTools() ?? []).map((tool) => tool.name))
for (const tool of parent.toolRegistry.list()) {
  if (managed.has(tool.name)) continue
  ...

And add a real-agent test for depth 2 and for a contextManager: 'auto' parent — the existing with real agents test is depth 1 on a parent without one, so it can't catch this.

Inline at strands-ts/src/vended-tools/subagent/subagent.ts:401:

🔴 Second interrupt of the same child fails: the first round's (already consumed) answer is replayed and the child rejects it.

parentState.interrupts keeps every interrupt registered for this in-flight tool call across resumes, so after round 1 is answered and the child interrupts again, answered contains both …:inner-1:confirmA (stale, already applied) and …:inner-2:confirmB. The child's _interruptState no longer knows inner-1, so child.stream([...]) throws and the model gets Subagent error: interrupt_id=<tool:inner-1:confirmA> | no interrupt found — the user's answer to B is dropped and the delegated task is lost. Reached by any user whose subagent calls two confirming tools in sequence (real-agent repro: seen = ['A'], expected ['A','B']).

agent-as-tool.ts:350 solves the same thing by reading parentState.resumeResponses (the answers supplied on this resume) instead of all answered interrupts.

    const own = Object.values(parentState.interrupts).filter(
      (interrupt) => interrupt.id.startsWith(prefix) && interrupt.response === undefined
    )
    const answered = (parentState.resumeResponses ?? [])
      .filter((r) => r.interruptResponse.interruptId.startsWith(prefix))
      .map((r) => ({ id: r.interruptResponse.interruptId, response: r.interruptResponse.response }))

(then drop the ! on interrupt.response at line 420). With this applied the two-interrupt repro passes and the existing 49 tests stay green.

Inline at strands-ts/src/vended-tools/subagent/subagent.ts:57:

🟡 AgentBuilder is part of the public options type but users can't import it. multiagent/index.ts:45 exports AgentSpec, Choice, Fixed, Inherit, Open, Option, Preset only, and there's no ./multiagent/spec subpath, so a standalone typed builder needs NonNullable<MakeSubagentOptions['builder']>. Python users can from strands.multiagent.spec import AgentBuilder; EdgeHandler (multiagent/index.ts:33) is the precedent for exporting a function type from this barrel.

Suggestion: export type { AgentBuilder } from './spec.js' in multiagent/index.ts.

Inline at strands-ts/src/vended-tools/subagent/subagent.ts:122:

🟡 defaultPreset isn't checked against presets, so a typo silently runs the child with an empty system prompt. makeSubagent({ presets: { researcher }, defaultPreset: 'reseacher' }) tells the model Omit to use the default ('reseacher'), and _resolveSpec (spec.ts:342-345) resolves preset = undefined → systemPrompt: '', no error anywhere. Same gap exists in Python.

  const defaultPreset = options.defaultPreset ?? Object.keys(presets)[0]
  if (defaultPreset !== undefined && !Object.hasOwn(presets, defaultPreset)) {
    throw new Error(`defaultPreset "${defaultPreset}" is not one of the presets: ${Object.keys(presets).join(', ')}.`)
  }

Inline at strands-ts/src/multiagent/spec.ts:439:

One addendum from the docs pass: vended-tools.mdx:836 and README.md:61 both state "Each child gets contextManager: 'auto' … and accesses the parent's long-term memory through inherited memory tools" — that is exactly the pairing that throws today, so whichever way this is fixed, those two lines should move with it.