Define canonical engine port and connection contracts - #447
Open
sethjuarez wants to merge 20 commits into
Open
Conversation
Define the schema-owned engine ports, runtime effect metadata, deterministic acceptance vector, and legacy harness preservation gate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Thread generated executor cancellation through the pipeline and provider SDK calls, with forwarding and pre-cancellation coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extend the canonical acceptance vector so PortError cannot become a generated model or wire export. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pin exact metadata, ordered parameters, wire exclusions, and complete native signatures for every configured target. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Open the Connection discriminator and pin exact forward-compatible payload preservation with shared known, unknown, and case-collision vectors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise generated Connection load/save APIs against the shared forward-compatibility vectors without editing generated runtime code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR formalizes cross-runtime contracts for (1) canonical turn-engine “ports” and (2) forward-compatible Connection.kind handling, backed by shared spec vectors and a new schema verification script intended to gate drift across generated runtimes.
Changes:
- Adds shared spec vectors for engine port metadata and
Connectionload→save→reload preservation (including unknownkind+ case-collision scenarios). - Introduces canonical engine port TypeSpec protocols and a
verify-engine-portsscript, wiring it intoschema’s build pipeline. - Threads runtime cancellation through the C# execution surface (Pipeline + executors) and adds a Rust vector-driven connection roundtrip test.
Show a summary per file
| File | Description |
|---|---|
| spec/vectors/model/connection_roundtrip_vectors.json | Adds cross-runtime vectors asserting exact preservation of unknown Connection.kind and payload. |
| spec/vectors/engine/port_contracts.json | Adds canonical engine port contract metadata (async/sync, cancellable, atomic, non-fatal). |
| spec/turn-engine.md | Updates spec narrative to define canonical engine ports and runtime-local cancellation seams. |
| spec/spec.md | Defines forward-compatible, case-sensitive Connection.kind preservation contract and links vectors. |
| schema/scripts/verify-engine-ports.mjs | New verifier intended to gate exported protocol surfaces and cancellation leakage. |
| schema/README.md | Documents the new verification script in the schema toolchain. |
| schema/package.json | Adds verify:engine-ports and runs it as part of schema build. |
| schema/model/pipeline/executor.tsp | Updates Executor method metadata to include runtime cancellation metadata. |
| schema/model/pipeline/engine-ports.tsp | Defines new canonical Engine*Port protocols and method metadata. |
| schema/model/main.tsp | Includes the new engine-ports TypeSpec definitions in the compilation root. |
| schema/model/connection/connection.tsp | Makes Connection.kind discriminator forward-compatible by allowing arbitrary strings. |
| runtime/rust/prompty/tests/connection_roundtrip_vectors.rs | Adds Rust conformance test for Connection vector roundtrips. |
| runtime/csharp/Prompty.OpenAI/OpenAIExecutor.cs | Adds CancellationToken plumb-through across OpenAI execution paths (chat/responses/embedding/image, streaming included). |
| runtime/csharp/Prompty.OpenAI.Tests/SpecVectorAgentTests.cs | Updates test executor implementations to match new cancellation-aware executor signature. |
| runtime/csharp/Prompty.OpenAI.Tests/OpenAIExecutorTests.cs | Adds cancellation test ensuring early cancellation is honored before connection validation. |
| runtime/csharp/Prompty.OpenAI.Tests/AgentLoopTests.cs | Updates mock executors to accept CancellationToken in agent loop tests. |
| runtime/csharp/Prompty.Core/Pipeline.cs | Forwards CancellationToken to the executor and through pipeline execution paths. |
| runtime/csharp/Prompty.Core.Tests/TracingTests.cs | Updates mock executor signature to accept CancellationToken. |
| runtime/csharp/Prompty.Core.Tests/StructuredOutputPipelineTests.cs | Updates raw executor signature to accept CancellationToken. |
| runtime/csharp/Prompty.Core.Tests/ResilienceTests.cs | Updates mock executor signature to accept CancellationToken. |
| runtime/csharp/Prompty.Core.Tests/PipelineTests.cs | Adds assertion that Pipeline forwards the provided CancellationToken to executors; updates mocks. |
| runtime/csharp/Prompty.Core.Tests/AgentLoopIntegrationTests.cs | Updates executor signature to accept CancellationToken. |
| runtime/csharp/Prompty.Anthropic/AnthropicExecutor.cs | Adds CancellationToken propagation through HTTP calls and streaming enumerables. |
| runtime/csharp/Prompty.Anthropic.Tests/AnthropicExecutorTests.cs | Adds cancellation test ensuring early cancellation is honored before connection validation. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+181
to
+186
| const file = join(schemaRoot, `${protocol}.yaml`); | ||
| const content = readFileSync(file, "utf8"); | ||
| assert( | ||
| /^properties:\s*\{\}\s*$/mu.test(content), | ||
| `${file}: protocol schemas must not expose properties`, | ||
| ); |
Comment on lines
+76
to
+83
| for (const [protocolName, expectedProtocol] of Object.entries( | ||
| vector.protocols, | ||
| )) { | ||
| const protocol = target.protocols.find( | ||
| (candidate) => candidate.name === protocolName, | ||
| ); | ||
| assert(protocol, `${target.target}: missing ${protocolName} protocol`); | ||
| assertUniqueNames( |
Comment on lines
+23
to
+25
| #[test] | ||
| fn connection_roundtrip_vectors() { | ||
| let raw = std::fs::read_to_string(vectors_path()) |
Join TextPart values with newlines in Python and TypeScript and return an empty string for empty TypeScript messages, matching the canonical contract and Rust behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
schema/scripts/verify-engine-ports.mjs:178
verifyNoWireCancellation()hard-requires Engine*Port YAML schemas (e.g. vscode/prompty/schemas/EnginePermissionPort.yaml), but those schemas are not present in the repo right now. As a result,npm run verify:engine-portswill throw onreadFileSync()unless schema generation has already produced (and ideally committed) those protocol schema files.
for (const protocol of [
"EnginePermissionPort",
"EngineToolPort",
"EngineDurabilityPort",
"EnginePostCommitPort",
runtime/rust/prompty/tests/connection_roundtrip_vectors.rs:24
- This test will fail with the current Rust
Connectionimplementation:Connection::load_from_value()falls back toConnectionKind::default()for unknownkindstrings (runtime/rust/prompty/src/model/connection/connection.rs), sokind_str()becomes "reference" and the payload is dropped—contradicting the new vectors’ required pass-through semantics. Since this PR adds the vectors + test but doesn’t update the generated Connection model yet, Rust CI (cargo test --workspace) will go red.
#[test]
fn connection_roundtrip_vectors() {
schema/model/pipeline/engine-ports.tsp:13
- This introduces canonical EnginePort protocols, but the repo’s convention is that Typra-emitted runtime models + VS Code JSON Schemas are committed alongside .tsp changes (schema/README.md). In the current tree there are no generated EnginePermissionPort/EngineToolPort/EngineDurabilityPort/EnginePostCommitPort artifacts under runtime//model/pipeline or vscode/prompty/schemas, so consumers and contract checks won’t see these protocols until generation output is committed.
@@protocol(EnginePermissionPort);
@@method(EnginePermissionPort,
"authorize",
"EnginePermissionDecision",
"Authorize one model-requested tool before execution",
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+280
to
+283
| expectAllMatches(join(root, "Executor.cs"), [ | ||
| /^\s*Task<object>\s+ExecuteAsync\(\s*Prompty agent,\s*List<Message> messages,\s*CancellationToken cancellationToken = default\s*\);/mu, | ||
| /^\s*Task<object>\s+ExecuteStreamAsync\(\s*Prompty agent,\s*List<Message> messages,\s*CancellationToken cancellationToken = default\s*\)/mu, | ||
| /^\s*List<Message>\s+FormatToolMessages\(\s*object rawResponse,\s*List<ToolCall> toolCalls,\s*List<string> toolResults,\s*string\? textContent\s*\);/mu, |
Document ContentPart as a closed, case-sensitive union and add shared known, unknown, and wrong-case acceptance vectors. Add a non-generated Rust gate that exposes the current generated fallback-to-text defect without modifying generated code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
runtime/rust/prompty/tests/connection_roundtrip_vectors.rs:26
- This new roundtrip test currently fails with the committed generated Rust
Connectionmodel: unknownkindvalues are coerced to the defaultConnectionKindvariant during load (seeConnection::load_from_value), andkind_str()always saves a known discriminator. Sinceprompty-rust-check.ymlrunscargo test --workspaceon PRs touchingruntime/rust/**, merging this as-is will break CI. If preserving unknown connection kinds is owned by a follow-up PR (as noted in the PR description), gate/ignore this test until the generated model supports unknown-kind passthrough.
#[test]
fn connection_roundtrip_vectors() {
let raw = std::fs::read_to_string(vectors_path())
.expect("failed to read Connection roundtrip vectors");
- Files reviewed: 30/30 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+23
to
+25
| #[test] | ||
| fn content_part_discriminator_vectors() { | ||
| let raw = std::fs::read_to_string(vectors_path()) |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+107
to
118
| public static async Task<object> ExecuteAsync( | ||
| Prompty agent, | ||
| List<Message> messages, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| return await Trace.TraceAsync<object>("Prompty.Core.Pipeline.ExecuteAsync", async (emit) => | ||
| { | ||
| emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count }); | ||
| var provider = agent.Model?.Provider ?? "openai"; | ||
| var executor = InvokerRegistry.GetExecutor(provider); | ||
| return await executor.ExecuteAsync(agent, messages); | ||
| return await executor.ExecuteAsync(agent, messages, cancellationToken); | ||
| }); |
Comment on lines
+194
to
+241
| function verifyNativeSignatures() { | ||
| const root = { | ||
| csharp: join( | ||
| repoRoot, | ||
| "runtime", | ||
| "csharp", | ||
| "Prompty.Core", | ||
| "Model", | ||
| "pipeline", | ||
| ), | ||
| go: join(repoRoot, "runtime", "go", "prompty", "model"), | ||
| python: join( | ||
| repoRoot, | ||
| "runtime", | ||
| "python", | ||
| "prompty", | ||
| "prompty", | ||
| "model", | ||
| "pipeline", | ||
| ), | ||
| rust: join( | ||
| repoRoot, | ||
| "runtime", | ||
| "rust", | ||
| "prompty", | ||
| "src", | ||
| "model", | ||
| "pipeline", | ||
| ), | ||
| typescript: join( | ||
| repoRoot, | ||
| "runtime", | ||
| "typescript", | ||
| "packages", | ||
| "core", | ||
| "src", | ||
| "model", | ||
| "pipeline", | ||
| ), | ||
| }; | ||
|
|
||
| verifyCSharpSignatures(root.csharp); | ||
| verifyGoSignatures(root.go); | ||
| verifyTypeScriptSignatures(root.typescript); | ||
| verifyRustSignatures(root.rust); | ||
| verifyPythonSignatures(root.python); | ||
|
|
||
| const durabilityFiles = [ |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
runtime/csharp/Prompty.Core/Pipeline.cs:118
Pipeline.ExecuteAsyncnow forwards aCancellationTokentoIExecutor.ExecuteAsync(...), but the generatedPrompty.Core.IExecutorinterface currently definesExecuteAsync(Prompty agent, List<Message> messages)without a cancellation token. As-is, this won’t compile (and it also contradictsschema/scripts/verify-engine-ports.mjs, which expects cancellable executor signatures). Regenerate/update the emitted C# protocol surface soIExecutor.ExecuteAsync(andExecuteStreamAsyncif applicable) includesCancellationToken cancellationToken = default, and ensure all executor implementations match.
public static async Task<object> ExecuteAsync(
Prompty agent,
List<Message> messages,
CancellationToken cancellationToken = default)
{
return await Trace.TraceAsync<object>("Prompty.Core.Pipeline.ExecuteAsync", async (emit) =>
{
emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count });
var provider = agent.Model?.Provider ?? "openai";
var executor = InvokerRegistry.GetExecutor(provider);
return await executor.ExecuteAsync(agent, messages, cancellationToken);
});
- Files reviewed: 40/40 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
21
to
+28
| public class OpenAIExecutor : IExecutor | ||
| { | ||
| public async Task<object> ExecuteAsync(Core.Prompty agent, List<Message> messages) | ||
| public async Task<object> ExecuteAsync( | ||
| Core.Prompty agent, | ||
| List<Message> messages, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); |
Comment on lines
+22
to
+33
| public async Task<object> ExecuteAsync( | ||
| Core.Prompty agent, | ||
| List<Message> messages, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var streaming = agent.Metadata?.TryGetValue("stream", out var streamVal) == true && streamVal is true; | ||
|
|
||
| if (streaming) | ||
| return ExecuteStreamAsync(agent, messages); | ||
| return ExecuteStreamAsync(agent, messages, cancellationToken); | ||
|
|
||
| return await ExecuteNonStreamAsync(agent, messages); | ||
| return await ExecuteNonStreamAsync(agent, messages, cancellationToken); |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
runtime/csharp/Prompty.Core/Pipeline.cs:118
Pipeline.ExecuteAsyncnow callsexecutor.ExecuteAsync(agent, messages, cancellationToken), but the generatedPrompty.Core.IExecutorinterface currently declaresTask<object> ExecuteAsync(Prompty agent, List<Message> messages)(no CancellationToken). This will not compile unless the generated interface (and any related registry signatures) are regenerated/updated to include the optional CancellationToken parameter (and implementations match it).
{
emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count });
var provider = agent.Model?.Provider ?? "openai";
var executor = InvokerRegistry.GetExecutor(provider);
return await executor.ExecuteAsync(agent, messages, cancellationToken);
});
- Files reviewed: 40/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add native public-API gates for the shared Connection load-save-reload vectors across C#, Go, Python, Rust, and TypeScript. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
schema/scripts/verify-engine-ports.mjs:141
method.paramsis assumed to always be defined here, but if Typra ever emits a protocol method with no params (or omitsparamsfor some target),Object.hasOwn(method.params, ...)will throw a TypeError and fail the verification script. Guarding againstundefinedkeeps this check robust without changing the intended validation.
- Files reviewed: 53/53 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+41
to
+45
| try { | ||
| ContentPart.load(vector.input); | ||
| } catch (error) { | ||
| diagnostic = String(error); | ||
| } |
Comment on lines
+32
to
+39
| expect(vector.operation).toBe("load-save-reload"); | ||
|
|
||
| const loaded = Connection.load(vector.input); | ||
| if (vector.expected.kind === "reference") { | ||
| expect(loaded).toBeInstanceOf(ReferenceConnection); | ||
| } else { | ||
| expect(loaded).not.toBeInstanceOf(ReferenceConnection); | ||
| } |
Comment on lines
+33
to
+37
| with pytest.raises(ValueError) as error: | ||
| ContentPart.load(vector["input"]) | ||
|
|
||
| diagnostic = str(error.value) | ||
| assert vector["expected"]["discriminator"] in diagnostic, vector["name"] |
Comment on lines
+25
to
+33
| assert vector["operation"] == "load-save-reload" | ||
| expected = vector["expected"] | ||
|
|
||
| loaded = Connection.load(vector["input"]) | ||
| if expected["kind"] == "reference": | ||
| assert isinstance(loaded, ReferenceConnection), vector["name"] | ||
| else: | ||
| assert not isinstance(loaded, ReferenceConnection), vector["name"] | ||
|
|
Define direct Property scalar coercion separately from named input shorthand and add one atomic shared vector across C#, Go, Python, Rust, and TypeScript. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
runtime/csharp/Prompty.Core/Pipeline.cs:117
Pipeline.ExecuteAsyncnow forwards aCancellationTokenintoIExecutor.ExecuteAsync(...), but the committed generatedPrompty.Core.IExecutorinterface (runtime/csharp/Prompty.Core/Model/pipeline/Executor.cs) still declaresExecuteAsync(Prompty agent, List<Message> messages)without a token. As-is, this call (and updated executors) won’t compile until the generated interface (and any generated protocol stubs) are regenerated/updated to include the optional cancellation parameter.
public static async Task<object> ExecuteAsync(
Prompty agent,
List<Message> messages,
CancellationToken cancellationToken = default)
{
return await Trace.TraceAsync<object>("Prompty.Core.Pipeline.ExecuteAsync", async (emit) =>
{
emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count });
var provider = agent.Model?.Provider ?? "openai";
var executor = InvokerRegistry.GetExecutor(provider);
return await executor.ExecuteAsync(agent, messages, cancellationToken);
schema/scripts/verify-engine-ports.mjs:248
verifyNativeSignatures()assumes the Typra-emitted engine port protocol files already exist in each runtime (e.g., C#EnginePermissionPort.cs, Python_EnginePermissionPort.py, TSengine-permission-port.ts). In the current tree these files are not present under the referenced roots (and evenExecutorsignatures differ), sonpm run verify:engine-ports/npm run buildwill fail unless the generated runtime model outputs are regenerated and committed (or the expected paths/names are aligned to the actual emitter output).
- Files reviewed: 59/59 changed files
- Comments generated: 0 new
- Review effort level: Lite
Define collection-context scalar shorthand precedence and add focused string, integer, float, and boolean vectors with a Rust no-degradation gate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consume the shared Record<unknown> vectors in C# and assert exact model-field coverage plus load/save/reload preservation of direct, nested, and list nulls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
sethjuarez
added a commit
that referenced
this pull request
Aug 4, 2026
Adds the Swift half of the shared strict-discriminator acceptance strengthened by Prompty PR #447 commit 0c64ef3. ContentPart is a closed, case-sensitive union: unknown kinds are rejected outright, rather than preserved like unknown Connection values or dispatched like unknown Tool values. This gate pins all four clauses of that contract: 1. text loads unchanged (round trip, not just field equality) 2. video is rejected 3. Text is rejected -- matching is exact and case-sensitive 4. rejection surfaces a structured diagnostic exposing the field `kind` and the offending raw value verbatim The canonical vector, spec/vectors/model/content_part_discriminator_ vectors.json, is not checked out on this branch but does exist in history at commit b820d78. Its three cases are transcribed verbatim, including payload fields the discriminator never reads (notably durationSeconds on the video input) so the suite cannot pass on a simplified input the real vector would reject. testCanonicalVectorWhenPresent drives the same assertions straight off the vector file and activates automatically once it lands, making the suite self-repointing; it was verified green against the real vector by materialising it locally, and verified non-vacuous by corrupting an expected value. Closedness is enforced at compile time by caseName(_:), an exhaustive switch with no default: any added enum case breaks the build. Two other exhaustive switches break first today (generated save() and OpenAIWire.part(_:)) but both are code an emitter change could update automatically; this one cannot be changed without editing a test. Rejection is also gated through both nesting paths (Message.parts and ToolResult.parts) and against a valid-sibling mask, each held to the same structured-diagnostic standard rather than a bare throws check. Mutation-proved against the generated loader, every mutation reverted and the tree verified byte-exact afterwards: accept "video" -> 6 red switch discriminator.lowercased() -> 11 red throw generic .invalidObject -> 23 red report a constant raw value -> 39 red add `case unknown([String: Any])` -> build fails in 3 places Test-only; no generated model, runtime source, schema or spec file is touched. 128 tests pass (1 skipped pending the vector), including 7 live OpenAI E2E. Reported upstream, not frozen here: the generated loader coalesces an absent `kind` to "" before dispatching, so a missing field is reported as though the caller wrote `kind: ""`. Rejection is correct either way; only the diagnostic is imprecise. The test asserts rejection without pinning the "" value, so a future emitter improvement will not go red. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
runtime/csharp/Prompty.Core/Pipeline.cs:118
Pipeline.ExecuteAsynccallsexecutor.ExecuteAsync(agent, messages, cancellationToken), but the generatedIExecutorinterface (runtime/csharp/Prompty.Core/Model/pipeline/Executor.cs) currently declaresTask<object> ExecuteAsync(Prompty agent, List<Message> messages)(noCancellationToken). As-is, this won’t compile and also indicates the C# generated pipeline contracts weren’t regenerated/committed to match the new cancellation seam.
public static async Task<object> ExecuteAsync(
Prompty agent,
List<Message> messages,
CancellationToken cancellationToken = default)
{
return await Trace.TraceAsync<object>("Prompty.Core.Pipeline.ExecuteAsync", async (emit) =>
{
emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count });
var provider = agent.Model?.Provider ?? "openai";
var executor = InvokerRegistry.GetExecutor(provider);
return await executor.ExecuteAsync(agent, messages, cancellationToken);
});
- Files reviewed: 60/60 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+10
to
+12
| "verify:engine-ports": "node scripts/verify-engine-ports.mjs", | ||
| "verify:typra": "node scripts/verify-typra.mjs", | ||
| "build": "npm run format:tsp && npm run generate && npm run format:rust" | ||
| "build": "npm run format:tsp && npm run generate && npm run format:rust && npm run verify:engine-ports" |
Assert the canonical IDictionary<string, object?> mapping and outer optionality for all nine shared Record<unknown> vector surfaces. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
schema/scripts/verify-engine-ports.mjs:16
verify-engine-ports.mjsassumes it is running in a Git working tree by callinggit rev-parse --show-toplevel. This makesnpm run verify:engine-ports(and nownpm run build) fail in environments wheregitis unavailable (e.g., vendored source trees, some CI sandboxes, tarball checkouts). Consider falling back to deriving the repo root from the script’s location when the Git call fails.
- Files reviewed: 61/61 changed files
- Comments generated: 0 new
- Review effort level: Lite
sethjuarez
added a commit
that referenced
this pull request
Aug 4, 2026
Follows PR #447 b633e77, which pins the tools_function_load bindings map key and value across runtimes. The literal ask -- compare FunctionTool.bindings count, key and input against the vector -- was already satisfied here. validateBindings in LoadVectorTests does exactly that, on both the Record<Binding> map form and the already-named list form. Mutating the shared vector proves all three axes are live (each mutation reverted, spec restored byte-exact and verified identical to b633e77): expected key unit -> unitMUT -> tools[0].bindings missing 'unitMUT'; got ["unit"] expected input preferred_unit -> ...MUT -> tools[0].bindings[unit].input mismatch expected count 1 -> 2 -> tools[0].bindings count: expected 2, got 1 What was *not* covered is one level up. validateBindings opens with guard let declared = expected["bindings"] else { return } so the whole expectation is opt-in by key. Deleting the bindings block from tools_function_load leaves every load vector green -- measured, 0 failures -- while the loader is free to stop emitting bindings entirely. The expectation guards the loader; nothing guarded the expectation. testFunctionToolBindingsArePinned closes that. It has two halves that fail for different reasons and neither can cover for the other: 1. the vector still declares bindings, exactly one, unit -> preferred_unit. Erasing the block now fails loudly. 2. the fixture, loaded through the real loader, yields exactly one binding named unit with input preferred_unit. Half two compares against literals rather than against half one's values, so the test cannot degrade into checking the file against itself. The literals are deliberate duplication: a rename must now touch fixture, vector and test together instead of sliding through. Bindings are read off the generated FunctionTool rather than the Tool.bindings convenience shim, which coalesces nil to [] and would mask a nil-emitting loader as merely empty. Two review findings folded in: - tools.first is unwrapped rather than subscripted. XCTAssertEqual records without halting, so indexing after a count assertion would trap on an empty list instead of failing cleanly. - the tripwire accepts map *and* list form. validateBindings already treats them as equivalent, so accepting only the map form would report a list-form re-emission as a missing expectation -- a false diagnostic pointing at the wrong problem. Verified both ways: list form passes, and a key mutated inside the list form still fails, so the added tolerance is not blanket permissiveness. Test-only. No generated model, runtime source, schema or spec file is modified; the vector mutations above were reverted and the tree verified clean. 129 tests pass (1 skipped), including 7 live OpenAI E2E. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
runtime/csharp/Prompty.Core/Pipeline.cs:118
Pipeline.ExecuteAsynccallsexecutor.ExecuteAsync(agent, messages, cancellationToken), but the generatedPrompty.Core.IExecutorcurrently only definesExecuteAsync(Prompty, List<Message>)(seeruntime/csharp/Prompty.Core/Model/pipeline/Executor.cs:16). This will not compile until the Typra-emitted interface (and other generated engine-port protocols) are regenerated to include the cancellation seam, or until the pipeline uses a compatibility adapter (e.g., new interface/extension method + fallback to the 2-arg signature).
{
emit("inputs", new Dictionary<string, object?> { ["agent"] = agent.Name, ["message_count"] = messages.Count });
var provider = agent.Model?.Provider ?? "openai";
var executor = InvokerRegistry.GetExecutor(provider);
return await executor.ExecuteAsync(agent, messages, cancellationToken);
});
runtime/csharp/Prompty.OpenAI/OpenAIExecutor.cs:29
OpenAIExecutorimplementsPrompty.Core.IExecutor, but itsExecuteAsyncsignature was changed to include aCancellationTokenparameter. The generatedIExecutorinterface currently requiresTask<object> ExecuteAsync(Prompty agent, List<Message> messages)(no token), so this is a compile-time interface mismatch unless the Typra-generated interface has been regenerated/updated in this PR.
public async Task<object> ExecuteAsync(
Core.Prompty agent,
List<Message> messages,
CancellationToken cancellationToken = default)
{
cancellationToken.ThrowIfCancellationRequested();
var apiType = agent.Model?.ApiType ?? "chat";
runtime/csharp/Prompty.Anthropic/AnthropicExecutor.cs:28
AnthropicExecutorimplementsPrompty.Core.IExecutor, but itsExecuteAsyncsignature now includes aCancellationTokenparameter. Unless the Typra-generatedIExecutorinterface has been updated/regenerated to match, this will not compile (current generated interface takes only(Prompty, List<Message>)).
public async Task<object> ExecuteAsync(
Core.Prompty agent,
List<Message> messages,
CancellationToken cancellationToken = default)
{
cancellationToken.ThrowIfCancellationRequested();
var streaming = agent.Metadata?.TryGetValue("stream", out var streamVal) == true && streamVal is true;
- Files reviewed: 61/61 changed files
- Comments generated: 0 new
- Review effort level: Lite
sethjuarez
added a commit
that referenced
this pull request
Aug 4, 2026
PR #447 defines direct Property scalar coercion with the shared vector spec/vectors/model/property_scalar_coercion_vectors.json: one atomic, ordered four-case group (string/integer/float/boolean), each pinning an inferred kind and the exact scalar stored in `example`. This wires that fixture into the Swift acceptance harness so Swift asserts the canonical bytes continuously rather than only its own dedicated cases. The vector is not on this branch yet, so the suite is self-activating: it skips while the file is absent and begins asserting the moment #447 lands. No copy of the shared fixture is vendored -- a private duplicate of a cross-runtime contract is how runtimes silently diverge. Correction to the request's premise: all four direct cases *fail* at the pin this PR is held on. Measured against the real fixture, every bare scalar throws invalidObject("Property") -- Property.load gates on TypraRuntime.object before reading the discriminator and the generated enum carries no scalar case, so direct coercion is absent rather than wrong. A passing result can only have come from a withdrawn candidate emitter and must not be recorded as Swift conformance. Three properties this gate defends, each mutation-proved: - Distinctness. Direct coercion stores the scalar in `example`; named-collection shorthand stores it in `default`. A loader routing the direct form into `default` would satisfy a naive "the scalar survived" check while breaking the contract, so the assertion pins `example` and the absence of `default`. - Type fidelity. Comparison goes through JSONSerialization rather than Spec.equal, which returns true for (0, false): its Bool branch precedes the NSNumber branch and Foundation bridges 0/1 to Bool. Via Spec.equal a boolean case degrading to 0 would pass silently -- the exact failure a coercion contract exists to catch. - Atomicity. The vector requires all four together, so partial support fails and names each case rather than reporting progress. The fixture itself is guarded too -- exactly four ordered cases, each with `expected.kind` matching its name and an `example` present. An expectation that quietly evaporates is worse than one never written, because the suite keeps reporting success over an assertion that is no longer there. Only absence may skip. Malformed JSON, a changed root type, and an unreadable file all fail loudly, and the blocked-baseline skip is tied to the emitter pin it describes: bump the pin without fixing coercion and the gate fails rather than skipping under stale prose. Assertions run against loaded.save() rather than matching an enum case, because the fixed emitter's case shape is not yet known while save() is the public wire contract for every case. Test-only; no source, generated model, spec fixture, or emitter pin is touched. 130 tests, 2 skipped, 0 failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
sethjuarez
added a commit
that referenced
this pull request
Aug 4, 2026
The name-keyed map form of `inputs`/`outputs`/nested `properties` is
Property shorthand: the key supplies `name`, a bare scalar infers `kind`,
and the scalar belongs in `default`. The loader was storing it in
`example`, conflating this with the separate direct `@coerce` contract
(a bare scalar loaded straight through `Property`), where `example` is
correct. Both contracts are now pinned, including the distinguishing
list-versus-map pair.
An immediate array is never a valid named-collection entry, but the
loader silently widened it into a `kind: array` property. It is now
rejected with a structured `LoadError.invalidNamedCollectionEntry`
carrying the full dotted path and the value category, plus a remedy in
the message. Arrays inside declared property fields (`default`, `items`)
stay valid.
BREAKING: `inputs: { tags: [a, b, c] }` previously loaded as an
array-typed input and now fails. Declare it as
`tags: { kind: array, default: [a, b, c] }` instead. The same applies
when a `${file:}` or `${env:}` reference resolves to an array.
Adds NamedCollectionShorthandTests, which covers the fix unconditionally
on this branch, and NamedCollectionVectorTests, which drives the shared
`spec/vectors/model/named_collection_vectors.json` fixture from PR #447
and skips until it lands. Seven of its thirteen vectors are fully
asserted; the other six are blocked only on the save-side
`collectionFormat: "object"` wire form, tied to emitter pin 0.4.2, with
their entry semantics still asserted.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
sethjuarez
added a commit
that referenced
this pull request
Aug 5, 2026
… alone validateBindings resolved every expected binding with actual.first(where:), which is unsound once names repeat. Two opposite defects followed: entries sharing a name and an input satisfied both expectations from the first entry, leaving the rest unverified; and a correctly ordered duplicate pair was falsely rejected, because the second expectation also matched the first entry. Object form cannot carry one key twice, and an empty key disqualifies it as well, so either one proves the source used the array fallback - the only ordered representation. Those entries are now compared positionally, by name and input at each index. Collections that qualify for object form stay name-addressed and order-agnostic, since both forms are legal for them and asserting order there would reject a conforming loader. No current vector declares a duplicate or empty binding name, so none of this is reachable through testLoadVectors; the new tests drive validateBindings directly to pin the rule before PR #447 lands vectors that rely on it. Mutation-proved: dropping the duplicate pre-scan, the empty-name disqualifier, the positional name comparison, or the map empty-key guard each kills exactly the test covering it, and forcing every collection positional kills the order-agnostic guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PortError, and project executor cancellation through the C# runtimeConnection.kindforward-compatible and case-sensitive, with shared known, unknown, and case-collision load/save/reload vectorsCustomToolsemantics separateMessage.text/toTextContentbehavior with the canonical newline contract and Rust parityValidation
npm run format:tsp:check- passed (48 files)npm run verify:engine-ports- passed, including the immutable legacy harness SHAtsc --noEmitpassedcargo test --manifest-path runtime/rust/Cargo.toml --workspace --no-fail-fast- every existing target passed; the newconnection_roundtrip_vectorstarget fails as designed against current generated bytes becausefuture-authis coerced toreferenceThe remaining Rust failure is the executable Typra emitter acceptance gate. PR #36 owns generation of an unknown Connection variant/value that retains the exact discriminator and raw payload; no generated file is patched here.