Skip to content

feat(usage): extend opt-in cache continuation diagnostics - #7479

Draft
yuanyuanlove wants to merge 6 commits into
lidge-jun:devfrom
yuanyuanlove:feature-20261021-cache-diagnostics
Draft

yuanyuanlove wants to merge 6 commits into
lidge-jun:devfrom
yuanyuanlove:feature-20261021-cache-diagnostics

Conversation

@yuanyuanlove

@yuanyuanlove yuanyuanlove commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Extend the existing default-off OPENCODEX_CACHE_DEBUG diagnostics with bounded continuation/prefix comparisons, independent session-header observations, and explicitly derived (not proven) affinity-key comparisons. No routing, cache-behavior or performance change is claimed.
  • Address the caller-isolation P2 in the source review: diagnostics now receive the existing authenticated admission principal through the private request-log hook. They no longer derive caller identity from Authorization headers that Chat/Messages bridges may strip or replace.
  • Different admitted keys remain isolated; one key remains the same principal across its supported authentication headers. No authentication decision, accepted-header policy or upstream credential handling changes. The principal is re-tagged into a private WeakMap, never added to a request header, body, log context or diagnostic record.
  • Without a trusted admission principal, including unauthenticated loopback, cross-request comparisons are uncomparable; per-request inbound/outbound comparisons still work. Raw credentials are never used as diagnostic history keys.
  • Preserve the original PR history while incorporating current dev at ceef4889e4666dc0492a375d272eafa5b785361e. The resulting PR diff is limited to the diagnostics enhancement and this identity fix (11 files); no cache-evidence changes from fix(usage): preserve source-aware cache evidence across attempts #7485 are included.

Verification

Validated head: bffa47bed782a5ca7454a3301f776e68cae61e82 (tree 27a118e083171b916aa108706abcf5a7fee22599). All work ran in an isolated Linux cloud workspace. Tests use synthetic callers and provider responses; the focused integration fixtures assert zero external network calls.

  • RED/GREEN reproduction against the exact original head aa3e7fdb0620cec0fa1b72161b005719a0acc781: the new 17-case integration suite had 2 passing authentication-refusal controls and 15 failing history-isolation regressions. All 17 pass after the fix, including after incorporating current dev.
  • Final focused tests run individually with the pinned Bun 1.4.3 test runner: tests/usage/cache-diagnostic.test.ts (34), cache-diagnostic-caller-isolation.test.ts (17), cache-diagnostic-hook-compat.test.ts (10), and tests/responses/protocol-antigravity-direct-eligibility.test.ts (19): 80 passed.
  • Integration coverage uses real admission resolvers, Responses/Chat/Messages handlers and bridges, and synthetic native Responses transport. It covers accepted header aliases, distinct callers, forged identity headers, stripped/replaced bridge Authorization, missing principals, unchanged authentication refusals, and absence of keys/principals/tags from persisted records and forwarded requests.
  • bun run typecheck, bun run privacy:scan, bun run structure:check, and git diff --check passed. Independent read-only review found no remaining actionable issue.
  • bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts tests/lab/core-lab-boundary.test.ts: 61 passed, no caps raised.
  • Backend bundling and native CLI compilation with Bun 1.4.4 succeeded. The compiled executable's isolated --version smoke returned opencodex 2.93.0. This is not a GUI/release-package build.
  • Offline docs build passed with ASTRO_TELEMETRY_DISABLED=1 OPENCODEX_DOCS_OFFLINE=1 bun run build: 593 pages and 84,861 internal links checked.

Remaining coverage: bun run test:changed was blocked by the validation environment's network policy when it attempted an external Google Cloud endpoint; it was not retried or treated as a passing suite. Full-suite, packaged GUI and native macOS/Windows validation were not run. This uses the repository's documented focused-validation scope exception. Keep this Draft pending exact-head CI approval/execution and maintainer privacy/diagnostic-contract review.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
  • Current-head CI and maintainer acceptance completed.

Review readiness checklist

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

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

⏳ 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

  • ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ 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.

Hygiene

✅ Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

Independent source review (gpt-6.1-sol, dispatched by the triage manager, 2026-10-24) at head aa3e7fdb06:

VERDICT: MUST-FIX
LANE_ALL_REQUIRED: no

Reviewed head: aa3e7fdb0620cec0fa1b72161b005719a0acc781.

  • [P2] Caller isolation uses the wrong identity — src/usage/cache-diagnostic.ts:272. codexLineageScopeKey reads only Authorization. Distinct callers authenticated through x-opencodex-api-key or x-api-key (src/server/auth-cors.ts:622–628) therefore share its local scope. Chat/Messages bridges also strip or replace caller Authorization (chat-completions.ts:427, claude-messages.ts:1046). With identical session/model/protocol, unrelated callers consequently share continuation history, producing false comparisons across principals. Fix: capture an opaque authenticated admission identity before bridging and carry it privately into diagnostics. Add regressions for both alternative authentication headers and bridged callers; the direct-helper test at tests/usage/cache-diagnostic.test.ts:373 misses these cases.

