Replace bespoke contract matcher with schema validation - #9
tigyijanos wants to merge 8 commits into
Conversation
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d441b09be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var outputSchemaWasDerived = !JsonSchemaUtilities.LooksLikeJsonSchema(responseContract); | ||
| var outputSchemaJson = JsonSchemaUtilities.NormalizeToSchemaJson(responseContract); | ||
| var outputSchema = JsonSchema.FromText(outputSchemaJson); |
There was a problem hiding this comment.
Treat
type-bearing examples as contracts, not schemas
This change now compiles outputSchemaJson immediately, but outputSchemaJson is still gated by LooksLikeJsonSchema, which classifies any object with keys like type as a schema. For endpoints whose response example includes a normal domain field such as "type": "transfer", we will skip derivation and try to compile the raw example as JSON Schema, causing contract resolution to throw and requests for that route to fail with server errors. Before this commit, that false-positive classification did not hard-fail request handling.
Useful? React with 👍 / 👎.
| var evaluation = resolvedContract.OutputSchema.Evaluate( | ||
| cliResponse.RootElement, | ||
| new EvaluationOptions | ||
| { | ||
| OutputFormat = OutputFormat.List |
There was a problem hiding this comment.
Preserve decimal numbers when validating derived schemas
Hard-guard validation now uses JSON Schema evaluation, so derived schemas enforce integer vs number. For example-style contracts that use whole-number samples (e.g. amount: 200 in bank endpoints), BuildJsonSchemaFromExample emits "type": "integer"; a valid runtime response containing a decimal amount (allowed by endpoint docs as a generic number) will now fail the guard and return 502. The previous matcher accepted both integer and decimal values because it only compared JsonValueKind.Number.
Useful? React with 👍 / 👎.
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
…sonSchemaUtilities Replace NormalizeResponseExampleToSchema benchmark with ValidateResponseAgainstSchema that exercises the new JsonSchema.FromText + Evaluate path. The old benchmark called JsonSchemaUtilities.NormalizeToSchemaJson which was deleted in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous bespoke response “contract matcher” with JSON Schema validation, requiring endpoint response contracts to provide explicit JSON Schemas and refactoring related runtime/CLI execution and warm-start code.
Changes:
- Switch hard-guard response validation to
JsonSchema.Netschema evaluation and require schemas (no example-payload inference). - Refactor cold CLI execution into
Runtime/CodexCliExecutor.csand simplify startup warmup wiring inProgram.cs. - Update instruction endpoint contract blocks from example payloads to JSON Schemas; remove/ignore generated runtime data artifacts.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/PhantomApi/RuntimeProfile.cs | Removes the standalone RuntimeProfile record (now represented via tuple alias). |
| src/PhantomApi/Runtime/CodexCliExecutor.cs | Adds centralized CLI process execution helper used by cold path / session pool. |
| src/PhantomApi/Program.cs | Wires JSON Schema validation, uses new CLI executor, and changes startup warmup/disposal behavior. |
| src/PhantomApi/PhantomStartupWork.cs | Removes warm-start wrapper record (startup flow no longer uses it). |
| src/PhantomApi/PhantomStartupHostedService.cs | Removes hosted service previously coordinating warmup + disposal. |
| src/PhantomApi/PhantomApi.csproj | Adds JsonSchema.Net dependency for schema compilation/evaluation. |
| src/PhantomApi/JsonSchemaUtilities.cs | Deletes schema inference/normalization utilities (schemas now required). |
| src/PhantomApi/EndpointContractCache.cs | Loads/compiles JSON Schema for endpoint output contracts and caches it. |
| src/PhantomApi/CodexCliExecutionResult.cs | Removes standalone result record (now tuple alias). |
| src/PhantomApi/CodexAppServerClient.cs | Removes runtime schema-derivation fallback; refactors event dispatch normalization. |
| instructions/framework/contract-discipline.md | Updates framework guidance to require JSON Schema contracts explicitly. |
| instructions/apps/task-board/endpoints/tasks/list.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/task-board/endpoints/tasks/create.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/task-board/endpoints/auth/login.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/bank-api/endpoints/bank/withdraw.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/bank-api/endpoints/bank/transfer.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/bank-api/endpoints/bank/get-balance.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/bank-api/endpoints/bank/deposit.md | Converts response contract block from example payload to JSON Schema. |
| instructions/apps/bank-api/endpoints/auth/login.md | Converts response contract block from example payload to JSON Schema. |
| data/framework/traces/events.jsonl | Removes tracked runtime trace artifact file. |
| data/framework/self-healing/validations.jsonl | Removes tracked runtime self-healing artifact file. |
| data/framework/self-healing/rollbacks.jsonl | Removes tracked runtime self-healing artifact file. |
| data/framework/self-healing/patches.jsonl | Removes tracked runtime self-healing artifact file. |
| data/framework/self-healing/diagnoses.jsonl | Removes tracked runtime self-healing artifact file. |
| data/framework/requests/ledger.jsonl | Removes tracked runtime request ledger artifact file. |
| data/framework/metrics/counters.json | Removes tracked runtime metrics counters artifact file. |
| data/framework/incidents/open.json | Removes tracked runtime incidents artifact file. |
| data/framework/audit/security.jsonl | Removes tracked runtime audit artifact file. |
| benchmarks/PhantomApi.Benchmarks/RuntimeBenchmarks.cs | Updates benchmark to validate a sample response against a compiled JSON Schema. |
| .gitignore | Ignores framework runtime artifact directories/files that are no longer tracked. |
| var stopwatch = Stopwatch.StartNew(); | ||
| var arguments = resumeSessionId is null | ||
| ? TokenizeArguments(BuildCliArgumentsTemplate(profile)) | ||
| : BuildResumeCliArguments(profile, resumeSessionId); | ||
|
|
There was a problem hiding this comment.
CodexCliExecutor.ExecuteAsync ignores PhantomOptions.CliArgumentsTemplate and always builds a fixed argument list via BuildCliArgumentsTemplate(profile). This breaks the documented {output} placeholder flow (and makes CliArgumentsTemplate effectively dead), and it also means the executor now assumes the CLI emits a clean JSON payload on stdout even though the config/example uses --output-last-message {output} to guarantee JSON. Consider re-introducing support for CliArgumentsTemplate (including {output} substitution when present) and/or restoring the output-file read path so RawResponse is reliably parseable JSON for JsonDocument.Parse callers.
| var defaultProfile = ResolveRuntimeProfile(phantomOptions, phantomOptions.FastModeEnabled); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(phantomOptions.CliArgumentsTemplate)) | ||
| { | ||
| phantomOptions.CliArgumentsTemplate = BuildCliArgumentsTemplate(phantomOptions, defaultProfile); | ||
| phantomOptions.CliArgumentsTemplate = CodexCliExecutor.BuildCliArgumentsTemplate(defaultProfile); | ||
| } |
There was a problem hiding this comment.
phantomOptions.CliArgumentsTemplate is still being defaulted here, but the cold-exec path no longer uses this template when invoking the CLI (the executor builds its own args). This makes the CliArgumentsTemplate setting ineffective and can also remove --output-last-message {output} from the default invocation, which may cause non-JSON stdout to reach JsonDocument.Parse. Either wire CliArgumentsTemplate through to the executor (and keep {output} behavior), or remove the setting entirely to avoid a misleading configuration knob.
| @@ -117,8 +122,12 @@ | |||
| ["serviceTier"] = defaultProfile.ServiceTier ?? "default" | |||
| }); | |||
| } | |||
| }, | |||
| WarmConfiguredEndpointsAsync: cancellationToken => WarmConfiguredEndpointsAsync( | |||
| }); | |||
| }); | |||
|
|
|||
| app.Lifetime.ApplicationStarted.Register(() => | |||
| { | |||
| _ = Task.Run(() => WarmConfiguredEndpointsAsync( | |||
| repoRoot, | |||
| defaultProfile, | |||
| phantomOptions, | |||
| @@ -127,10 +136,13 @@ | |||
| endpointContractCache, | |||
| execSessionPool, | |||
| cliArgumentsTemplateWasProvided, | |||
| cancellationToken))); | |||
| builder.Services.AddHostedService<PhantomStartupHostedService>(); | |||
| CancellationToken.None)); | |||
| }); | |||
|
|
|||
| var app = builder.Build(); | |||
| app.Lifetime.ApplicationStopping.Register(() => | |||
| { | |||
| _ = appServerClient?.DisposeAsync().AsTask(); | |||
| }); | |||
There was a problem hiding this comment.
These startup warmup tasks are fire-and-forget (Task.Run) with CancellationToken.None, so they won’t stop promptly on shutdown and any exception escaping WarmConfiguredEndpointsAsync will be unobserved. Also, DisposeAsync() is triggered on ApplicationStopping but not awaited, which can leave the app-server process running past shutdown. Consider reintroducing an IHostedService/BackgroundService (or otherwise linking to app.Lifetime.ApplicationStopping and logging failures) so warmup and disposal are coordinated and exceptions are observed.
| var outputSchemaElement = responseContractDocument.RootElement.Clone(); | ||
| if (!LooksLikeJsonSchema(outputSchemaElement)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"The first json code block in {sourcePath.Replace('\\', '/')} must be valid JSON Schema."); | ||
| } | ||
|
|
||
| var outputSchemaJson = outputSchemaElement.GetRawText(); | ||
| var outputSchema = JsonSchema.FromText(outputSchemaJson); | ||
|
|
There was a problem hiding this comment.
LooksLikeJsonSchema is a heuristic and will reject valid JSON Schemas that don’t use any of the listed keywords (e.g. schemas built around allOf/anyOf/oneOf/$ref without type/properties). Since the error message says “must be valid JSON Schema”, it would be more accurate to attempt parsing/compiling with JsonSchema.FromText and report a parse/validation failure (including sourcePath) rather than relying on this keyword check.
Summary
Verification