Two cases to read the code. I know everything is nuanced, skills vary, and the scope of what we’re building varies. I’m aware even Kent Beck recently said “TRY to automate not reviewing the code”. This is just 2 examples where reading the code is required, even today, with multiple + wonderful LLM support tooling.
I’m reviewing a mid-level dev’s PR who’s not on our team. First concern is its on our most important repo with high consequence of failure. The work could be piloted on a less risky repo, but she chose this one. She may not be aware of that context. Nowhere in the docs does it state that for the LLM’s to know, more is there a “map between repos” for our team for the LLM to have context. Should we build that? Yes. Do we have that now for the LLM? No. Does the dev know this? No. So I explain all that. She had no idea. She questioned the entire approach. This is the good part of iterative development; failing quickly; reducing the time it takes to learn. This is normal and good.
Second concern is it’s quite large. Not just many files, but many new files, some are quite long. I explain even without Github Stacks, we should be doing small PR’s with few changes and architected for a smaller blast radius. She appreciated the review; apparently getting PR reviews, from a human, are rare in some teams. :: face palm :: She had never heard of Github stacks or even just branching off of a branch; I explained the easier version if she wanted to avoid the github complexity rabbit hole that is gh stacks. Newer LLM’s have some of this in their training data, but unless you specify it, I’ve not seen them recommend it unless the repo has rules.
Third, approaching the work this was new for her. I don’t know what team experiences she had, nor where they are in in their CD journey, but I explained we don’t really plan that out. The whole point is to take small steps, learn, and use those learnings to inform where we should go next. So I don’t really plan or even know what the next PR’s will be, their size, nor the files. That said, I looked at her original PR and gleaned 3 smaller PR’s with only a few changes each that might form a great exercise to learn how to work in small steps since she had already trod this path once AND it’d be easier for me to have a more focus discussion on architecture since the changes are small. The LLM’s can help here if you know how to ask; I don’t think she even know you could ask such a thing yet.
Fourth, there were various architecture challenges, many of which she’d have no context unless she was on our team. Much of the code base was OOP, little dependency injection, heavy use of mocks, little types, and a consistent use of exceptions to drive responses. I’m attempting to move the team towards a more functional, typed, and errors as values approach. LLM’s _really_ struggle with navigating this as each integration has a flurry of options, and unless you narrow each one, it’ll just best guess. This is compounded in Brownfield code bases. This takes 110% of my focus just to help navigate this large space frigate through an asteroid field while drinking my coffee using sketchy inertial dampeners. There’s no way a Junior or Mid-level could both navigate this, know all the options, nor the vision not being on our team. I suggested we pair program on this so I could show how this vision has challenge, and what to do when you’re presented with a crossroads of architecture decisions to lead by example.
Fifth, one architecture one that was prevalent was a lack of DRY around a new framework they are using. It also uses exceptions, but checks the 5 types via instanceof; kind of like their own version of Promise.then/catch. The issue is all the parsing code is on the developer; since they didn’t use error types, you get error:unknown, and about 20+ lines of type narrowing code ensues in at least 4 call sites. At first I’m like “use types” to prevent all this. Then I was like “well, geez… ok, now you need a Zod schema to at least parse to one of the error shapes you’re expecting”. Then I was like “wait a minute, you’re doing this all over the place, why isn’t the framework handling this all for you via abstractions and types?”. Errors as types, types as values, Zod schemas parsing to known shapes, and leaky abstractions are all new concepts to her, and when you throw all of these at once, it’s quite overwhelming. We had a good brief discussion on perhaps making some new PR’s to the framework itself to help here since many others are going to run into the same implementation challenge. The framework has 200% docs, rules, and skills setup for LLM’s to be mostly Spec Driven Development style, but none of that was linked here in our repo, in her rules, or in however this was coded, LLM or not, I suppose. Again, the LLM is having to integrate this framework which has a different style into our repo which has 2 styles, with very little guidance on what to do at crossroads, so you need human in the loop guidance here.
Sixth, the shotgun parsing and lack of an anti-corruption layer was heavy, which in turn caused a lot of shotgun parsing because the maybe’s/optional’s were everywhere. In Brownfield, that’s fine, you just slowly add the parsing higher up, leaving it in a better place than you found it. But in the Greenfield code we’re adding, you have an opportunity to do things better, while still interfacing with the old in clear boundaries. So I explained Parse, Don’t Validate, Shotgun Parsing, and how you “design for the types you want, not what you’re given by some back-end” and how Zod can handle all this in colocated code. Again, it’s a lot throw at someone who’s never heard of all this a cohesive concept. LLM’s, even Opus 3.5, has all this in its training data. It may not be aware of Zod Codecs or the encoding angle, but even ghetto-fabulous SWE 1.6 can pick that up quickly if you give it a golden example. You have to be clear, though, about all of those rules, AND how to integrate them, (e.g. use Zod .transform/.pipe to map from downstream Data Transfer Objects, aka DTO’s ,to our domain Value Objects aka VO’s, so nice types come out).
Seventh, the LLM she was using read our code and started throwing custom class exceptions. The miss was we have a mapping layer at the top to ensure the responses match what we tell the user, so both the exception has to be shaped a certain way, the tests have to check for specific id’s, AND placement has to be thought out because since there are no types, the tests have to be aware of what level this is thrown from to test it. I explained we’re moving away from this approach to Railway programming using Neverthrow to have typed errors. I still can’t tell if this landed, but I know LLM’s can help you a ton here; I still use LLM’s to help debug Promise/ResultAsync chains when the return type isn’t requite right; they have an uncanny ability to fix it, even if you use the wrong explicit return type. Errors as Values is a huge mind shift for those used to imperative coding, especially those not from Go or Rust or FP languages. Especially tricky is the boundaries we have to map back to exceptions so our harness to “read all the errors and translate to the correct user response”. Easy with a .map, hard if you’re like “why do I need a .map?” LLM’s can grok that part easily, what’s hard for them is identifying the correct boundary. I don’t know all the boundaries, nor does she; we’d have to explore together to figure them out.
Eighth, and this is extremely common among TypeScript devs not from an FP background, the types are more for spelling, not for doing any type of Type Driven Development, or “if it compiles, it works” philosophy. So I had to give multiple ideas for certain sections on what could be done to offload to the type system instead of numerous tests, mocks or not. Some situations were impossible, and I had to give a couple of examples to show how you can model it so those impossible situations don’t occur anymore. There were a few switch statements that had default in them, and I had to explain how exhaustiveness checking in TypeScript works, and how the LLM’s will tell you to use never, but ignore them, and how some of that wasn’t her fault because the data they were using was draw DTO’s with optionals and nulls, and some approaches on how to possibly refactor and fix that. The LLM’s are extremely aware of how to type well, and they can easily bounce between OCAML pragmatism eschewing currying, to full Scala (feels like Java) where you don’t need tests “because we typed the universe”. You have to know those styles, AND the verbosity that TypeScript entails, AND figure out where your team falls in that extremely long spectrum, AND what to type now vs later given TypeScript is gradually typed. All of that, and if you’re learning this? Oh man, it’s harder in TypeScript because it’s so verbose compared to other languages, so the LLM (or me) has to know the level you’re at to teach it. Whew!
Had I not reviewed the code:
– the blast radius was high for this PR, with no easy ability to identify what would have caused it given so many changes
– condoning this behavior, human or LLM, sets a precedent that’s ok to make large PR’s. It’s not ok, and I will not condone it.
– would have missed so many wonderful opportunities to teach; I even got a message saying “Thanks for the review, those are rare nowadays” which about made my heart sink; happy she appreciated the attention, but this should be the norm to teach, not to be a meat proxy and just “have the LLM approve it”
– she now knows of all of this to better drive the LLM; before she wouldn’t even think to ask, and the code style + lack of rules-in-repo ensured the LLM would never volunteer alternatives
I got to help teach, she got setup for success, we both learned how to better drive the LLM’s. THIS is why I read the code.
2nd case is a lot simpler. Junior has a small PR to move a service to a new one with a feature flag. This new service is GraphQL, not REST, but the idea is the data will come back and map to our same domain objects. The issue, however, is despite using Zod in the new code, he’s still got Shotgun Parsing going on. If Zod safeParse works, he returns void, and continues on vs. using that safe-to-use domain data. There’s a fundamental understanding of how data validation should work, how TypeScript type narrowing works, and an LLM misunderstanding of how our code base wants you to do things given it’s brownfield and is lacking repo rules.
Again, I get to level him up, and we both learned how we could better articulate to the LLM how to do things, both in prompts and rules, and adding yet another golden data set to give to LLM’s for the future. THAT again, is why I have to read the code.
Yes, LLM’s are helping me. I’ve got Claude and Codex running my 3 Domain Driven Design, Unit Test, and Architecture review skills as well as SWE 1.6 in Windsurf. We have an Enterprise LLM code review that I’ve contributed to as well when I saw a core review was a bit off. So all hands are a deck for this, and helping. None of them, though, know the purpose, or does the Mid-level dev nor Junior know some of these fundamentals in architecture to ASK the LLM’s for help. Together, we all get smarter, and help the LLM’s help us more.
Yes, there are cases where I’m not reading the code; libraries that we use, integration points that never change, infra configurations that a multitudes of red tape and cyber checks to ensure they work and are safe. However, I’m erring on the side of read all of it because I’m consistently seeing code and architecture I wouldn’t want to be on call for in production, there are so many wonderful opportunities to teach and learn myself, misunderstandings from both Dev’s and LLM’s, and all of these help me refine the LLM’s to help me, and hopefully others self-teach, in this regard.
Finally, the most important part, is to learn. Are these requirements correct? Does this architecture allow future change more easily? Is this safe, or will we get dinged by cyber? I could go on and on; so far, the LLM’s aren’t catching all of these things, or are confused on when to apply my rules correct to all situations.
Leave a Reply