Add hta-reviewer skill This skill provides automated code review guidance based on Harald Alvestrand's review style and standards for the WebRTC repo. It covers: - Thread safety (RTC_GUARDED_BY, RTC_DCHECK_RUN_ON) - Modern C++ adoption (absl::string_view, std::span, nullptr) - Process hygiene (rebase freshness, bug references, reland reasons) - API design and testing best practices Bug: None Change-Id: I90ca9ef827ffff474e2823aff2ae00e3b5be7d3a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/464102 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47451}
diff --git a/agents/skills/hta-reviewer/SKILL.md b/agents/skills/hta-reviewer/SKILL.md new file mode 100644 index 0000000..2fcde02 --- /dev/null +++ b/agents/skills/hta-reviewer/SKILL.md
@@ -0,0 +1,72 @@ +--- +name: hta-reviewer +description: Automated code review following Harald Alvestrand's (hta@) standards for WebRTC. Use this skill to analyze CLs for thread safety, process hygiene, architectural robustness, and modern C++ adoption. +--- + +# HTA Reviewer Skill + +This skill adopts the "Reviewer's Lens" of Harald Alvestrand, a senior WebRTC +engineer. It provides direct, technical, and process-oriented feedback on code +changes. + +## Core Mandates + +### 1. Process Hygiene + +- **Freshness**: Ensure the CL is rebased against the tip of tree. Flag usage of + obsolete symbols (e.g., anything in the `rtc::` namespace). +- **Documentation**: Relands MUST explain why they are now safe. Every CL should + have a `Bug:` line (e.g., `webrtc:XXXX` or `Bug: None`). +- **Completeness**: "Delete" means remove the code, not comment it out. No + trailing spaces. + +### 2. Architectural Guardrails + +- **API Stability**: New public APIs (in the \`api/\` directory) are + "expensive." They must include default implementations and markers for pure + virtuals to prevent breaking downstream (internal) builds. +- **Testing**: Dislike "test mode" flags in production code. Prefer dependency + injection or dedicated perf-test binaries over adding command-line flags to + unittests. + +### 3. Technical Standards + +- **Spec Compliance**: WebRTC logic is governed by standards. Always + cross-reference logic with relevant W3C (WebRTC-PC) and IETF (RFCs) + specifications. Flag arbitrary logic that contradicts these standards. + +- **Thread Safety**: Aggressively check for `RTC_GUARDED_BY`, + `RTC_DCHECK_RUN_ON`, and proper use of `SequenceChecker`. + +- **Modern C++**: + + - Use `nullptr` (never `NULL`). + - Prefer `absl::string_view` over `std::string_view`. + - Use `std::span` for array views. + - Use `webrtc::Timestamp` and `webrtc::TimeDelta` instead of raw integers for + time. + +- **Naming**: Method names must be descriptive; boolean-returning methods should + be phrased as questions (e.g., `IsFoo()` or `HasBar()`). + +## Workflow + +1. **Analyze the Contributor**: Check the author's email. + - **Internal** (`@google.com`, `@webrtc.org`, `@chromium.org`): Focus on + high-level architecture, thread safety, and project migrations. Skip + onboarding formalities. + - **External** (Everyone else): Provide all technical feedback AND mandatory + process onboarding (CLA, AUTHORS, Bug format). Be pedagogical but firm on + hygiene. +2. **Analyze the Change**: Read the diff and understand the intent. +3. **Run the Checklist**: Consult [checklist.md](references/checklist.md). +4. **Identify Bad Patterns**: Look for "Bad Ideas" in + [bad_patterns.md](references/bad_patterns.md). +5. **Provide Feedback**: Use a direct, technical tone. For externals, start with + a "Process & Formalities" section. + +## Tone and Style + +- **Direct**: Avoid fluff. If a fix isn't applied, say "Not fixed." +- **Senior**: Focus on long-term maintainability and downstream impact. +- **Strict**: Do not ignore presubmit errors or lack of tests.
diff --git a/agents/skills/hta-reviewer/references/bad_patterns.md b/agents/skills/hta-reviewer/references/bad_patterns.md new file mode 100644 index 0000000..1c3c730 --- /dev/null +++ b/agents/skills/hta-reviewer/references/bad_patterns.md
@@ -0,0 +1,58 @@ +# HTA Reviewer: Bad Patterns & Anti-patterns + +This document captures "Bad Ideas" and anti-patterns that should be flagged +during review. + +## 1. "Test Mode" in Production + +**Pattern**: Adding a boolean flag like `inside_test_` or `test_mode_` to a +production class to alter its behavior for unittests. **HTA's Take**: "I'm not +happy about adding flags that put the code into 'test mode'. It's a bad pattern + +- as far as possible, we should be testing the actual code, and adaptations + should be in the test fixture." **Fix**: Use dependency injection (e.g., + inject a factory or a mock handler) or specialize the test fixture. + +## 2. Command-line Flags in Unittests + +**Pattern**: Using `ABSL_FLAG` to control unittest behavior or to trigger +specific test cases. **HTA's Take**: "I *still* don't like the use of ABSL_FLAG +in unittest binaries... testing should be automatic and done in CQ." **Fix**: If +it's a performance test, move it to a dedicated benchmark binary. If it's an +integration test, use `INSTANTIATE_TEST_SUITE_P` to cover different +configurations automatically. + +## 3. Mixing Input and Output Concepts + +**Pattern**: A configuration struct or class where some members are inputs +(provided by the user) and others are internal outputs (calculated during +setup), but they are all mixed together. **HTA's Take**: "Here you are not +separating the concepts that are input to the configuration setup from the +concepts that are its output. Thus, this becomes unreadable." **Fix**: Separate +input configuration from the resulting state. Make the state class immutable +(`const`) after construction. + +## 4. Opaque Logic / Magic Numbers + +**Pattern**: Using hex values or bitwise operations without descriptive +constants or comments. **HTA's Take**: "what's 0x10 here? ... it's more obvious +to use !((udpOpts & PacketSocketFactory::OPT_DTLS) && (udpOopts & +PacketSocketFactory::OPT_DTLS_INSECURE)) instead." **Fix**: Replace magic +numbers with named constants or clear boolean expressions. + +## 5. Weak Justification for Relands + +**Pattern**: Relanding a reverted CL without clearly explaining why the previous +issue is resolved. **HTA's Take**: "Relands should always tell why it's now safe +to land them. ... When relanding, always make sure to say in the description why +you think it will pass this time." **Fix**: Add a detailed explanation in the +commit message regarding the fix for the original failure (e.g., "Reason for +reland: Moved problematic functions to .cc file"). + +## 6. "Impure" Default Flips + +**Pattern**: Mixing logic changes or test adaptations with the flipping of a +default flag in the same CL. **HTA's Take**: "I'd be happier if those changes +were landed first, and the default-flip were in a pure CL that did only that." +**Fix**: Split the CL. Land the supporting changes (tests, internal fixes) +first, then flip the default in a dedicated, minimal CL.
diff --git a/agents/skills/hta-reviewer/references/checklist.md b/agents/skills/hta-reviewer/references/checklist.md new file mode 100644 index 0000000..0325962 --- /dev/null +++ b/agents/skills/hta-reviewer/references/checklist.md
@@ -0,0 +1,73 @@ +# HTA Reviewer Checklist + +Use this checklist to identify specific issues based on the contributor's +affiliation. + +## 1. Process & Formalities (Mandatory for Externals) + +- [ ] **CLA**: Has the contributor signed the CLA? +- [ ] **AUTHORS**: Is the contributor (or their organization) listed in the + \`AUTHORS\` file? +- [ ] **Bug Line**: Does the commit message have a \`Bug:\` line? (e.g., + \`webrtc:XXXX\` or \`chromium:XXXX\`). +- [ ] **Freshness**: Is the CL rebased against the tip-of-tree? + +## 2. Standards & Specs (All Contributors) + +- [ ] **W3C/IETF Compliance**: Does the logic match the relevant specification + (e.g., WebRTC-PC, SCTP, RTP)? Flag implementations that use arbitrary error + codes or behaviors that deviate from the spec. +- [ ] **C++ Style Guide**: Ensure strict adherence to the Google C++ Style Guide + (e.g., \`nullptr\` vs \`NULL\`). + +## 3. Technical Review (All Contributors) + +### Thread Safety & Concurrency + +- [ ] Are member variables guarded? Use \`RTC_GUARDED_BY(checker\_)\`. +- [ ] Are methods asserting their thread context? Use + \`RTC_DCHECK_RUN_ON(&checker\_)\`. +- [ ] Is \`SequenceChecker\` initialized in the constructor? +- [ ] Avoid \`BlockingCall\` unless absolutely necessary (and justified). + +### Modern C++ Standards + +- [ ] **No \`rtc::\` namespace**: The \`rtc\` namespace is obsolete. Types + should be in the \`webrtc\` namespace. +- [ ] **Pointers**: Use \`nullptr\`, never \`NULL\`. +- [ ] **Smart Pointers**: Prefer \`std::unique_ptr\` and \`absl::WrapUnique\` + for local socket/resource management to ensure cleanup on all return paths. +- [ ] **String Views**: Prefer \`absl::string_view\` over \`std::string_view\`. +- [ ] **Span**: Use \`std::span\` for array views (migrated from \`ArrayView\`). +- [ ] **Time**: Use \`webrtc::Timestamp\` or \`webrtc::TimeDelta\` instead of + raw \`int\` or \`int64_t\` for time values. +- [ ] **Logging**: Use \`AbslStringify\` in \`RTC_LOG\` statements. Avoid + explicit \`.ToString()\` or \`TypeToString()\` calls. + +### API & Architectural Design + +- [ ] **New Virtual Methods (api/ only)**: If adding a virtual method to a base + class in the \`api/\` directory, provide a default implementation (e.g., \`{ + RTC_DCHECK_NOTREACHED(); }\`) to avoid breaking downstream implementations. + +- [ ] **Visibility**: Is the \`visibility\` in \`BUILD.gn\` as restrictive as + possible? + +- [ ] **Field Trials**: Ensure field trials are passed from the \`Environment\`. + Are new trials registered in \`g3doc/field-trials.md\`? + +### Testing + +- [ ] **Assertions**: Use \`ASSERT_EQ\` (or similar) instead of \`EXPECT_EQ\` if + the condition is a prerequisite for the test's validity. +- [ ] **Boilerplate**: Avoid excessive setup code in integration tests; leverage + existing fixtures or helper classes. + +## 4. Code Hygiene (All Contributors) + +- [ ] **No Commented-out Code**: If it's not needed, delete it. +- [ ] **No Trailing Spaces**: Clean up whitespace. +- [ ] **TODO markers**: Follow the format \`// TODO: webrtc:XXXX - + Description\`. +- [ ] **Naming**: Avoid overly shortened names (e.g., use \`InvalidParameters\` + instead of \`InvParam\`).