[codex] Compact when comp_hash changes - #27520
Conversation
64e76ec to
5a77281
Compare
35ea7a5 to
67cd3d9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cd3d9a67
ℹ️ 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".
| model_info_with_comp_hash(previous_model, "hash-a"), | ||
| model_info_with_comp_hash(next_model, "hash-b"), |
There was a problem hiding this comment.
This test seeds both the saved and target models with comp_hash values, so it misses legacy resumed rollouts where TurnContextItem.comp_hash is absent and the new path must compact once from None -> Some. Please add integration coverage for that behavior per guidance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e31456182
ℹ️ 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".
| let comp_hash_changed = previous_turn_settings | ||
| .comp_hash | ||
| .as_deref() | ||
| .zip(turn_context.comp_hash.as_deref()) | ||
| .is_some_and(|(previous, current)| previous != current); |
There was a problem hiding this comment.
When a user resumes a rollout written before comp_hash was persisted, previous_turn_settings.comp_hash is None while the current model metadata can now have Some(...); Option::zip makes that look unchanged, so the pre-turn compaction is skipped and the first upgraded turn can send history that was never compacted for the advertised compatibility hash. Fresh evidence: this version still gates the decision on Option::zip here, so None -> Some cannot trigger the one-time recovery compaction.
Useful? React with 👍 / 👎.
|
@codex review |
## Summary - add optional `comp_hash` metadata to `ModelInfo` - update `ModelInfo` fixtures for the shared schema change - keep older model responses compatible by defaulting the field to `None` ## Why The models endpoint needs an opaque identifier for compaction-compatible model configurations. This PR only exposes that value in model metadata; it does not add it to turn context or change runtime behavior. Follow-up #27520 carries the value through turn context and rollouts, then uses it to trigger compaction. ## Stack - based on #27115 - functionality follow-up: #27520 ## Testing - `just test -p codex-protocol model_info_defaults_availability_nux_to_none_when_omitted` - `just fix -p codex-core -p codex-protocol -p codex-analytics -p codex-models-manager`
9681a29 to
3cc928a
Compare
|
@codex review |
## Summary - add optional `comp_hash` metadata to `ModelInfo` - update `ModelInfo` fixtures for the shared schema change - keep older model responses compatible by defaulting the field to `None` ## Why The models endpoint needs an opaque identifier for compaction-compatible model configurations. This PR only exposes that value in model metadata; it does not add it to turn context or change runtime behavior. Follow-up #27520 carries the value through turn context and rollouts, then uses it to trigger compaction. ## Stack - based directly on `main` - replaces #27519, which was accidentally merged into the wrong base branch - functionality follow-up: #27520 ## Testing - `just test -p codex-protocol model_info_defaults_availability_nux_to_none_when_omitted` - `just fix -p codex-core -p codex-protocol -p codex-analytics -p codex-models-manager`
Summary
comp_hashintoTurnContextwhen the turn is created and use that snapshot as the downstream source of truthcomp_hash_changedas the compaction reasonWhy
History produced under one compaction-compatible model configuration may not be safe to carry directly into another. Compacting at the turn boundary converts that history before context updates and the new user message are added. Persisting the turn snapshot in
TurnContextItemmakes the same protection work after resuming a rollout.A missing hash is not treated as evidence of incompatibility.
None → Some,Some → None, andNone → Nonedo not trigger compaction; onlySome(previous) → Some(current)with unequal values does.Stack
mainTesting
just test -p codex-core pre_sampling_compact_— 6 passedjust test -p codex-core turn_context_item_uses_turn_context_comp_hash_snapshot— passedjust fix -p codex-core -p codex-protocol -p codex-analytics -p codex-models-manager