Readiness: exact-head CI is action_required; no submitted reviews or unresolved review threads. The branch is 72 commits behind live dev; GitHub reports MERGEABLE. Maintainer privacy/diagnostic-contract review remains outstanding.

Static inspection: template complete; test registration present in both maps; layout 1,999 lines, structure doc 600; no changed-file ratchet violation. No introduced Windows-specific sensitivity warrants mandatory lane=all.

NOT RUN: tests, typecheck, builds, or validation gates, as requested.

This is a read-only source review; merging still requires the gates named above (exact-head CI, lane=all where marked, and maintainer acceptance where required).

yuanyuanlove and others added 2 commits October 24, 2026 11:35
Carry the existing admission principal through the private diagnostic hook instead of deriving caller scope from rewritten Authorization headers. Preserve the original PR history and incorporate current dev for validation.
# Conflicts:
#	src/server/responses/request-prepare.ts
#	structure/transports/responses.md
#	tests/fixtures/test-layout-expected.json
@lidge-jun

Copy link
Copy Markdown
Owner

Refreshed onto current dev as one ordinary merge commit, ab3ffd058, on top of your bffa47bed (your commits unchanged). The three conflicts were additive: request-prepare.ts keeps both your contextPrincipalIdOf import and dev's turn-membership import, test-layout-expected.json keeps dev's new entries plus your two test mappings, and your diagnostics sentence in structure/transports/responses.md is appended to dev's paragraph so the doc stays within its line budget.

bffa47bed addresses the 2026-10-24 review finding: diagnostics now scope continuation history by the admitted principal (contextPrincipalIdOf(admission)) carried out of band, and cache-diagnostic-caller-isolation.test.ts covers authorization, x-opencodex-api-key and x-api-key across Responses, Chat and Messages, plus forged headers, loopback admission and a rewritten stored-main Authorization.

Local, Bun 1.4.4: bun run typecheck (2,227 files), bun run structure:check and bun run privacy:scan pass; cache-diagnostic, cache-diagnostic-caller-isolation, cache-diagnostic-hook-compat, both test-layout tests and the file-size ratchet: 99 pass, 0 fail. I approved the PR CI for this head after checking the diff touches no workflow, dependency or release file. An independent security review of this head is in progress; the maintainer's privacy/diagnostic-contract review remains a separate gate.

lidge-jun and others added 2 commits October 28, 2026 18:16
…ed off

Each on/off transition of OPENCODEX_CACHE_DEBUG starts a new retention epoch: continuation history and pending drafts are cleared, and a draft stamped in an earlier epoch is neither merged, compared nor written.

Co-authored-by: yuanyuanlove <yuanyuanlove@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Follow-up on the refresh above. An independent security review of ab3ffd058 confirmed the caller-isolation fix (admission identity survives header aliases and Chat/Messages rewrites; forged headers and keyless loopback cannot establish a continuation scope; the scope stays an opaque in-memory tag) and found one retention issue: turning OPENCODEX_CACHE_DEBUG off kept the continuation history, so after 1 → 0 → 1 the first request compared equal against pre-disable history and a pre-disable draft could still be written.

  • e0144b635 (my commit): each observed on/off transition starts a new retention epoch. History and pending drafts are cleared, drafts are stamped with their epoch, and a stale draft is never merged, aliased, observed outbound or written. The regression in tests/usage/cache-diagnostic.test.ts fails on ab3ffd058 and passes now; the three cache-diagnostic files plus the file-size ratchet are 71 pass / 0 fail, and typecheck, structure:check and privacy:scan pass.
  • 4dd7878b5: the structure doc now says observed transitions, since the gate is evaluated on each diagnostic call.

A second security review round passed with no blocking findings. I approved the PR CI for the new head. The maintainer's privacy/diagnostic-contract decision is still the remaining gate.

@lidge-jun

Copy link
Copy Markdown
Owner

Owner decision: accepted — the extended, default-off continuation diagnostics contract is approved: active only with OPENCODEX_CACHE_DEBUG=1, a single env comparison per request when off, bounded in-memory tags only (no raw content or credentials, at most 512 drafts), history keyed by the admitted principal and cleared on every on/off transition. Two independent security reviews passed. Remaining before merge: green exact-head CI and the readiness boxes; it will be merged in the batch after v2.94.0.

Refresh lidge-jun#7479 onto dev 9c33e46 (ordinary merge, no conflicts) so the branch is within the readiness window.

Co-authored-by: yuanyuanlove <yuanyuanlove@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants