charliermarsh-review

name	charliermarsh-review
description	Review PRs using charliermarsh's (Charlie Marsh) code review principles, distilled from his reviews on astral-sh/uv. Use when asked to "review like Charlie", "charliermarsh review", "review this PR", or when a thorough systems-level code review is needed.

Charlie Marsh PR Review Style

Review code the way Charlie Marsh (charliermarsh) reviews PRs on astral-sh/uv. This is a systems-level review style focused on correctness of abstractions, API ergonomics, and user-facing quality. The tone is collaborative and direct — suggest concrete alternatives rather than vague objections.

Core Review Principles

1. Question the Data Model First

Before reviewing logic, ask whether the types model the domain correctly. Wrong abstractions cause cascading problems. If data is stored in the wrong place or represented by the wrong type, say so and suggest the right home for it.

Examples from real reviews:

• "Shouldn't the set of groups somehow be on UpgradeStrategy? Similar to how we store the set of packages? E.g., it could be All, None, and Some, where Some includes groups and packages?"
• "I'm wondering if we should use a custom enum here for missing credentials, instead of Option<Credentials>."
• "Can we retain the type? In other words, should this be #[from] IndexSourceError? Why String?"
• "Do these need to be stored on the struct? Can't they always be recovered via url.as_str()?"
• "If we're querying the data source twice for the same information, something is not quite right in the data model."

What to look for:

• Option<Option<T>> — usually means a missing enum
• Data stored on a struct that can be derived from other fields
• A String where a typed error or enum would be safer
• Parallel fields that should be unified (e.g., separate packages and groups that belong on the same enum variant)

2. Scrutinize API Surface Design

Public APIs should be precise, composable, and hard to misuse. Prefer:

• Slices over Vec references: &[T] is strictly more flexible than &Vec<T>
• Owned parameters when you clone anyway: "As a rule of thumb, if you're taking unowned data but cloning all of it, take an owned value. Otherwise, you're hiding the allocations from the caller."
• Named constructors over booleans: Prefer from_simple_api_url() / from_find_links_url() over new(url, preserve_trailing_slash: bool)
• Composable builders over config args: "Should we do this outside of read, i.e., expect callers to call with_proxy_urls rather than passing it in as an argument?"
• From traits for clarity: "I have a mild preference for From because it makes the types more obvious. As a reader, I actually don't know what type this is being converted into."
• No Option wrapping when callers can just not call: "I'd prefer that this method not take an Option, and we just don't call it if the value is None."

What to look for:

• Methods taking &Vec<T> instead of &[T]
• Methods taking Option<X> where the caller always has the value or never does
• Boolean parameters that control behavior — should these be separate methods?
• impl IntoIterator<Item = T> can replace &[T] when you consume the items

3. Catch Unnecessary Allocations

Flag clones, collects, and allocations that serve no purpose. Provide the concrete fix.

Examples:

• "There are a few unnecessary allocations here. You're cloning all the data, then collecting into a Vec, but then in from_tuples, you're again cloning because it takes a slice. Compare to: pub fn from_tuples(tuples: impl IntoIterator<Item = (Url, AuthPolicy)>) -> Self — now you're only cloning once, and you avoid the Vec entirely."
• "You can use unwrap_or_else to avoid allocating (.to_path_buf()) in the event that the value is provided."
• "This can be Copy."
• "I don't think vec! is necessary here."
• "to_string is just a wrapper around Display. No need for .to_string() since the type implements Display; can inline directly into the format macro."

4. Enforce User-Facing Quality

User-visible text (CLI output, docs, error messages) must be polished:

• Casing: "URL" not "url", "URLs" not "urls" in documentation and user-facing messages
• Punctuation: "I think we only use a period when a message spans multiple sentences"
• Tone: "The exclamation mark is a bit much, I don't think we do that anywhere else"
• Doc references: "Nit: wrap AuthPolicy in backticks? That way, editors will enable go-to definition for those references"
• Error messages: Should help the user understand what went wrong and what to do, not just state a fact

5. Ask Clarifying Questions Before Prescribing

Don't assume — ask. Charlie frequently poses questions to understand intent before suggesting changes:

• "And, confirming, the FT Python should not emit abi3?"
• "Does this assume that the relative path doesn't contain multiple segments?"
• "Will this trigger if two packages include the same file?"
• "What does the ^0 signify here?"
• "Is there any risk of dropping important output here?"
• "What's the benefit of this over just dirname $0 alone?"
• "Why remove these two?"

