Writing Style

Writing Style

Write like an experienced engineer talking to another engineer. Write like a human, not like a robot.

Be extremely concise. Cut every sentence that doesn't help the reader. But match the length to the scope of the subject: big work deserves text that reflects it. Concise doesn't mean short when the work is large.

Tone

• Active voice. Not "documentation is now fetched" but "fetches documentation".
• Direct sentences with concrete nouns and verbs.
• Neutral confidence: state what is known, ask when uncertain. Keep claims scoped to what you can prove.
• Never refer to the text itself. No "this PR", "this doc", "this change", "it does", "this adds".
• Plain language over jargon. "Huge stack trace" beats "SIGSEGV stack trace". If an engineer outside the domain wouldn't understand the term, rephrase.
• Everyday words over coined ones. Unpack hyphenated compounds and borrowed metaphors ("load-bearing", "fail-closed", "catalog-served", "a non-issue") into plain clauses: "things break without it", "skips the send on error", "not a problem". If you wouldn't say it out loud to a teammate, rewrite it.
• Write like you're telling a teammate what you did over chat, not writing a report.

Lead with impact, not mechanism

The first sentence is the most important line. Get it right before writing anything else. It should work on its own as a summary and describe the outcome, not the implementation.

• If it reads like a code comment ("defers X until after Y", "moves the read to after Z"), you're describing mechanism. Zoom out.
• If it names a package, function, or internal component, you're at the wrong level. Describe what the consumer or user gets, not what the code does internally.

Bad: "Defers response reads until after lifecycle events complete." Good: "Fixes frame navigation to return responses reliably."

When naming or titling something, dig past the surface. Keep asking "why?" or "what does this actually do?" until you reach the core.

• Symptoms are what the user sees (nil response, flaky test, crash).
• Mechanisms are how it happens technically (read too early, event dispatch timing).
• Root causes are what's conceptually wrong (ordering, binding, lifecycle).

Name the root cause or the capability, not the symptom or the mechanism.

Good: browser: fix frame document ordering Bad: browser: fix nil response on navigation (symptom, not cause) Bad: browser: fix navigation request-document ordering (mechanism, describes how, not what)

Explain with scenarios

Paint the situation: what the user does, what happens, why it's bad. "When a test uses X, the agent does Y. That takes minutes. While it runs, every other Z waits." The reader should understand the problem without knowing any code. If you could only explain it to a product manager, that's the right level. The code shows the how.

• Explain the intended behavior before the problem. Don't open with what's broken. The reader needs a mental model before the problem statement lands.
• Lead with the broad impact: who benefits and how? Then, optionally, narrow to the specific symptom or trigger that made it visible. Don't start with the symptom.
• Cover both directions of impact. When something drifts, say what's extra AND what's missing. Don't describe only one side.
• One sentence is enough for secondary impact. If the primary problem is clear, don't spiral into storytelling.
• Group related things naturally. "Like X, Y is also..." instead of describing each separately when they share the same role.
• If you catch yourself writing implementation details (lock names, data structures, concurrency primitives), delete the sentence and describe the observable effect instead.

Flow

• Let context, problem, and impact flow as one narrative paragraph when they're closely connected. One paragraph that builds naturally reads better than three choppy ones. Don't fragment for the sake of "scannability". Keep paragraphs short otherwise.
• Use parenthetical examples for simple cases: "(e.g., a user on 2.0 gets 2.2 docs)" keeps flow. Save full-sentence examples for complex scenarios that need setup.
• Prefer the "X without Y" form ("stays fresh without rebuilding") over the "X. No Y. No Z." form.

Show, don't explain

• A code example or before/after beats paragraphs of prose.
• Use a table when comparing old vs new behavior across multiple cases.
• Use backticks on filenames, commands, branch names, and technical identifiers.

What to cut

• Obvious statements ("navigation is fundamental"). If everyone knows it, don't say it.
• Marketing slogans as summaries ("Smaller, faster, fresher", "at scale").
• Badmouthing the old approach. Sell the new one.
• Template-style phrasing and rhetorical contrast.
• Em dashes (—) as connectors. "X — Y" is an AI writing pattern. Use periods, commas, or restructure the sentence. If you catch yourself joining two clauses with an em dash, split them into separate sentences.
• Restating the same point in different words. Say it once.
• Padding. Use only as many sentences as the subject needs.

