This looks surprisingly good! I can see the quick sanity checks being very useful in cases like the mismatched env variables. That said, since you probably already know what works well, here's some constructive criticism:
First, I'm pretty unimpressed with the PR descriptions. I'd be frustrated if my company adopted this and I started seeing PRs like the one from continuous-eval that you linked to first. It's classic LLM output: lots of words, not much actual substance. "This PR updates simple.py", "updates certain values". It's the kind of information that can be gleaned in the first 5 seconds of glancing through a PR, and if that creates the illusion that no more description is needed then we'll have lost something.
Second, in the same PR: when writing a collection to a jsonl file, I would expect an empty collection to give me an empty file, not no file. Further, I haven't looked at the rest of the context, but it seems extremely unlikely to me that dataset_generator.generate would somehow produce non-serializable objects, and a human would easily see that. These two suggestions feel at best like a waste of time and at worst wrong, and it's concerning to me that the habits this tool encourages led to the suggestions being uncritically adopted and incorporated and that this seemed to you to be a good example of the tool in use.
The second PR you linked to is, I think, a better example, but I'm still not sold on it. Similar to the PR descriptions, I'm concerned that a tool like this would create an illusion of security against the "simple" problems (leaving reviewers to focus on the high level), whereas I'd hope that the human reviewer would still read every line carefully. And if they're reading every line carefully, then have we really saved them that much time by paying for an LLM reviewer to look over it first? Maybe the time it takes to type out a note about a misnamed environment variable.
Similar to the PR descriptions, I'm concerned that a tool like this would create an illusion of security against the "simple" problems (leaving reviewers to focus on the high level), whereas I'd hope that the human reviewer would still read every line carefully.
PR descriptions should be about intention, not what changed (what changed is in the PR itself), so I agree there.
With the other stuff though, if you treat it as a linter, that might be fine? The problem is when you justify getting it as it'll save you engineering time, but it won't actually do that significantly unless you have a lot of people using it.
Comments
This looks surprisingly good! I can see the quick sanity checks being very useful in cases like the mismatched env variables. That said, since you probably already know what works well, here's some constructive criticism:
First, I'm pretty unimpressed with the PR descriptions. I'd be frustrated if my company adopted this and I started seeing PRs like the one from continuous-eval that you linked to first. It's classic LLM output: lots of words, not much actual substance. "This PR updates simple.py", "updates certain values". It's the kind of information that can be gleaned in the first 5 seconds of glancing through a PR, and if that creates the illusion that no more description is needed then we'll have lost something.
Second, in the same PR: when writing a collection to a jsonl file, I would expect an empty collection to give me an empty file, not no file. Further, I haven't looked at the rest of the context, but it seems extremely unlikely to me that dataset_generator.generate would somehow produce non-serializable objects, and a human would easily see that. These two suggestions feel at best like a waste of time and at worst wrong, and it's concerning to me that the habits this tool encourages led to the suggestions being uncritically adopted and incorporated and that this seemed to you to be a good example of the tool in use.
The second PR you linked to is, I think, a better example, but I'm still not sold on it. Similar to the PR descriptions, I'm concerned that a tool like this would create an illusion of security against the "simple" problems (leaving reviewers to focus on the high level), whereas I'd hope that the human reviewer would still read every line carefully. And if they're reading every line carefully, then have we really saved them that much time by paying for an LLM reviewer to look over it first? Maybe the time it takes to type out a note about a misnamed environment variable.
PR descriptions should be about intention, not what changed (what changed is in the PR itself), so I agree there.
With the other stuff though, if you treat it as a linter, that might be fine? The problem is when you justify getting it as it'll save you engineering time, but it won't actually do that significantly unless you have a lot of people using it.