Skip to content

feat(codex): add session ID auto-completion for Codex requests #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ding113 wants to merge 1 commit into from
Closed

feat(codex): add session ID auto-completion for Codex requests #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions drizzle/0054_shiny_quentin_quire.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL;
Copy link

@greptile-apps greptile-apps bot Jan 12, 2026

Choose a reason for hiding this comment

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

the database setting enable_codex_session_id_completion is added but is never checked or used anywhere in the codebase. Searching for this setting reveals it only exists in the migration files. This means:

  1. The feature cannot be disabled even though a toggle was created for it
  2. The setting serves no purpose in its current state
  3. There's likely missing code that should check this setting before calling CodexSessionIdCompleter.complete()

The integration code should check this setting (probably from system_settings table) before applying session ID completion

Prompt To Fix With AI 
This is a comment left during a code review.
Path: drizzle/0054_shiny_quentin_quire.sql
Line: 1:1

Comment:
the database setting `enable_codex_session_id_completion` is added but is never checked or used anywhere in the codebase. Searching for this setting reveals it only exists in the migration files. This means:
1. The feature cannot be disabled even though a toggle was created for it
2. The setting serves no purpose in its current state
3. There's likely missing code that should check this setting before calling `CodexSessionIdCompleter.complete()`

The integration code should check this setting (probably from system_settings table) before applying session ID completion

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Contributor

@github-actions github-actions bot Jan 12, 2026

Choose a reason for hiding this comment

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

[CRITICAL] [STANDARD-VIOLATION] Database setting added but never used

Why this is a problem: The migration adds enable_codex_session_id_completion to system_settings, but this setting is never checked in the code. This violates CLAUDE.md's requirement that migrations must be generated from schema changes, and indicates the toggle functionality is non-functional.

Evidence:

  • CLAUDE.md line 54-62: "IMPORTANT: Never create SQL migration files manually. Always follow this workflow: 1. Modify schema - Edit src/drizzle/schema.ts"
  • Searched codebase for enable_codex_session_id_completion - only found in migration and snapshot, never used in TypeScript code
  • The CodexSessionIdCompleter.complete() method has no conditional check for this setting

Suggested fix:

// 1. First add to src/drizzle/schema.ts:
export const systemSettings = pgTable('system_settings', {
  // ... existing fields
  enableCodexSessionIdCompletion: boolean('enable_codex_session_id_completion')
    .notNull()
    .default(true),
});

// 2. Then in session-completer.ts:
import { systemSettingsRepo } from "@/repository/system-settings";

static async complete(
  keyId: number,
  headers: Headers,
  requestBody: Record<string, unknown>
): Promise<CodexSessionIdCompletionResult> {
  const settings = await systemSettingsRepo.getSettings();
  if (!settings.enableCodexSessionIdCompletion) {
    return {
      applied: false,
      action: "noop",
      sessionId: null,
      fingerprint: null,
      redis: { used: false, hit: false },
    };
  }
  // ... rest of existing logic
}

Copy link
Contributor

@github-actions github-actions bot Jan 12, 2026

Choose a reason for hiding this comment

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

[HIGH] [LOGIC-BUG] enable_codex_session_id_completion is added to DB but not usable from code

Why this is a problem: This migration introduces a new flag:

ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL;

…but there is no corresponding field in the Drizzle schema / SystemSettings types, and no runtime code reads it. That makes the flag effectively dead configuration and risks schema drift (future migration generation may try to revert it).

Suggested fix:

// src/drizzle/schema.ts (systemSettings table)
enableCodexSessionIdCompletion: boolean("enable_codex_session_id_completion").notNull().default(true),
// src/types/system-config.ts
export interface SystemSettings {
  // ...
  enableCodexSessionIdCompletion: boolean;
}

export interface UpdateSystemSettingsInput {
  // ...
  enableCodexSessionIdCompletion?: boolean;
}
// Example usage once wired (e.g. src/app/v1/_lib/proxy/session-guard.ts)
const settings = await getCachedSystemSettings();
if (settings.enableCodexSessionIdCompletion) {
  await CodexSessionIdCompleter.complete(keyId, session.headers, session.request.message);
}

Loading
Loading