Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/request/prompt-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function normalizeForkSuffix(forkId: string): string {
return trimmed.replace(/\s+/g, "-");
}

const PROMPT_CACHE_METADATA_KEYS = [
export const PROMPT_CACHE_METADATA_KEYS = [
"conversation_id",
"conversationId",
"thread_id",
Expand All @@ -67,7 +67,7 @@ const PROMPT_CACHE_METADATA_KEYS = [
"chatId",
];

const PROMPT_CACHE_FORK_KEYS = [
export const PROMPT_CACHE_FORK_KEYS = [
"forkId",
"fork_id",
"branchId",
Expand Down
4 changes: 2 additions & 2 deletions lib/session/session-manager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { createHash, randomUUID } from "node:crypto";
import { SESSION_CONFIG } from "../constants.js";
import { logDebug, logWarn } from "../logger.js";
import { PROMPT_CACHE_FORK_KEYS } from "../request/prompt-cache.js";
import type { CodexResponsePayload, InputItem, RequestBody, SessionContext, SessionState } from "../types.js";
import { cloneInputItems, deepClone } from "../utils/clone.js";
import { isAssistantMessage, isUserMessage } from "../utils/input-item-utils.js";
Expand Down Expand Up @@ -120,7 +121,6 @@ function extractConversationId(body: RequestBody): string | undefined {
function extractForkIdentifier(body: RequestBody): string | undefined {
const metadata = body.metadata as Record<string, unknown> | undefined;
const bodyAny = body as Record<string, unknown>;
const forkKeys = ["forkId", "fork_id", "branchId", "branch_id"];
const normalize = (value: unknown): string | undefined => {
if (typeof value !== "string") {
return undefined;
Expand All @@ -129,7 +129,7 @@ function extractForkIdentifier(body: RequestBody): string | undefined {
return trimmed.length > 0 ? trimmed : undefined;
};

for (const key of forkKeys) {
for (const key of PROMPT_CACHE_FORK_KEYS) {
const fromMetadata = normalize(metadata?.[key]);
if (fromMetadata) {
return fromMetadata;
Expand Down
30 changes: 30 additions & 0 deletions spec/issue-23-session-fork-alignment.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Issue #23 – SessionManager fork alignment

## Context

- Issue: https://github.com/open-hax/codex/issues/23
- Follow-up to PR #20 CodeRabbit review discussion on `extractForkIdentifier`.
- Problem: `lib/session/session-manager.ts` uses fork hints limited to `forkId|fork_id|branchId|branch_id` (~lines 120-143). Prompt cache fork derivation in `lib/request/prompt-cache.ts` also accepts `parentConversationId|parent_conversation_id` (~lines 70-132). Requests that set only parent conversation IDs diverge: prompt cache key suffix includes fork hint, session key does not.

## Affected areas

- `lib/session/session-manager.ts` (extract fork keys, session key construction)
- `lib/request/prompt-cache.ts` (source of fork hint keys)
- Tests: `test/session-manager.test.ts` (missing coverage for parent conversation fork hints)

## Requirements / Definition of Done

- Session key and prompt cache key derivation use the same set of fork hint keys (including parent conversation IDs) so forks stay consistent regardless of hint field used.
- Normalize/trim behavior remains consistent with existing fork handling; no regressions for current fork/branch keys.
- Add/adjust tests to cover parent conversation fork hint path.
- Build/tests pass.

## Plan (phases)

1. Analysis: Confirm current fork key sources in session manager vs prompt cache; note normalization differences and existing tests.
2. Design/Implementation: Share or mirror fork key list to include parent conversation IDs in session manager; keep trim behavior; ensure prompt cache alignment comments updated. Update session manager logic accordingly, adjusting helper if needed.
3. Verification: Add/extend session manager tests for parent conversation fork hints and run relevant test subset (session manager + prompt cache if needed).

## Changelog

- 2025-11-20: Exported prompt cache fork key list for reuse, aligned SessionManager fork extraction with parent conversation hints, and added session-manager tests covering parent conversation fork identifiers.
14 changes: 6 additions & 8 deletions test/compaction-helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe("compaction helpers", () => {
expect((body as any).parallel_tool_calls).toBeUndefined();
});

it("applies compaction when no user message exists", () => {
it("returns original items when no user message exists", () => {
const originalInput: InputItem[] = [
{
type: "message",
Expand All @@ -41,17 +41,15 @@ describe("compaction helpers", () => {

const decision = applyCompactionIfNeeded(body, {
settings: { enabled: true },
commandText: "codex-compact",
commandText: null, // No command, so no compaction should occur
originalInput,
});

expect(decision?.serialization.totalTurns).toBe(1);
expect(decision?.serialization.transcript).toContain("system-only follow-up");

// Verify RequestBody mutations
// No compaction should occur when there's no command text
expect(decision).toBeUndefined();
// Verify RequestBody mutations - body should remain unchanged
expect(body.input).toBeDefined();
expect(body.input?.length).toBeGreaterThan(0);
expect(body.input).not.toEqual(originalInput);
expect(body.input).toEqual(originalInput);
expect((body as any).tools).toBeUndefined();
expect((body as any).tool_choice).toBeUndefined();
expect((body as any).parallel_tool_calls).toBeUndefined();
Expand Down
25 changes: 25 additions & 0 deletions test/session-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

interface BodyOptions {
forkId?: string;
parentConversationId?: string;
parent_conversation_id?: string;
}

function createBody(conversationId: string, inputCount = 1, options: BodyOptions = {}): RequestBody {
Expand All @@ -15,6 +17,12 @@
if (options.forkId) {
metadata.forkId = options.forkId;
}
if (options.parentConversationId) {
metadata.parentConversationId = options.parentConversationId;
}
if (options.parent_conversation_id) {
metadata.parent_conversation_id = options.parent_conversation_id;
}

return {
model: "gpt-5",
Expand Down Expand Up @@ -156,7 +164,7 @@
const manager = new SessionManager({ enabled: true });
const body = createBody("conv-metrics");
let context = manager.getContext(body) as SessionContext;
context = manager.applyRequest(body, context) as SessionContext;

Check warning on line 167 in test/session-manager.test.ts

View workflow job for this annotation

GitHub Actions / Lint & Typecheck

'context' is assigned a value but never used. Allowed unused vars must match /^_/u

Check warning on line 167 in test/session-manager.test.ts

View workflow job for this annotation

GitHub Actions / Lint & Typecheck

'context' is assigned a value but never used. Allowed unused vars must match /^_/u

const metrics = manager.getMetrics();
expect(metrics.enabled).toBe(true);
Expand Down Expand Up @@ -222,6 +230,23 @@
expect(betaContext.state.promptCacheKey).toBe("conv-fork::fork::beta");
});

it("derives fork ids from parent conversation hints", () => {
const manager = new SessionManager({ enabled: true });
const parentBody = createBody("conv-fork-parent", 1, { parentConversationId: "parent-conv" });
let parentContext = manager.getContext(parentBody) as SessionContext;

Check warning on line 236 in test/session-manager.test.ts

View workflow job for this annotation

GitHub Actions / Lint & Typecheck

'parentContext' is never reassigned. Use 'const' instead

Check warning on line 236 in test/session-manager.test.ts

View workflow job for this annotation

GitHub Actions / Lint & Typecheck

'parentContext' is never reassigned. Use 'const' instead
expect(parentContext.isNew).toBe(true);
expect(parentContext.state.promptCacheKey).toBe("conv-fork-parent::fork::parent-conv");
manager.applyRequest(parentBody, parentContext);
expect(parentBody.prompt_cache_key).toBe("conv-fork-parent::fork::parent-conv");

const snakeParentBody = createBody("conv-fork-parent", 1, {
parent_conversation_id: "parent-snake",
});
const snakeParentContext = manager.getContext(snakeParentBody) as SessionContext;
expect(snakeParentContext.isNew).toBe(true);
expect(snakeParentContext.state.promptCacheKey).toBe("conv-fork-parent::fork::parent-snake");
});

it("scopes compaction summaries per fork session", () => {
const manager = new SessionManager({ enabled: true });
const alphaBody = createBody("conv-fork-summary", 1, { forkId: "alpha" });
Expand Down
Loading