Conversation
|
Reviewed the three minor notes from internal review; they are valid observations, with one clarity-only follow-up:
Follow-up commit: |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the persistent fallback poisoning; the main premise is confirmed on current main.
Problems
agent/model_metadata.py:1856-1876keeps one process-global Codex catalogue cache, not keyed by credential. The new persistence gate atagent/model_metadata.py:2329labels an in-process cache hit as"live", so a later credential can persist the first account's entitlement-specific value. Please key that cache by credential (without retaining raw credentials) or distinguish a fresh probe from an in-process hit, with a two-token regression test.cli.py:12179-12181still invokes the resolver withoutprovider=self.provider. Because the PR's disk-cache bypass is provider-gated before provider inference (agent/model_metadata.py:2109), this @-reference path can still return a stale Codex row. Please propagate the provider and cover that path.
Suggested changes
- Preserve the disk-cache fix, but make provenance account-safe before allowing persistence.
- Add the missing CLI provider propagation and regression coverage.
Automated hermes-sweeper review.
| # persist. The static fallback is deliberately runtime-only so a | ||
| # transient OAuth/network failure cannot poison future probes. | ||
| if base_url and codex_source == "live": | ||
| save_context_length(model, base_url, codex_ctx) |
There was a problem hiding this comment.
codex_source == "live" also covers _fetch_codex_oauth_context_lengths()'s process-global one-hour cache, which is not keyed by credential (agent/model_metadata.py:1856-1876). Since this PR treats the catalogue as entitlement-specific, a second account can persist the first account's cached value here. Please credential-scope the cache or distinguish a fresh probe from an in-process cache hit before writing to disk.
|
Addressed Teknium's review findings in
Validation:
|
NoxsMedia
left a comment
There was a problem hiding this comment.
Validated the latest PR head merged cleanly onto current origin/main@8a5f8379e.
What I checked:
- The credential-fingerprinted in-process catalogue cache prevents one OAuth account's entitlement metadata from being reused as fresh evidence for another account.
- The
live/memory/fallbackprovenance split ensures only a fresh authenticated/modelsresponse can update persistent metadata. - The CLI
@-context path now propagatesprovider=self.provider, so Codex cannot hit the generic disk-cache path there. scripts/run_tests.sh tests/agent/test_model_metadata.py tests/cli/test_cli_codex_context_reference.py tests/hermes_cli/test_model_switch_context_display.py tests/hermes_cli/test_apply_model_switch_result_context.py tests/hermes_cli/test_context_switch_guard.py tests/hermes_cli/test_gpt56_registration.py -qpassed: 148 tests.py_compile, Ruff, andgit diff --checkpassed on the merged review tree.- Live regression probe against the current Codex catalogue: seeded a temporary
gpt-5.6-soldisk entry at372000; the PR bypassed it, resolved272000from authenticated/models, and persisted272000.
No blocking findings.
Non-blocking test suggestion: parameterize the stale-cache reconciliation regression in both directions (272000 -> 372000 and 372000 -> 272000). The implementation already handles both; the reverse case now reflects the current provider rollback and would guard future product-limit changes explicitly.
|
Thanks @NoxsMedia for the thorough review. I've added test coverage for Codex context changes in both directions (272k → 372k and 372k → 272k), including the recent rollback case. |
|
Merged via PR #68554. Your four commits were cherry-picked onto current main with your authorship preserved in git log — thanks for a well-built fix (the source-tracked persistence and per-credential catalog cache were exactly right). We added one small follow-up on top so the autoraise notice reports the live-resolved window instead of a hardcoded 272K. |
Bug Description
get_model_context_length()could return a stale value from~/.hermes/context_length_cache.yamlbefore reaching the authenticated Codex/backend-api/codex/modelsresolver. A fallback such as272000could thereforebe persisted after a transient probe failure and continue to mask a later live
allocation (for example,
372000) across restarts,/model, and model-pickerchanges for the same model/base URL.
Fixes #
Root Cause
The generic persistent context cache was checked before the provider-specific
Codex OAuth resolver. Codex fallback values and live catalogue values were both
persisted without source information, so a cache hit prevented revalidation.
Fix
openai-codex, while retainingthe existing in-process Codex catalogue cache.
or the static fallback table.
/modelsresponse; fallback valuesremain runtime-only and cannot poison future probes.
Codex rows are bypassed and replaced on the next successful live resolution.
How to Verify
gpt-5.6-terra@https://chatgpt.com/backend-api/codex: 272000./modelsresponse containingcontext_window: 372000.372000, and replacesthe cache entry with
372000.back to
272000without writing that fallback to disk.Test Plan
Validation:
scripts/run_tests.sh tests/agent/test_model_metadata.py tests/hermes_cli/test_model_switch_context_display.py tests/hermes_cli/test_apply_model_switch_result_context.py tests/hermes_cli/test_context_switch_guard.py tests/hermes_cli/test_gpt56_registration.py -qgit diff --checkand Python compilation passed.Risk Assessment
Low — the change is scoped to the
openai-codexprovider. Explicit contextconfiguration remains higher priority, and other providers retain their
existing persistent-cache behavior. When the Codex endpoint is unavailable,
the conservative static fallback is still used for the current resolution but
is no longer persisted as authoritative metadata.