fix(chat): structured output vs grounding and output masking - #52
Merged
Conversation
Closes #50. `grounding: strict` makes the gateway own the system prompt, which demands {found, answer, quotes}. A caller `json_schema` constrains the provider to a DIFFERENT shape, so the model returns valid JSON with none of those keys, verifyGrounding fails closed, and EVERY answer comes back as the refusal — after a paid model call each time, and indistinguishable from a genuine "not in the knowledge base" result. #43 (structured output) and #45 (configurable grounding) landed back to back and neither knew the other existed. That is my miss: the gateway owning the prompt is fundamentally incompatible with the caller owning the schema, and nothing said so. Now a 400 `grounding_response_format_conflict` before any provider spend, mirroring streamSafetyGate, which already rejects the same class of conflict for streaming. `json_object` stays allowed: grounding already requires JSON, so that agrees with the prompt rather than fighting it. Not done: the issue also suggested catching this in `modelgov validate`. responseFormat is a per-REQUEST field, not something a feature declares in config, so there is nothing for config validation to inspect. It can only be a request-time gate.
Closes #51. /chat with responseFormat and output PII masking ran inspectOutput over the SERIALIZED JSON as if it were prose. Two problems. Structure: rewriting spans in serialized text can damage the document, not just its values — a detected span covering a delimiter breaks the parse. A test demonstrates a prose-style pass producing unparseable JSON where the leaf-walk cannot. Now the payload is parsed and its string LEAVES are masked, which keeps it well-formed by construction. Object keys are not touched: they are the caller's schema, not model output. Signalling: /documents reports structuredWithheld and a structured_withheld reason code; /chat reported nothing comparable. Added `structuredMasked` on the response and a `structured_masked` audit reason code. Two corrections to the issue as filed. It says the rewrite is silent — it is not, `safety.piiMasked` was already returned; the gap is that nothing said the STRUCTURED payload specifically was touched. And it understates the sharpest consequence: a masked value is `[REDACTED]`, so a json_schema response can now violate the very schema the caller declared (a number, date or enum field returning a redaction string). That is documented on the field, and the real fix for an extraction feature is #46's per-entity policy allowing the entity types it exists to extract. Falls back to prose masking when the model ignored responseFormat, when the payload is a bare scalar, or when the guard cannot batch — never skipping output safety. Fails closed on a mismatched masked-value count rather than pairing a value with the wrong field.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JSON-aware PII masking for chat structured responses, masking metadata and audit reasons, plus pre-provider validation for incompatible grounded response formats. Updates response contracts and tests for structured masking, prose fallback, and grounding behavior. ChangesChat safety and grounding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPipeline
participant maskStructuredOutput
participant OutputSafety
participant ChatResponse
ChatPipeline->>maskStructuredOutput: formatted response content and safety plan
maskStructuredOutput->>OutputSafety: inspect JSON string leaves
OutputSafety-->>maskStructuredOutput: masked values and findings
maskStructuredOutput-->>ChatPipeline: serialized content and masking status
ChatPipeline->>ChatResponse: structuredMasked and piiMasked safety metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/modules/chat/pipeline.ts`:
- Around line 157-171: Update the post-provider error handling around
maskStructuredOutput in the pipeline try block to catch structured-masking
failures, including plain Error instances from mismatched batch and JSON leaf
counts. Route these failures through the existing post-provider failure path so
the audit result is written and settleBilling is called exactly once before
returning the safe failure response.
In `@packages/api/src/modules/chat/structuredOutput.ts`:
- Around line 86-87: Update the structured-output handling around the none
fallback and parsing flow so valid JSON objects or arrays are parsed before
checking inspectOutputMany; when batching is unavailable, withhold or fail
closed instead of applying prose masking. Keep prose fallback only for invalid
or scalar JSON, and add coverage in structured-output-masking.test.ts for the
fail-closed result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96008d97-1954-4cf1-a206-ac5ef4ec6a3d
⛔ Files ignored due to path filters (1)
packages/api/openapi.jsonis excluded by!packages/api/openapi.json
📒 Files selected for processing (10)
packages/api/src/modules/chat/mapper.tspackages/api/src/modules/chat/pipeline.tspackages/api/src/modules/chat/prepare.tspackages/api/src/modules/chat/schemas.tspackages/api/src/modules/chat/structuredOutput.tspackages/api/src/modules/chat/types.tspackages/api/src/modules/usage/auditLogRepo.tspackages/api/test/chat.integration.test.tspackages/api/test/grounding.integration.test.tspackages/api/test/structured-output-masking.test.ts
…k JSON Two Major findings from review, plus a bug my own suite caught while fixing them. BILLING LEAK. maskStructuredOutput threw a plain Error on a masked-value count mismatch, but the pipeline's catch handles SafetyServiceError only — so it escaped past settleBilling. The provider call had already happened and cost real money, and the reservation was never settled. Now a SafetyServiceError, which is semantically right (the guard violated its contract) and routes through the settling path. An integration test asserts reserved_usd returns to zero. PROSE-MASKING VALID JSON. The batch-support check ran BEFORE the parse, so a guard without inspectOutputMany sent a structured payload down the prose path — the delimiter-corruption risk this module exists to prevent. Parses first now and fails closed for objects/arrays, reserving the fallback for genuinely non-JSON content. ...and that fail-closed branch then fired regardless of the plan, so a `pii: off` feature that merely asked for structured output 503'd on any guard lacking the optional batch method. Caught by an existing test of mine that passes in isolation and fails in the full suite. Now returns early when the plan would not mask output at all.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/modules/chat/structuredOutput.ts`:
- Around line 77-80: Update the result contract comment near the
structured-output guard to document that structured: false is also returned when
pii is "off" or piiScope is "input" because masking is a no-op. Preserve the
existing description for non-object JSON and the fail-closed behavior for
unmaskable structured payloads.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e402db2-8df4-4d18-b129-9e771da63557
📒 Files selected for processing (3)
packages/api/src/modules/chat/structuredOutput.tspackages/api/test/chat.integration.test.tspackages/api/test/structured-output-masking.test.ts
structured:false now has two causes, not one — non-object JSON, and a plan that would not mask output at all. The comment claimed only the first.
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
1 participant
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.
Closes #50. Closes #51.
Both are interactions between features that landed back to back in this batch and never knew about each other. Verified against the code before fixing, not taken at face value.
#50 —
json_schema+grounding: strictrefused every answerConfirmed exactly as filed.
prepare.tspassedresponseFormatthrough at both call sites with no grounding check, whilestreamSafetyGatesits 250 lines above rejecting the same class of conflict for streaming.Reproduced: the gateway's prompt demands
{found, answer, quotes}; a caller schema constrains the provider to a different shape;verifyGroundingfinds none of those keys and fails closed. Every request returns the refusal, after a paid model call, indistinguishable from a genuine "not in the knowledge base".Now a 400
grounding_response_format_conflictbefore any provider spend — asserted in the test via a provider-call counter.json_objectstays allowed, since grounding already requires JSON.Not done: the issue suggested also catching this in
modelgov validate.responseFormatis a per-request field, not something a feature declares in config, so there is nothing for config validation to inspect. It can only be a request-time gate.#51 — structured output rewritten by output masking
Real, with two corrections to the report.
It is not silent.
mapper.tsalready returnedsafety.piiMasked, so a caller did get a signal that something was masked. The genuine gap is that nothing said the structured payload specifically was rewritten.The sharpest consequence is understated. A masked value is
[REDACTED], so ajson_schemaresponse can violate the very schema the caller declared — a field typed as a number, date, or enum comes back as a redaction string. Schema-constrained extraction returning schema-violating data is worse than a missing flag. Documented on the field; the real fix for an extraction feature is #46's per-entity policy allowing the entity types it exists to extract.What changed:
structuredMaskedon the response and astructured_maskedaudit reason code, mirroring/documents'structuredWithheld/structured_withheld.responseFormat, when the payload is a bare scalar, or when the guard cannot batch — never skipping output safety.One thing that nearly shipped broken: the flag was set correctly but invisible, because Fastify's response serializer drops fields absent from the declared schema. Caught by dumping the real response instead of trusting the code path.
Verification
pnpm verifygreen: 1285 tests (up from 1268). 19 new cases acrossstructured-output-masking.test.ts, the chat integration suite (flag + reason code end to end, and absence on prose responses), and the grounding integration suite (conflict rejected with no provider call,json_objectstill allowed, ungrounded features unaffected).Summary by CodeRabbit
New Features
json_objectformats and clearly reject unsupportedjson_schemaformats.Bug Fixes
Tests