Add review code command - #28
Conversation
|
|
| // Keep the enabled state in sync when active cell or cell content changes. | ||
| notebookTracker?.activeCellChanged.connect((_, cell) => { | ||
| if (cell) { | ||
| ensureInitialSource(cell); |
There was a problem hiding this comment.
We should probably keep initial source only on first opening the document, as it reflect the instructor document as it was sent.
IMO, if the student create a new cell, there should not be initial source.
There was a problem hiding this comment.
I updated the code to run ensureInitialSource only when the notebook document is first loaded. New student cells no longer have an initial_source automatically added to them. If a student requests a review on a cell without an initial_source, the tutor will evaluate the code cell directly.
EDIT: I removed it completely for now, I think it's better to have this feature in a separate PR.
| formattedBody += `\n\n<evaluation_criteria>\n${evaluationCriteria}\n</evaluation_criteria>`; | ||
| } | ||
| if (initialSource && typeof initialSource === 'string') { | ||
| formattedBody += `\n\n<diff>\n${computeDiff(initialSource, source)}\n</diff>`; |
There was a problem hiding this comment.
We should probably stick to the initial source.
Setting up our own comparison tool could be a source of errors, whereas the LLM should handle it quite easily.
There was a problem hiding this comment.
I reverted it back to use initial_source and I've removed computeDiff from utils.ts and updated back formattedBody to include the <initial_source> block instead of .
However it's still worth noting that some models (at least mistral/ministral-8b-latest which I'm using to test) are very inconsistent and seem to not be able to spot small differences in the code.
Here's what I mean, in this screencast, I fixed a one letter typo in the code cell and asked for review, and the model was only able to see I fixed the bug 3 out of 5 times:
Screencast.from.2026-08-07.13-11-28.mp4
This is the reason I implemented a TS diff function: to avoid the randomness of LLMs and make the review process more deterministic. I'm not sure what exactly LLMs do when you ask them to compare text, maybe the stronger models are better at this. I think I'll open an issue/PR to look into how I can improve this.
There was a problem hiding this comment.
Thanks for testing and opening the issue.
It looks good to handle it in a separate PR 👍
| except StreamClosedError: | ||
| pass | ||
|
|
||
| def _filter_prompt_for_action(self, raw_prompt: str, action: str) -> str: |
There was a problem hiding this comment.
This will be overwritten if we move to a template.
Maybe for this PR we should keep the whole content for now, but add the expected action in the formatted body instead.
Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>
|
Great, thanks @brichet for reviewing this! |
2 participants
This PR adds a new "Review Code" command in the cell toolbar which sends the code cell to the agent to review the changes made by the user.
Key Changes:
-Added
computeDiff()function inutils.tsto generate the diffs between initial_source and current code (source). This is better than giving the LLM the initial code and the current code and letting it make out the diff itself as that's not deterministic like it is this way and, from what I tested, the LLM often can't accurately make out the diff on its own.-Added a
<diff>tag to the prompt payload sent to the LLM, containing this computed diff. It's now sent instead ofinitial_sourcebecause of the aforementioned reasons, but it could still be useful to sendinitial_sourcetoo, for now I just removed it for 'economical' reasons.-Added
Review Codecommand and its button next to theExplain Codebutton in the cell toolbar.-Auto-initializes
initial_sourcecell metadata on load if missing. Since this metadata is necessary for theReview Codefeature to function properly, if it's missing, then we just save the current content asinitial_source.-The
Review Codebutton is disabled if thediffis empty, i.e if the student hasn't yet made any changes. In this case, there is nothing to review and I found that this disabling it in this case is simpler and better than telling the agent how to handle it.Screencast.from.2026-07-24.12-52-41.mp4