Skip to content

Feat: CXA-1831 Persist latest model and reasoning effort in sqlite#14859

Merged
shijie-oai merged 4 commits intomainfrom
shijie/cxa-1831-1
Mar 17, 2026
Merged

Feat: CXA-1831 Persist latest model and reasoning effort in sqlite#14859
shijie-oai merged 4 commits intomainfrom
shijie/cxa-1831-1

Conversation

@shijie-oai
Copy link
Collaborator

@shijie-oai shijie-oai commented Mar 16, 2026

Summary

The goal is for us to get the latest turn model and reasoning effort on thread/resume is no override is provided on the thread/resume func call. This is the part 1 which we write the model and reasoning effort for a thread to the sqlite db and there will be a followup PR to consume the two new fields on thread/resume.

part 2 PR is currently WIP and this one can be merged independently.

@shijie-oai shijie-oai changed the title Persist latest model and reasoning effort in sqlite Feat: CXA-1831 Persist latest model and reasoning effort in sqlite Mar 16, 2026
Comment on lines +1 to +2
ALTER TABLE threads ADD COLUMN model TEXT;
ALTER TABLE threads ADD COLUMN reasoning_effort TEXT;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Capture the latest model and reasoning effort per thread.

@shijie-oai shijie-oai force-pushed the shijie/cxa-1831-1 branch 2 times, most recently from ced5beb to 8bb84a0 Compare March 17, 2026 04:31
Comment on lines +52 to +66
impl FromStr for ReasoningEffort {
type Err = String;

fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"none" => Ok(Self::None),
"minimal" => Ok(Self::Minimal),
"low" => Ok(Self::Low),
"medium" => Ok(Self::Medium),
"high" => Ok(Self::High),
"xhigh" => Ok(Self::XHigh),
_ => Err(format!("invalid reasoning_effort: {s}")),
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A simple string to enum parser so that read from db can be correctly converted. Better to keep such parser on the enum def side so that we can reuse down the line.

}

impl FromStr for ReasoningEffort {
type Err = String;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generic string type error - open to have a custom error type if necessary but feel like an overkill.

@shijie-oai shijie-oai marked this pull request as ready for review March 17, 2026 05:01
@shijie-oai
Copy link
Collaborator Author

@codex review

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c992e04eaa

ℹ️ 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".

Copy link
Collaborator

@charley-oai charley-oai left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@@ -0,0 +1,2 @@
ALTER TABLE threads ADD COLUMN model TEXT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@charley-oai if there is a migration here, maybe good to package yours in the same release

}

#[tokio::test]
async fn apply_rollout_items_without_turn_context_persists_null_model_and_reasoning_effort() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the 3 tests in this file are a bit redundant. Can we merge them please?
We shouldn't have test for every combinatorial possibility and try to build smarter tests instead. The CI pipeline is already pretty slow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

@shijie-oai shijie-oai requested a review from jif-oai March 17, 2026 16:18
@shijie-oai shijie-oai merged commit 8e258eb into main Mar 17, 2026
33 checks passed
@shijie-oai shijie-oai deleted the shijie/cxa-1831-1 branch March 17, 2026 17:14
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants