fix(vision,telemetry): sync captions into the passthrough body and persist ordinary attempts - #1139
Conversation
Source PR: #1047 Co-authored-by: baileyh8 <baileyh8@gmail.com>
Source PR: #1093 Ingress-span persistence is intentionally excluded because the public admitted endpoint cannot establish trusted producer provenance for a client-supplied header. Co-authored-by: Takashi Yamashiro <44048851+irdtechbook@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90fa982321
ℹ️ 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".
| const attempt = beginRequestAttempt( | ||
| (logCtx.attempts?.length ?? 0) + 1, | ||
| logCtx.provider, | ||
| route.modelId, |
There was a problem hiding this comment.
For OpenAI virtual models such as gpt-5.6-sol-pro, applyOpenAiVirtualModel has already rewritten route.modelId to the wire model (gpt-5.6-sol) while retaining the selected model in logCtx.model. Persisting the rewritten value here changes usage reporting because usageAttributions treats any non-empty attempts array as authoritative and ignores the top-level model/resolvedModel pair. Consequently, all newly logged ordinary Pro requests are grouped and priced as the base model instead of the selected Pro model. Preserve the selected model in the attempt (or carry selected/resolved identities separately and update the usage projection).
Useful? React with 👍 / 👎.
| ); | ||
| logCtx.activeAttempt = attempt; | ||
| logCtx.activeAttemptStartedAt = Date.now(); | ||
| (logCtx.attempts ??= []).push(attempt); |
There was a problem hiding this comment.
Once every ordinary request gets this non-empty attempts array, an ordinary request whose usage is absent or whose model has no matched price is misclassified by unavailableCostReason: that function treats any non-empty attempts list as a combo and returns combo_attempt_unavailable before reaching usage_missing or price_unmatched. The Logs dashboard therefore tells users that a combo attempt could not be priced even when no combo was used. Update the management-side classification to distinguish ordinary attempts from combo attempts, or persist that distinction with the attempt data.
Useful? React with 👍 / 👎.
2 participants
Summary
Two independent fixes: the vision sidecar's captions now actually reach the forwarded payload, and ordinary request attempts are persisted to the usage log.
Vision raw-body sync.
describeImagesInPlaceandstripImagesInPlacerewroteparsed.context.messagesbut never touchedparsed._rawBody. The native Responses passthrough serializes_rawBody, so on that path the original pixels were still sent to a text-only upstream after the sidecar produced a caption. The two are now synchronized.Fail-closed on the failure paths. The version in #1047 returned early when there were no captions, and returned the original
input_imagepart when a caption was missing for an image — so a failed or partial caption pass still leaked raw pixels, which is precisely the case that matters. Both now substitute the same[image omitted: ...]textstripImagesInPlaceuses. New tests assert that noinput_imagesurvives in_rawBodyon the zero-caption and fewer-captions-than-images paths.Usage attempts. Ordinary request attempts are created after final adapter resolution using the existing attempt owner, and explicit empty arrays now survive normalization — so "no attempts" and "not recorded" stay distinguishable.
Attribution
Co-authored-by.What I withheld from #1093, and why: its ingress-span half reads a client-supplied correlation header at the public admitted endpoint (
src/server/index.ts), which lets any admitted client forge a regex-shaped "guard-issued" span. Persisted telemetry an untrusted caller can shape is worse than absent telemetry because it looks authoritative.src/server/index.tsandtests/server-auth.test.tshave zero diff in this PR, verified. That half is worth landing once there is a trusted producer boundary — the attempt-recording half needed no such gate, so it goes now.#1047 and #1093 are left open for their authors. Planning unit:
devlog/_plan/260806_stacked_bug_campaign/(phases 080, 130).Stack 7 of the 260806 attribution campaign, stacked on #1138.
Verification
bun test tests/vision-sidecar-e2e.test.ts tests/request-log.test.ts tests/usage-log.test.ts— 85 pass, 0 fail (re-run after rebase onto stack 6)bun run typecheck— exit 0bun run privacy:scan— passedChecklist