[pull] main from danny-avila:main - #212
Merged
Merged
Conversation
* 🌌 feat: Enforce GPT-6 Astra Request Constraints OpenAI's GPT-6 Astra rejects several parts of a request its predecessors accepted. None of it is expressible at configuration time, so LibreChat cannot register the model until the SDK enforces the constraints on the wire. - Route tool-bearing turns to the Responses API. Astra serves tool calls only from Responses, and `_useResponsesApi` sees the resolved `options.tools`, so the gate is exactly the documented rule. A non-tool turn keeps Chat Completions rather than being routed defensively — the config-time GPT-5.6 rule has to over-approximate to "any reasoning set" precisely because tools bind after it runs. - Strip `temperature`, `top_p` and `top_logprobs` on both paths, `logprobs` on Chat Completions, and `message.output_text.logprobs` from the Responses `include`. LangChain's builders emit the sampling fields from instance fields unconditionally, so leaving them unset upstream is not enough; they are removed after `super.invocationParams`. - Substitute the rejected reasoning efforts. `none` returns a 400 and `minimal` is not offered; OpenAI's migration guide says to start with `low`. The substitution lands in the merged `getReasoningParams`, so it covers both wire shapes — the scalar `reasoning_effort` on Chat Completions and the nested `reasoning.effort` on Responses — and applies to every caller, including a stored agent configuration carrying an effort the previous model accepted. - Request encrypted reasoning for Astra, which supports persisted reasoning. `isGpt6AstraModel` matches the model id rather than a `gpt-6` family cutoff. Every gate here removes capability, so a false positive silently degrades a sibling model that never needed it; the match stays narrow until OpenAI documents the same rules more widely. A provider or deployment prefix is stripped first so gateway-style ids resolve. Tests cover detection (snapshots, prefixes, case, and the near-miss ids `gpt-6`, `gpt-6-mini`, `gpt-6-astral`), routing on both paths, each stripped parameter, effort substitution and pass-through on both wire shapes, and that GPT-5.6 is unaffected. Eight of them fail with the source change reverted; the rest are characterizations proving no behavior changed for other models. Ref: https://developers.openai.com/api/docs/models/gpt-6-astra Ref: https://developers.openai.com/api/docs/guides/latest-model * 🔒 fix: Keep the GPT-6 Astra Rules Off Proxy-Routed Ids [Codex P2, resolved in the opposite direction] The detection helper stripped a `provider/` prefix before matching, so `openai/gpt-6-astra` matched. Because `ChatOpenRouter extends ChatOpenAI`, that applied OpenAI's request rules to OpenRouter. Codex reported the narrower symptom — the effort substitution reaching only one of `buildOpenRouterReasoning`'s three merge steps — and suggested completing the sanitization across the other two. Applying it more widely is the wrong direction: on OpenRouter `effort: 'none'` is a *supported* value meaning "disable reasoning", mapping to `include_reasoning: false`. Substituting it with `low` would silently turn reasoning off into reasoning on, changing behavior and cost to avoid an error that does not occur there. The routing gate was the larger half and was not reported: `ChatOpenRouter` branches on `this._useResponsesApi(options)`, so a prefixed Astra id with tools bound would have been forced onto the Responses path — an endpoint shape a proxy may not serve. Dropping the prefix stripping fixes all three gates at once. A slash means a proxy owns the request contract; bare ids reach the first-party OpenAI and Azure surfaces these rules actually describe. Tests now pin that `openai/gpt-6-astra` keeps Chat Completions with tools bound, keeps its sampling parameters, and keeps `effort: 'none'` unsubstituted. * 🎯 fix: Gate the GPT-6 Astra Rules on the Request Endpoint [Codex P2] Scoping the rules by model id alone left a gap the prefixed-id fix did not close: a bare `gpt-6-astra` routed through a compatibility gateway via `configuration.baseURL` (or the `OPENAI_BASE_URL` fallback) still matched, so every gate fired against a proxy — tool calls forced onto Responses, sampling fields stripped, `none` reasoning rewritten to `low`. Both halves must now hold: the model is Astra *and* the request reaches the first-party surface these rules describe. That is not a new concept here — `isOfficialOpenAIBaseURL` and `isFirstPartyAzureEndpoint` already gate the first-party streamed-tool-call adapter for the same reason, and the Azure helper's own comment gives the rationale: a custom base path "routes through a proxy or Azure-compatible endpoint whose stream contract is unknown". The predicate is resolved per class through an `astraRulesApply` getter — the OpenAI variant against `clientConfig.baseURL`, the Azure variant also against `azureOpenAIBasePath` — and the helpers now take that resolved boolean instead of re-deriving it from a model string. `shouldIncludeEncryptedReasoning` gains it as a defaulted third argument, so GPT-5.6 behavior is untouched. Tests pin both directions on the same model id: a first-party client applies every gate (Responses routing, sampling stripped, `none` becomes `low`), while the same id behind a custom `baseURL` keeps Chat Completions, its sampling parameters, and `none`. Verified by neutering the base-URL half of the guard — the proxy test fails and nothing else does. * 🔗 fix: Compare the OpenAI Base URL by Parsed Origin [Codex P2] `isOfficialOpenAIBaseURL` matched textually against `^https://api\.openai\.com(/|$)`, so two spellings of the same first-party endpoint were read as proxied: an explicit default port (`https://api.openai.com:443/v1`) and any non-lowercase host (`https://API.OpenAI.com/v1`). That predated this branch and only gated the streamed-tool-call adapter, where the cost was a missing stamp. The previous commit made the GPT-6 Astra rules depend on it too, so the same gap now also drops Astra's Responses routing and leaves rejected sampling and reasoning fields on the wire — a first-party request treated as someone else's endpoint. Compare through the URL parser instead. It normalizes host case and drops the default `:443`, which are the two spellings at issue, while keeping the distinctions that matter: a lookalike host such as `api.openai.com.example.net` parses to a different hostname, a non-default port is someone else's listener, a plaintext scheme is not the official API, and an unparseable value is not a URL at all. All four stay proxied. Because the helper is shared, the streamed-tool-call adapter gets the same correction; its suite gains cases for both accepted spellings and all four rejected ones. The Astra suite asserts the same normalization through the gates it now controls. Verified by restoring the old pattern: exactly the four new first-party-spelling tests fail, across both suites. * 🧭 fix: Finish the Endpoint-Identity Class and Type the Strip Boundary Two findings, resolved together because the first is the other half of a class this branch has been fixing one member at a time. [Codex P2] `isFirstPartyAzureEndpoint` still compared textually, so a mixed-case but equivalent host (`https://RESOURCE.OPENAI.AZURE.COM/...`) read as a proxy. That is the same defect the previous commit fixed for OpenAI, in the predicate written directly beneath it — it should have been swept then rather than found now. Both endpoint-identity checks now parse the URL and compare the host, and a sweep of the file confirms these two are the only such comparisons. Azure keeps accepting any port, as its previous pattern did. [Codex P1] The strip boundary declared its fields as `unknown`, against AGENTS.md's rule to avoid `unknown` where an explicit type exists. It now derives from the SDK's own request types — `Pick` over `ChatCompletionCreateParams` and `ResponseCreateParams`, made optional so keys stay deletable — so the boundary keeps the SDK's constraints instead of discarding them, and the rejected `include` entry is typed `ResponseIncludable` rather than a bare string. The adapter suite gains both mixed-case Azure spellings and the rejected plaintext/unparseable ones. Verified by restoring the case-sensitive comparison: exactly the two mixed-case tests fail. * 🧷 fix: Type the Tools Boundary and Isolate the Env the Gate Reads Two of round six's findings; the two Azure endpoint-identity ones are held pending a design decision rather than patched. [Codex P1] `hasBoundTools` still declared `tools?: unknown` after the previous commit typed the strip boundary — the same AGENTS.md rule, fixed in one place and not the other. It now takes `t.ChatOpenAICallOptions['tools']`. [Codex P2] The Astra tests created clients with no `baseURL`, and the endpoint gate falls back to `OPENAI_BASE_URL`. A developer or CI shell pointing at a compatibility gateway therefore turned every gate off and failed six tests for a reason unrelated to the implementation. The describe block now clears and restores that variable, matching the sequential-tool-call suite, which isolates it for exactly this reason. Reproduced and verified rather than assumed: with `OPENAI_BASE_URL=https://gateway.internal/v1` set, the suite fails 6 of 25 without the isolation and passes 25 of 25 with it. * 🪧 refactor: Let the Caller Declare the First-Party Endpoint Replaces the inferred endpoint check behind the GPT-6 Astra rules with a declared `firstPartyEndpoint` field, defaulting to off. Inferring it was the wrong seam, and the review history shows why: six findings across four rounds were all the same question asked at a new surface — a provider-prefixed model id, a custom `baseURL`, how that URL was compared, the Azure spelling of the same comparison, Azure configured through `configuration.baseURL`, and Azure identifying its model by deployment name. Each fix was correct and none of them converged, because a base URL cannot answer the question being asked of it: only the caller knows whether a given URL is a faithful first-party route, a gateway, or a proxy with its own semantics. The seam now follows what each layer actually knows. The SDK knows which models carry which constraints; the caller knows which endpoint it is talking to. LibreChat already computes exactly this once, in `isCanonicalOpenAIBaseURL`, so declaring it keeps that decision in one place rather than duplicating it here where the two can drift. Defaults to off, so an undeclared client keeps its current behavior and a misconfigured Astra call fails with the provider's own error — which names the remedy — instead of being silently rewritten. `isOfficialOpenAIBaseURL` and `isFirstPartyAzureEndpoint` keep the parsing corrections from earlier commits. They now serve only the streamed-tool-call adapter they were written for, where the earlier textual comparison was a genuine defect; those changes are independent of Astra and could be split out. Tests now cover the declared and undeclared paths on the same model id, and that declaring the endpoint does not widen the gates to another model. * 🔓 fix: Publish the Endpoint Declaration and Guard Injected Delegates [Codex P1] `firstPartyEndpoint` was declared only on the internal field types, so a caller going through `initializeModel`/`ProviderOptionsMap` could not set it without a cast — and since the gate defaults to off, a typed Astra configuration silently kept Chat Completions for tools. It now lives on `ManagedRequestOptions`, which is documented as the shared OpenAI/Azure passthrough and already mixes into both `OpenAIClientOptions` and `AzureClientOptions`, so one declaration covers both providers. [Codex P2] `withLibreChatOpenAIFields` keeps a caller-supplied `completions`/`responses` delegate instead of building its own, but the parameter stripping and effort substitution live inside the LibreChat delegates. The routing gate would therefore have sent a tool-bearing turn to an injected Responses delegate carrying the very parameters Astra rejects — worse than not routing at all. `ChatOpenAI` now records whether it owns both delegates and the routing gate stands down when it does not. `AzureChatOpenAI` constructs its delegates unconditionally, so it is unaffected. The delegates still owned keep enforcing what they can: with only `responses` injected, the completions path continues stripping the rejected parameters, and a tool call fails with the provider's own error instead of a silently invalid request. Writing the test surfaced that — the first expectation assumed the whole feature switched off, which was wrong. Neither finding is the endpoint-identity class the previous commit collapsed. * ✂️ refactor: Leave API Selection to the Caller Drops the invocation-time Responses routing for GPT-6 Astra. What remains here is request shaping the model requires on either API: the sampling and logprob parameters it rejects, the reasoning efforts it does not accept, and the encrypted reasoning it supports. Routing at invocation time was wrong, and a review finding on the LibreChat side showed why concretely: the caller shapes the max-tokens field from the API it believes it is using, so switching afterwards sends `max_completion_tokens` to an endpoint expecting `max_output_tokens`. Any request field whose shape depends on the endpoint has the same problem, so the decision has to be made before shaping, in one place, by the layer that shapes. OpenAI's guidance points the same way — "Use the Responses API" for Astra generally, not only for tool calls — so a caller can decide this statically without needing the resolved tool list, which was the original argument for deciding it here. Removing it also removes the delegate-ownership guard added a commit ago, which existed only to stop that routing from half-applying to an injected delegate, and `hasBoundTools` with it. The remaining shaping is unaffected by injected delegates: each LibreChat delegate enforces its own. * 🏷 feat: Accept a Declared Served Model [Codex P1] Azure addresses a deployment rather than a model, so callers set `model` to the deployment name. The request-shaping rules keyed off `model` and therefore saw an arbitrary alias, applying nothing to a deployment not literally named `gpt-6-astra`. Adds `servedModel` to `ManagedRequestOptions`, used in preference to `model` when detecting which constraints apply. Declared rather than inferred, for the same reason as `firstPartyEndpoint`: asking this layer to recover a model from a deployment alias is the same category error as asking it to recover an endpoint from a base URL, which is what produced most of this branch's review findings. Tests cover a deployment alias serving Astra, and one serving something else so the declaration cannot widen the rules by itself. * 🧩 fix: Resolve the Served Model in the Reasoning Guard Too [Codex P2] The previous commit taught `astraRulesApply` to prefer `servedModel` but left `getGatedReasoningParams` resolving `isReasoningModel` against `model`. Only the Azure delegates run that guard, and an Azure deployment alias matches no model pattern, so it returned before any Astra handling — the effort was dropped instead of substituted, and `servedModel` did not deliver what it promised on the one path it exists for. The existing tests could not catch this: they construct `ChatOpenAI`, which resolves reasoning directly, so the guard is Azure-only. Added an `AzureChatOpenAI` case with a deployment alias serving Astra; reverting the guard fails exactly it. [Codex P2] `firstPartyEndpoint`'s public JSDoc still claimed it gates Astra's "Responses-only tool calls", which stopped being true when API selection moved to the caller. It now describes shaping only and points at `useResponsesApi` for the API choice. * 🧪 test: Select the API Under Test and Keep the Constructor Contract [Codex P2] Removing the routing override left the effort tests binding tools to reach the Responses API, which no longer routes anything. Both branches of the effort helper were therefore reading the Chat Completions scalar, so the nested `reasoning.effort` shape the Responses delegates build was untested — coverage that quietly disappeared when the source changed under it. Each shape is now selected explicitly: `completionsEffort` and `responsesEffort` build their own client, the second with `useResponsesApi: true`. The Responses `include` case selects the API the same way instead of relying on tools. [Codex P1] The `astra` helper typed its overrides as `Record<string, unknown>`, against AGENTS.md's rule, which let a misspelled option compile and silently exercise a different constructor state. It now uses `ConstructorParameters<typeof ChatOpenAI>[0]`, so the tests are checked against the real constructor contract without exporting an internal type. * 🧹 test: Name the Probed Request Fields Completes the AGENTS.md sweep the previous commit only started. That commit typed the constructor overrides but left nine `as Record<string, unknown>` assertions on the `invocationParams` results, which is the same rule and the same weakness one step later: a renamed or mistyped field reads as `undefined` and the expectation quietly stops asserting anything. The probed fields are now a named type covering both APIs, so those become compile errors instead. * 🧹 test: Replace the last wrapped Record cast One `as Record<string, unknown>` survived the previous sweep because prettier had split it across three lines and the search matched the single-line form. Found by re-checking the branch diff rather than trusting the sweep. * 📄 docs: State the Injected-Delegate Boundary [Codex P2] A caller that supplies its own `completions`/`responses` delegate bypasses the request shaping, since that lives in this SDK's delegates. Documented rather than enforced, because neither proposed remedy holds up. Normalizing at the outer invocation boundary does not reach the request path: `_generate` delegates to `this.responses._generate`, which calls its own `invocationParams`, so the router's is never consulted for the actual request. Wrapping a supplied delegate would discard the behavior the caller injected it to get, which is the only reason the option exists. What is left is a contract, so it now says so on the public type: shaping runs in this SDK's own delegates, and a caller that replaces one owns request shaping for it. Nothing in this repository injects delegates, and the failure mode is the provider's own error rather than a silently wrong request. * ✂️ refactor: Drop the Azure Deployment-Alias Support Removes `servedModel` and the alias resolution it fed. It existed to recognize GPT-6 Astra behind an Azure deployment name, but OpenAI does not document Astra as available on Azure OpenAI — its model page lists the API and OpenAI's own subscription plans, and no third-party cloud surface. So this was support for a deployment the model is not documented to reach, and it was expensive: the alias resolution had to be threaded into every gate, and missing one of them produced a review finding of its own. Better to carry nothing than to carry a guess. The reasoning guard returns to resolving `model` directly. If Astra reaches Azure later, the declared-field pattern this branch establishes is the shape to reintroduce it in.
…zer (#507) `JsonPlusSerializer` up to 1.1.3 resolved an `lc:2` constructor record by matching the *trailing* segment of a payload-supplied `id`, then invoked a payload-supplied static `method` with payload-supplied `args`: constructor[revivedObj.method](...revivedObj.args || []) Checkpoint blobs are data, not instructions — and for any host running a durable checkpointer they are reachable by whoever can write to the store. 1.1.4/1.1.5 replace the name lookup with a validated allowlist (Set, Map, RegExp, Error, Uint8Array) and keep anything else inert. Reproduced against both versions with a record carrying `id: ['not','real','Uint8Array'], method: 'of'`: 1.1.3 INVOKED Uint8Array.of() from the payload -> [1,2,3] 1.1.5 inert, returned as plain data Legitimate values still round-trip on 1.1.5 (Set/Map/RegExp/Uint8Array/Error all reconstruct with values intact). `@langchain/langgraph` 1.4.8 already declares `^1.1.3`, so this is a lockfile lift with no change to the graph runtime — an `overrides` entry rather than a version bump, matching how the other transitive floors here are held. Consumers resolving the caret themselves were already getting 1.1.5; this closes the gap for our own installs and CI. `@langchain/langgraph-checkpoint-mongodb` moves to ^1.4.1, which is byte-identical to 1.4.0 apart from raising its peer to `^1.1.4` — upstream's own way of requiring the patched serializer. Deliberately NOT bumping `@langchain/langgraph` past 1.4.8. 1.4.9 changes the `Topic` checkpoint shape to a flat values list, and while restore reads the legacy `[seen, values]` form, the reverse does not hold: 1.4.8 throws on any checkpoint written by 1.4.9+, empty ones included. That makes the runtime bump a one-way door for persisted threads and it needs its own migration plan. Verified the format is untouched here — Topic still checkpoints `[[], [...]]`. Full suite matches the pre-change baseline exactly (5046 passed, same 4 credential/binary-dependent suites failing before and after); `tsc --noEmit` and `npm run build` clean.
`rerankerType: 'none'` is a schema-valid option, but it made web search answer with links and no text at all. The chain: `createReranker` returns `undefined` for 'none' by design, so `getHighlights` bailed out on `!reranker` and returned `undefined`, so the source reached `expandHighlights` with content but no highlights -- and that function strips raw content it cannot expand. Every scraped page was discarded on the way to the model. `getHighlights` now separates "nothing to rank" from "no reranker configured". Without a reranker the chunks pass through in their original order via `getDefaultRanking` -- the very ranking each reranker already falls back to when it fails, which is why `tool.ts` already logs "Using default ranking" for this case. `getDefaultRanking` moves out of `BaseReranker` into an exported function so the pipeline can use it without instantiating a reranker; the protected method stays and delegates, so no subclass changes. `highlights.ts` is deliberately untouched. "Raw scraped content must never leave this function" still holds for every other case: a source that truly has no highlights is still stripped, and even on the pass-through path the `content` field itself is still removed -- only highlights travel onward. Note the size of what passes through: `getDefaultRanking` keeps the first `topK` candidates, so a source contributes roughly `topResults * chunkSize` characters (5 x 150 by default), not the whole page. That matches the existing fallback behaviour and keeps the chunk budget that stops a search from flooding the context. A test pins this down so the limit is visible rather than surprising. Tests: 8 new in src/tools/search/no-reranker.test.ts; the full src/tools/search suite passes (250/250). Co-authored-by: Paul <200737214+SSIG-IT@users.noreply.github.com>
* 🪵 fix: Log Code API Auth Failures Before Sanitizing
Two unrelated conditions produce the identical model-facing string
`Code execution is not authorized. Verify access before trying again.`:
the dynamic auth-header callback throwing (no request is ever sent), and
the Code API answering 401/403. Neither recorded anything before
replacing the cause, so a recurring production failure could not be
attributed to either branch — the absence of upstream service logs was
equally consistent with both.
Log the cause before sanitizing, with distinct messages per branch. The
model-facing text is unchanged; the sanitization tests still assert the
secret name and namespace never reach the conversation. The status
mapper now also records method, endpoint, status and a bounded response
body for every rejection it maps, at warn for 429 so retries do not read
as failures. Payloads pass through `redactSecrets`, since a rejected
request's body can echo the credentials that were sent.
* 🔐 fix: Redact Credential Shapes Embedded in Diagnostic Text
`redactSecrets` only redacted values whose property NAME looked sensitive
plus credentials inside a parsed URL, so a credential sitting in prose
survived: the new diagnostics name their fields `message`, `stack` and
`body`, and a rejected request's body can echo the header that was sent.
Add `redactSecretText`, applied to every string the walker visits. It is
shape-based, not keyword-based — auth-scheme values (`Bearer <blob>`),
JWTs, embedded URL credentials, and `key|token|secret|...=<value>`
assignments. Prose that merely names a secret ("credential helper failed
for secret codeapi-signing-key") is the operator's main clue and is left
intact, which a test now pins.
The rejection body is scrubbed before truncation so a cut cannot strand a
partial credential that no longer matches a pattern.
* 🩹 fix: Make Free-Text Redaction Linear, Idempotent and Header-Aware
The keyword scan added in the previous commit had four defects, all of
them consequences of matching "a word containing `key`" rather than a
credential shape.
Backtracking: the unbounded `[A-Za-z0-9_-]*` runs on either side of the
keyword alternation overlapped it, so a body of repeated `akey` cost
3.9ms at 500 chars, 26.8ms at 1000 and 202ms at 2000 — and the rejection
body is scanned at full length before it is sliced. The alternation is
now literal on both sides and every quantifier owns a disjoint character
class: 200,000 chars scan in 0.63ms.
Coverage: `Digest` and `Cookie` carry credentials across a parameter list
rather than one opaque blob, so scheme-plus-blob matching left the nonce,
response digest and session value intact. Whole header lines are redacted
instead, which also covers `Set-Cookie` and `Proxy-Authorization`. The
embedded-URL pattern now accepts `user@host`, matching what
`redactUrlCredentials` already did for a string that starts with a URL.
Idempotency: the value class excluded `]` but not `[`, so a second pass
matched its own placeholder and `{"token":"[REDACTED]"}` degraded to
`{"token":[REDACTED]]}` — the operator's only copy, malformed. No
replacement now emits a character a value pattern can begin with.
Prose that merely names a secret is still left intact, and a test pins
each of the four properties.
* 🧷 fix: Redact Serialized Headers and Short Scheme Values
Three residual gaps in the free-text scrubber, all found by review.
The same credential headers reach us JSON-serialized, where the quote
before the colon defeats the whole-header-line rule and the assignment
rule stopped at the first delimiter — leaving a Digest response hash or
every cookie pair after the first. A credential-bearing key now redacts
its complete quoted value, escapes included, so `{"Authorization":
"Digest username=\"…\", response=\"…\""}` collapses to one placeholder.
An eight-character minimum on scheme values dropped short but valid
credentials (`Basic dTpw` encodes `u:p`). The minimum was standing in for
the real discriminator, which is that RFC scheme values are opaque while
"bearer token expired" is prose: the value must now carry a character
that is not a lowercase letter, and scheme matching is case-sensitive so
that requirement means what it says. Actual headers are matched
case-insensitively by the header-line rule regardless.
The header separator accepted `\s*`, so an empty `Authorization:` ate the
newline and the following `X-Request-Id` line with it. It is now
horizontal whitespace only.
* 🔻 fix: Log Only What This Module Produced, Never Upstream Text
Four review rounds against the free-text scrubber produced nine findings
and showed no sign of converging, which is the count being the signal.
The decisive one came from the host side: a malformed
`CODEAPI_JWT_PRIVATE_JWK_JSON` makes `JSON.parse` throw a Node 24
`SyntaxError` whose message quotes an excerpt of the source — private key
material, in a bare base64 fragment that no pattern can recognize. The
scrubber was never going to win that game.
So the surface is gone rather than patched. The rejection diagnostic
keeps method, endpoint and status and drops the response body; the auth
diagnostic keeps the error's type and its stack frames and drops the
message. Every value logged is now one this module produced or chose,
never upstream or host free text, which makes the leak class structurally
impossible instead of pattern-dependent — and the status alone still
distinguishes the two branches that share one model-facing string, which
is the whole point of the change.
`redactSecrets` is reverted to its previous form; it still runs over the
payload, since a configured base URL can embed credentials. Property
reads are guarded, so a rejection whose accessors throw yields a named
placeholder instead of losing both the log and the rejection.
* 🔒 fix: Narrow the Rejection Diagnostic to the Backend Authority
A configured base URL can carry a capability in its path, and the files
route puts a session id in one, so logging the full endpoint put host
text into the diagnostic. Only the authority is logged now, which is also
the field that answers what a rejection actually raises — which backend
refused this call, the stateless one or the stateful one.
`error.name` is writable, so a rejection can carry arbitrary text there.
It is kept only when it is shaped like the class identifier it is
supposed to be. `frames` is still read from the rejection, so a host that
forges a stack can put text in it; that is the same trust boundary as a
host that throws its credential in the first place, and the comment now
says so rather than claiming the log is secret-free by construction.
`fetchSessionFiles` is a best-effort lookup that reports its own outcome
and returns an empty list, so a 404 there was raising an error-level line
for a call that succeeds. It now opts out of the shared diagnostic
instead of logging the same failure twice.
* 🧱 fix: Classify the Auth Failure Locally Instead of Quoting It
A message containing a newline and ` at ` puts its own text through
any stack-frame filter — V8 begins a default stack with the message, so
this needs no `.stack` overwrite at all:
new Error('parse failed\n at MIIEvgIBAD').stack
-> frames[0] === ' at MIIEvgIBAD'
That falsifies the reasoning the previous commit used to keep `frames`.
`name` is no better: it is writable, and an opaque credential can be
identifier-shaped, so validating its syntax proved nothing about its
provenance.
Nothing is read off the rejection now. It is classified against the
built-in error types, and every value logged is a literal in this file or
one of the fixed strings `typeof` returns. A malformed signing key still
surfaces as `SyntaxError`, which is the lead the diagnostic exists to
give, and it is now secret-free by construction rather than by assertion.
Classification also cannot be trapped: `instanceof` walks the prototype
chain and never reaches an accessor.
`fetchSessionFiles` opts out of the resolver diagnostic as well as the
response one — its auth failures are already reported by its own warning,
and the same failure is logged at error by the exec call that follows.
* 🏷️ fix: Name the Backend by Profile and Log Before Draining
A non-OK response can leave a chunked body open, and there is no read
timeout on these requests, so awaiting `response.text()` first would
withhold the diagnostic indefinitely — for exactly the backend it exists
to identify. The status is known from the headers, so it is logged first;
the body is now read only in the 429 branch, which is the only place it
was ever used.
A hostname is host-configured text like the path before it: a capability
can sit in the authority (`https://<token>.gateway.example`) just as
easily as in a path segment. The rejection is labelled with the execution
profile instead — `default`, `stateful`, or `unset` — which is a value
this module owns, comes from a two-member union, and answers the question
a rejection actually raises better than an address does: which of the two
configured backends refused the call.
* 🧿 fix: Make a Leaking Diagnostic Fail to Compile
Every security finding on this branch has been one sentence: a diagnostic
carried a value the module did not own. Review enumerated the carriers
one at a time — response body, error message, stack, name, frames,
endpoint path, endpoint authority — and each fix removed one carrier
without preventing the next.
The rule is now enforced by a type. `CodeApiDiagnosticDetail` is a closed
union of values these tools produce: an HTTP status, a method drawn from
a two-member union, an execution profile, a built-in error class, a
`typeof` result. Host-authored text cannot reach a log without failing to
compile, and adding a field means widening that union in `diagnostics.ts`
— which is the point at which the question gets asked.
Narrowing `buildCodeApiHttpErrorMessage`'s `method` immediately proved
the guard's worth: it accepted an arbitrary `string` that flowed straight
into a log.
Three pre-existing diagnostics were in the same class and are converted:
the `session_id` debug lines in all three tools, and the session file
lookup's warning, which quoted `error.message` verbatim. That last one
matters beyond its own leak — suppressing this branch's resolver
diagnostic for the recoverable path was justified by that warning
reporting the outcome, so the justification rested on a leaking line.
Hosts correlate these lines with their own request-scoped logs, which is
where a received identifier such as a session id belongs.
* 🔌 fix: Convert the Fourth Executor and Free Discarded Bodies
`BashProgrammaticToolCalling` interpolates a received `session_id` into a
debug line, the same carrier converted in the other three executors. The
previous commit said "all three tools" and there are four; adding the
source to the union and routing that site completes what it set out to do.
Logging before draining left non-429 bodies neither read nor cancelled,
so node-fetch held the stream and its socket open for payloads this
module deliberately discards — a leak introduced by the previous fix, not
by the original code. Non-429 bodies are destroyed after the diagnostic;
429 still reads its body for the retry delay.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )