10 ms·

To me, the thing that stands out about the whole state we're in here is PR review. Yes, in an ideal world, PRs read well, are a joy to review, reflect what you

by rglynn 2mo ago

To me, the thing that stands out about the whole state we're in here is PR review. Yes, in an ideal world, PRs read well, are a joy to review, reflect what you discussed etc etc. We have to be real; there is only so much we can do to that end. I'm not sure how the best teams do PR review, from my perspective it sucks. I'm talking specifically about the UX. I've always hated Github's PR page, so I typically reviewed by pulling down the branch and opening the diff with $EDITOR. These days I think there's really no excuse for the awful UX. Linear (a company that isn't even in the domain of code review) put out a basic PR review feature[0] that is already better than what GH offers. It's simple: point a small model at the PR, group file changes together based on theme, add some commentary and sort by importance (schema changes > openapi spec). Immediately, so much mental load has been reduced without the reviewer or the requester doing anything. This feature is pretty damn basic, and I think there are obvious next steps like generating visualisations which a dedicated product could find the time to implement. Keen to hear others thoughts on why this is the wrong approach, or if there are tools in wide use that solve for this, or why this isnt the right problem to focus on. 0 - https://linear.app/docs/diffs#guides https://linear.app/docs/diffs#guides

xorcist 2mo ago
> group file changes together based on theme, add some commentary Isn't that what commits are? Or ... should be?
- lozenge 2mo ago
  No, commits always display the files in a fixed order, and then display changes from line 1 to line N. An AI could select the order to display in, add per hunk commentary and automatically adjust how much context lines are displayed.
  - xorcist 2mo ago
    But if you need to split the commit in hunks, and add a commentary per hunk, isn't that just a sign that you really should just split your commits? That's the most killer feature of git, that it's so easy to slice your commits any way you desire, and then redo again. The use case of taking a chunk and commit separately is so common it even got a special mode in the add command. That, and the super fast jumping between branches is what set it apart from contemporary version control systems. The extra context provided by the review tool is gone when the review is done anyway. Review systems come and go, but the commit log is for eternity.
2001zhaozhao 2mo ago
I think the right shape is to review and merge directly from the agent window.
- 0xblacklight 2mo ago
  reviewing code as it's being written & re-steering it > reviewing it once 20k lines have been written
  - 2001zhaozhao 2mo ago
    Yep i aim to split my work into tickets that are <=1k lines of code each. In the end it all gets squashed into a mega 10k commit regardless but i can keep track of the quality much more that way. (more precisely the sub-tickets merge into a bigger feature branch, which in turn gets squash merged into the main branch at the end. i have a tool that automates this flow with a gui and ticket management.)
4lx87 2mo ago
Gating integration behind code review is futile. I (and many other engineers) already automated it. My agent responds to review requests and reviews as me. Company policies enforcing human code review are futile. I think all these platforms chasing code review are doomed. My LLM doesn't need any of this tooling. We should be reviewing the actual working software. Systems that make it easy and instant to demo any proposed change are what is needed. Code (and specs) are going to fade into obscurity. PR review has already shifted towards validating the product (working software) over the process (code). The future of software production is more like Replit – not GitHub.
- tripleee 2mo ago
  What's your job as an "engineer" in this post-automated world? QA?
  - 4lx87 2mo ago
    Producing features and fixing bugs, same as it was before. The organizational process of software development has not changed much with AI: execs decide direction and initiatives, PMs decide what to build, which is broken down into features and bug fixes that SWEs produce. In my experience organizations don't actually care how SWEs produce features, except insofar as it relates to how many and how fast the features can be pumped out. Organizations see code review as a process to prevent bugs. Humans are not as good as LLMs at reviewing code for bugs. Abstract notions of code style and quality that programmers care about is not why organizations enforce code review.
    - necovek 2mo ago
      Before LLMs, "organizations" have seen code review as multiple things: * Share knowledge about a particular area of the codebase between multiple people * Share overall engineering knowledge between the parties in the review * Ensure maintainability of the codebase long-term * Ensure readability of the code * Catch architectural/directional omissions (yes, from the planning/architecture phase) before it's really too late and non-reversible change goes in (eg. large destructive DB schema change) * Ensure changes are small, self-contained, and as often as possible, reversible * Do some basic manual QA * Do comprehensive integration testing with a fully built-out system ... * And yes, catch bugs before they hit production. A lot of the above could be fun and engaging, and especially knowledge sharing and ensuring maintainability/readability was a very motivating thing for me as a more experienced engineer having learned so much from getting good reviews when I was less experienced. Programmers care about style to ensure readability and thus maintainability of the code, but also to keep changes minimal — if every diff converted between tabs and spaces in the entire codebase, it'd be impossible to see what has really changed with the simple tooling we generally use (one could build diffing tools that ignore changes like these, and they even exist, but are not ubiquitous).
tcoff91 2mo ago
PRs sucked to review long before Agents were a thing, but now it really sucks because there are more to review.
- dboreham 2mo ago
  Right on. In many ways AI has just exposed how bogus and delusional our industry's "process" has been for years.
- viccis 2mo ago
  Like you said, they sucked back then but it was more rare to get PRs of many thousand lines to review multiple times a day in my experience. But I also run into a problem now in which I have coworkers in two camps. There are some that, when they send me a PR and I have serious concerns about it, it's because they disagree with me fundamentally on some concept and we need to chat and discuss it. And then there's some coworkers who send me with a PR I have serious concerns about because it's very clear they never reviewed the LLM output before making a PR. Those people also tend to take my comments, run them against their LLM session, and slack me back some Claude list-of-questions stuff, and often also respond to me with LLMese. I firmly believe there's no reason to employ the latter group. I have several coworkers like this that I could replace with a Claude session or two every week and I'd still have to push back on work and review a lot, but I'd still wind up saving time because then I'd talk directly to Claude. So I guess my point is that I dislike AI era code review because for some subset of it, I basically can't even assume a human has ever looked at it, and that by doing so, I'm doing the work for one of my coworkers who is probably doing his grocery shopping at 2PM as I read it.
  - sevenseacat 2mo ago
    I received two 15,000-line PRs the other day, from the same person. I am currently ignoring them.
    - tcoff91 2mo ago
      I very much remember whose PRs I can trust and whose PRs are likely to be slop, and I prioritize my time accordingly. Slop merchants get slop reviews. I just point my clanker at their PRs and ask them to critically review the PR and they typically find lots of issues.
WorldMaker 2mo ago
> I'm not sure how the best teams do PR review, from my perspective it sucks. I'm talking specifically about the UX. I've always hated Github's PR page, so I typically reviewed by pulling down the branch and opening the diff with $EDITOR. When $EDITOR = VSCode there's a shortcut on the GitHub PR page: if you type a . it opens in github.dev in a VSCode instance.
boron1006 2mo ago
I just dont think LLMs are very good at judging importance or summarizing code. I tried experimenting with what is ultimately a treesitter based approach - https://github.com/0x007BA7/codebook https://github.com/0x007BA7/codebook And really liked it. Definitely nowhere near production ready but I think theres room for a player to come in and do something similar.
- dhorthy 2mo ago
  yeah that was another thing i hoped would pour through here - that deterministic systems are much better for evaluating quality (test, linters, cyclomatic complexity, etc) - but that we don't have such a system for code maintainability, at least not one that's widely accepted or adopted
claytonjy 2mo ago
thanks for pointing out the linear PR stuff, hadn’t seen that. Interesting that while a dozen other companies are trying to muscle in on the hosting/versioning side of github, rather fewer are working on the PR side.
danpalmer 2mo ago
PR review sucks because engineers often don't optimise their code authoring for it. When they do it's a joy. PR review of agent generated code sucks because agents are, in my experience, very poor at authoring for review. This makes sense because the review process is not represented in the resulting code artifact which is what they're trained on. Agents are always producing much larger changes than should be in a single step, and often touch unrelated code with poor judgement of whether it should be included or not.
- latentsea 2mo ago
  Agents are also controllable to a degree. How agents author PRs is now an engineering problem.
  - danpalmer 2mo ago
    But this is my point, they're not really controllable in this way. There are no fixed rules (like tests or linters) for small PRs, it's not good enough to do it by line count. A good PR tells a story to the reader that leaves them confident in the result. That means knowing the reader and how they think, and that is something LLMs are very bad at.
    - dhorthy 2mo ago
      yes well said
    - latentsea 2mo ago
      And you can't think of a way to bring an engineers mindset to solve that? If you leave it up to raw LLM outputs you're not going to get the results you're after. If you put on your engineering hat and think about your problem statement and what tools you can bring to bear on it you'll find you can likely craft a PR authoring skill and a set of evals for that skill that satisfies your requirements with a high enough degree of reliability.
      - danpalmer 2mo ago
onion2k 2mo ago
I'm not sure how the best teams do PR review.. They don't do them. They discuss the changes as the team are doing them (software design and architecture), they automate all the things that would be nitpicks (linting, formatting, etc), and they adhere to a strict 'Do not break the build' rule so merging in something can't be disastrous (with a lot of automated checks and tests to prove that's true). They also make sure there's a robust rollback process just in case. Once you do all that the PR process is pointless. It never catches anything useful. The team can trust one another to merge without that gate.
- kkapelon 2mo ago
  > Once you do all that the PR process is pointless manual PR reviews can catch things that llms currently miss. Examples are duplicated code, lack of unit tests or introducing security issues. None of these really break the build. So just requiring "do not break the build" is a very low barrier. Tests also are useless unless you have a smart system that runs the new test WITHOUT the changes and see it break. Most teams I see today have the LLM write a test along with the change, without a guarantee that the tests actually guard the feature.
  - onion2k 2mo ago
    manual PR reviews can catch things that llms currently miss I didn't say LLMs though. I'm talking about the sort of code quality tooling that's been in use for decades - Sonarqube, Codacy, CodeClimate, etc for code quality, eslint for AST-based code rule checking (easily picks up duplication for example), Istanbul for code coverage, Wiz or Github Advanced Security for security. These are well known industry standards for knowing a code change is correct without giving it to an LLM to guess at. If you have them, and they're well configured, and you maintain the tooling properly, then PRs become unhelpful gates that don't find problems.
    - kkapelon 2mo ago
      I am not familiar with all the tools you listed so please correct me if i am wrong, but all these will catch stuff that LLMs can potentially catch as well (if configured correctly). They will not handle any architectural problems (i.e. this method is correct but doesn't belong on this package, or this method is called isX - but has side effects). And I am pretty sure that none of them do what I am saying with the tests. i.e. run tests without the associated code change and see them fail. I am also pretty certain that none of them will understand problems with breaking backward compatibility (i.e., a fix that is correct that breaks the setup of all existing users). In other words, it is possible today to create a PR that passes all checks, all security scans, all analysis tools, all test suite while still being wrong due to architecture, backwards compatibilty, wrong scope etc. So even before and even after LLMs a human is needed there. PRs might become unhelpful as you say if in the future one of the two things we happen 1) LLMs have unlimited context so you can pass all source code plus all architectural designs them 2) We have a super smart analysis tool that is one level above what we have today (including llms) We are not there yet.

by rglynn 2mo ago

xorcist 2mo ago

> group file changes together based on theme, add some commentary Isn't that what commits are? Or ... should be?

lozenge 2mo ago
No, commits always display the files in a fixed order, and then display changes from line 1 to line N. An AI could select the order to display in, add per hunk commentary and automatically adjust how much context lines are displayed.
- xorcist 2mo ago
  But if you need to split the commit in hunks, and add a commentary per hunk, isn't that just a sign that you really should just split your commits? That's the most killer feature of git, that it's so easy to slice your commits any way you desire, and then redo again. The use case of taking a chunk and commit separately is so common it even got a special mode in the add command. That, and the super fast jumping between branches is what set it apart from contemporary version control systems. The extra context provided by the review tool is gone when the review is done anyway. Review systems come and go, but the commit log is for eternity.

2001zhaozhao 2mo ago

I think the right shape is to review and merge directly from the agent window.

0xblacklight 2mo ago
reviewing code as it's being written & re-steering it > reviewing it once 20k lines have been written
- 2001zhaozhao 2mo ago
  Yep i aim to split my work into tickets that are <=1k lines of code each. In the end it all gets squashed into a mega 10k commit regardless but i can keep track of the quality much more that way. (more precisely the sub-tickets merge into a bigger feature branch, which in turn gets squash merged into the main branch at the end. i have a tool that automates this flow with a gui and ticket management.)

4lx87 2mo ago

Gating integration behind code review is futile. I (and many other engineers) already automated it. My agent responds to review requests and reviews as me. Company policies enforcing human code review are futile. I think all these platforms chasing code review are doomed. My LLM doesn't need any of this tooling. We should be reviewing the actual working software. Systems that make it easy and instant to demo any proposed change are what is needed. Code (and specs) are going to fade into obscurity. PR review has already shifted towards validating the product (working software) over the process (code). The future of software production is more like Replit – not GitHub.

tripleee 2mo ago
What's your job as an "engineer" in this post-automated world? QA?
- 4lx87 2mo ago
  Producing features and fixing bugs, same as it was before. The organizational process of software development has not changed much with AI: execs decide direction and initiatives, PMs decide what to build, which is broken down into features and bug fixes that SWEs produce. In my experience organizations don't actually care how SWEs produce features, except insofar as it relates to how many and how fast the features can be pumped out. Organizations see code review as a process to prevent bugs. Humans are not as good as LLMs at reviewing code for bugs. Abstract notions of code style and quality that programmers care about is not why organizations enforce code review.
  - necovek 2mo ago
    Before LLMs, "organizations" have seen code review as multiple things: * Share knowledge about a particular area of the codebase between multiple people * Share overall engineering knowledge between the parties in the review * Ensure maintainability of the codebase long-term * Ensure readability of the code * Catch architectural/directional omissions (yes, from the planning/architecture phase) before it's really too late and non-reversible change goes in (eg. large destructive DB schema change) * Ensure changes are small, self-contained, and as often as possible, reversible * Do some basic manual QA * Do comprehensive integration testing with a fully built-out system ... * And yes, catch bugs before they hit production. A lot of the above could be fun and engaging, and especially knowledge sharing and ensuring maintainability/readability was a very motivating thing for me as a more experienced engineer having learned so much from getting good reviews when I was less experienced. Programmers care about style to ensure readability and thus maintainability of the code, but also to keep changes minimal — if every diff converted between tabs and spaces in the entire codebase, it'd be impossible to see what has really changed with the simple tooling we generally use (one could build diffing tools that ignore changes like these, and they even exist, but are not ubiquitous).

tcoff91 2mo ago

PRs sucked to review long before Agents were a thing, but now it really sucks because there are more to review.

dboreham 2mo ago
Right on. In many ways AI has just exposed how bogus and delusional our industry's "process" has been for years.
viccis 2mo ago
Like you said, they sucked back then but it was more rare to get PRs of many thousand lines to review multiple times a day in my experience. But I also run into a problem now in which I have coworkers in two camps. There are some that, when they send me a PR and I have serious concerns about it, it's because they disagree with me fundamentally on some concept and we need to chat and discuss it. And then there's some coworkers who send me with a PR I have serious concerns about because it's very clear they never reviewed the LLM output before making a PR. Those people also tend to take my comments, run them against their LLM session, and slack me back some Claude list-of-questions stuff, and often also respond to me with LLMese. I firmly believe there's no reason to employ the latter group. I have several coworkers like this that I could replace with a Claude session or two every week and I'd still have to push back on work and review a lot, but I'd still wind up saving time because then I'd talk directly to Claude. So I guess my point is that I dislike AI era code review because for some subset of it, I basically can't even assume a human has ever looked at it, and that by doing so, I'm doing the work for one of my coworkers who is probably doing his grocery shopping at 2PM as I read it.
- sevenseacat 2mo ago
  I received two 15,000-line PRs the other day, from the same person. I am currently ignoring them.
  - tcoff91 2mo ago
    I very much remember whose PRs I can trust and whose PRs are likely to be slop, and I prioritize my time accordingly. Slop merchants get slop reviews. I just point my clanker at their PRs and ask them to critically review the PR and they typically find lots of issues.

WorldMaker 2mo ago

> I'm not sure how the best teams do PR review, from my perspective it sucks. I'm talking specifically about the UX. I've always hated Github's PR page, so I typically reviewed by pulling down the branch and opening the diff with $EDITOR. When $EDITOR = VSCode there's a shortcut on the GitHub PR page: if you type a . it opens in github.dev in a VSCode instance.

boron1006 2mo ago

I just dont think LLMs are very good at judging importance or summarizing code. I tried experimenting with what is ultimately a treesitter based approach - https://github.com/0x007BA7/codebook https://github.com/0x007BA7/codebook And really liked it. Definitely nowhere near production ready but I think theres room for a player to come in and do something similar.

dhorthy 2mo ago
yeah that was another thing i hoped would pour through here - that deterministic systems are much better for evaluating quality (test, linters, cyclomatic complexity, etc) - but that we don't have such a system for code maintainability, at least not one that's widely accepted or adopted

claytonjy 2mo ago

thanks for pointing out the linear PR stuff, hadn’t seen that. Interesting that while a dozen other companies are trying to muscle in on the hosting/versioning side of github, rather fewer are working on the PR side.

danpalmer 2mo ago

PR review sucks because engineers often don't optimise their code authoring for it. When they do it's a joy. PR review of agent generated code sucks because agents are, in my experience, very poor at authoring for review. This makes sense because the review process is not represented in the resulting code artifact which is what they're trained on. Agents are always producing much larger changes than should be in a single step, and often touch unrelated code with poor judgement of whether it should be included or not.

latentsea 2mo ago
Agents are also controllable to a degree. How agents author PRs is now an engineering problem.
- danpalmer 2mo ago
  But this is my point, they're not really controllable in this way. There are no fixed rules (like tests or linters) for small PRs, it's not good enough to do it by line count. A good PR tells a story to the reader that leaves them confident in the result. That means knowing the reader and how they think, and that is something LLMs are very bad at.
  - dhorthy 2mo ago
    yes well said
  - latentsea 2mo ago
    And you can't think of a way to bring an engineers mindset to solve that? If you leave it up to raw LLM outputs you're not going to get the results you're after. If you put on your engineering hat and think about your problem statement and what tools you can bring to bear on it you'll find you can likely craft a PR authoring skill and a set of evals for that skill that satisfies your requirements with a high enough degree of reliability.
    - danpalmer 2mo ago

onion2k 2mo ago

I'm not sure how the best teams do PR review.. They don't do them. They discuss the changes as the team are doing them (software design and architecture), they automate all the things that would be nitpicks (linting, formatting, etc), and they adhere to a strict 'Do not break the build' rule so merging in something can't be disastrous (with a lot of automated checks and tests to prove that's true). They also make sure there's a robust rollback process just in case. Once you do all that the PR process is pointless. It never catches anything useful. The team can trust one another to merge without that gate.

kkapelon 2mo ago
> Once you do all that the PR process is pointless manual PR reviews can catch things that llms currently miss. Examples are duplicated code, lack of unit tests or introducing security issues. None of these really break the build. So just requiring "do not break the build" is a very low barrier. Tests also are useless unless you have a smart system that runs the new test WITHOUT the changes and see it break. Most teams I see today have the LLM write a test along with the change, without a guarantee that the tests actually guard the feature.
- onion2k 2mo ago
  manual PR reviews can catch things that llms currently miss I didn't say LLMs though. I'm talking about the sort of code quality tooling that's been in use for decades - Sonarqube, Codacy, CodeClimate, etc for code quality, eslint for AST-based code rule checking (easily picks up duplication for example), Istanbul for code coverage, Wiz or Github Advanced Security for security. These are well known industry standards for knowing a code change is correct without giving it to an LLM to guess at. If you have them, and they're well configured, and you maintain the tooling properly, then PRs become unhelpful gates that don't find problems.
  - kkapelon 2mo ago
    I am not familiar with all the tools you listed so please correct me if i am wrong, but all these will catch stuff that LLMs can potentially catch as well (if configured correctly). They will not handle any architectural problems (i.e. this method is correct but doesn't belong on this package, or this method is called isX - but has side effects). And I am pretty sure that none of them do what I am saying with the tests. i.e. run tests without the associated code change and see them fail. I am also pretty certain that none of them will understand problems with breaking backward compatibility (i.e., a fix that is correct that breaks the setup of all existing users). In other words, it is possible today to create a PR that passes all checks, all security scans, all analysis tools, all test suite while still being wrong due to architecture, backwards compatibilty, wrong scope etc. So even before and even after LLMs a human is needed there. PRs might become unhelpful as you say if in the future one of the two things we happen 1) LLMs have unlimited context so you can pass all source code plus all architectural designs them 2) We have a super smart analysis tool that is one level above what we have today (including llms) We are not there yet.