-
Notifications
You must be signed in to change notification settings - Fork 5k
Review Mode (Core) #3401
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.
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
Review Mode (Core) #3401
Conversation
There was a problem hiding this 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 suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of not necessary clone()
Handling the memory in Rust is not easy but we should not take the habit to clone everything as it can have a large impact on performances at large scale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments. The main one (but non-blocking) is around removing is_review_mode from the session and encapsulating it in the Task::review()
overload.
This is a great feature! Can't wait to see how it performs. It looks like the review thread only gets custom instructions + whatever input the /review is called with. If /review is used without any input would it make sense to give the thread a curated view of recent activity? i.e. recent user instructions and perhaps just the patches applied or recent tool calls? Or does this not make sense given how the review models are trained? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall layering looks good, please address @jif-oai's comments before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the pipeline but LGTM 👍
📝 Review Mode -- Core
This PR introduces the Core implementation for Review mode:
Op::Review { prompt: String }:
spawns a child review task with isolated context, a review‑specific system prompt, and aConfig.review_model
.EnteredReviewMode
: emitted when the child review session starts. Every event from this point onwards reflects the review session.ExitedReviewMode(Option<ReviewOutputEvent>)
: emitted when the review finishes or is interrupted, with optional structured findings:Questions
Why separate out its own message history?
We want the review thread to match the training of our review models as much as possible -- that means using a custom prompt, removing user instructions, and starting a clean chat history.
We also want to make sure the review thread doesn't leak into the parent thread.
Why do this as a mode, vs. sub-agents?