Skip to content

Match Codex compaction and model-transition behavior - #6

Merged
YeungKC merged 15 commits into
mainfrom
parity/three-phases
Aug 17, 2026
Merged

Match Codex compaction and model-transition behavior#6
YeungKC merged 15 commits into
mainfrom
parity/three-phases

Conversation

@YeungKC

@YeungKC YeungKC commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • Match Codex V1/V2 compaction retention, native checkpoint hashes, deferred model transitions, and eligible fallback behavior.
  • Normalize cross-model Pi/provider history, including tools, reasoning, IDs, and image representations.
  • Preserve Pi session context across native, built-in, legacy, token-budget, and provider-switch boundaries.
  • Serialize concurrent transition/automatic compactions per session and preserve persisted request users exactly once across replay and mid-turn continuation.
  • Keep malformed/auth/policy/image/cancellation failures fail-closed.
  • Use best-effort BodyAfterPrefix/total token accounting with documented Pi-core limits.

Validation

  • bun test — 90 passing, 0 failing
  • git diff --check
  • Audited 505 accessible Pi session JSONL files: 25 native checkpoint records were parser-readable; no session-file repair was needed.
  • Multi-agent adversarial review completed; latest verified findings fixed in 391f147.

Known parity boundaries

Exact Codex tokenizer usage, Responses-Lite wire shape, mid-turn continuation beyond the extension seam, fresh token-budget windows, model-switch world-state injection, compaction-window metadata, and canonical provider capability metadata remain unavailable or insufficiently represented in Pi extensions.

@YeungKC
YeungKC requested a balanced review from Copilot August 17, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns compaction, model-transition, history normalization, fallback, and token-accounting behavior with Codex.

Changes:

  • Adds compaction-hash-aware transitions and checkpoint recovery.
  • Normalizes cross-model history and tightens fallback eligibility.
  • Improves retention and approximate token accounting.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
session-coordinator.ts Updates transition and recovery orchestration.
session-coordinator.test.ts Expands transition and normalization tests.
scheduler.test.ts Tests prefix-aware token thresholds.
remote-compaction.ts Persists checkpoint hashes.
README.md Documents compatibility boundaries.
package.json Bumps package version.
native-compaction.ts Updates normalization, retention, accounting, and fallback logic.
native-compaction.test.ts Tests new compaction behavior.
index.ts Adds prefix accounting and signal propagation.
docs/adr/0001-remote-native-checkpoint.md Updates architecture decisions.
CONTEXT.md Revises transition and accounting boundaries.
capabilities.ts Adds compaction-hash resolution.
capabilities.test.ts Tests hash resolution.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread native-compaction.ts
Comment thread native-compaction.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

native-compaction.ts:785

  • The fallback status whitelist omits HTTP 404, so a retired previous model returning model_not_found is explicitly marked ineligible and the coordinator will not try the current model. The existing not found body check suggests this is intended to be an eligible model failure; include 404 and apply the same model/not-found body gate while retaining the deny list.
