fix(telemetry): persist ingress spans and ordinary request attempts

[yamashirotakashi]

Summary

• Capture only guard-issued x-opencodex-ingress-span values on ordinary Responses requests and carry them through request and usage logs.
• Create exactly one durable attempt after final initial adapter resolution for ordinary non-combo requests, preserving combo and retry ownership.
• Preserve explicit attempts: [] while omitting malformed non-array values; reject malformed or secret-shaped ingress values.

Scope

• Base: 99440ecd91a951e5b580fdde843998d99181400a
• Files: four source files and three focused test files
• No provider fallback, credential forwarding, retry behavior, or public API contract changes.

Validation

• bun test tests/request-log.test.ts tests/usage-log.test.ts tests/server-auth.test.ts
• Result: 147 passed, 0 failed, 802 expectations
• git apply --check against a clean pinned base: passed

Security

Ingress values are accepted only in the positive issuer shape. Representative gho_, github_pat_, Bearer, and sk- values are rejected. Logs persist correlation metadata only, never raw content or credentials.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I fixed all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• New Features

  • Added request correlation tracking for valid ingress spans across request and usage logs.
  • Added durable attempt tracking for standard requests.
  • Preserved explicitly empty attempt metadata in persisted logs.

• Bug Fixes

  • Rejects malformed or sensitive-looking correlation values.
  • Prevents duplicate upstream requests in passthrough streaming scenarios.
  • Ensures invalid persisted correlation data is omitted.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

📝 Walkthrough

Walkthrough

The request pipeline now validates and propagates ingress spans through request logs and usage records. Ordinary Responses requests create durable attempt metadata. Persistence retains explicitly defined empty attempt arrays.

Changes

Ingress correlation and request attempts

Layer / File(s)	Summary
Metadata validation and persistence src/server/request-log.ts, src/usage/log.ts, tests/request-log.test.ts, tests/usage-log.test.ts	ingressSpanFromHeader trims and validates correlation tokens at src/server/request-log.ts:39-52. Request-log hydration and final persistence preserve validated spans and defined attempts arrays at lines 242-244, 271, 337, 366, 825, and 853. Usage normalization applies the same span validation and preserves empty attempts at src/usage/log.ts:100-107, 343-345, and 401. Tests cover valid, malformed, and secret-shaped values.
Request wiring and durable attempts src/server/index.ts, src/server/responses/core.ts, tests/server-auth.test.ts	The Responses handler extracts x-opencodex-ingress-span into request context at src/server/index.ts:845-851. Ordinary requests create a durable attempt after adapter resolution at src/server/responses/core.ts:1632-1644. SSE tests verify the span, first-attempt metadata, and one upstream request.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• lidge-jun/opencodex#494: Both changes extend persisted request-log metadata in request-log.ts and usage/log.ts.
• lidge-jun/opencodex#1008: Both changes modify usage-entry normalization and persisted usage handling.

Suggested reviewers: lidge-jun, wibias

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the two main changes: persisting ingress spans and creating ordinary request attempts.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0039ce969

ℹ️ 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".

[chatgpt-codex-connector]

Capture effort after constructing the ordinary attempt

For ordinary Cursor requests, applyRouteDependentNormalization calls recordAttemptRequestedEffort before this block assigns logCtx.activeAttempt. Cursor uses runTurn, so it never reaches a later recordAdapterReasoning call that would backfill the field, even though its request builder uses the selected effort to choose the wire model. Record the settled effort immediately after assigning the new attempt so persisted Cursor attempt telemetry includes requestedEffort.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Distinguish ordinary attempts from combo attempts

Once every ordinary request receives this non-empty attempts array, existing management consumers classify it as a combo solely from entry.attempts?.length; for example, src/server/management/shared.ts:112-115 now reports combo_attempt_unavailable for an unpriced ordinary custom-provider request. Update those consumers to detect an actual combo (such as by the top-level provider/metadata) before using combo-only diagnostics.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Create attempts only when an upstream dispatch starts

When a request resolves the Kiro adapter but carries an unexpanded previous_response_id, the validation immediately below returns 400 without sending upstream; because this block has already pushed an attempt, the deferred finalizer persists a status-400 attempt with sendCount: 0. Later local-only rejections such as the image-bridge stream=true check have the same problem, so request history reports physical attempts that never occurred. Delay attempt creation until the selected dispatch path starts, or omit attempts whose send count remains zero.

Useful? React with 👍 / 👎.

[github-actions]

⏳ DRAFT

