A case study in engineering judgment
How to think about a production feature
General engineering judgment, shown through two weeks of review on one AI feature at Adbrew.
Contents
Prologue: one feature, two weeks
What this is, who is in it, and how to read it.
In September 2026, an engineer at a small ad-tech company built a feature. A more senior engineer reviewed it. The review ran for about two weeks across seven or eight conversations: some forty minutes long, one eight minutes long because the demo broke.
Most of the code worked from the first day. That isn't what the two weeks were about. They were about everything around the code: who owns which piece, what counts as reuse, what gets tested and how, what is stored and for how long, which questions the engineer should settle alone, and which ones needed someone else in the room.
That second list is what separates code that runs on your laptop from code that lives in production for years. Personal projects teach you the first part well. The second part is usually learned slowly, by sitting in reviews like these. This essay tries to speed that up.
The company is Adbrew. It helps sellers run advertising on Amazon. You don't need to know anything about Amazon ads to follow along. When the domain matters, I'll explain it in a sentence.
How it's organized. Part one describes the feature and walks through the questions a production engineer asks between "we should build this" and "it's live." Part two is the core: twelve short chapters, each built on one idea. Each chapter states the idea, explains why it matters in production, tells the story from the reviews where it came up, and ends with a takeaway you can use on your own projects. Part three is about people: what to decide yourself and what to bring to someone who has been around longer. A checklist closes the essay.
Dates and class names show up where they help the story. They aren't the point. If you forget every class name but remember the questions, the essay did its job.
The two weeks at a glance (optional)
- 2 Sept
- Original design review. "Is this engine really shared?" Leaky layers, empty wrappers, and two loops doing one job.
- 7 Sept
- Cleanup walkthrough. The duplication is gone. "Can you migrate chat later?" "Yes, with testing."
- 8 Sept
- Two sessions. How a recommendation becomes something you can apply, validators reused, testing strategy, storage, infrastructure.
- 12 Sept
- The apply path has started changing core chat. Blocked until the chat owner is involved.
- 15 Sept
- Reuse the conversation manager, draft-then-publish, freshness belongs to the insight, plugin handlers.
- 16 Sept
- Freshness moved to its owner. One design call is left to the implementer. Guiding tenets are written down.
- 17 Sept
- A different product ships by reusing only one layer of the new engine.
Part one · The feature
1The feature in plain English
A chat assistant learns to work while nobody is asking it anything.
Adbrew already had a live chat assistant. A seller types "why did my ad spend jump last week?" An AI agent reads the account's performance data through a set of read-only tools, thinks, calls more tools, and answers. Sometimes it proposes a change, like "lower the bid on these four keywords." That proposal appears in the chat as an interaction: a small structured card the seller can approve, and the change goes out to Amazon.
The new feature was AI Smart Recommendations. It uses the same intelligence, but nobody has to ask. A scheduled job wakes up, analyzes an account, and writes a handful of insight cards: "These targets spent $400 with no sales in 14 days. Consider pausing them." Each card has an Apply button. Clicking it opens a chat sheet that already contains the proposed change, ready to approve. Users can also dismiss a card, and the system remembers that so it won't keep suggesting the same thing.
On the surface, that's a scheduled job, a prompt, and some cards. The hard part was the ambition behind it.
The team didn't want a second AI agent. They wanted one brain shared by every product: live chat, the background recommendations, and later a separate generator for Amazon's display-ads platform (DSP) and an analytics product (AMC). If the brain gets better at reasoning or calling tools, every product should benefit. If a bug gets fixed, it should be fixed once.
Picture a restaurant whose dining room (live chat) is busy and doing well. Now it wants to start catering (scheduled insights). It could build a second kitchen, which is quicker at first. Six months later, the dining-room chef has improved the sauce and the catering kitchen is still serving the old one. Or it can run catering out of the same kitchen with a different kind of order ticket. That's harder up front, because the kitchen was designed around a dining room. The two weeks of review were about making the second option real and not just a label.
Here is the shape the team settled on. Don't memorize it. Skim it now and come back to it later. Each box gets its own chapter later on.
Around this sit the product-specific pieces: an insight service that owns insight cards, an agent service that creates chats, a thin seeder that opens a chat with its first messages, and the infrastructure that runs the job on a schedule.
2Idea to production: twelve questions
The thought process, start to end, before any chapter goes deep.
When a feature is proposed, experienced engineers don't start with code. They run through a set of questions, often without noticing. Here is that set made explicit, with what each question looked like for this feature. The rest of the essay expands on them one at a time.
- What is the user-visible noun? Not "an AI job." The user sees an insight card, an Apply button, and a chat sheet that opens with a change ready to approve. Start with the things users can point at. Every one of them needs an owner.
- What already exists that this must not break? Live chat, which paying customers use every day. The validators inside an existing diagnosis tool. A usage dashboard that reads token counts. Make the list before you write code, not after the bug report.
- What is shared kernel, and what is this product's shell? The reasoning loop and model adapter are shared. The prompt, tool list, and job limits belong to this product.
- What is the contract between the pieces? A recommended change from the AI must turn into exactly the same interaction payload that chat already knows how to apply.
- Who owns each noun, including its freshness and persistence? The insight service owns "is this insight still true?" The conversation manager owns "save this message." The seeder owns very little, on purpose.
- What is the write path, and does it special-case the core? Clicking Apply creates a chat. Does that need new branches inside core chat code? (It started to. That was the most expensive part of the review.)
- How does it run in production? A scheduler, a queue, and a worker, plus time limits, a JSON output schema, one repair attempt, and a forced finish when the budget runs out.
- How will we know it works? End-to-end tests with the AI provider mocked and everything else real, regression tests for every chat action, and an evaluation framework before shipping anything that judges AI output.
- What do we store, for how long, with which indexes? Applied insights are kept indefinitely as a quality record. Pending ones are kept without an index for now, to be revisited once there is data.
- What must someone senior or a core owner decide? Anything that changes how core chat behaves. Anything that depends on where the company is going.
- What ships now, and what ships later? The adapter now, the shared loop later. The validator is ready, but merging waits for the evaluation framework. The LLM gateway service isn't being built yet.
- What would the tenth product need from this design? If product number ten has to edit core code to plug in, the design isn't finished.
You won't answer all twelve on day one, and you shouldn't try. Some answers depend on data that doesn't exist yet. The point is to know which questions are still open, so that "we'll decide later" is a deliberate choice and not something you forgot.
Part two · The ideas
3Shared kernel, product shells
Draw the boxes, then ask who would change each knob.
The idea
Once a platform has more than one product, its code splits into two kinds. The kernel must behave the same for every caller: how the model is called, how tools run, how the reasoning loop proceeds. The shells are product-specific choices: which prompt, which tools, how long a job may run, what the output looks like.
The test for which side something belongs on is simple: who would want to change this, and why? If the answer is "the chat team, to improve chat," it belongs in a chat shell. If the answer is "anyone who finds a bug in tool calling," it belongs in the kernel.
Why it matters in production
When the kernel is duplicated, fixes land in one copy. When product logic leaks into the kernel, every product starts paying for one product's quirks. Both failures are quiet. Nothing crashes. The two copies just drift apart a little more each month.
From the review
The first review didn't find bugs. It found four shapes that each hinted the boxes were drawn in the wrong place.
- Tools passed twice. The caller had to give the tool list to two different layers. If two layers both need the same thing from you, they disagree about who owns it. That's a leaky abstraction.
- An empty wrapper. A function called
run_tool_loopdid nothing except callrunner.run(). A layer that adds a name but no decision is just one more place to read. - Conversation and telemetry code inside the loop. The brain was also saving chat messages. (Chapter 5 covers this.)
- Two loops. The old chat loop was still running alongside the new shared one. Ayush's words, roughly: "Right now that PR is counter-evidence. It shows reuse is not happening."
The fixes were mostly about placing each knob in the right box:
- Each product gets a generator: its prompt, its model, and a curated, frozen list of allowed tools. A lookup table maps each platform to its generator. Sponsored Ads gets one, and DSP gets its own later, on the same engine.
- The allowed tools are a fixed, read-only list and not "whatever chat has." That's deliberate. When the chat team adds a new tool next quarter, possibly one that writes, a scheduled job running unattended across thousands of accounts shouldn't pick it up silently.
- The tool registry is built from the adapter's tools, so the two lists can't drift. Generate one from the other, and you never have to keep two in sync by hand.
- Reasoning effort (how hard the model thinks) moved out of the generator and into the adapter/runner. It's about how the model is called and how much time the job has. It's not part of what the product is asking for.
Even names got attention. model_event became model_chunk because it is a chunk of a stream, not an event. build_tool_adapter became tool_adapter_factory because that is the pattern it implements. Names are the cheapest documentation you'll ever write.
The ownership map that came out of the two weeks fits in a small table. Compare it with your own projects:
| Question | Owner |
|---|---|
| Which prompt, model, and tools for this product? | Generator |
| How do we talk to the model provider? | Model adapter |
| Think, call tools, think again | Agent loop |
| How long may a job run? What if the output is malformed? | Headless runner |
| Save chat messages and token counts | Conversation manager (as a listener) |
| Create a chat, add its first messages and interaction | Agent service |
| Open a chat with a given starting state | Seeder (kept thin) |
| Is this insight still true? | Insight service |
| How much did this run cost? | Generation service (product bookkeeping) |
| How does core chat behave? | The chat owners |
Draw your boxes. For every setting, flag, and list, ask who would want to change it and why. If you have to pass the same thing to two layers, or a layer adds a name but makes no decision, the boxes are in the wrong place.
4Reuse is a caller test
A module isn't shared because you extracted it. It's shared when a second caller calls it.
The idea
Moving code into a folder called shared/ proves nothing. Code is reusable once something other than its first user depends on it and works. Until then, call it what it is: extracted.
Why it matters in production
A "shared" module with one caller is shaped entirely by that caller. Its interfaces fit that caller's needs, its defaults are that caller's defaults, and its hidden assumptions are that caller's assumptions. When a second caller finally shows up, it often finds the module doesn't fit. Then it forks the module, or bends it with special cases, and you end up with two kitchens after all.
From the review
Akshit's natural order was to ship the new product first, insights on the new engine, and migrate live chat afterward. That's a reasonable order for shipping. It's a risky order for proving a design, because the proof keeps being pushed back.
The walkthrough looked good. The duplicate loop was gone, the empty wrappers were gone, the existing prompt builder was reused instead of a hand-rolled second one, and model settings were in one place. Then the reviewer asked: "Are you confident you can migrate chat later?"
"Yes, with testing."
That's an honest answer. It's also a forecast, not evidence. Ayush had said it directly a few days earlier: he couldn't judge whether the design was right "unless I see how it is used there." He wasn't asking for the chat migration to ship. He was asking to see it, so the design could be judged against a real second caller.
The proof he wanted was concrete: live chat calling the shared agent_loop.stream() and its old hand-written while loop deleted. Through the 16 September review, that had been claimed but not shown.
Then the proof came from somewhere else.
The AMC team (Amazon's analytics product) needed to get their agent to production on the newer model API. They took only the model adapter from the new engine. Not the loop, not the runner, not the files around them. The instruction was explicit: don't copy extra files from the old streaming session code, or the change becomes impossible to regression-test. Joining the shared loop was planned for later.
That's adapter first, loop second, and it says more about the design than any walkthrough did. One layer could be lifted out alone, by a team that didn't build it, and put into production without pulling the rest along. That's what a correct layer boundary looks like from the outside. It also showed that reuse can come in stages, one layer at a time, with each stage proven before the next.
When you say "this is reusable," name the second caller and when it will call. Showing that caller working, even on a branch, is worth more than any amount of "yes, with testing." Partial reuse of one layer by a real caller is good evidence that your layer boundaries are right.
5Persistence and telemetry are listeners
The loop thinks. Other things take notes.
The idea
A reasoning loop has one job: think, call tools, think again, until it's done. Saving the chat message, recording how many tokens were used, emitting a latency metric: these happen because the loop did something. They aren't part of how the loop does it. They should listen to the loop through callbacks, not live inside it.
Why it matters in production
A background job has no chat. If saving chat messages is built into the loop, the job has to either fake a chat or grow if headless: branches throughout the brain. Both are bad. With listeners, each caller plugs in what it needs. Chat attaches a conversation manager. The job attaches its own recorder. The loop doesn't know either one exists.
From the review
The first version had conversation-saving code inside the loop. Ayush's question was simple: "Why are conversation manager things here? Why are you not providing a callback?" The loop should call something like record_response, and whoever is listening decides what that means.
The same question came up about cost. The generation service was computing a cost breakdown from model tokens and cached tokens. Ayush's point: the raw numbers come from the loop, so every caller needs them, and the loop should just hand them out. What the insight job does with them, recording a run cost for its own bookkeeping, belongs to the generation service. The generic part goes to everyone. The product-specific part stays with the product.
By 7 September, telemetry was a shared emitter that each caller creates for itself: tagged with a run ID for jobs and a chat ID for chat. An unused flag for streaming output was deleted instead of kept around "just in case."
One small detail from that walkthrough is worth taking home. The telemetry records a hash of the prompt template used for each run.
Here's why. Three months from now, someone says the recommendations feel worse lately. The first question is always "did anything change?" With the hash stored, you can answer it with a single query: group runs by prompt hash and compare quality. Without it, you're digging through git history and guessing which deploy reached which run. It's a tiny piece of data that settles a whole category of arguments.
One more rule: telemetry has consumers. Adbrew had an internal usage page that reads token counts. If you move where token usage gets recorded, that page doesn't crash. It quietly shows zero. The 8 September session made this explicit: the LLM utilities, token usage, and every telemetry consumer must be tested after the refactor.
Keep your core loop about its one job. Saving, metering, and logging attach as listeners that each caller provides. Record which version of the prompt or config produced each result. And when you change where data is written, find out who reads it.
6One contract, two directions
And reuse the rules from the code that already enforces them.
The idea
When two parts of a system exchange data, the format between them is a contract. Contracts often run in two directions. You tell one side what it's allowed to produce, then you translate what it produced into what the other side consumes. Both directions should come from one definition. If they're defined separately, they will drift.
The waiter and the cook read the same order ticket. If the waiter's pad says "medium-rare" and the kitchen's ticket format has no field for it, the steak comes out wrong and nobody can say whose fault it was.
Why it matters in production
For this feature, the AI recommends changes, and a human approves them through the existing chat interaction. If the AI can recommend something the apply path can't handle, the user clicks Apply and gets an error, or worse, a slightly different change than the one the card described.
From the review
The demo failed because of a local git setup problem, so the meeting lasted eight minutes. It still produced one of the clearest designs of the two weeks: a two-way contract.
- In: tell the AI exactly which kinds of entities it may recommend changes to: an allow-list. (It was still hard-coded at that point, and everyone knew that was a debt.)
- Out: translate a recommended change into the exact payload chat's existing interaction tool already uses.
A single shared validation class covers both, so "a change chat already proposes" and "a change the job recommends" can't end up in different formats. A transfer to chat happens only when there's a chat sheet to put it in.
The reviewer's request was the most useful line of the meeting: open the class, and show me the input, then the output.
That request is worth remembering. A described contract is a story about the code. A walked-through file is the code. When you present a contract in review, put a real input on screen, run it, and show the real output. Gaps you can talk around in a description become obvious when there's actual data on screen.
Extract the rules from the tool that already knows them
The rules for what counts as a valid campaign, target, or product change already existed. They lived inside the performance-diagnose tool that chat used. Instead of writing new validation for insights, Akshit pulled those validators out and reused them when an insight's recommended change was saved.
That created a new risk. Now "what the AI may recommend" and "what the apply path accepts" are tied to the same tools. If a tool is added or removed in one place and not the other, they fall out of sync. Tests have to guard exactly that edge. The next step agreed on was to replace loose dictionaries with a concrete object (the changes plus their target type), so nested cases have a clear shape rather than a convention.
Define the format once and use it in both directions. Get your validation rules from the code that already enforces them, not from a second copy. When you review a contract, show it running on real input. Once the shape stabilizes, replace loose dicts with a typed object.
7Testing what can actually break
Mock the expensive, flaky thing. Keep everything else real.
The idea
A test is valuable to the extent it would catch a real bug. Unit tests that mock every collaborator mostly test your mocks. For a system like this, the bugs live between the pieces: the loop hands the wrong shape to a validator, or a tool result is formatted differently than the payload expects.
So mock the one dependency that's expensive, slow, and nondeterministic, which here is the AI model provider. Keep the real loop, the real validators, the real payload building, and the real database.
Why it matters in production
If a test patches the database, it can't catch a query that's wrong. If a test patches the loop, it can't catch a loop that sends tool results in the wrong order. Every mock is a spot where the test has agreed not to look.
From the review
The direction was clear. Prefer an end-to-end test of the whole tool loop with the model provider mocked, the same approach the team already used for its Amazon Ads API mock. Avoid unit tests that exist only to raise coverage. Do not patch the database inside the end-to-end test.
Making something optional is a behavior change
The less obvious lesson came on 12 September. To make the Apply path work, some fields on chat actions, like bid and state, had become optional. Some "No targets provided" guidance messages that the model used to receive were also being removed.
Each of those looks like a harmless relaxation. Each one is actually a change to how every existing chat behaves. A field that used to be required now isn't, so the model may skip it, and the chat may do something it never did before. The review called this out: every chat action affected needs a regression test, because this isn't new behavior for insights. It's changed behavior for chat.
Another point from the same session: Akshit should check the edge cases himself, by looking at them, before handing ten scenarios to QA. QA is a second line of defense. The engineer who wrote the change knows where the edges are, and is the cheapest person to check them.
Some code waits for its evaluator
On 15 September, a shared validation class was ready. It had been checked against three months of production data and matched. It still didn't merge. The rule was that it waits until an evaluation framework exists. When code judges AI output, "it matched historical data once" isn't enough. You need a repeatable way to check that it keeps matching as prompts and models change. A ticket was opened for the evaluation work, and the validator waited.
Mock what's costly and random. Keep everything else real, especially the database. Treat every "now optional" or "message removed" as a behavior change for existing users, and write the regression test. Check your own edges before QA does. If code judges AI output, build the way to evaluate it before you merge.
8How a background job really runs
Scheduler, queue, worker, and a policy for when things go wrong.
The idea
On your laptop, a "background job" is a function you call in a loop. In production, it's usually three separate pieces:
The scheduler wakes up and puts one message on a queue for each account that needs work. It does no heavy work itself. The queue absorbs bursts, retries failures, and keeps one broken account from blocking the others. The worker picks up one message, does one account, and exits.
At Adbrew this was a scheduler Lambda, an insight-generator queue, and a processor Lambda, confirmed on 8 September.
Why it matters in production
A single process looping over every account is fragile. If account 312 throws an exception, accounts 313 through 5,000 don't run. If the process takes longer than its time limit, you can't tell where it stopped. Splitting the work so each account is its own unit of retry turns one large failure into a few small, visible ones.
A job needs a policy, not just a loop
Chat has a human watching. If the model rambles, the user stops it. A background job has nobody watching, so the headless runner carries the rules a human would otherwise enforce:
- Limits. A maximum number of tool calls and a maximum time. The job can't think forever.
- A schema. The final answer must be structured JSON that matches the insight-card shape.
- One repair. If the JSON is malformed, the runner asks the model to fix it once. Not in a loop. Exactly once.
- Force-finalize. When the budget runs out, the runner tells the model to write its answer now with what it has, instead of discarding all the work so far.
None of these rules belong in the shared loop. Chat doesn't want a forced finish. They belong to the job's shell.
The AMC session ran into a physical limit. With a newer reasoning model, a single turn could take two to three minutes, and the API stack cut requests off at two minutes. Options discussed: a priority service tier from the provider to cut latency, and reusing the provider's stored reasoning context between turns (via a previous-response ID) so the model doesn't start from scratch each time.
There was a catch on that last one. The provider deletes stored responses after roughly 30–60 days, so it's a performance cache, not storage you can rely on. Someone also floated a dedicated LLM gateway service to centralize all of this. It's a one-to-two-month project, and the decision was not now. More on that in chapter 15.
Split scheduled work into scheduler, queue, and worker, one unit per retryable item. Give unattended jobs an explicit policy: limits, schema, one repair, forced finish. Find your infrastructure's timeouts before you choose a slow model, and know which provider features are caches and which are storage.
9Thin at the edge, fat in the owner
The host who seats you doesn't cook your dinner.
The idea
New features usually enter a system through a small piece of glue: a seeder, a handler, an adapter. That glue is tempting because it's yours and it's new. You understand it, and nobody else is reviewing it closely. So logic piles up there.
Keep the glue thin. It should say what it needs and hand the work to whatever already owns that work. The owner is "fat": it holds the real logic, which is shared and tested.
Why it matters in production
Logic in the glue gets duplicated. The glue's version of "add a message to a chat" is the second copy of that operation in the codebase. The first copy gets a bug fix next month and the second one doesn't. Eventually the two disagree about what a chat looks like.
From the review
To make Apply work, changes had spread into the base chat agent, the chat processor, and a new "partially applied" state. A senior reviewer who joined that day said plainly that he wouldn't approve core chat changes made this way, and that this needed the chat owner, Omkar. (Chapter 15 picks this up.)
The redesign that came out of it was mostly about drawing a thin edge:
- The seeder says only two things: "these are the opening messages" and "this is the interaction."
- The agent service, which already owns chats, does the actual work: creates the chat, sets up telemetry, adds the system and user messages, and adds the synthetic interaction.
The new seed_chat function had its own prompt building, its own system messages, and its own message-append logic. All three already existed in the conversation manager. The direction was to reuse it.
There was a performance worry: adding messages one at a time costs two or three database calls. Should there be a new batch API? The answer: two or three calls is fine. Measure the latency before inventing an API to save it. A new batch method is new code to maintain, and the problem it solves hadn't been shown to exist.
Before writing logic in your new glue code, search for who already owns that operation and call it. Keep the glue to "what," and let the owner handle "how." Don't optimize a cost you haven't measured, especially by adding a new API.
10Whoever owns the noun owns its freshness
An insight from Monday, clicked on Thursday.
The idea
Data gets stale. The job writes an insight on Monday: "lower this bid from $1.20 to $0.90." The user clicks Apply on Thursday. By then, someone may have changed the bid by hand to $0.95. Is the insight still valid? Partly? Not at all?
Answering means comparing three values: old (what the insight saw), proposed (what it recommends), and current (what's true now). The rule: that comparison belongs to whoever owns the noun. Here that's the insight service, because only it knows what the insight meant.
Why it matters in production
If freshness is computed somewhere else, it depends on that other place's internals. That other place will change for its own reasons, and your freshness check breaks without anyone touching it.
From the review
The first staleness check lived in a proposal-preparation policy, and it read values the chat enricher had written to the database. That worked. But the enricher was a candidate to be replaced by an MCP-based service. If that happened, the staleness check would break even though nobody had touched it.
The fix was to move freshness to its owner. The insight service compares old vs. proposed vs. current itself.
The stale check moved in-house, with no hard-coded fields, and was added to existing classes rather than new ones. The flow became: the agent service asks for a freshness assessment, then calls the seeder and passes the interaction and tool-call metadata along. The result, fresh, stale, or needs review, is stored on the chat. That stored value drives a Review button on the card, so users can see when something changed before they apply it.
Any time the user acts on something computed in the past, someone has to compare old, proposed, and current. Put that comparison in the service that owns the thing, and have it read current state from a stable source, not from another component's internals. Then show the result to the user.
11Writes that can fail halfway
Draft, then publish. And find out how often it actually happens.
The idea
Creating a seeded chat isn't one write. It's several: create the chat, add a system message, add a user message, add the synthetic interaction. If the third write fails, the user sees half a chat, which is confusing at best and broken at worst.
There are a few textbook answers: database transactions, a new all-at-once batch API, or careful cleanup on failure. There's also a simple one most systems already have: draft, then publish. Create the chat marked as a draft. The UI ignores drafts. When every write has succeeded, clear the flag. If something fails, the draft stays invisible and can be cleaned up later.
Why it matters in production
Multi-step writes fail halfway more often than you'd expect: timeouts, deploys mid-request, a worker killed for memory. But the right fix depends on how often it actually happens. A transactional system for a one-in-a-million failure costs more than it saves.
From the review
On the 15th the direction was clear: don't invent a new flow. Create the chat as a draft, let the UI filter it, and unmark it when complete. Chats already had the concepts needed.
On the 16th the question was framed more broadly: first estimate the probability of partial or broken chat states, then decide how much engineering to spend on them. Optimizing a failure mode you haven't sized is the same mistake as building a batch API for latency you haven't measured.
For multi-step writes, reach for draft-then-publish before inventing transactions or new APIs. Check whether your system already has a draft or hidden state. Estimate how often the failure happens before deciding how hard to defend against it.
12Extension points that don't rot
Origin callbacks, typed plugins, and the flag that is really two functions.
This chapter collects three small design decisions with the same root: each was about how to let future products plug in without the core learning their names.
1 · Origin callbacks, not product-specific indexes
When a user applies an insight's change in chat, the insight should be marked "applied." The first version did this with special handling, including a database index that only existed for chats whose origin was an AI insight.
The redesign: every chat carries an origin saying where it came from. When an interaction is applied, the existing context updater looks at the origin and calls back to whoever owns it. Insights are one origin. Next year there might be five more. The core knows "chats have origins and origins get callbacks." It doesn't know the word "insight."
For the lookup itself, the 12 September guidance was to prefer an existence check or an application-level check over a Mongo index that served exactly one product's value. When indexes did come up again on 15 September, they were partial indexes on general concepts: chats indexed by origin, insights by applied or archived status.
2 · Typed handlers and a factory, not getattr and string imports
The context updater found its handlers dynamically: build a string, import a module by name, getattr a function. It works, but your editor can't follow it, your type checker can't verify it, and a typo becomes a runtime error in production.
The direction: an abstract base class that defines what a handler must implement, plus a factory that returns the right handler for an origin. Also, pass the chat object in. Don't make the handler fetch it again by ID. The caller already has it, and a second fetch is a second round trip and a chance to read different data.
3 · An optional flag that forks behavior is two functions
Akshit proposed letting the conversation manager accept an optional chat object. If one is passed, update that object in memory. If not, write to the database as usual.
The reviewer pushed back on the shape. An if chat is not None branch inside the manager means one function name now covers two different behaviors: two functions wearing one name. Every future reader has to work out which mode they're in, and every future change has to be tested both ways. He preferred a separate implementation.
Then he did something worth noticing: he left the decision to Akshit and said he wouldn't re-review it. More on why in chapter 15.
Let the core know general concepts (origin, handler, callback), never specific products. Prefer typed interfaces and factories to string-based lookup. If an optional parameter changes what a function does, rather than only how it's tuned, you probably have two functions.
13Storage: keep, index, or expire
Decide each kind of record separately, and decide with evidence.
The idea
Every record you write has a lifetime, whether you choose it or not. The three choices are keep forever, keep and index (so it stays fast to query as it grows), and expire (a TTL, so old records delete themselves). Different kinds of records in the same feature often deserve different answers.
Why it matters in production
Storage decisions are cheap to make early and expensive to change later. A TTL deletes data you might wish you had. An index slows every write and takes memory. Keeping everything unindexed is free until the collection gets large, and then every query gets slow at once. None of these is always right, and guessing early usually means guessing wrong.
From the review
The team sorted insight records by what they'll be used for:
- Applied and archived insights: keep indefinitely. They're the quality record: what the AI recommended, what humans accepted, and what happened afterward. That history is how you'll judge whether the feature is any good.
- Pending and dismissed insights: keep for now, with no index. Let the collection grow, then decide between a TTL and an index once there's real data about size and query patterns. A retention window of about six months for quality analysis was floated, not decided.
Dismissed records also had a live product use. Recently applied or dismissed insights go into a suppression window, so the next run doesn't recommend the same thing again. Storage choices are product choices too.
For each kind of record, ask what it will be used for and for how long. Keep what you'll learn from. It's fine to defer the index-vs-TTL decision on low-value records, as long as you've written down that you deferred it and what evidence will settle it.
14Room for ten future use cases
Leave room for them. Don't build them yet.
The idea
A good design for product number one also has a reasonable answer for product number ten. That doesn't mean building for ten. It means checking that product ten could plug in without editing the core.
The tension is real. Overbuild, and you ship a framework nobody asked for. Underbuild, and product two forks your code. The practical test sits between the two: imagine the tenth caller and ask what it would need to touch. If the answer is "add a new shell," you're fine. If it's "add an elif to the core," you're not.
From the review
By the end, the reviewer stated three guiding tenets out loud:
- No regression to existing behavior.
- Don't tightly couple insights and recommendations. They're related, not the same thing.
- Leave room for roughly ten future use cases.
You can see that third tenet in decisions spread across all two weeks:
- Generators per platform. DSP gets its own generator on the same engine. Adding a platform means adding an entry, not modifying the loop.
- Scoped account context. Saved account context is scoped by agent type, plus an "all agents" scope. Smart Recommendations got its own scope instead of borrowing the performance-diagnose context, so the two products can evolve independently.
- Origin callbacks (chapter 12). The tenth origin plugs in a handler.
- Adapter-first reuse (chapter 4). A product can adopt one layer without adopting all of them.
And the counterweight: the LLM gateway, a service that would centralize all model calls, was clearly a good idea for the long term. It would also take one to two months. It wasn't built. Leaving room for the future isn't the same as building it now.
Write down your tenets. Test your design against an imagined tenth caller: would it add a shell or edit the core? Build for the products you have, and leave the extension points for the ones you can see coming.
Part three · People
15Deciding, proposing, and waiting for the owner
The part no tutorial covers, because it's about people.
Everything so far has been about code. But the most important decisions in these two weeks weren't really technical. They were about who should make the call.
A useful way to sort any decision is into three buckets.
Bucket 1 · Decide it yourself
Implementation choices inside your own box, where both options are reasonable and the result is easy to change. Your reviewer can have an opinion. You still own it.
The reviewer thought the conversation manager change should be a separate implementation, not an if/else. He said so, explained why, and then left the call to Akshit, saying he wouldn't re-review it. He also said the context-updater change didn't need his review at all.
That's what earned trust looks like in a review. After two weeks of close review, the reviewer was handing decisions back. The goal isn't to need fewer reviews because you've learned to avoid them. It's to need fewer because your decisions have become predictable to the people who know the system.
Bucket 2 · Bring options, not a decision
Some decisions depend on where the company is going, and the person reviewing you knows that better than you do. That isn't because they're smarter. It's because they're in conversations you aren't in. For these, your job is to lay out the options clearly, with costs, and let them choose.
Examples from these weeks:
- Merge the ready validator, or wait for an evaluation framework? The code was done and had been checked against production data. Waiting was the right call, but it depended on how seriously the company planned to invest in AI quality, which was the reviewer's call to make.
- Build an LLM gateway now? Clearly useful, one to two months of work. Whether that's worth it now depends on the roadmap.
- TTL or index for pending insights? Deferred deliberately until there's data.
The decision: one sentence. Option A / Option B (rarely C): what it is, what it costs, what it makes easy later, and what it makes hard later. What I'd pick and why. What would change my mind.
Say which one you'd pick. Options without a recommendation hand your thinking off to the reviewer. A recommendation with options gives the reviewer something specific to agree with or correct.
Bucket 3 · The core owner must be in the room
Some code is the foundation other people build on. Changing its behavior affects every product that sits on it, and the person who owns it knows about plans, past incidents, and promises that aren't written down anywhere you'd look.
The apply path had grown into changes to core chat: the base agent, the processor, a new partially applied state, optional fields that changed existing actions. A senior reviewer looked at it and said, in effect: I won't approve core chat changes made this way. Talk to Omkar. A meeting with Omkar was set for Monday.
That wasn't a judgment of the code's quality. Some of the changes might have been fine. It was a judgment about process: core behavior was changing, and the person responsible for core behavior hadn't been part of the decision. From then on, Omkar reviewed the chat seeder, and validation work was coordinated with him.
How to avoid ending up there
The 12 September stop was predictable, and the reviewer had tried to prevent it on 2 September with a simple request: a daily 15–20 minute sync on the shared core work before any large design call. In his words, roughly: don't make such big changes on your own, because then we have to revisit them again and again.
That request is a big part of the answer. Short, frequent check-ins on shared foundations cost fifteen minutes a day. Finding out after a week that core changes need to be redone costs a week.
Two more habits from these weeks help:
- Walk the file, don't describe it. Show real input and real output (chapter 6). Reviewers trust what they can see.
- Keep a list of unresolved review comments. On 8 September the instruction was to list every open PR comment explicitly, so nothing gets dropped between sessions. Over two weeks and seven meetings, things get lost without a list.
Before each decision, ask which bucket it's in: mine, options for my reviewer, or the core owner's. When you're unsure, check in early and briefly instead of late and at length. For anything that changes how a shared foundation behaves, bring the owner in before you write the code, not after.
Keep this
16The checklist
For your next feature, at work or on your own project.
Before you write code
- I can name the user-visible nouns, and each one has an owner.
- I've listed what already exists that this must not break, including dashboards and other consumers of the data I touch.
- I've drawn the boxes: shared kernel vs. this product's shell. For each setting, I know who would change it and why.
- I know which decisions are mine, which need options for a reviewer, and which need a core owner in the room.
While designing
- Each contract between pieces is defined once and used in both directions.
- Validation rules come from code that already enforces them, not a new copy.
- Saving, metering, and logging are listeners, not built into the core loop.
- New glue code is thin. It calls existing owners instead of reimplementing them.
- Freshness (old vs. proposed vs. current) is computed by the owner of the noun.
- Multi-step writes use draft-then-publish or something equally simple, sized to how often they actually fail.
- The core knows general concepts (origin, handler), never product names.
- No optional flag secretly turns one function into two.
Before you call it shared
- A second caller actually calls it, even if only one layer and only on a branch.
- The tenth imagined caller would add a shell, not edit the core.
Running it
- Scheduled work is split into scheduler → queue → worker, one retryable unit each.
- Unattended jobs have limits, an output schema, one repair attempt, and a forced finish.
- I know my infrastructure's timeouts and my slowest dependency's latency.
- Telemetry records which version of the prompt or config produced each result.
Proving it
- End-to-end tests mock only the costly, nondeterministic dependency. The database and the core logic are real.
- Every "now optional" or "message removed" has a regression test for existing users.
- I've checked the edge cases myself before handing them to QA.
- If code judges AI output, there's an evaluation method before it merges.
Storing it
- Each kind of record has a decision: keep, index, or expire, or an explicit "deferred until we see X."
- I keep what I'll learn from.
Shipping it
- I know what ships now and what ships later, and "later" is written down.
- Every open review comment is on a list.
- I'm not building next year's infrastructure this month.
Epilogue
Two weeks of review on a feature whose first version mostly worked can look like a lot. Here's what that time bought.
The engine was cleanly enough layered that another team could take one piece of it and ship. A recommended change and an applied change share one format, so they can't drift apart. Core chat didn't pick up quiet behavior changes. Freshness lives where it can't break when a neighboring component changes. Every scheduled run records which prompt produced it. And there's a written list of what was deliberately left for later, which is different from what was simply forgotten.
None of that shows up in a demo. All of it shows up a year later, when someone else is changing this code and it holds up.
To compress the essay into one habit: for every piece of your system, be able to say who owns it, who calls it, and how you'd know if it broke. The rest of the chapters elaborate on those three questions.
— end —