Skip to content

fix(runtime/inference): canonical-json crash, missing tool schemas, MCP capability scope - #21

Open
Robin-fang611 wants to merge 3 commits into
CodeSoul-co:devfrom
Robin-fang611:fix/canonical-json-tools-mcp-capability-dev
Open

fix(runtime/inference): canonical-json crash, missing tool schemas, MCP capability scope#21
Robin-fang611 wants to merge 3 commits into
CodeSoul-co:devfrom
Robin-fang611:fix/canonical-json-tools-mcp-capability-dev

Conversation

@Robin-fang611

Copy link
Copy Markdown
Contributor

Summary

Three framework-level defects found while integrating Hypha as the agent runtime. All three are verified fixed locally (typecheck passes, fixes validated in a running deployment).

1. fix(inference): canonicalizeJson crash on undefined optional prompt block fields

ResolvedAgentPromptBlock optional fields (ownerId / tenantId / provenance / metadata) were assigned directly even when undefined. canonicalizeJson rejects undefined values with RUNTIME_INVALID_INPUT, so any ReAct run whose prompt block omits the optional fields fails at startup with RUNTIME_INVALID_INPUT.

Fix: conditionally spread the fields only when defined. The same crash class existed in resolveChatAgent (promptResolution / toolRefs) and is guarded the same way.

2. fix(runtime): ReAct reasoning never receives tool schemas

createCanonicalReActAgentRuntime.reason() returned an InferenceRequest without the tools field. The model receives no tool definitions, so it fabricates JSON tool calls as plain text that are never executed — tool use silently does nothing.

Fix: resolve each context.agent.toolRefs entry via ToolManager.describeTool and attach the descriptors to the request.

3. fix(runtime): MCP tools rejected by TOOL_CAPABILITY_SCOPE_DENIED

createEffectiveAgentCapabilitySnapshot runs capabilityConstraint for agent and domain scopes, both defaulting allowedMCPServerIds to empty. With the config's default allowlist empty, every MCP-backed tool is rejected with TOOL_CAPABILITY_SCOPE_DENIED even though the tool is explicitly referenced by the agent.

Fix: derive the MCP server ids from the agent's toolRefs (via ToolManager.describeTool serverId / capabilityId) and pass them into both constraints; also forward the effective capability snapshot ref to the tool runner when available.

Verification

  • npm run typecheck passes on the branch
  • All three fixes validated in a live deployment (canonical-json crash gone, ReAct tool calls executed, MCP tools usable)

Notes

  • Base branch dev per CONTRIBUTING.md (Framework source fixes integrate via dev)
  • Local-only config.yaml additions (environment-specific MCP servers) intentionally excluded

…mpt block fields

ResolvedAgentPromptBlock optional fields (ownerId/tenantId/provenance/metadata)
were assigned directly even when undefined. canonicalizeJson rejects undefined
values with RUNTIME_INVALID_INPUT, so any ReAct run with a prompt block failed
at startup.

Conditionally spread the fields only when defined. Also applies the same guard
to promptResolution/toolRefs in resolveChatAgent (EventRuntime.ts), which had
the identical crash class.
createCanonicalReActAgentRuntime.reason() returned an InferenceRequest
without the tools field. The model received no tool definitions and
resorted to fabricating JSON tool calls that were never executed.

Resolve tool descriptors via ToolManager.describeTool for each
context.agent.toolRefs entry and attach them to the request.
…ty scoping

createEffectiveAgentCapabilitySnapshot ran capabilityConstraint with empty
allowedMCPServerIds for both agent and domain scopes, so any MCP-backed tool
was rejected with TOOL_CAPABILITY_SCOPE_DENIED.

Derive the MCP server ids from the agent's tool refs (ToolManager.describeTool
serverId/capabilityId) and pass them into both constraints. Also forward the
effective capability snapshot ref to the tool runner when the contract
snapshot is available.

@erwinmsmith erwinmsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该 PR 当前不能合并,需按 Owner 分支重新拆分提交。