• review readiness checklist open (0/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

• ⬜ All CI tests are green on my local testing.
• ⬜ I pushed my PR to the latest dev commit.
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

[lidge-jun]

Retargeted this to dev for you, so the wrong-branch gate is cleared and you do not need to reopen anything. The [WRONG BRANCH] title prefix was added by the bot — feel free to drop it now.

The content is worth landing: ingress-span propagation and durable per-attempt records do not exist on dev today, and nothing merged since has covered them, so this is not duplicated work.

Two things still stand between this and review:

1. Rebase onto current dev. Your branch is based on 99440ecd9, and dev has moved a long way since — including the v2.10.2 release merge. src/server/responses/core.ts and src/usage/log.ts in particular have changed underneath you, so please rebase and re-run the suite rather than merging dev in.
2. The readiness checklist in the description is at 0/4; the PR stays in draft until all four are ticked. The gate verifies two of those claims itself — the head's ci check must be green, and the branch must be on the latest dev commit or at most 10 behind — so do the rebase first and tick afterwards, otherwise the boxes get unticked automatically.

One design question to answer in the description while you are in there: what happens to an ingress span that is well-formed but arrives on a request that never reaches adapter resolution? Your summary says exactly one attempt is created "after final initial adapter resolution," which reads correct, but the failure path before that point is where telemetry work usually leaks or double-counts, and a reviewer will look there first.

bun run typecheck and bun run test before you tick — CI runs them on Linux, Windows, and macOS.

[lidge-jun]

Thanks for retargeting — the branch is on dev now and the wrong-base gate is clear. Reviewing the substance.

Neither behavior exists on dev today: src/server/request-log.ts:219-256 persists no ingress-span field, and src/server/responses/core.ts:1631-1641 creates no attempt record for ordinary requests. So this is additive, nothing merged since has covered it, and the correlation data is genuinely useful. The serialization tests are real — the round-trip, the malformed-input omission, and the explicit-empty-array preservation each pin distinct behavior.

The blocker is a missing contract, not a missing test.

The description says the change captures "only guard-issued" ingress spans. The code accepts any client-supplied header value matching a shape:

const INGRESS_SPAN_RE = /^[A-Za-z0-9_-]{24}_[0-9a-f]{16}$/;
return INGRESS_SPAN_RE.test(span) ? span : undefined;

A regex is a format check, not provenance. Nothing issues these values, nothing signs them, and nothing prevents a caller from sending any conforming string — including one chosen to collide with another caller's span. Once persisted into usage records and /api/logs, that data looks authoritative to anyone reading it later. Whether that is acceptable depends entirely on what the field is for, and no linked issue or reproduction says.

Three things needed before this can be reviewed on the merits:

1. Who issues the span, and why is a client-supplied value trustworthy enough to persist? If a component of ours mints it, name that component and say how a request that did not come through it is treated. If it is genuinely caller-supplied correlation data, say so in the description and drop "guard-issued" — that framing is doing work the code does not support.
2. What happens before adapter resolution? The attempt record is created after final adapter resolution, so requests failing at auth or routing produce a span with no attempt. Is that intended, or is the pre-resolution path supposed to record something? This is where telemetry work usually leaks or double-counts, and a reviewer will look there first.
3. Correct the contract claim. The description says there are no public API contract changes, but the diff adds fields to persisted usage records and to /api/logs responses. Both are consumed by the dashboard and by anyone parsing the log format, so this is a contract change and should be described as one.

Also still worth doing: rebase onto current dev. The branch is well behind, and src/server/responses/core.ts and src/usage/log.ts have both moved. Please rebase rather than merging dev in.

For clarity — the bug label is on this PR but I could not find the defect it fixes. If this is a capability rather than a bug fix, saying that plainly in the description would help; there is nothing wrong with proposing it as an enhancement.

None of the above is about code quality. The implementation is careful. It just needs someone to state what the data means before we start writing it down permanently.

[lidge-jun]

Thank you — the attempt-persistence half of this is landing as #1139, credited to you via Co-authored-by.

Adopted: creating the ordinary request attempt after final adapter resolution using the existing attempt owner, and preserving explicit empty arrays through normalization so "no attempts" and "not recorded" stay distinguishable. That distinction is genuinely useful and the usage log was poorer without it.

What I withheld, and why. The ingress-span half reads a client-supplied correlation header at the public admitted endpoint (src/server/index.ts), which means any admitted client can forge a regex-shaped "guard-issued" ingress span. Persisted telemetry that an untrusted caller can shape is worse than absent telemetry, because it reads as authoritative to whoever looks at it later. src/server/index.ts and tests/server-auth.test.ts therefore have zero diff in the stack PR.

This is not a rejection of the idea. Ingress spans are worth having — they need a trusted producer boundary first, where the span is minted by something the client cannot influence. If you want to pursue that shape, I would review it.

Verified: 85 pass / 0 fail on the affected suites; full suite 9,478 pass / 0 fail; typecheck and privacy scan clean.

Leaving this PR open for you.