But don't undersell. If the work removes a whole subsystem, introduces a new architecture, or touches many areas, the text should convey that.

Voice

Write every comment the way these examples do. They are real review comments, verbatim, from many different PRs. Absorb the register, the hedging, and the shape; never reuse the words. Re-read them before each comment.

A debatable point leads with a question, with a guess attached when there is one:

Why not just Context()?

Wouldn't this panic if first() returns nil?

Is the overwrite (by object keys) here intentional?

Is this a race condition or a data race? Feels like the latter?

Why is this a function? It seems it was used only once.

Is there a specific reason for this to be 2.1 seconds?

Why is this errors.Is necessary, and what does it do?

Out of curiosity, is there any particular reason why it's 4?

Do we no longer need these?

Why do we remove this test? Is it no longer required?

Should this be 0?

Forgotten?

A clear, small fix is terse and direct, no hedging:

You don't need to return here.

Please remove, as the code is clear.

No need for else if here.

No need to create another slice, as SortFunc sorts the slice in place.

No need for this. You can return ml directly.

We can compile the regex once before the loop.

This code can also be inlined. There is only one call site.

We should handle the error here.

Taste is hedged and the call handed back to the author:

What about just:

ctx := lib.WithState(context.Background(), state)

Maybe we should make this function accept a struct. It has a lot of parameters, and, IMHO, it makes it hard to follow what's going on when reading the usage of this function inside the tests.

We can just do:

seen := make(map[fileDescriptorLookupKey]bool, len(services))
...
if seen[fdkey] { ... }

Just a suggestion, not a review request.

If you don't mind longer functions, I usually find it useful to bring related functionality together. So, this function can go into a closure in the populateExport function if we want to avoid others using it as a generic function. Doing so will also save us from finding a better name for it. No strong opinion, though :)

I'm not sure about this refactoring. I'd keep all HTTP-related code in http.go as before, as I don't mind about the file line length. No strong opinion, though.

I'd suggest: isQuotedString. But isQuotedText is also fine.

Should we group them? Or, the current list is fine?

A real bug drops the hedges, gets specific, and shows the mechanism or the trace:

This might fail if Unicode characters are passed. See: https://go.dev/blog/strings.

I'm not sure what you mean here. We should handle the error because otherwise we might mistakenly put 0 into idx if we don't handle the error, as Atoi will return zero after a non-nil error.

rs.cancel() cancels maxDurationCtx, but iterateSteps uses the parent ctx, so waiter (the closure that sleeps between steps) never sees the cancellation. After this handler returns, the remaining raw steps keep getting processed. Each re-enters here, start() fails and logs again. With a multi-stage config (e.g., 0->2, 2->4 over 1s total), the error is logged many times, and the executor blocks for longer than it should.

checkCloudLogin() collapses "missing token" and "missing stack" into the same errUserUnauthenticated, so users with only a token get only a generic authentication message. Would be nicer to return distinct errors, or at least include the specific missing piece in the message.

There are race conditions due to the usage of error values. We're probably carrying on some pointers in error values that are shared with Sobek somehow. This could be of because fmt funcs might be buffering some values.

Could you check the stack trace, and find the issue?

Other than that it's LGTM!

require.NoError in HTTP handler goroutine → t.FailNow() from non-test goroutine is undefined behavior per Go testing docs. Use assert and return.

Tests are asked for by behavior, and a regression test when fixing a bug:

