✅ Done! I reviewed both PRs in full (SDK

#5151

and Canvas

#17516

), including the diffs, the migration table, the new catalog/seal logic, the resolver changes, and the Canvas editor flow. Overall the architecture is sound — server-driven catalog, a single defaulting point, capability-gated feature detection, and draft-based preview parity are all the right calls. Below are the design points I agree with, then the edge cases and breaking changes I found — including one likely-unintended regression. What's well-designed ✅ • Single source of truth. The server answers both "what can a user pick?" (

GET /api/tools/catalog

) and "what does this profile resolve to?" (draft

materialize

). No tool names/default list/exclusions live in Canvas, so the picker can't drift (the

#4958

complaint). •

resolve_tool_specs

as one defaulting point, with

enable_browser

passed explicitly instead of post-launch injection. This is what makes

materialize

finally mirror a real launch. • Sealing the catalog at startup is correct and clearly documented; conversation-scoped registrations (client tools,

tool_module_qualnames

) correctly never enter the picker. • Capability-gated (

tool_catalog_v1

,

agent_profile_draft_materialize_v1

) rather than version-gated — right call, and the "hide the section on old servers, omit

tools

on save so a foreign selection survives" behavior is genuinely careful. • Whole-profile-overwrite safety in the editor: a stored tool the catalog no longer offers is kept listed/selected rather than silently dropped. • Built-ins offered under snake_case name (

switch_llm

vs

SwitchLLMTool

), with

resolve_tool

accepting it — good cleanup, and consistent with the rest of the catalog. • Migration table is fully pinned by

test_v2_*

cases and the schema-version bump to v3 is clean. Breaking changes / edge cases ⚠️ 1. The global Agent-settings page loses the toggles too (likely unintended). The Canvas PR removes the "Enable sub-agents" and "Enable LLM switching" switches from

AgentSettingsScreen

unconditionally — the block wasn't gated on

embedded

, and

AgentSettingsScreen

is the same component used for the standalone Settings → Agent page. So this is not scoped to the profile editor: legacy

agent_settings

