[onion2k]

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]

> 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]

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]

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.

[spacebanana7]

> They discuss the changes as the team are doing them (software design and architecture)

I find the format of PRs to be better suited to discussion than slack/teams messages. Key developers need to actively sign off, code suggestions are much easier, and the CICD makes sure we're not wasting time discussing code that's not compliant with obvious standards.

Obviously some level of agreement is needed before creating a ticket at all, but implementation details around say creating a terraform module to share functionality between teams do need to be discussed somewhere - and it can be difficult for any individual dev to have all the low level context.

[rglynn]

Thank you for this response. I do think this hits home as the most elegant and reasonable approach. For my teams currently, I can see us already moving toward this implicitly, but perhaps needing to take a more explicit approach. I think your framing crystalises it well.