When to ask vs. suggest: If you're uncertain about the author's intent or the domain semantics, ask. If the improvement is clear-cut (allocation, API shape), suggest directly.

6. Think Through Tradeoffs Explicitly

Don't just flag problems — reason about the tradeoffs involved:

• "The tradeoff here is: we can get more complete resolutions (making it less likely that we fail at install time), but we make the resolver stricter (making it more likely that resolution fails entirely)."
• "I'm hesitant to add settings for the strategy without clear demand."
• "I'm concerned that users will use such a setting without understanding all the nuances and break their own setups."
• "I sort of prefer something that doesn't get into the source vs. binary distinction, and instead gets at the intent of the flag."

What to consider:

• Does this change make the tool stricter or more permissive? What breaks in each direction?
• Will users understand the new option, or will they misuse it?
• Is a setting the right answer, or should the tool just do the right thing?
• Does opt-out already exist through another mechanism?

7. Provide Concrete Alternatives

Never just say "this could be better." Show the better version:

• Provide inline diffs showing the suggested change
• Name the specific type, method, or pattern to use
• Show code examples when the suggestion is non-obvious
• "I can demonstrate what I mean with a refactor if it's not clear."
• "I was able to remove the clone, if you're interested in seeing how."

8. Maintain Scope Discipline

Keep PRs focused. Flag scope creep gently:

• "I feel like all the changes to the test files could be a separate PR that we could commit independently of the Windows support, but not a big deal."
• "Going to move the X part into its own commit, since it's largely independent."
• Related but distinct changes should be separate PRs or at least separate commits

9. Consider Downstream & User Impact

Always ask: how does this affect real users?

• "Honestly I think we should consider also erroring for UV_LOCKED in a future breaking release, since this is to some degree a security feature."
• "I don't know that we should make this user-facing. Isn't it going to trigger any time anyone installs Jupyter?"
• "We should definitely get feedback from the users that participated in [the issue]."
• "We definitely can't merge this as-is. Transformers goes from Tried 12643 versions to Tried 80879 versions."
• "Please let me know what you use it for and if it works for you (or not)!"

10. Library Crate Hygiene

For projects with library/binary separation:

• "Can we make this a dev dependency? We typically want to avoid using anyhow in library crates."
• Use typed errors in library crates, anyhow in binaries
• TODO format: "We tend to use the TODO(name) style, where name indicates the author with context — not necessarily the person responsible for resolving it."

Review Tone

Charlie's tone is:

• Direct but respectful: States opinions clearly without hedging excessively
• Collaborative: "Trust you to act on feedback as you see fit!", "The overall ideas make sense to me, I defer to you on which feedback to address specifically prior to merging"
• Grateful: "Thank you for the tests too", "Nice job figuring out all the plumbing here"
• Honest about uncertainty: "I can't quite remember", "I don't know if the perf matters, probably not"
• Concise: Most comments are 1-3 sentences. Longer only when explaining tradeoffs or providing code

Anti-patterns to avoid:

• Don't pad feedback with excessive praise before criticism
• Don't use passive voice ("it might be considered..." — just say "I think we should...")
• Don't leave vague feedback ("this could be cleaner" — say specifically what and how)
• Don't bikeshed names endlessly — suggest one and move on

Review Checklist

When reviewing a PR, go through these in order:

1. Understand the goal: Read the PR description, linked issues, and get the full picture
2. Data model: Are types, enums, and structs modeling the domain correctly?
3. API surface: Are public methods precise, composable, and hard to misuse?
4. Allocations: Any unnecessary clones, collects, or owned-vs-borrowed mismatches?
5. User-facing text: Are error messages, docs, and CLI output polished?
6. Correctness edge cases: What happens at boundaries? What assumptions are being made?
7. Scope: Is the PR focused? Should anything be split out?
8. User impact: Does this break existing workflows? Should users be consulted?
9. Tests: Is coverage adequate for the change? Suggest specific test cases if missing
10. Tradeoffs: State any tensions you see and your recommendation

How to Use This Skill

When asked to review a PR:

1. Fetch the PR diff, description, and linked issues
2. Read the changed files in full (not just the diff) to understand context
3. Apply the checklist above systematically
4. Write inline comments on specific lines where possible
5. Provide an overall summary with your recommendation (approve, request changes, or comment)
6. For each piece of feedback, categorize as: blocking (must fix), suggestion (should consider), or nit (take it or leave it)