users (who don't use profiles) also lose the ability to turn on sub-agents, and LLM-switching can't be toggled from Canvas at all anymore. The PR body even says "only the profile loses the flag" and _"the agent_settings launch path keeps its own enable_sub_agents, which the SDK still honours"_ — but

AgentSettingsConfig

keeps both fields, while the UI no longer edits them. This looks like a real regression and is worth confirming before merge. 2.

resolve_agent_profile

output is now materially different. It changes from passing

tools: None

through to returning a concrete resolved list. Any downstream consumer that checks

resolved_settings.tools is None

to mean "server default" will behave differently. The PR notes this, but the blast radius is wider than the two repos: the Docker runtime resolves profiles on the host (

#5141

), and cloud/embedding paths consume

resolve_agent_profile

. Worth a cross-repo sweep for

tools is None

/ tri-state consumers. 3. "All default profiles lose `switch_llm`" is a bigger deal than the table suggests.

enable_switch_llm_tool

defaulted to true, so dropping it without compensation means every existing v2 profile that relied on the default silently loses

switch_llm

(not just ones that explicitly set it). The migration table's "sub-agents off, switch-llm on/off →

null

(unchanged)" hides that "unchanged" here means "the tool is now gone until someone re-selects it." It's an acknowledged, one-time loss, but it deserves a loud release-note callout, not just a table row — and it creates a subtle divergence:

AgentSettingsConfig

still defaults

enable_switch_llm_tool=True

, so the legacy and profile paths now give different tool sets for the same conceptual config. 4. Pinning

browser_tool_set

into migrated lists is only safe by a non-obvious invariant. A sub-agents profile migrates to

[terminal, file_editor, task_tracker, browser_tool_set, task_tool_set]

with browser included unconditionally, even on hosts without Chromium. It "works" only because

BrowserToolSet.create

returns

[]

when unusable and

resolve_tool_specs

returns an explicit list verbatim. That's a fragile coupling — worth a regression test asserting a browser-pinned list on a browserless runtime resolves to an empty (harmless) browser contribution rather than ever crashing. 5. New-profile "Standard tools" preview is name/LLM-gated.

standardToolsDraft

requires both

profileName && llmProfileRef

, so in create mode (name empty until typed) the preview and the "standard → choose" seeding are effectively disabled/locked. Minor UX inconsistency, but the mode dropdown being disabled on

isStandardToolsPending

can feel stuck until a name is entered. 6. Draft materialize decrypts an arbitrary referenced LLM profile. Extending

materialize

to arbitrary draft bodies means the (authenticated) caller can point it at any

llm_profile_ref

and trigger a dry-run resolve/decrypt. Output is redacted, so not a secret-exposure risk — just noting the endpoint is now a slightly wider oracle than before. Minor •

list_tool_catalog

recomputes

usable

per request (calls

is_usable()

for every entry) and scans built-ins with a linear

next(...)

. Negligible at current tool counts. • The duplicate-registration

logger.warning

in

register_tool

(pre-existing typo "registerd") plus the new

_TOOL_CLASSES

dict now means a duplicate registration silently keeps the last class — consistent but worth a glance. Verdict Architecturally this is the right direction and well-tested. I'd ask for one blocking clarification on finding #1 (the global settings-page toggle removal) and a follow-up sweep for #2, then strongly recommend an explicit release note for #3. The rest are edge cases that could ride as follow-ups. This review was created by an AI agent (OpenHands) on behalf of the requester.

✅ Done! Following up on the two coupled PRs — I re-read the full diffs for both SDK (#5151) and Canvas (#17516), not just the descriptions. The architecture holds up well. Here's my assessment, the edge cases I found, and the breakage to watch. Architecture — solid The core move is right: the server becomes the single source of truth for both questions a picker needs ("what may I pick?" and "what does this profile resolve to?"), and

tools

becomes the only tool control on a profile. Retiring

enable_sub_agents

/

enable_switch_llm_tool

and folding them into

tools

removes the contradiction where two switches could disagree with the list sitting next to them. Specific decisions I verified and agree with: •

resolve_tool_specs

as the single defaulting point, with

enable_browser

as an explicit resolver input instead of post-launch injection. This is what makes the

materialize

preview actually equal the launch — the old split (browser appended only on the launch path) is the bug this correctly kills. •

list_tool_catalog

offering built-ins under snake_case (

switch_llm

, not

SwitchLLMTool

), and

resolve_tool

accepting that same name via

BUILT_IN_TOOL_CLASSES_BY_TOOL_NAME

. A stored pick now resolves cleanly. • The

seal_tool_catalog()

mechanism is the right call — it cleanly excludes per-conversation client tools and

tool_module_qualnames

imports without needing to enumerate them. • The v2→v3 migration being behaviour-preserving — the table in the PR body matches the code in

fold_tool_switches_into_tools

exactly (defaults stay

None

, "opt-out of switch_llm" gets pinned, every divergence gets said explicitly). Good. Edge cases worth a look 1. Old client + new server = 422 on save (the big one).

AgentProfileBase

is

extra="forbid"

. Any client that still emits `enable_sub_agents`/`enable_switch_llm_tool` in a v3 payload will get the whole save rejected. The two PRs must land in lockstep, and this is the one failure mode that can't be papered over by the migration — because the migration only runs when

schema_version

is still

< 3

, but an old client saving a stored (already-migrated v3) profile will re-add the retired keys at v3. Worth a note in both PRs that they're hard-coupled. 2. Draft

materialize

returns empty when the LLM ref dangles. In Canvas,

ToolCatalogService.getResolvedToolNames

maps a missing

resolved_settings.tools

to

[]

. During profile creation (before an LLM ref is chosen)

standardToolsDraft

is

null

and the query is disabled, so this is mostly fine — but if a draft is posted with a dangling/empty

llm_profile_ref

, the "standard" preview renders empty and

standardToolsUnresolved

goes false (it only gates on

undefined

, not on failure). Switching to "Choose tools" then seeds from

standardToolNames ?? []

→ an empty custom list. It's recoverable (the user re-picks), but a failed materialize should probably keep the mode locked rather than silently seeding an empty selection. 3.

is_tool_usable(browser_tool_set)

on a server where the browser preset isn't registered returns

False

(that's the documented contract of

is_tool_usable

). Not a regression — the old code did the same — but it means a minimal server config that doesn't import the browser tool reports browser as unavailable even if chromium is present. Fine, just a property to keep in mind. 4. The materialize draft endpoint is a (slightly) widened oracle — an authenticated caller can now dry-run

POST /{name}/materialize

against any

llm_profile_ref

. You already flagged this and output stays redacted, so I treat it as noted-not-defect. Just confirm the route is behind the same auth as the rest of the profile router. 5.

standardToolsUnresolved

naming/severity nit — it explicitly doesn't use

isPending

to avoid stranding the create form, which is correct, but the fallback when the server errors (rather than pending) is indistinguishable from "not asked yet." Consider keying the disabled state on

isError

too so a failed catalog fetch doesn't silently render an empty standard set. Breaking changes checklist (confirmed) • Profile fields

enable_sub_agents

and

enable_switch_llm_tool

removed →

extra="forbid"

rejects them (see #1). •

resolve_agent_profile

now returns a concrete

tools

list where it used to pass

None

through — callers checking

resolved_settings.tools is None

to mean "server default" will now see a list. You called this out; worth a deliberate sweep of non-canvas consumers. •

enable_switch_llm_tool: true

on a stored profile pins

switch_llm

into

tools

— correct, but any downstream logic that treats "has an explicit

tools

list" as "is fully user-pinned" now sees pinned lists it didn't before. Small nits •

_TOOL_CLASSES

now holds a strong ref to every registered tool class for the life of the process. That's fine for a server, but it's a (tiny) change from "modules are tracked by qualname string" to "classes are held." No action needed, just noting it's no longer purely stringly-typed. •

list_tool_catalog

hardcodes

user_selectable=True

on the built-ins branch while relying on the outer

if tool_class.user_selectable

filter — it's correct, but the two sources of that flag (hardcoded vs.

tool_class.user_selectable

) are easy to drift. Consider reading

tool_class.user_selectable

in the entry too for symmetry. Verdict: the design is sound and the migration is careful — I don't see anything that should block. The one thing I'd insist on before merge is #1 (hard coupling/dispatch between the two PRs, ideally with a release note that they ship together), plus a quick decision on #2 (lock the mode on materialize failure rather than seeding from an empty answer). I didn't push any review comments — happy to post this to either PR if you'd like.