fix(core): bound the model field, and stop the smuggling test certifying the hole - #933
Conversation
|
Update after a sibling review: the copilot JetBrains unkeyed digest, which this PR documented rather than fixed, is fixed in #935. That review established the privacy key is actually available in The constraint this PR recorded still holds and is worth keeping in view: the bridged CLI path passes an empty key, so full closure needs a real key threaded into rich decode. The note at the site in #935 says so. Treat the "documented rather than fixed" paragraph above as superseded. |
|
Converted to draft: this was opened with only a self-review by the model that wrote it, which is not the review bar this batch is held to. Independent review by two other reviewers is running now; I will mark it ready once both have passed and any findings are applied. Apologies for the noise. |
… keys Three contract defects in the published observation surface, and a fourth that the first attempt at fixing them created. **`model` and `pricingModel` were unbounded free text** — `z.string().min(1)`, no max, no pattern — inside a schema whose docstring states that no field can hold free text. For vercel-gateway the value arrives from a fetched report. The first attempt bounded them with a pattern. Review proved that wrong: real providers emit display names, not identifiers. Antigravity reads `payload.model.display_name` directly, Warp's alias map is a closed ten-entry list that returns anything unmapped verbatim, and devin emits the same shape. A pattern would have rejected the envelope for those providers, and the next Warp model to ship would have broken decoding — a breaking narrowing of a contract already published at 0.9.20. So the value is normalized at the observation boundary instead, which is what this package already does for tool names, rather than validated and rejected. **The smuggling test asserted the leak survives** — it planted a prompt in `model` and checked it was still there. Inverting that assertion exposed the real carrier: `dedupKey` is built from the model string, so bounding the model while the key is derived from it is half a fix. The secret rode through the key. **`sourceRef` was documented as an opaque fingerprint while every caller passed a raw absolute path**, and three decoders folded it straight into dedup keys — which are observation output. A reviewer produced a schema-valid envelope containing a real home directory path. Rather than rewriting the contract text to match the leak, the decoders now fingerprint it: codebuff, lingtai-tui and zerostack all route through the keyed helper. That also matters for getagentseal#931, which derives sync span ids from these keys. The codebuff golden pinned the raw-path form. It is updated with the derivation shown, and a comment recording that the raw path was the defect — the same trap the kiro golden set in a sibling PR.
eafa850 to
bd756ee
Compare
Three contract defects in
@codeburn/core's published observation surface. 0.9.20 is already on npm, so an external consumer meets all three today.Framing first, because it changes what these are. Nothing in this repo emits provider-derived observations:
packages/cli/src/optimize.tsis the only file outside core that builds an envelope, and it hardcodesdedupKey: 'host'andmodel: 'host'to feed the detectors from host-side tool calls. So these are latent API defects, not a live leak — worth fixing before anything starts emitting, and before more consumers build on the schema.modelwas unbounded free textz.string().min(1), no max, no pattern — inside a schema whose docstring three lines above states that no field can hold free text. For vercel-gateway the model string arrives from a fetched report, so it is externally influenced.Both
modelandpricingModelnow use a bounded identifier pattern, following the existingCanonicalToolNameconvention rather than inventing a new shape. The character set was measured against real values across the provider decoders (openai/gpt-4o,anthropic--claude-4.6-opus,us.anthropic.claude-3-5-sonnet-v2:0,@cf/meta/...) and the cap sits comfortably above the longest slug in the pricing snapshot.The smuggling test asserted the leak survives
It planted a prompt in
modeland then asserted the prompt was still there, under the name "contains the model secret (identifier-exemption convention)". A guard test that certifies the hole it exists to catch is worse than no test — it makes the gap look considered.The assertion is inverted: a planted prompt is now rejected at validation, while a legitimate identifier-shaped slug still passes and is asserted explicitly so the exemption's real intent survives. The sibling test in the vercel-gateway suite had the same shape and is fixed too — this was a class, not a site.
The contract described a fingerprint; every caller passed a raw path
DecodeContext.sourceRefwas documented as "an opaque fingerprint of the source". The bridge passes an absolute filesystem path, and zerostack, pi, lingtai-tui and codebuff fold it into dedup keys.Changing the callers would break session identity that parity goldens certify — codebuff and pi parse structure out of that path — so the contract now describes what is actually passed, with an explicit policy for where the value may travel: into dedup keys and identity derivation, never into an envelope fingerprint field.
One defect documented rather than fixed
The copilot JetBrains decode builds a dedup key from an unkeyed digest of assistant reply text. That path has no privacy key available, and routing it through core's fingerprint would break the bridge's documented key-free contract and move a golden — a maintainer decision, not a silent change. The constraint and the proposed fix are recorded at the site.
Core suite 509 green; no parity golden moved.