fix(tools): correct Python SDK fidelity and language-dispatch contract
Address ds-review-bot v5/v6 review on the Python SDK renderer:
- resolveFlavor now takes a peekRuntime() reader: undefined (no runtime,
the doc-catalog harvest) degrades to the TS flavor, but a mounted
runtime whose language is absent from RUN_CODE_FLAVORS fails loud. This
removes the try/catch that silently swallowed the invalid-language path
and drops the /* v8 ignore */ that hid the flavor guard from coverage;
wireSchemas validates the runtime before projecting schemas so the
renderer-table rejection stays the canonical assembly error.
- py-types RESERVED drops the soft keywords match/case: they are legal as
TypedDict fields and methods, so keeping them needlessly degraded
common search/regex arg objects to dict[str, Any].
- py-types treats an object with omitted properties as {} like the unified
validator and TS renderer do, so a closed empty object declares an empty
TypedDict instead of a permissive dict[str, Any].
- README: symmetric jsonSchemaToPy->Any note; a stale zh SDK bullet and
limitation corrected; link the service-wide-language limitation to its
Agent Note.
This commit is contained in:
@@ -120,29 +120,23 @@ const RUN_CODE_DESCRIPTION_PARAM_DESCRIPTION
|
||||
/**
|
||||
* Resolve the {@link RunCodeFlavor} for the loaded runtime's language, read at
|
||||
* schema-emission time so the model-visible `run_code` schema always matches
|
||||
* the SDK section's language. When no runtime is mounted, or one whose language
|
||||
* has no renderer is, the schema harvest degrades to {@link TYPESCRIPT_FLAVOR}
|
||||
* (a doc-only path — a real assembly always mounts a valid runtime, and
|
||||
* `requireCodeRuntime` rejects an invalid language there first). A mounted
|
||||
* runtime whose language passes that guard but is absent from this table fails
|
||||
* loud, keeping this table coupled to `SDK_RENDERERS`.
|
||||
* the SDK section's language. `peekRuntime` returns `undefined` only when no
|
||||
* runtime is mounted — the static schema harvest (doc catalog), which never
|
||||
* reaches a model — so that path degrades to {@link TYPESCRIPT_FLAVOR}. A
|
||||
* mounted runtime whose language has no flavor entry fails loud, exactly as
|
||||
* `requireCodeRuntime` rejects it at assembly: this keeps the table coupled to
|
||||
* `SDK_RENDERERS` and never emits a wrong-language schema for a real runtime.
|
||||
*/
|
||||
function resolveFlavor(requireRuntime: () => CodeRuntime): RunCodeFlavor {
|
||||
let runtime: CodeRuntime
|
||||
try {
|
||||
runtime = requireRuntime()
|
||||
} catch {
|
||||
// Reached only by the static schema harvest (doc catalog), which never
|
||||
// feeds a model: either no runtime is mounted, or requireRuntime rejected
|
||||
// a language with no renderer. Both degrade to the TS default here; a real
|
||||
// assembly hits requireCodeRuntime's loud rejection before this runs.
|
||||
function resolveFlavor(peekRuntime: () => CodeRuntime | undefined): RunCodeFlavor {
|
||||
const runtime = peekRuntime()
|
||||
if (runtime === undefined) {
|
||||
// No runtime mounted: reached only by the doc-catalog schema harvest,
|
||||
// which never feeds a model. Degrade to the TS default.
|
||||
return TYPESCRIPT_FLAVOR
|
||||
}
|
||||
// Own-property read: a language like `toString`/`constructor` would otherwise
|
||||
// resolve an inherited Object.prototype member as a flavor.
|
||||
const flavor = RUN_CODE_FLAVORS[runtime.language]
|
||||
/* v8 ignore next 3 -- requireRuntime rejects a language absent from SDK_RENDERERS, whose keys
|
||||
mirror RUN_CODE_FLAVORS; the guard is defense-in-depth against the two tables drifting. */
|
||||
if (!Object.hasOwn(RUN_CODE_FLAVORS, runtime.language) || flavor === undefined) {
|
||||
throw new Error(`dsh-tools: no run_code schema flavor registered for runtime language ${JSON.stringify(runtime.language)}`)
|
||||
}
|
||||
@@ -287,6 +281,12 @@ type RunCodeOutput = { logs: string[]; result?: JsonValue }
|
||||
export interface RunCodeBridgeOptions {
|
||||
/** Resolves `ctx.codeRuntime` or throws the loud misconfiguration error (shared with the registry's assembly-time checks). */
|
||||
requireRuntime: () => CodeRuntime
|
||||
/**
|
||||
* Reads `ctx.codeRuntime` without throwing: `undefined` when none is
|
||||
* mounted. Lets schema emission tell "no runtime" (the doc-catalog harvest,
|
||||
* degrade to TS) apart from "unknown language" (fail loud).
|
||||
*/
|
||||
peekRuntime: () => CodeRuntime | undefined
|
||||
/** The run's overlap cap for parallel-classified sub-calls (the registry passes its validated `maxParallelSubCalls`). */
|
||||
maxParallel: number
|
||||
/** Runs the contained `tools/code-dispatch-log` waterfall over one settled sub-dispatch (the registry's private invoker). */
|
||||
@@ -305,7 +305,7 @@ export interface RunCodeBridgeOptions {
|
||||
* @returns the registry-ready definition.
|
||||
*/
|
||||
export function createRunCodeTool(registry: ToolRegistry, options: RunCodeBridgeOptions): ToolDefinition {
|
||||
const { requireRuntime, maxParallel, shapeDispatchLog } = options
|
||||
const { requireRuntime, peekRuntime, maxParallel, shapeDispatchLog } = options
|
||||
const definition = defineTool({
|
||||
name: RUN_CODE_NAME,
|
||||
// The description and `code` parameter description are placeholders here:
|
||||
@@ -668,14 +668,14 @@ export function createRunCodeTool(registry: ToolRegistry, options: RunCodeBridge
|
||||
// is the least invasive point that still emits the loaded runtime's language.
|
||||
Object.defineProperty(definition, 'description', {
|
||||
enumerable: true,
|
||||
get: () => resolveFlavor(requireRuntime).description,
|
||||
get: () => resolveFlavor(peekRuntime).description,
|
||||
})
|
||||
Object.defineProperty(definition, 'parameters', {
|
||||
enumerable: true,
|
||||
// Recompile through the same spec→schema projection defineTool used, so
|
||||
// the emitted shape can never drift from the validated one.
|
||||
get: () => parameterSchemaSpecToJsonSchema({
|
||||
code: { type: 'string', required: true, description: resolveFlavor(requireRuntime).codeDescription },
|
||||
code: { type: 'string', required: true, description: resolveFlavor(peekRuntime).codeDescription },
|
||||
description: { type: 'string', required: true, description: RUN_CODE_DESCRIPTION_PARAM_DESCRIPTION },
|
||||
}) as unknown as Record<string, unknown>,
|
||||
})
|
||||
|
||||
@@ -768,6 +768,7 @@ export class ToolRegistry extends Service {
|
||||
? undefined
|
||||
: createRunCodeTool(this, {
|
||||
requireRuntime: () => this.requireCodeRuntime(),
|
||||
peekRuntime: () => this.ctx.get('codeRuntime'),
|
||||
maxParallel: resolveMaxParallelSubCalls(config.maxParallelSubCalls),
|
||||
shapeDispatchLog: dispatch => this.shapeDispatchLog(dispatch),
|
||||
})
|
||||
@@ -778,9 +779,9 @@ export class ToolRegistry extends Service {
|
||||
order: SDK_SECTION_ORDER,
|
||||
// Regenerate from the calling scope's visible tools in stable order,
|
||||
// picking the renderer that matches the loaded runtime's language.
|
||||
// `requireCodeRuntime` already validated the language is in the
|
||||
// table, so the fallback here is defense-in-depth against a caller
|
||||
// that bypassed the guard (impossible under normal composition).
|
||||
// `requireCodeRuntime` already validated the language is in the table,
|
||||
// so the guard below is defense-in-depth against a caller that bypassed
|
||||
// it (impossible under normal composition).
|
||||
text: (context) => {
|
||||
const runtime = this.requireCodeRuntime()
|
||||
// Own-property read: a language like `toString`/`constructor` would
|
||||
@@ -802,15 +803,17 @@ export class ToolRegistry extends Service {
|
||||
*/
|
||||
private wireSchemas(scope?: ScopeKey): ToolProviderResult {
|
||||
const view = this.view(scope)
|
||||
const schemas = [...view.visible.values()].map(definition => this.schemaOf(definition, false))
|
||||
if (this.mode === 'native') {
|
||||
const schemas = [...view.visible.values()].map(definition => this.schemaOf(definition, false))
|
||||
return { schemas, knownNames: [...view.knownNames] }
|
||||
}
|
||||
// Redundant with the per-getter resolveFlavor path (schemaOf's run_code
|
||||
// description/parameters getters call requireCodeRuntime again): kept as a
|
||||
// single explicit gate so a mode collapse rejects here regardless of
|
||||
// whether any getter runs. The call is idempotent (ctx.get + Object.hasOwn).
|
||||
// Validate the runtime language BEFORE projecting schemas: schemaOf reads
|
||||
// run_code's language-aware description/parameters getters, whose own
|
||||
// flavor-table guard would otherwise surface first. This keeps the
|
||||
// renderer-table rejection the canonical assembly-time error for a
|
||||
// language with no SDK renderer.
|
||||
this.requireCodeRuntime()
|
||||
const schemas = [...view.visible.values()].map(definition => this.schemaOf(definition, false))
|
||||
if (this.mode === 'code') {
|
||||
return {
|
||||
schemas: schemas.filter(schema => schema.name === RUN_CODE_NAME),
|
||||
|
||||
@@ -21,22 +21,24 @@ import type { ToolSdkSchema } from './ts-types.ts'
|
||||
const IDENTIFIER = /^[A-Za-z_][A-Za-z0-9_]*$/
|
||||
|
||||
/**
|
||||
* Python 3.x soft-keyword-inclusive reserved set. A tool named ``class`` or
|
||||
* ``lambda`` is legal on the wire but not as an attribute (``tools.class``
|
||||
* would be a SyntaxError in the model program), so we render it under
|
||||
* subscript access — the model still reaches every tool without collisions.
|
||||
* Underscore-leading names (``_x``, ``__class__``) are also subscript-only:
|
||||
* dunders resolve on ``object`` before the proxy's fallback hook, and the
|
||||
* subscript path is the one guaranteed bridge route for them.
|
||||
* The same set rejects an argument field whose name would be an illegal
|
||||
* class-syntax `TypedDict` attribute, degrading that object to
|
||||
* ``dict[str, Any]``.
|
||||
* Python hard keywords: reserved everywhere, so a tool or field named
|
||||
* ``class`` or ``lambda`` is legal on the wire but not as an attribute
|
||||
* (``tools.class`` would be a SyntaxError in the model program) and not as a
|
||||
* class-syntax `TypedDict` field. Such a tool renders under subscript access
|
||||
* and such an object degrades to ``dict[str, Any]`` — the model still reaches
|
||||
* every tool and field without collisions.
|
||||
* Soft keywords (``match``, ``case``, ``type``, ``_``) are deliberately
|
||||
* ABSENT: they are only special in statement position, so ``match: str`` as a
|
||||
* field and ``async def match(...)`` as a method are both legal, and including
|
||||
* them would needlessly degrade common search/regex tool fields to
|
||||
* ``dict[str, Any]``. Underscore-leading names are handled separately (dunders
|
||||
* name-mangle or resolve on ``object`` before the proxy hook), not here.
|
||||
*/
|
||||
const RESERVED = new Set([
|
||||
'False', 'None', 'True', 'and', 'as', 'assert', 'async', 'await', 'break', 'class',
|
||||
'continue', 'def', 'del', 'elif', 'else', 'except', 'finally', 'for', 'from', 'global',
|
||||
'if', 'import', 'in', 'is', 'lambda', 'nonlocal', 'not', 'or', 'pass', 'raise',
|
||||
'return', 'try', 'while', 'with', 'yield', 'match', 'case',
|
||||
'return', 'try', 'while', 'with', 'yield',
|
||||
// Not a keyword, but CPython refuses to ASSIGN it at compile time
|
||||
// (`SyntaxError: cannot assign to __debug__`), which is what a TypedDict
|
||||
// field, a parameter name, and a keyword argument all are.
|
||||
@@ -310,13 +312,14 @@ function renderType(schema: unknown, className: string, state: RenderState): str
|
||||
break
|
||||
}
|
||||
case 'object': {
|
||||
const properties = node.properties
|
||||
if (typeof properties !== 'object' || properties === null) {
|
||||
state.typing.add('Any')
|
||||
finish('dict[str, Any]')
|
||||
break
|
||||
}
|
||||
const entries = Object.entries(properties as Record<string, unknown>)
|
||||
// A missing `properties` is an empty property map, exactly as the
|
||||
// unified validator and the TS renderer read it — NOT an unknown
|
||||
// shape. assertSupportedJsonSchema already rejected a non-object
|
||||
// `properties` (degraded to `Any` above), so the only non-map case
|
||||
// left is omission. The openness of the resulting empty object is
|
||||
// decided below, so a closed empty object still declares an empty
|
||||
// TypedDict rather than a permissive `dict[str, Any]`.
|
||||
const entries = Object.entries((node.properties ?? {}) as Record<string, unknown>)
|
||||
// An empty `className` marks the context-free `jsonSchemaToPy` entry:
|
||||
// there is no naming context to declare into, so degrade. A field
|
||||
// name that is not a legal Python attribute is inexpressible as a
|
||||
|
||||
Reference in New Issue
Block a user