Code Review

The core contribution — resolving TS7006 for execute parameters via satisfies ModelDefinition<typeof Schema> and the defineModel helper — is well-designed, correctly implemented, and thoroughly tested. The MethodContext<TGlobalArgs> parameterisation is backward compatible, the type-level regression tests are solid, and the compat test guards against drift on the main types. The skill documentation and README updates are clear and well-scoped.

Blocking Issues

None.

Suggestions

1. ResourceOutputSpec / FileOutputSpec field mixing (packages/testing/model_definition_types.ts): The testing ResourceOutputSpec includes streaming?: boolean (lines 55) which only exists on the canonical FileOutputSpec, and is missing tags?: Record<string, string> which exists on the canonical ResourceOutputSpec. Conversely, the testing FileOutputSpec includes sensitiveOutput?: boolean and vaultName?: string (lines 68-69) which only exist on the canonical ResourceOutputSpec, and is also missing tags. It looks like some fields were accidentally swapped between the two specs. This means an extension author using satisfies ModelDefinition<...> who declares resource tags will get a spurious TS error, while streaming on a resource will silently compile but do nothing. Suggest fixing the field assignments to match the canonical types in src/domain/models/model.ts:49-94.

2. CheckResult extra field (packages/testing/model_definition_types.ts:109): The testing CheckResult includes warnings?: string[] which doesn't exist on the canonical CheckResult (src/domain/models/model.ts:519-522). Extension authors returning warnings would think they're handled when they're not.

3. Compat test gaps for output specs (src/domain/models/testing_package_compat_test.ts): The compat test validates ModelDefinition and MethodDefinition field shapes but doesn't cover ResourceOutputSpec, FileOutputSpec, or CheckResult. Adding structural checks for these types (similar to _checkMethodDefinitionFields) would catch the field-mixing issues in suggestions 1-2 and prevent future drift.

4. Minor: multi-line doc block in model_definition_types.ts — The file has several multi-paragraph JSDoc blocks (e.g., lines 30-37, 107-130, 148-168). CLAUDE.md convention is "never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The module-level doc block explaining the purpose is understandable given this is a public API for extension authors, but the internal implementation comments could be trimmed.

Adversarial Review

Critical / High

None found.

Medium

1. Testing CheckResult adds warnings field not present in canonical — packages/testing/model_definition_types.ts:108 defines warnings?: string[] on CheckResult, but the canonical CheckResult at src/domain/models/model.ts:519-522 has only pass and errors. The runtime in method_execution_service.ts:521-531 and validation_service.ts:413-427 only reads .pass and .errors — any warnings returned by an extension author's check will be silently dropped. An author importing CheckResult from the testing package will see warnings in autocomplete and could write code relying on it, not realizing the runtime ignores it. Additionally, no compat test covers CheckResult, so this drift won't be caught by CI.

   Suggested fix: Either remove warnings from the testing CheckResult to match canonical, or add it to canonical and wire it through the runtime. Also add a _checkCheckResultFields function to testing_package_compat_test.ts.

Low

2. as never cast defeats execute-signature compat check — src/domain/models/testing_package_compat_test.ts:170: the assignment def.execute as never is a type-level no-op — never is assignable to everything, so this will compile even if the testing MethodDefinition.execute signature diverges completely from canonical (e.g., returns Promise<void> instead of Promise<MethodResult>). The branded DataId workaround is understandable, but a stronger check could use an intermediate type that erases the brand without erasing the entire assignment.

3. Incorrect comment in compat test — src/domain/models/testing_package_compat_test.ts:154 says "The testing type omits kind" but packages/testing/model_definition_types.ts:85 includes kind?: "create" | "read" | "update" | "delete" | "list" | "action". The comment is factually wrong. The inline union also risks drifting from canonical MethodKind if new kinds are added.

4. Compat test coverage gap for new types — ResourceOutputSpec, FileOutputSpec, CheckDefinition, CheckResult, and VersionUpgrade from model_definition_types.ts are exported to consumers but have no compat checks against their canonical counterparts. The comment at testing_package_compat_test.ts:201-203 claims they're "covered transitively" but the function only checks methods entries and upgrades.toVersion — it doesn't verify any resources, files, or checks record value shapes.

5. defineModel widens literal types differently from satisfies — The docs at packages/testing/model_definition_types.ts:160 and references/typing.md:203 claim the contract is "identical" to satisfies. At runtime this is true, but at the type level defineModel returns ModelDefinition<TGlobalArgs>, which widens .type from the literal (e.g., "@myorg/my-model") to string and .methods from the literal record to Record<string, MethodDefinition<...>>. With satisfies, TypeScript preserves the literal. This matters if downstream code relies on specific key names. Consider noting this distinction in the doc.

Verdict

PASS — This is a well-structured, additive change that solves a real DX pain point (TS7006 under strict mode) without breaking existing consumers. The generic parameterization of MethodContext is backward-compatible via defaults, the type-level tests are load-bearing, and the compat tests cover the critical paths. The medium finding (warnings on CheckResult) should be addressed before or shortly after merge to avoid misleading extension authors, but it doesn't block.