Skip to content

fix(codex): pass maxRenderedContextChars to legacy mirrored-history fallback#94595

Closed
xydigit-zt wants to merge 1 commit into
openclaw:mainfrom
xydigit-zt:fix/84084-mirrored-history-context
Closed

fix(codex): pass maxRenderedContextChars to legacy mirrored-history fallback#94595
xydigit-zt wants to merge 1 commit into
openclaw:mainfrom
xydigit-zt:fix/84084-mirrored-history-context

Conversation

@xydigit-zt

@xydigit-zt xydigit-zt commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Real behavior proof

Behavior or issue addressed: Legacy mirrored-history fallback in run-attempt.ts was calling projectContextEngineAssemblyForCodex without maxRenderedContextChars, causing it to fall back to DEFAULT_RENDERED_CONTEXT_CHARS = 24000 instead of deriving a budget-scaled cap from contextTokenBudget. This affected both the stale-resume path and the fresh-thread path.

Real environment tested: macOS 14.6.1, Node 22, openclaw main @ 967fb8d

Exact steps or command run after this patch:

node --import tsx -e "
import { resolveCodexContextEngineProjectionMaxChars } from './extensions/codex/src/app-server/context-engine-projection.ts';

console.log('Default (no budget):', resolveCodexContextEngineProjectionMaxChars({}));
console.log('Budget 16k:', resolveCodexContextEngineProjectionMaxChars({ contextTokenBudget: 16_000 }));
console.log('Budget 80k:', resolveCodexContextEngineProjectionMaxChars({ contextTokenBudget: 80_000 }));
console.log('Budget 272k:', resolveCodexContextEngineProjectionMaxChars({ contextTokenBudget: 272_000 }));
"

Evidence after fix:

Default (no budget): 24000
Budget 16k: 32000
Budget 80k: 240000
Budget 272k: 1000000

Both legacy projection paths (fresh-thread at line 906, stale-resume at line 1094) now pass maxRenderedContextChars derived from contextTokenBudget, matching the active context-engine pattern at line 952.

Unit tests (2 new regression tests for #84084):
✓ "uses the runtime token budget for legacy fresh-thread continuity projection (#84084)" — verifies budget cap on fresh-thread path
✓ "uses the runtime token budget for stale-binding continuity projection (#84084)" — verifies budget cap on stale-binding path

Observed result after fix: Default 24,000-char cap becomes a budget-scaled cap (e.g., 1,000,000 chars for 272k token budget). All 107 run-attempt tests pass including the 2 new regression tests.

What was not tested: Live Codex session with mid-session thread rebuild trigger. The code path requires a running Codex OAuth session; the change is structurally identical to the already-merged active context-engine path (PR #80761, lines 952-957).

Out-of-scope follow-ups

None.

@openclaw-barnacle openclaw-barnacle Bot added extensions: codex size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close: the merged current-main replacement at #94093 already includes the useful fix from this PR, adds the missing turn input-limit fitting and stronger continuity tests, and this branch is now conflicting with main.

Canonical path: Keep the broader #94093 implementation on main, ship it in the next release, and close this conflicting duplicate branch.

So I’m closing this here and keeping the remaining discussion on #94093.

Review details

Best possible solution:

Keep the broader #94093 implementation on main, ship it in the next release, and close this conflicting duplicate branch.

Do we have a high-confidence way to reproduce the issue?

Yes, the original bug is source-reproducible on the PR base and in the linked issue: the legacy continuity calls omitted maxRenderedContextChars while the helper defaulted missing caps to 24,000 chars. Current main no longer reproduces that source gap because #94093 added the cap at both call sites.

Is this the best way to solve the issue?

No, this PR is no longer the best way to solve the issue. The best current solution is the merged replacement, which preserves this parity fix and also handles Codex turn/start input limits and forced fresh-thread continuity.

Security review:

Security review cleared: The PR diff only changes Codex runtime projection sizing and tests; it does not add dependencies, workflows, secrets handling, permissions, or package execution surfaces.

AGENTS.md: found and applied where relevant.

What I checked:

Likely related people:

  • VACInc: Authored the merged replacement PR that put the two legacy budget-scaled projection call sites and related continuity tests on current main. (role: canonical replacement author; confidence: high; commits: ea76a45917ca, 72e164a3fe20; files: extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/run-attempt.test.ts, extensions/codex/src/app-server/context-engine-projection.ts)
  • jalehman: Authored the input-limit portion of the merged replacement and merged the earlier budget-aware projection PR that established the parity target. (role: adjacent projection/input-limit contributor; confidence: medium; commits: 1f3ced8f6343, f7ab8c26b1e7; files: extensions/codex/src/app-server/context-engine-projection.ts, extensions/codex/src/app-server/run-attempt.ts)
  • 100yenadmin: Authored the merged PR that added resolveCodexContextEngineProjectionMaxChars and wired the active context-engine path this bug fix mirrors. (role: introduced budget-aware projection behavior; confidence: high; commits: f7ab8c26b1e7; files: extensions/codex/src/app-server/context-engine-projection.ts, extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/context-engine-projection.test.ts)

Codex review notes: model internal, reasoning high; reviewed against 49e6f5a524bc; fix evidence: commit ea76a45917ca, main fix timestamp 2026-06-18T22:33:48Z.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 18, 2026
@xydigit-zt
xydigit-zt force-pushed the fix/84084-mirrored-history-context branch from 0a63a46 to d9ada5d Compare June 18, 2026 15:48
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
@xydigit-zt

xydigit-zt commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author
image

@xydigit-zt

xydigit-zt commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 18, 2026
@xydigit-zt

xydigit-zt commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jun 18, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Merged
@xydigit-zt
fix(codex): pass maxRenderedContextChars to legacy mirrored-history f…
4a091cd 
…allback

The legacy mirrored-history fallback at run-attempt.ts:1088-1092 was calling
projectContextEngineAssemblyForCodex without maxRenderedContextChars,
falling back to DEFAULT_RENDERED_CONTEXT_CHARS=24000 instead of deriving
a budget-scaled cap from contextTokenBudget.

This fix copies the maxRenderedContextChars kwarg from the active-context-engine
branch (lines 952-957) into the legacy fallback call site, matching the
behavior added in PR openclaw#80761 for issue openclaw#80760.

Fixes openclaw#84084
@xydigit-zt
xydigit-zt force-pushed the fix/84084-mirrored-history-context branch from d9ada5d to 4a091cd Compare June 18, 2026 23:33
@xydigit-zt

xydigit-zt commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper

clawsweeper Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

  • Action: closed this PR.
  • Close reason: duplicate or superseded.
  • Evidence: durable ClawSweeper review.
  • Coverage proof: PR B is not merely related: the source report identifies it as the merged current-main implementation of PR A's two concrete legacy projection cap fixes, with broader safety work and tests on the same Codex continuity surface. Any remaining PR A proof or review concerns are either addressed by PR B's broader coverage or belong on PR B/current main rather than requiring PR A to stay open. Covering PR: Prevent Codex thread rotation from losing next-step context #94093.

@clawsweeper clawsweeper Bot closed this Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@xydigit-zt