feat(temporal): anti-drift injection for all provider requests#2574
feat(temporal): anti-drift injection for all provider requests#2574dwillitzer wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces model health monitoring, OpenRouter metadata enrichment, and a temporal injection feature that adds current date and time context to LLM requests. It also expands model metadata in API responses and updates routing logic for Claude-related User-Agents. Feedback focuses on thread-safety issues in the new temporal package, specifically recommending the use of atomic operations for the global request counter to prevent race conditions.
| } | ||
|
|
||
| // requestCounter tracks how many requests have been seen for interval-based injection. | ||
| var requestCounter uint64 |
There was a problem hiding this comment.
The global variable requestCounter is not thread-safe. In a concurrent environment like a web server, this will lead to race conditions when multiple requests are processed simultaneously. Consider using sync/atomic to increment the counter safely.
| if cfg.InjectInterval <= 0 { | ||
| return true | ||
| } | ||
| requestCounter++ |
There was a problem hiding this comment.
The increment operation on requestCounter is not atomic. Use atomic.AddUint64(&requestCounter, 1) to ensure thread safety.
| requestCounter++ | |
| atomic.AddUint64(&requestCounter, 1) |
| return true | ||
| } | ||
| requestCounter++ | ||
| return requestCounter%uint64(cfg.InjectInterval) == 0 |
There was a problem hiding this comment.
The read operation on requestCounter is not atomic. Use atomic.LoadUint64(&requestCounter) to ensure thread safety.
| return requestCounter%uint64(cfg.InjectInterval) == 0 | |
| return atomic.LoadUint64(&requestCounter)%uint64(cfg.InjectInterval) == 0 |
![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: 71a6b44de3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| system.ForEach(func(_, v gjson.Result) bool { | ||
| newSystem = append(newSystem, v.Value()) |
There was a problem hiding this comment.
This rewrite path is entered whenever a top-level system key exists, but injectIntoClaudeSystem only preserves existing entries by iterating system.ForEach and never normalizes scalar system values. The repo still supports string system prompts elsewhere (e.g. Claude executor handles system strings), so payloads like {"system":"...","messages":[...]} can have their original system instruction dropped or reshaped when temporal injection runs, changing model behavior.
Useful? React with 👍 / 👎.
| var newMessages []any | ||
| newMessages = append(newMessages, temporalMsg) |
There was a problem hiding this comment.
Prepending the temporal message at messages[0] breaks the Imagen compatibility path that derives prompt text from the first non-empty messages.#.content entry (internal/runtime/executor/gemini_vertex_executor.go loop at lines 125-130). After this change, messages-format Imagen requests can use the injected temporal block as the prompt instead of the user’s prompt, producing incorrect image generations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115cb5f0ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // OpenAI format: messages array exists | ||
| if messages := gjson.GetBytes(payload, "messages"); messages.IsArray() { | ||
| return injectIntoMessages(payload, messages, text) | ||
| } | ||
|
|
||
| return payload |
There was a problem hiding this comment.
InjectIntoPayload only handles top-level system and messages, then returns the payload unchanged for other schemas. Native Gemini/Vertex requests use contents payloads, so this path skips temporal injection entirely and anti-drift is not applied on those provider routes, despite this change advertising provider-wide request injection.
Useful? React with 👍 / 👎.
| globalRegistry := registry.GetGlobalRegistry() | ||
|
|
||
| // Get all available models with full metadata | ||
| models := globalRegistry.GetAvailableModels("openai") |
There was a problem hiding this comment.
GetModelsHealth starts from GetAvailableModels("openai"), which excludes models whose clients are fully suspended/unavailable. That causes /models/health to omit exactly the models operators need to debug during outages, contradicting the handler’s stated goal of reporting all registered models.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ed89fcbd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Inject temporal context into the request payload | ||
| if temporal.ShouldInject(temporal.DefaultConfig()) { | ||
| req.Payload = temporal.InjectIntoPayload(req.Payload, req.Model) |
There was a problem hiding this comment.
This injects temporal text based on req.Model before alias/model-pool resolution, so the image-model safeguard can be bypassed when users call an aliased route model (for example, img) that later resolves to an Imagen upstream model. In that case InjectIntoPayload sees a non-image alias and prepends a system message, then executeMixedOnce rewrites to the Imagen model (execReq.Model = upstreamModel), and Imagen prompt extraction can consume the injected block instead of the user prompt. Fresh evidence: alias remapping happens only after this line, so the prior Imagen fix does not apply to aliased image routes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb9735bfa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| enhancedModels = append(enhancedModels, enhancedModel) | ||
| } | ||
| // Build the sources map indicating where context_length came from | ||
| sources := registry.BuildModelSources(globalRegistry) |
There was a problem hiding this comment.
This change introduces calls to registry.BuildModelSources, registry.GetOpenRouterLastRefresh, registry.TriggerOpenRouterRefresh, and registry.StartOpenRouterEnrichment, but no corresponding definitions are added in the internal/registry package. That leaves these references unresolved at build time, so server/management packages cannot compile until the symbols are implemented (or the calls are removed).
Useful? React with 👍 / 👎.
| // OpenAI format: messages array exists | ||
| if messages := gjson.GetBytes(payload, "messages"); messages.IsArray() { | ||
| return injectIntoMessages(payload, messages, text) |
There was a problem hiding this comment.
InjectIntoPayload only injects when a top-level system or messages field is present, and otherwise returns the body unchanged. Requests sent through /v1/responses use the Responses schema (input/instructions) and are forwarded as-is, so temporal anti-drift is skipped for that endpoint family despite this feature being applied in the central conductor path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8ed002894
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if info := registry.GetModelInfo(modelID, ""); info != nil { | ||
| info.ContextLength = ctxLen | ||
| enrichedCount++ |
There was a problem hiding this comment.
enrichModelsFromOpenRouter calls GetModelInfo and assigns info.ContextLength, but GetModelInfo returns a cloned ModelInfo, so this write is discarded and never updates r.models. That makes enrichment effectively a no-op even though enrichedCount is incremented and logs say models were enriched, leaving downstream model metadata unchanged.
Useful? React with 👍 / 👎.
| lastRefresh := registry.GetOpenRouterLastRefresh() | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "status": "refreshed", | ||
| "enriched_count": count, |
There was a problem hiding this comment.
RefreshModels publishes enriched_count from TriggerOpenRouterRefresh, but that function returns the size of the OpenRouter cache (len(contextLength)), not how many registered models were enriched in this refresh. This can report misleadingly large counts (or wrong error semantics when cache is empty due fetch failure), so operators cannot trust the endpoint output.
Useful? React with 👍 / 👎.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
I found two blocking issues in this change set.
Key findings:
- Blocking: the temporal injector is enabled unconditionally via
temporal.DefaultConfig()in the execution path, so every proxied request now gets a hidden system prompt with no user opt-in and no configuration escape hatch. That changes request semantics for all providers and breaks API compatibility rather than preserving it. - Blocking:
internal/registry/openrouter_enrichment.goadds a newhttp.Client{Timeout: 30 * time.Second}/context.WithTimeoutfetch path. This repo explicitly restricts new timeout-driven network behavior to a small set of documented exceptions, and this enrichment path is not one of them.
Test plan:
- Reviewed the temporal injection flow and the new registry enrichment loop plus management endpoints.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
I like the goal, but this PR currently mixes the temporal injection feature with unrelated registry/management work and introduces a couple of blockers.
Blocking findings:
sdk/cliproxy/auth/conductor.gohard-codestemporal.DefaultConfig()at the injection call sites instead of wiring the feature to runtime config, which makes this always-on request mutation impossible to disable or roll back if it causes provider regressions.internal/registry/openrouter_enrichment.goadds a new long-lived background HTTP poller with its own timeout/refresh policy. That is a separate runtime/network behavior change from the temporal feature and should be reviewed independently instead of landing as part of this PR.
Test plan:
- Reviewed the temporal injection path, the new management endpoints, and the OpenRouter enrichment worker.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fb9745510
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| func (m *Manager) temporalConfig() temporal.Config { | ||
| cfg, _ := m.runtimeConfig.Load().(*internalconfig.Config) | ||
| if cfg != nil { | ||
| return cfg.Temporal |
There was a problem hiding this comment.
temporalConfig() returns cfg.Temporal whenever a runtime config is loaded, but Config.Temporal.Enabled defaults to false when the temporal block is absent in YAML. Because SetConfig always stores a non-nil config, ShouldInject sees Enabled=false and skips injection for all normal deployments unless users explicitly add temporal.enabled: true, which silently disables the feature this commit introduces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956ce7f040
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if temporal.ShouldInject(m.temporalConfig()) { | ||
| execReq.Payload = temporal.InjectIntoPayload(execReq.Payload, upstreamModel) |
There was a problem hiding this comment.
Temporal rewriting is added for Execute/ExecuteStream here, but executeCountMixedOnce still sends the original payload to CountTokens. Endpoints that preflight with token-count APIs (for example Claude/Gemini count routes via ExecuteCountWithAuthManager) will undercount because the injected temporal system block is missing there, so a request can pass counting and then fail real execution with token/context limits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33f0a789b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if cfg != nil && cfg.Temporal != nil { | ||
| return *cfg.Temporal |
There was a problem hiding this comment.
temporalConfig() returns *cfg.Temporal as-is whenever the block exists, so omitted YAML fields keep Go zero-values. A config like temporal: { inject_interval: 5 } leaves Enabled=false, causing ShouldInject to skip all requests even though defaults are documented as enabled-by-default; this silently disables the feature for partial configs instead of applying defaults plus overrides.
Useful? React with 👍 / 👎.
| for modelID, ctxLen := range openRouterContextLengths { | ||
| openRouterStore.contextLength[modelID] = ctxLen |
There was a problem hiding this comment.
The enrichment cache stores keys from the OpenRouter response (openRouterContextLengths), but source lookup later is done with local registry IDs via GetOpenRouterContextLengthSource(modelID) in BuildModelSources. For non-exact matches (the same function explicitly supports cases like local gemini-3.1-pro matched from provider-prefixed IDs), source attribution will miss and /models/health reports provider/static even when context length came from OpenRouter.
Useful? React with 👍 / 👎.
| // Get all registered models and enrich those lacking context_length | ||
| allModels := registry.GetAvailableModels("openai") |
There was a problem hiding this comment.
The enrichment loop uses GetAvailableModels("openai"), which excludes models whose clients are currently suspended/unavailable, but this function is intended to enrich registered models lacking context_length. During outages or quota suspension, those models are skipped entirely and remain unenriched until they become available again, undermining the management health metadata this change introduces.
Useful? React with 👍 / 👎.
dwillitzer
force-pushed
the
feat/temporal-anti-drift
branch
from
1124bcd to
375e840
Compare
Adds a background enrichment job that populates `context_length` metadata for registered models by fetching OpenRouter's public models catalog (https://openrouter.ai/api/v1/models). Runs once on startup and then refreshes every 24 hours. ## Why Clients that surface model capabilities (e.g. "supports 1M context") currently rely on hand-maintained entries in internal/registry/models/ models.json. OpenRouter already publishes authoritative context_length values for every routed model; consuming that upstream eliminates the maintenance burden for any model OpenRouter fronts and keeps the catalog accurate as providers bump windows. ## Behavior - Startup fetch populates the in-memory enrichment store; if the fetch fails (network, upstream outage, etc.) the registry falls back to the static JSON with no service impact. - 24-hour periodic refresh keeps the cache fresh without hot-pathing the OpenRouter API. - Management endpoint exposes enriched metadata so the UI and clients can display accurate context windows. - model_registry.RefreshModels() now writes through to the live registration so the enriched count is observable and returned. ## Timeout pattern Uses `http.Client{Timeout: 30s}` + `context.WithTimeout` — structurally identical to the existing precedent at `internal/misc/antigravity_version.go` (commits 3774b56, 8d5e470), which was accepted upstream for the same class of periodic metadata fetch. Happy to tighten the timeout or rewrite the pattern if the maintainer prefers — just point at the approved shape. ## Scope This PR is intentionally scoped to OpenRouter enrichment only. The temporal-anti-drift feature that was previously bundled in the same branch is now split out to router-for-me#2574. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@luispater — force-pushed a full rewrite addressing both blocking findings. This PR is now temporal-only; the OpenRouter enrichment you flagged separately is split out to #2880. Blocker #1 — unconditional injection / no opt-in
Blocker #2 — OpenRouter enrichment
Scope
Also fixed under this scopeTwo injection-stripping paths that were silent in the prior revision:
VerifiedEnd-to-end against a running instance:
Ready for re-review when you have time. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 375e840514
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if temporal.ShouldInject(temporal.ConfigOrDefault(e.cfg.Temporal)) { | ||
| req.Payload = temporal.InjectIntoPayload(req.Payload, baseModel) | ||
| } |
There was a problem hiding this comment.
The Antigravity executor calls ShouldInject again after signature validation even though the conductor already evaluated it for the same outbound attempt, and req.Payload is reset to originalPayload before this second check. That means interval mode increments the global counter twice per Antigravity request; for example, with inject_interval: 2, the second check is always true so Antigravity requests are injected every time instead of every second request, and other interval values are similarly phase-shifted.
Useful? React with 👍 / 👎.
|
Resolved merge conflict with upstream Conflict: upstream renamed a test comment ("survive conversion" → "survive forwarding"). Our temporal preservation regression test was adjacent. Resolution: Kept the Still awaiting @luispater re-review of the clean rewrite (both original blockers addressed in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 827545ff30
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if strings.Contains(txt, "<temporal_context>") { | ||
| preservedTemporal = txt | ||
| return false |
There was a problem hiding this comment.
This branch preserves any system text containing "<temporal_context>" and later excludes matching blocks from the forwarded/sanitized user-system path, so in OAuth cloaking mode a caller can wrap arbitrary instructions in a fake temporal wrapper and have them forwarded unsanitized in system. That bypasses the cloaking behavior that was intended to strip third-party prompt structure, and it was introduced by the new preservation logic.
Useful? React with 👍 / 👎.
| if bytes.Contains(payload, []byte("<temporal ")) { | ||
| return payload |
There was a problem hiding this comment.
The dedup check only looks for raw "<temporal " bytes, but JSON payloads frequently encode < as \u003c (including output produced by sjson in this codepath). When a request already contains an escaped temporal tag, this guard misses it and injects a second temporal block, producing duplicated/conflicting temporal context instead of the intended no-op.
Useful? React with 👍 / 👎.
|
Addressed both findings from the Codex review on P1 — Validate temporal marker before bypassing OAuth sanitization (
P2 — Match escaped temporal tags in dedup guard (
Test fix: The Build passes, all temporal and executor tests green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 389f2e303c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if temporal.ShouldInject(m.temporalConfig()) { | ||
| execReq.Payload = temporal.InjectIntoPayload(execReq.Payload, upstreamModel) |
There was a problem hiding this comment.
temporal.ShouldInject(...) is evaluated inside the per-model execution loop, so a single user request can increment the global interval counter multiple times when fallback models/providers are tried. That breaks the documented inject_interval semantics (“every Nth request”) and makes injection depend on retry behavior (e.g., interval=2 can inject on the first request if the first candidate fails). Compute the inject decision once per inbound request and reuse it for all retries/attempts (same pattern also appears in the streaming path).
Useful? React with 👍 / 👎.
| // Re-apply temporal injection after signature validation (which forces | ||
| // req.Payload back to the raw original). Without this, the conductor's | ||
| // anti-drift hook is silently stripped for every Antigravity request. | ||
| if temporal.ShouldInject(temporal.ConfigOrDefault(e.cfg.Temporal)) { |
There was a problem hiding this comment.
These new calls dereference e.cfg.Temporal without checking whether e.cfg is nil. NewAntigravityExecutor accepts a *config.Config and existing tests construct it with nil, so any valid request that reaches this path will panic with a nil-pointer dereference. Add an e.cfg nil guard (or pass a nil *temporal.Config to ConfigOrDefault) before reading .Temporal.
Useful? React with 👍 / 👎.
| // <temporal_context> wrapper around a single self-closing <temporal .../> tag | ||
| // with recognizable date attributes and nothing else. | ||
| var validTemporalRe = regexp.MustCompile( | ||
| `^<temporal_context><temporal\s+day="[A-Z][a-z]+" date="\d{4}-\d{2}-\d{2}"[^>]*/></temporal_context>$`, |
There was a problem hiding this comment.
IsValidTemporalBlock is intended to prevent bypassing OAuth sanitization, but the regex allows arbitrary extra attributes via [^>]* after the required day/date fields. In checkSystemInstructionsWithSigningMode, any block passing this validator is preserved in system and excluded from sanitized user-system forwarding, so callers can still inject instructions in added attribute values. Fresh evidence: the new validator itself permits attacker-controlled attributes because of this wildcard.
Useful? React with 👍 / 👎.
Adds an optional temporal anti-drift system that injects a short
<temporal_context> XML element into the system prompt of outbound LLM
requests. Long-running agent sessions (>20 turns) frequently hallucinate
the current date; grounding every request in fresh wall-clock context
eliminates the drift.
Addresses the two blocking findings on the prior revision:
1. OPT-IN (was: unconditionally enabled)
- DefaultConfig() now returns {Enabled: false}. When the `temporal`
YAML block is omitted, behavior is identical to the non-temporal
build. No surprise request mutation.
- Enable via:
temporal:
enabled: true
inject_interval: 0
2. CONFIG WIRING (was: hardcoded temporal.DefaultConfig() at call sites)
- Config.Temporal is a *temporal.Config pointer, preserving the
nil-vs-explicit-zero distinction YAML can't otherwise express.
- Conductor reads the live runtime config on every request via
Manager.temporalConfig(); no reference to DefaultConfig() in the
hot path.
- ConfigOrDefault() helper exposes the same resolution logic to
downstream executors that need to re-apply injection.
Cross-cutting correctness:
- Claude OAuth (claude_executor.go): checkSystemInstructionsWithSigningMode
rebuilds the system array to mimic Claude Code's shape. It now
preserves any injected <temporal_context> block across the rewrite and
excludes it from the OAuth-sanitized forwarded user-system text, so
injection survives both the rewrite and the sanitizer.
- Antigravity (antigravity_executor.go): validateAntigravityRequestSignatures
followed by `req.Payload = originalPayload` discards any conductor
mutation. The executor now re-applies temporal.InjectIntoPayload after
validation at all three generation sites (Execute, streaming, retry
stream). CountTokens is intentionally left alone — it never reaches
generation.
- Other executors (OpenAI-compat, Codex, Gemini) don't overwrite
req.Payload, so the single conductor-level hook is sufficient.
Safety properties (covered by unit tests):
- No PII — only UTC and local timestamps.
- Format-aware — Claude (top-level `system`) vs OpenAI (`messages`).
- Dedup guard — payload already containing <temporal skips injection.
- Image-model skip — imagen / image-gen are never modified.
- Thread-safe — request counter uses sync/atomic.
Observability:
- Manager.SetConfig logs the effective temporal state on startup and on
every reload: `temporal injection state: enabled=%t interval=%d source=%s`
where source is `default` (YAML omitted) or `config` (explicit block).
Helps diagnose the class of reload-state regressions that are very
hard to catch otherwise.
Files:
- internal/temporal/temporal.go (+package, +doc)
- internal/temporal/temporal_test.go (6 unit tests)
- internal/config/config.go (Temporal *Config field)
- sdk/cliproxy/auth/conductor.go (hook + config resolution + log)
- internal/runtime/executor/claude_executor.go (OAuth preserve)
- internal/runtime/executor/claude_executor_test.go (regression test)
- internal/runtime/executor/antigravity_executor.go (re-inject x3)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P1: Prevent OAuth cloaking bypass via fake <temporal_context> wrappers. The Claude executor now validates that preserved temporal blocks match the shape produced by BuildTemporalTag (regex-checked) before bypassing sanitization. Arbitrary content in a <temporal_context> wrapper is no longer forwarded unsanitized in the system array. P2: The dedup guard now also checks for <-escaped temporal tags, which some JSON serializers produce. This prevents double injection when the payload already contains an escaped temporal block. Also fixes the PreservesTemporalContext test to properly JSON-escape inner quotes in the hardcoded payload (the previous version produced malformed JSON that gjson silently truncated). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dwillitzer
force-pushed
the
feat/temporal-anti-drift
branch
from
389f2e3 to
0da0939
Compare
Adds a background enrichment job that populates `context_length` metadata for registered models by fetching OpenRouter's public models catalog (https://openrouter.ai/api/v1/models). Runs once on startup and then refreshes every 24 hours. ## Why Clients that surface model capabilities (e.g. "supports 1M context") currently rely on hand-maintained entries in internal/registry/models/ models.json. OpenRouter already publishes authoritative context_length values for every routed model; consuming that upstream eliminates the maintenance burden for any model OpenRouter fronts and keeps the catalog accurate as providers bump windows. ## Behavior - Startup fetch populates the in-memory enrichment store; if the fetch fails (network, upstream outage, etc.) the registry falls back to the static JSON with no service impact. - 24-hour periodic refresh keeps the cache fresh without hot-pathing the OpenRouter API. - Management endpoint exposes enriched metadata so the UI and clients can display accurate context windows. - model_registry.RefreshModels() now writes through to the live registration so the enriched count is observable and returned. ## Timeout pattern Uses `http.Client{Timeout: 30s}` + `context.WithTimeout` — structurally identical to the existing precedent at `internal/misc/antigravity_version.go` (commits 3774b56, 8d5e470), which was accepted upstream for the same class of periodic metadata fetch. Happy to tighten the timeout or rewrite the pattern if the maintainer prefers — just point at the approved shape. ## Scope This PR is intentionally scoped to OpenRouter enrichment only. The temporal-anti-drift feature that was previously bundled in the same branch is now split out to router-for-me#2574. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0da09392ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if messages := gjson.GetBytes(payload, "messages"); messages.IsArray() { | ||
| return injectIntoMessages(payload, messages, text) |
There was a problem hiding this comment.
When temporal injection is enabled for a native Claude Messages request that omits the optional top-level system field, this branch treats the messages array as OpenAI chat format and prepends a role: "system" message. The Claude executor later translates claude → claude via the registry fallback, so that injected message is forwarded unchanged instead of being moved into top-level system (the repo’s OpenAI→Claude translator does that conversion explicitly). Native Claude messages only support conversational roles, so otherwise valid Claude requests without system start failing once temporal injection is enabled; pass the source format into this helper or inject a top-level Claude system block for Claude-shaped payloads.
Useful? React with 👍 / 👎.
2 participants
Summary
<temporal>tags into every outbound request to all providerssystemarray) vs OpenAI format (messagesarray) — no 400 errorsWhy This Matters
In long agent sessions, AI models hallucinate dates — wrong days, wrong weeks, wrong months. This cascades into incorrect tool calls, wrong file paths, and stale context decisions. By injecting fresh temporal context on every request, models stay grounded in the current date/time.
Pattern validated in
pi-rswhereTemporalDriftDetectorfound real drift in sessions >20 turns.Changes
internal/temporal/temporal.go— temporal tag builder + format-aware payload injection (~120 lines)sdk/cliproxy/auth/conductor.go— injects beforeExecute()andExecuteStream()Test plan
go build -o cliproxyapi ./cmd/server/— compilation succeedscurl -s -X POST localhost:8317/v1/chat/completions -H "Authorization: Bearer $KEY" -H "Content-Type: application/json" -d '{"model":"claude-sonnet-4-6","messages":[{"role":"user","content":"what day is it"}],"max_tokens":100}'🤖 Generated with Claude Code