firebase-tools-pr-review

1. Quick Author & Reviewer Checklist

Before submitting or approving any PR on firebase/firebase-tools, verify:

• TypeScript Precision: No any or as any. Use generic parameters (client.request<T>()), explicit interfaces, or readonly string[] for unmutated inputs.
• No null: Use undefined or optional type annotations (string | undefined) instead of null. Drop redundant = undefined initializers.
• CLI UX & Options Objects: Functions accept options objects rather than long positional argument lists. Command interfaces favor explicit flags over interactive pickers (CI safety).
• Clean CLI Initialization: No expensive background commands (e.g. npx, sub-process execution) are triggered during CLI startup.
• Error Remediation & FirebaseError: Errors throw FirebaseError with clear explanations and explicit remediation instructions (telling the user/agent what flag or tool to run).
• Pristine stdout (jq Safety): When --json is set, all logs, warnings, and spinners output to stderr so stdout remains valid, parseable JSON.
• Downgrade & Backward Safety: Disk state, emulator schemas, and CLI configs are backward-compatible to ensure developers can safely downgrade CLI versions.
• Testing Discipline: Unit tests exercise public APIs (experiments.setEnabled) rather than stubbing private helpers. Mocks are cleaned up in afterEach (sinon.restore(), nock.cleanAll()).
• CHANGELOG.md Rules: User-facing bug fixes and new features include a changelog entry starting with a past-tense verb, with no [] annotations, and ending with (#<issue_number>) if applicable. Omit changelogs for unreleased flags or internal refactors.
• No AI/Agent Junk: Unused comment blocks, residual issue numbers, or boilerplate generated by AI agents are stripped out.

---

2. Detailed Review Standards by Category

Category A: Code Style, Syntax & Anti-Patterns

1. Strict Type Safety & Type Guard Functions

   • Rule: Avoid any, as any, or loose casting. Use generic type parameters at call sites (e.g., client.request<T>()), explicit type guard functions (val is TargetType), or stepped assertions (as unknown as TargetType).
   • Immutability: Mark non-mutated array parameters as readonly (e.g., readonly string[]).

2. Type Literals over Native Enums

   • Rule: Do not use native TypeScript enum. Always use union type literals (e.g., type FeatureFlag = "foo" | "bar";).

3. Avoid Wrapper Classes & Complex Collections in Interfaces

   • Rule: Do not use Map, Set, String, Number, etc. as the return value of any function or a field of any interface. Standardize on primitive types (string, number) and plain object maps (Record<string, T>) or arrays (T[]).

4. Prefer Interfaces over Classes

   • Rule: Always use interface declarations over class definitions when defining data shapes or public API contracts.

5. undefined over null

   • Rule: Standardize on undefined rather than null. null returns in utility functions create unneeded type ambiguity.
   • Clean syntax: Remove explicit = undefined variable initializers (e.g., let token: string; instead of let token: string | undefined = undefined;).

6. Top-Level Constants & Clean Quotes

   • Rule: Extract magic numbers, default timeouts, repeatedly used strings, and regex patterns into named top-level constants (UPPER_SNAKE_CASE).
   • Formatting: Do not add unnecessary quotes around object keys unless syntactically required.

7. Readable Control Flow & Loop Safety

   • Guard Clauses: Use early return, continue, or break to minimize nesting whenever possible. Avoid deep nested if { ... } else { ... } control flow.
   • for .. of over forEach: Use for .. of loops instead of .forEach(...) callbacks for better async handling, readability, and early exit capability.
   • Array Idioms: Use .push() instead of .splice(len, 0, ...) for appending elements.

---

Category B: Architecture, Abstraction & Component Design

1. Options Objects over Positional Parameters

   • Rule: Functions accepting multiple arguments should accept a single options object ({ project, force, quiet }). This makes call sites self-documenting and future-proof.

2. Tool Consolidation & Parameterization

   • Rule: Avoid creating separate tools or handlers for individual sub-features (e.g., create_android_app, create_ios_app). Consolidate them into a single parameterized tool (create_app({ platform: z.enum(['web', 'ios', 'android']) })). Use snake_case for all exposed tool names and schema fields.

3. Decoupled CLI Entries & Pure Utilities

   • Rule: Keep CLI command definitions (src/commands/) lightweight. Move business logic, GCP REST API calls, and complex data manipulation into dedicated modules (src/gcp/, src/emulator/, etc.).

4. Zero Overhead at CLI Startup

   • Rule: Never execute sub-processes (such as npx, node, or external binaries) or network calls during CLI command initialization, even in a non-blocking background fashion.

5. Declarative Config over JS Mutation

   • Rule: Configure runtime behaviors via .env, JSON, or YAML files. Do not modify user JavaScript files or parse ASTs unless explicitly required.

6. Dedicated Experiment Gating

   • Rule: Gate unreleased features under specific experiment flags (kits, autheventarc) rather than generic flags.

---

Category C: Error Handling, Retries & Async Safety

1. Actionable Error Remediation & Non-Zero Exit

   • Rule: Error messages must state what failed, why it failed, and how to fix it. Tell the user or automated agent the exact CLI flag or tool call needed to recover.
   • Non-Zero Exit Status: FirebaseError thrown for assertion failures or invalid user state must always specify a non-zero exit status (e.g., { exit: 1 }).

2. Structured FirebaseError Usage

   • Rule: Always throw FirebaseError rather than generic Error. Preserve the underlying cause using original: err and specify proper HTTP/exit status codes.

3. Asynchronous Retry Isolation

   • Rule: Handle retries for asynchronous race conditions (e.g., IAM Service Account propagation or Cloud Run readiness) inside dedicated executor/retry queues rather than polluting call sites.

4. Process & Stream Safety

   • Rule: Handlers executing child processes (child_process.spawn) must attach asynchronous 'error' listeners and drain stdout/stderr streams to prevent buffer deadlocks.

---

Category D: Testing, Edge Cases & Mock Verification

1. Test Public Interfaces over Private Helpers

   • Rule: Write unit tests against exported public methods or command entry points (e.g., experiments.setEnabled()) rather than stubbing unexported internal helpers using Sinon.

2. Full Permutation Coverage

   • Rule: Verify full state transition matrices (create, update set, update remove, delete, empty/missing inputs).

3. Mock Isolation & Teardown

   • Rule: Always clean up test stubs and network intercepts in afterEach hooks (sinon.restore(), nock.cleanAll()). Unit tests must never depend on external network calls or ambient environment variables.

4. LLM Schema Smoke Testing

   • Rule: Zod schemas exposed to MCP tools or AI assistants must be smoke-tested against real LLM APIs (e.g., Gemini API) to verify schema constructs (such as dictionary maps) do not trigger parser errors.

---

Category E: API, CLI UX & Backward Compatibility

1. Clean stdout for Machine Composability

   • Rule: When --json is specified, all human-readable logging, spinners, and warnings MUST be routed to stderr. stdout must contain only valid JSON parseable by UNIX utilities like jq.

2. Mandatory CLI .help() Text

   • Rule: Every CLI command definition must provide detailed .help() text explicitly documenting allowed values for arguments and flags.

3. Downgrade Safety & Major Release Discipline

   • Rule: Local disk structures, emulator state schemas, and project configs must remain backward-compatible to allow developers to downgrade CLI versions safely. Breaking CLI flag removals or schema changes must be held for major release cycles.

4. Strict CHANGELOG.md Governance

   • Rule:
     • Require CHANGELOG: User-facing bug fixes and new features. Entries must start with a past-tense verb, contain no [] annotations (e.g. no [fixed] or [added]), and end with (#<issue_number>) if addressing an issue.
     • Omit CHANGELOG: Internal code refactors, test updates, or features behind unreleased experiment flags.

---

Category F: Review Tone & Feedback Guidelines

1. Clear Feedback Classification

   • Rule: Distinguish between blocking requirements and non-blocking suggestions. Prefix non-blocking comments with Nit: or Optional: and provide actionable inline GitHub suggestion blocks (````suggestion`).

2. Scrub AI/Agent Artifacts

   • Rule: Actively inspect PR diffs for residual empty comment blocks, dead boilerplate, or issue numbers left behind by automated AI coding agents.

---

3. PR Review Response Template (Inline Comments + Top-Level Summary)

When delivering a PR review using this skill, structure the payload with a top-level summary body and a list of comments targeting specific file paths, line numbers, and sides (RIGHT for additions):

{
  "body": "> ⚠️ **Disclaimer:** This draft review was generated by an experimental AI review agent. Please verify all findings before acting on them.\n\n## Code Review Summary: `firebase/firebase-tools`...\n\n### 🟢 Strengths & LGTM Aspects\n- [High-level summary of strengths]\n\n### 🔴 Overview of Findings\n- [Short summary of blocking items & nits]",
  "comments": [
    {
      "path": "src/extensions/export.ts",
      "line": 527,
      "side": "RIGHT",
      "body": "🔴 **[TypeScript & Runtime Safety] Title**\n\n*Rationale*: Explanation of why this rule matters.\n\n*Suggested Fix*:\n```ts\n// Corrected code example\n```"
    },
    {
      "path": "src/commands/ext-export.ts",
      "line": 205,
      "side": "RIGHT",
      "body": "🟡 **Nit: Title**\n\nExplanation of cosmetic or non-blocking nit."
    }
  ]
}

To submit this review via gh api (using the standard 2-step file approach to avoid terminal stdin pipe parsing issues):

1. Save the JSON payload to a file in ~/.reviews/:

mkdir -p ~/.reviews
cat << 'EOF' > ~/.reviews/pr_<PR_NUMBER>_review.json
<JSON_PAYLOAD>
EOF

1. Submit the review pointing to the review file:

gh api repos/firebase/firebase-tools/pulls/<PR_NUMBER>/reviews -X POST --input ~/.reviews/pr_<PR_NUMBER>_review.json