阻断问题:

  1. apps/server/src/services/EventRuntime.ts 将请求中的 toolRefs 解析为 allowedMCPServerIds,并把同一结果同时写入 agent/domain constraints;显式空列表 [] 还会被 fallback 覆盖。这会让请求内容形成 MCP 自授权,并绕过 agent/domain 能力交集。空列表必须保持 deny,授权只能来自已验证、冻结的能力快照。
  2. canonical ReAct reasoning 从实时 ToolManager 读取 tool schemas,而不是读取 Run 冻结的 ToolContractSnapshot/capability snapshot。同一 Run 可能因 registry 变化得到不同 schema,破坏重放与审计;未知 tool ref 也被静默丢弃,必须 fail closed。
  3. 一个 PR 同时修改 inference 与 runtime Owner 内容,head 分支名称不在仓库 allowlist,且三个行为变化均未提供能够在回归时失败的正向、负向测试。

认可的方向:过滤 prompt block 中的 undefined、在 runtime context 中省略 undefined 字段、向 canonical reasoning 提供受治理的 tool schemas、传递精确 capabilitySnapshotRef。请分别从 approved inference/runtime Owner 分支创建职责单一的新 PR;若改变 Tool/MCP 授权契约,再由 tools 分支单独提交。必须补齐 explicit [] deny、agent/domain intersection、snapshot mismatch、registry drift、unknown tool ref、replay deterministic 等回归测试。现有提交不能直接追加补丁后继续合并。

@erwinmsmith

erwinmsmith commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This PR was closed under the repository Branch Ownership and security gates. It was not merged, so no remote revert is required.

The valid implementation directions should be preserved, but fixes must not be appended to this cross-Owner PR:

  • inference: undefined-safe prompt serialization;
  • runtime: snapshot-bound reasoning context and the exact capabilitySnapshotRef;
  • tools (only if the public contract must change): MCP capability authorization semantics.

Please submit separate, commit-specific PRs from the approved Owner/source branches, with the required regression and negative tests.

@erwinmsmith

Copy link
Copy Markdown
Contributor

Maintainer review summary (English):

The reported needs are valid: canonical JSON must not receive undefined, canonical ReAct reasoning needs governed tool schemas, and governed tool calls must carry the exact capability snapshot reference. However, the current implementation cannot be merged as-is.

Blocking defects:

  1. Requested toolRefs are used to derive allowedMCPServerIds and the same derived list is injected into both Agent and Domain constraints. An explicit empty list is also replaced by the derived value. This makes the request self-authorizing and bypasses the Agent/Domain capability intersection. An explicit empty list must remain deny-all.
  2. Tool schemas are read from the live ToolManager during a Run instead of from the immutable Run tool/capability snapshot. Registry changes can therefore change reasoning input for the same Run, breaking deterministic replay and authorization identity. Unknown or mismatched refs must fail closed.
  3. The PR crosses inference and runtime ownership, uses a non-allowlisted branch name, and adds no behavior-focused regression tests.

I will validate and retain the required behavior through the approved Owner branches, with snapshot-bound authorization and regression coverage. This PR remains closed and unmerged; no revert has occurred.

@erwinmsmith

Copy link
Copy Markdown
Contributor

Maintainer final outcome

The required behavior has been validated and adopted through the repository’s legal Owner branches. PR #21 remains closed and unmerged because its fork branch and mixed ownership path do not satisfy this repository’s branch policy. No revert was needed because none of its commits reached the protected repository branches.

Accepted and safely rewritten work:

  • inference@55b2834: omit undefined optional prompt evidence so canonical JSON remains valid.
  • runtime@c5eb927: expose canonical tool schemas from the immutable Run ToolContractSnapshot, fail closed on registry drift, and attach the exact capabilitySnapshotRef to governed calls.
  • Both commits retain Robin-fang611 as co-author.

The proposed MCP self-authorization behavior was not adopted. Requested MCP tool references cannot create their own allowlist; an explicit empty capability scope remains deny-by-default.

Validated release path:

  • dev@05d25c9
  • domain@51226bd
  • cache-base@5385b70
  • dev-domain-merge@ba95fa9
  • dev-merge@f3e7b7f
  • main@6d3b35e

Release evidence under Node 24.11.1:

  • lint, typecheck, and build passed
  • 297 unit tests passed
  • 2,410 package/contract tests passed
  • 43 integration tests passed against real local MongoDB and Redis
  • all source-to-release ancestry checks passed

Thank you for identifying the valid canonical prompt and tool-snapshot gaps. Future changes should be submitted from the approved Owner source branch and must preserve explicit MCP authorization boundaries.

@erwinmsmith erwinmsmith reopened this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants