Skip to content

Conversation

@gpeal
Copy link
Collaborator

@gpeal gpeal commented Oct 28, 2025

This PR adds an option to app server to allow conversation summaries to be fetched from just the conversation id rather than rollout path for convenience at the cost of some latency to discover the rollout path.

This convenience is non-trivial as it allows app servers to simply maintain conversation ids rather than rollout paths and the associated platform (Windows) handling associated with storing and encoding them correctly.

@gpeal gpeal requested a review from bolinfest October 28, 2025 21:02
@gpeal gpeal changed the title [App Server] Allow fetching a conversation summary from the conversation id [App Server] Allow fetching or resuming a conversation summary from the conversation id Oct 28, 2025
@gpeal gpeal force-pushed the gpeal/conversation-id-summary branch from cb78efd to a5b38e1 Compare October 28, 2025 22:55
@gpeal gpeal requested a review from pkomlev October 28, 2025 22:56
pub rollout_path: PathBuf,
#[serde(untagged)]
pub enum GetConversationSummaryParams {
/// Provide the absolute or CODEX_HOME‑relative rollout path directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a relative path, we should require that it is "normalized," i.e., no funny stuff with ../ to read paths outside $CODEX_HOME/sessions.

Come to think of it, we should probably resolve the path to a canonical (or at least absolute without hitting disk: https://crates.io/crates/path-absolutize) and then verify it is under $CODEX_HOME/sessions, which is simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This just documents the existing behvaior, do you want me to update that as part of this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gpeal It can be done in a separate PR, but I think it's important that it's done.

Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

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

Nice!

@gpeal gpeal merged commit 1d76ba5 into main Oct 29, 2025
25 checks passed
@gpeal gpeal deleted the gpeal/conversation-id-summary branch October 29, 2025 00:17
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2025
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