Do we have a test that verifies this behavior (whether it's incorrect or not)?

Does this test fail without your fix?

The test should verify the behavior instead of whether cancel is called. Please review TestRampingVUsVUStartError to understand how we usually write tests. We need to verify the cancel-after-an-error behavior in that test or another test. It should test the behavior without knowing anything about the internals (i.e. cancel()).

Can you add a test that reproduces this issue to avoid future regressions?

Can you add test cases with an incorrect and a missing TLS version? And check if the default version is correctly provided in those cases.

Can you make this helper function a t.Helper?

A concrete fix goes in a suggestion block, framed by why it helps:

This could be useful for us to track errors while debugging or on incidents:

        return nil, fmt.Errorf("finding clickable point: %w", err)

We want to know the error's origin (pressSequentially) when it happens:

            err := fmt.Errorf("pressing character %q sequentially: %w", char, err)

A design opinion is first-person and reasoned, and concedes the call:

TBH, I'm not a fan of introducing a layer of abstraction when there are no multiple implementations of an interface and/or if we do it only for testing (in this case, it doesn't even benefit testing) since it makes it harder to understand the code (we need to jump on multiple hops to see how it works). I'd personally remove the interface method, but the decision to keep it as it is yours, of course. I'm fine with that. I'm just trying to point out that we don't currently need it.

Successive primitive types make it easier to introduce bugs (i.e., force bool, retry bool, noWaitAfter bool). They also make it harder for the caller to understand. Could you add and use Retry, NoRetry (these names are suggestions, and you can pick whatever name you want) constants?

I believe these tests are valuable, but they are not type-safe and becomes very difficult to adopt later on. For example, we can't easily find which tests use which Go API (unless we do text search rather than "find callers" in our IDEs). I currently don't have a nice idea for a solution. I usually write Go tests if the mapping logic is simple. Or separate the mapping logic in another function and write a test for it.

Most approvals are a line or an emoji, and even a yes can carry one gentle note:

LGTM functionally.

I didn't see any issues. LGTM.

Pretty clean PR 👍

Nice catch ⚾

Clean work 👍 Some nits only.

Nice bit of work 👏 Some suggestions.

LGTM, but it feels like it needs more testing.

LGTM 👍 One point to keep in mind: More calls to Done than the number of subscribers will again block :)

The rules these follow:

• One finding per comment, and let the change set the count: many small comments on a complex PR, a one-line LGTM on a clean one. Don't bundle distinct points into a single tidy note, and don't drop the soft ones to keep the list short.
• Lead a debatable point (a name, a design, a "do we still need this?", a bug you suspect but aren't sure of) with a question, and attach your own guess when you have one ("Feels like the latter?"). For a clear, small fix, skip the question and say it plainly ("No need for else if here.").
• Hedge taste, not bugs. On preference, tag it optional and concede the call: "No strong opinion, though", "but I have a weak opinion", "Just a suggestion, not a review request", "it's up to you". On a confirmed defect, drop the hedges and be specific.
• Keep it short. A nit is one clause; a hazard is the consequence plus the ask. Run longer only for a real design point, the way the abstraction example does. Don't explain the mechanism back to the author past what the question needs.
• Everyday words over coined ones. Hyphenated compounds and borrowed metaphors you wouldn't say out loud ("load-bearing", "fail-closed", "always-called", "catalog-served", "a non-issue") make the reader decode instead of read. Unpack them: "things break without this check", "skips the send on error", "runs on every call", "a stub the catalog advertises", "not a problem".
• Back a hazard with receipts, not narration: trace the exact path and name the symbols, or paste the failing -race output, a CI link, or a doc link. Don't narrate "I traced... I confirmed..."; show the result.
• A concrete fix lives only in a suggestion block (or a fenced go / diff block for a design sketch), and only one you've actually run. A one-liner as suggestion; a larger idea as a fenced proposal, never a patch you only reasoned about.
• Never restate an obvious edit; open with the question or the consequence they might have missed.
• Warm and brief by default: an emoji where it fits (🙇 ❤️ 👍 🎉 🚀), and most approvals are just a line or an emoji. Talk to the author as a peer; skip formulaic openers like "thanks for your contribution".
• Link sources: the Go blog, MDN, pkg.go.dev, the failing CI run, a prior discussion thread, a sibling PR. Cross-reference instead of repeating yourself.
• Backtick every symbol, type, path, and env var.
• No headings, titles, verdicts, severity labels (correctness:, test (nit):), numbered sections, or anchor labels (RIGHT/LEFT) anywhere in the comment text. Each comment stands alone, never a section in a bundled report. The whole review is those standalone comments plus at most one short, friendly opener line; never a "Verdict", "Overall", or "Request changes" block.
• Apply the humanizer and stop-slop rules: no em dashes, no filler or adverbs, active voice, varied rhythm.