function canFallbackForStatus(status: number, body: string): boolean {
	if (![400, 403, 408, 409, 429].includes(status) && status < 500) return false;
	if (status === 403 && !/(?:model|invalid request|not found|overloaded)/i.test(body)) return false;
	return !/(?:malformed|misalignment_policy|cyber_policy|invalid[_ ]image|content policy|unauthorized|forbidden|permission|api key|authentication|invalid[_ ]token|cancel(?:led|lation)?|aborted)/i.test(body);

session-coordinator.ts:55

  • The broad token deny term runs before the eligible context-window checks. An unmarked SSE error such as “context token limit exceeded” is therefore treated as fail-closed even though it is a context-capacity failure that should retry on the current model. Narrow this term to authentication-token forms.
	if (/(?:abort|cancel|auth|token|account|malformed|invalid compaction|misalignment_policy|cyber_policy|invalid[_ ]image|content policy|unauthorized|forbidden|permission|api key)/i.test(message)) return false;

native-compaction.ts:481

  • The new tool-item accounting misses standard function_call items because their type does not contain the word tool; they still use the old arguments-only estimate and omit names, IDs, and structural tokens. Histories with many small calls can consequently remain substantially undercounted. Include function_call in the serialized-item branch.

This issue also appears on line 782 of the same file.

	if (typeof item.type === "string" && item.type.includes("tool") && item.type !== "function_call_output") {
		return Math.max(1, Math.ceil(JSON.stringify(item).length / 4));

Comment thread session-coordinator.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

session-coordinator.ts:389

  • This lock is checked only after transition/recovery work. If model A is automatically compacting and a request for model B starts, B can run transition compaction concurrently with A. Because conversationLeafId ignores the custom checkpoints, both operations may append; if A finishes last, B resumes here and returns A's now-latest checkpoint without rerunning the hash compatibility check above. Serialize automatic and transition compactions through one per-session operation, or await this operation before transition/recovery and restart request preparation afterward.
			const activeAutomaticCompaction = automaticCompactionBySession.get(sessionId);
			if (activeAutomaticCompaction) {
				await activeAutomaticCompaction;
			} else {

native-compaction.ts:868

  • Eligibility here ignores the failure message, so any invalid_prompt response is explicitly marked retryable even when the message identifies a fail-closed condition such as invalid_image, cyber_policy, or malformed input. The explicit marker bypasses the coordinator's deny-list. Apply the same fail-closed classification used for HTTP failures before setting the marker.
			const code = typeof failure.code === "string" ? failure.code : "response.failed";
			const message = typeof failure.message === "string" ? failure.message : "OpenAI Codex compaction ended with response.failed.";
			const eligible = code === "context_length_exceeded" || code === "invalid_prompt";
			throw markFallbackEligibility(new NonRetryableCompactionError(`OpenAI Codex compaction failed (${code}): ${message}`), eligible);

Comment thread session-coordinator.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

native-compaction.ts:497

  • An image-bearing function_call_output now contributes the fixed image weight, but trimFunctionCallHistoryToFitContextWindow delegates to truncateFunctionOutput, which only changes string outputs. Thus an image-only output that exceeds the remaining budget is returned unchanged (for example, 1,200 estimated tokens with a 100-token limit), so the supposedly trimmed remote request can still exceed the context window. Replace image-bearing output with bounded text when it must be trimmed.
	const imageTokens = imagePartCount(item) * 1_200;
	if (imageTokens > 0) return imageTokens + Math.max(0, Math.ceil(responseItemText(item).length / 4));

native-compaction.ts:805

  • The fail-closed deny list still misses common machine-readable auth/policy forms. For example, a 400 body containing only {"code":"invalid_api_key"} or {"code":"policy_violation"} does not match api key/content policy, so it is explicitly marked eligible and the coordinator retries it with the current model. Match underscore/hyphen variants (and generic policy-violation codes) before setting fallback eligibility.
	return !/(?:malformed|misalignment_policy|cyber_policy|invalid[_ ]image|content policy|unauthorized|forbidden|permission|api key|authentication|(?:invalid|expired|bearer|refresh)[ _-]?token|cancel(?:led|lation)?|aborted)/i.test(body);

Comment thread session-coordinator.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

session-coordinator.ts:94

  • Content equality alone cannot identify the turn that preservedInput belongs to. If an earlier checkpoint preserved "hello" and a later user sends "hello" again, this returns true and lines 467-469 remove the new request from the tail; during automatic compaction, the similar check in preserveCurrentUser also mistakes the older copy for the current one and fails to persist it. Use branch ordering to require the checkpoint to occur after the persisted current-user entry before treating that user as already preserved.
	const preservedUser = checkpoint.status === "valid" ? checkpoint.checkpoint.details.preservedInput?.findLast((item) => item.role === "user") : undefined;
	return Boolean(requestUser && preservedUser && textContent(preservedUser.content) === textContent(requestUser.content));

native-compaction.ts:886

  • A machine-readable SSE error with an eligible code is not marked eligible here. For example, { type: "error", code: "context_length_exceeded", message: "too large" } throws an unmarked error whose message does not match the coordinator's fallback heuristic, so the current-model fallback is skipped. Mark known request/model codes explicitly while marking unknown and fail-closed codes false, as the response.failed branch already does.
			const error = new NonRetryableCompactionError(event.message);
			throw code && isFailClosedCompactionError(code)
				? markFallbackEligibility(error, false)
				: error;

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

session-coordinator.ts:275

  • In the unavailable-previous-model recovery path, the current user may already be part of the compaction input (for example, during a tool-turn continuation), but omitting that input from preserveCurrentUser always adds the matching user to preservedInput. V2 then retains the user in replacementHistory and replays it again from preservedInput. Reuse the exact input sent to createCheckpoint when deciding whether preservation is needed.
					deps.appendCheckpoint({
						...preserveCurrentUser(native.details, branch, requestInput),
						modelKey: modelKey(model),
						...(currentHash ? { compHash: currentHash } : {}),
					});

native-compaction.ts:826

  • The explicit eligibility marker returned here bypasses the coordinator's additional auth|account|invalid compaction fail-closed check. Consequently, bodies such as account suspended, authentication required, or invalid compaction state on an otherwise eligible status are retried with the current model, contrary to the fail-closed contract. Apply the same deny terms before marking the HTTP error eligible.
function canFallbackForStatus(status: number, body: string): boolean {
	if (![400, 403, 404, 408, 409, 429].includes(status) && status < 500) return false;
	if (status === 403 && !/(?:model|invalid request|not found|overloaded)/i.test(body)) return false;
	if (status === 404 && !/(?:model|invalid request|overloaded)/i.test(body)) return false;
	return !isFailClosedCompactionError(body);

session-coordinator.ts:95

  • This deduplication relies only on content in preservedInput. A transition followed immediately by automatic compaction absorbs that preserved current user into the second checkpoint's replacementHistory, leaving preservedInput empty, so lines 469-470 keep the same user in the request tail and send it twice. Conversely, a later new turn that repeats the old preserved text is removed. The checkpoint needs an unambiguous current-turn identity/marker rather than a content-only test across checkpoint generations.
function checkpointPreservesCurrentUser(branch: SessionEntry[], requestInput: ResponseItem[] | undefined): boolean {
	const checkpoint = findNativeCheckpoint(branch);
	const requestUser = requestInput?.findLast((item) => item.role === "user");
	const preservedUser = checkpoint.status === "valid" ? checkpoint.checkpoint.details.preservedInput?.findLast((item) => item.role === "user") : undefined;
	return Boolean(requestUser && preservedUser && textContent(preservedUser.content) === textContent(requestUser.content));

session-coordinator.ts:109

  • Matching any user in the compaction input by content does not prove that the current turn was included. If an older user has the same text as the current persisted user, branchBeforeCurrentUser excludes the current entry but this check matches the older one and skips preservedInput; later replay then loses the current user at its post-checkpoint position. Pass an explicit indication that the current branch entry was excluded instead of inferring it from content.
	const currentUser = branch.findLast((entry) => entry.type === "message" && entry.message.role === "user");
	if (!requestUser || !currentUser || currentUser.type !== "message") return details;
	if (compactionInput?.some((item) => item.role === "user" && textContent(item.content) === textContent(requestUser.content))) return details;
	return textContent(currentUser.message.content) === textContent(requestUser.content)
		? { ...details, preservedInput: [structuredClone(requestUser)] }

@YeungKC
YeungKC merged commit 7963557 into main Aug 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants