Defer persistence of rollout file#11028
Conversation
- Defer rollout persistence for fresh threads (`InitialHistory::New`): keep rollout events in memory and only materialize rollout file + state DB row on first `EventMsg::UserMessage`. - Keep precomputed rollout path available before materialization. - Change `thread/start` to build thread response from live config snapshot and optional precomputed path. - Improve pre-materialization behavior in app-server/TUI: clearer invalid-request errors for file-backed ops and a friendlier `/fork` “not ready yet” UX. - Update tests to match deferred semantics across start/read/archive/unarchive/fork/resume/review flows.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0101f2c2ad
ℹ️ 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".
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Code generally seems good by me, though the inherent complexity in supporting the laziness feels a bit unsatisfactory since anything dealing with rollouts needs to defend against the case where nothing may have hit disk yet? But if this is important to support, I suppose it can't be avoided...
| }) | ||
| } | ||
|
|
||
| fn open_log_file(path: &Path) -> std::io::Result<File> { |
There was a problem hiding this comment.
I don't think async makes sense in this case. Let me know if you're seeing something I'm not or feel strongly about it.
There was a problem hiding this comment.
I think this has been removed in the revised version, but I suggested this because I thought it was called from an async function (doing I/O).
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
I think we should avoid having rollout recorder from interpreting items and knowing when to "commit". Instead it should be an explicit gesture from the agent loop.
3 participants
InitialHistory::New): keep rollout events in memory and only materialize rollout file + state DB row on firstEventMsg::UserMessage.thread/startto build thread response from live config snapshot and optional precomputed path./fork“not ready yet” UX.For Reviewers:
Testing:
/forkin TUI generates visible error if used prior to first turn