Fix revert handling for tool calls in the same step as the reverted edit - #522
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Posted 1 actionable inline review finding. |
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AshishKumar4
changed the title
|
Performance is inconclusive because every cohort is marked non-comparable due to changed eval definitions. Across three trials each, pass counts stayed 0/3, 0/3, and 3/3, while mean durations fell 8.9%, 6.5%, and 36.9%, respectively. Project-doc most concretely skipped a README read more often, with mean tool calls dropping 4.67 to 4.33 and cost dropping $0.0111 to $0.00845. With only three stochastic trials, these differences could be sampling variation. |
|
Ignore the last bonk comment |
30a5be3 to
f4991e7
Compare
|
No additional actionable findings beyond the existing inline review comments. |
|
Performance is inconclusive because all cohorts are non-comparable after eval-definition changes. Appointment stayed 10/10, expense-ledger fell 7/10 to 5/10, and project-doc rose 8/10 to 9/10. Most notably, expense-ledger used 38.5% fewer tool calls yet took 30.7% longer. With 10 stochastic trials per cohort, sampling variation may explain these mixed results. |
f4991e7 to
7f8922c
Compare
github-actions
Bot
added
workshop/frontend
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7f8922c to
82e3640
Compare
AshishKumar4
changed the title
github-actions
Bot
removed
workshop/frontend
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 1 actionable inline review comment. |
kentonv
left a comment
kentonv
left a comment
There was a problem hiding this comment.
I think this is actually buggy, but I also think maybe we can just remove this entirely?
|
Performance is inconclusive because all cohorts are non-comparable after eval-definition changes. Across 10 trials each, appointment fell from 10/10 to 9/10, while expense-ledger and project-doc each improved by one pass; mean durations fell 6.8%, 39.9%, and 8.8%, respectively. Expense-ledger showed the clearest tool-use shift, dropping from 26.8 to 23.4 calls and 1.2 to 0.7 errors, with cost down 36.8%. These modest pass-rate changes and efficiency gains could reflect stochastic variation. |
82e3640 to
c7aa651
Compare
|
|
Performance is inconclusive because every cohort is non-comparable after eval-definition changes. Appointment stayed 10/10, expense-ledger rose 7/10 to 8/10, and project-doc rose 7/10 to 9/10. Project-doc improved without changing mean tool use at 4 turns, 5 calls, and 0 errors, while mean durations fell 12.2%, 1.8%, and 1.1%, respectively. With 10 stochastic trials per cohort, these differences could reflect sampling variation. |
c7aa651 to
7eaffa7
Compare
|
LGTM! |
|
Performance is inconclusive because all cohorts are non-comparable after eval-definition changes. Across 10 trials, appointment stayed 10/10, expense-ledger stayed 6/10, and project-doc rose from 7/10 to 9/10. Mean duration rose 33.0% for appointment but fell 9.6% for expense-ledger and 7.4% for project-doc. Expense-ledger showed the clearest tool-use improvement, with turns, calls, and errors falling from 16.8/19.7/1.5 to 11.6/15.4/0.1, though stochastic variation could explain the result. |
When the user rewinds an agent step, replay replaces that step's readFile results with a note, so the model doesn't keep believing content that no longer exists. The check looked only at the message holding the tool calls. But a step's edits land in a "changes" message written after that one, with any action, useGadget or connectionRequest records of the step in between, and the rewind starts at the changes message. So a read in the same step as an edit was never marked and replayed with the reverted content. The check now also finds the step's changes message, past those records, and consults its status. It stops at the next assistant message or at a user's own changes, since a step that made no edits has no changes message and a later one must not be taken for it. The writeFile and editFile un-marking added in #513 is removed: the revert message already tells the model what was undone, and un-marking was not intended. The test writes a file, reads it back, and requests a connection in one step (the request record sits between the tool-call message and the changes message), rewinds the step, and asserts the replayed read is elided. It fails without the fix, and it fails with a check that looks only at the adjacent message. The scripted model can now answer one request with several tool calls, and the session driver gains revertChanges, both for the test.
7eaffa7 to
f72f35b
Compare
|
LGTM! |
2 participants
When the user rewinds an agent step, replay replaces that step's
readFileresults with a note. The check only looked at the message holding the tool calls. A step writes more than that though: the tool-call message, then anyaction,useGadgetorconnectionRequestrecords from the step, then a "changes" message with the edits. The rewind starts at the "changes" message, so a read in the same step as an edit was never marked, and replay showed the reverted content as current.Concretely: the agent writes
secret = 42and reads the file back in the same step. The user rewinds. Next turn, the model still seessecret = 42.The check now finds the step's "changes" message, past any of those records, and consults its status. It stops at the next assistant message or at a user's own changes, since a step that made no edits has no changes message and a later one must not be taken for it.
The
writeFile/editFileun-marking from #513 is removed, per the review: the revert message already says what was undone.Found while adding
grepin #494, which records its output the same way.The test writes, reads, and requests a connection in one step, so the request record sits between the tool-call message and the changes message. It rewinds the step and checks the replayed read is elided. It fails on
main, and it fails with a check that only looks at the adjacent message. To write it I had to let the scripted model return more than one tool call per step and addrevertChangesto the test session driver.