test: add review guidelines skill for testing bot guideline adherence

.factory/droids/file-group-reviewer.md

@@ -2,7 +2,7 @@
 name: file-group-reviewer
 description: Reviews an assigned subset of PR files for bugs, security issues, and correctness problems. Spawned in parallel by the main review agent to ensure thorough coverage.
 model: inherit
-tools: ["Read", "Grep", "Glob", "LS"]
+tools: ["Read", "Grep", "Glob", "LS", "Skill"]
 ---
 
 You are a senior staff software engineer and expert code reviewer.
@@ -28,16 +28,17 @@ Your task: Review the assigned files from the PR and generate a JSON array of **
   </review_guidelines>
 
 <workflow>
-1. Read each assigned file in full to understand the context
-2. Read the relevant diff sections provided in the prompt
-3. Read related files as needed to fully understand the changes:
+1. **Load custom review guidelines**: Before starting your review, invoke the `review-guidelines` skill using the Skill tool. The skill provides repository-specific review guidelines configured by the maintainers. Apply these guidelines throughout your review in addition to the standard guidelines above. If the skill is not available or returns nothing, proceed with only the standard guidelines.
+2. Read each assigned file in full to understand the context
+3. Read the relevant diff sections provided in the prompt
+4. Read related files as needed to fully understand the changes:
    - Imported modules and dependencies
    - Interfaces, base classes, and type definitions
    - Related tests to understand expected behavior
    - Callers/callees of modified functions
    - Configuration files if behavior depends on them
-4. Analyze the changes for issues matching the bug patterns above
-5. For each issue found, verify it against the actual code and related context before including it
+5. Analyze the changes for issues matching the bug patterns above, incorporating any custom review guidelines loaded in step 1
+6. For each issue found, verify it against the actual code and related context before including it
 </workflow>
 
 <output_format>

.factory/skills/review-guidelines/SKILL.md

@@ -0,0 +1,60 @@
+# Review Guidelines for droid-action
+
+These are the repository-specific review guidelines. Reviewers MUST follow all rules below when evaluating pull requests.
+
+## Naming Conventions
+
+- All boolean variables must be prefixed with `is`, `has`, `should`, or `can` (e.g., `isValid`, `hasPermission`). Flag any boolean that uses other prefixes or no prefix.
+- Avoid single-letter variable names everywhere, including loop iterators. Use descriptive names like `index` or `item` instead of `i` or `x`.
+- Constants must use SCREAMING_SNAKE_CASE. If a value is hardcoded and never reassigned, it should be extracted into a named constant.
+
+## Error Handling
+
+- Never swallow errors silently. Every `catch` block must either re-throw, log with `console.error`, or return a meaningful error value. Empty catch blocks are always a bug.
+- All async functions that interact with external systems (network, file system, GitHub API) must have explicit error handling -- do not rely on callers to catch.
+- When logging errors, always include the original error object or message for debuggability. Logging a generic string like `"something went wrong"` without context is a bug.
+
+## TypeScript-Specific Rules
+
+- Prefer `interface` over `type` for object shapes that could be extended. Use `type` only for unions, intersections, or mapped types.
+- Never use `any`. If a type is truly unknown, use `unknown` and narrow appropriately. The only exception is test files where mocking requires it.
+- All exported functions must have explicit return type annotations. Inferred return types are acceptable only for private/internal functions.
+- Prefer `readonly` arrays and properties when mutation is not needed.
+
+## Code Structure
+
+- Functions must not exceed 50 lines (excluding comments and blank lines). If a function is longer, it should be decomposed.
+- No more than 3 levels of nesting (if/for/while). Prefer early returns to reduce nesting depth.
+- Avoid `else` after `return` -- use early return pattern instead:
+  ```typescript
+  // Bad
+  if (condition) {
+    return x;
+  } else {
+    return y;
+  }
+
+  // Good
+  if (condition) {
+    return x;
+  }
+  return y;
+  ```
+
+## Comments
+
+- Every comment must start with a capital letter and end without a period. Comments ending with periods are too formal for code.
+- TODO comments must include an author tag: `// TODO(username): description`
+- Do not comment obvious code. A comment like `// increment counter` above `counter++` is noise and should be flagged.
+
+## Imports
+
+- Imports must be organized in three groups separated by blank lines: (1) external/node modules, (2) internal project imports, (3) relative imports from the same module.
+- Prefer named imports over default imports. Default imports make refactoring harder.
+- Never import from a barrel file (index.ts) when you can import directly from the source module.
+
+## Security
+
+- Never log sensitive data: tokens, keys, passwords, or full request/response bodies that might contain PII.
+- All string interpolation into shell commands must use proper escaping or parameterized execution. Template literal concatenation into `execSync` is always a bug.
+- Environment variable access must have fallback handling. Bare `process.env.X!` non-null assertions are not acceptable -- always provide a default or throw a descriptive error.

.github/workflows/droid-review.yml

@@ -25,7 +25,7 @@ jobs:
           fetch-depth: 1
 
       - name: Run Droid Auto Review
-        uses: Factory-AI/droid-action@v3
+        uses: Factory-AI/droid-action@feat/review-guidelines-to-skill
         with:
           factory_api_key: ${{ secrets.FACTORY_API_KEY }}
           automatic_review: true

src/utils/example-violations.ts

@@ -0,0 +1,106 @@
+/**
+ * This file intentionally contains violations of the review guidelines
+ * defined in .factory/skills/review-guidelines/SKILL.md for testing
+ * whether the review bot catches them.
+ */
+
+// VIOLATION: single-letter variable names (rule: no single-letter vars)
+export function processItems(items: string[]) {
+  const result: string[] = [];
+  for (let i = 0; i < items.length; i++) {
+    const x = items[i]!.toUpperCase();
+    result.push(x);
+  }
+  return result;
+}
+
+// VIOLATION: boolean without is/has/should/can prefix
+export function checkPermissions(user: { role: string }) {
+  const valid = user.role === "admin" || user.role === "editor";
+  const enabled = true;
+  return valid && enabled;
+}
+
+// VIOLATION: silent catch (empty catch block)
+export function loadConfig(path: string): Record<string, any> {
+  try {
+    const raw = require(path);
+    return raw;
+  } catch (e) {
+    // swallowed silently.
+  }
+  return {};
+}
+
+// VIOLATION: uses `any`, comments end with periods.
+export function transformData(input: any) {
+  // normalize the input data.
+  const output = { ...input };
+  // apply the default values.
+  output.timestamp = Date.now();
+  return output;
+}
+
+// VIOLATION: else after return, deeply nested (>3 levels)
+export function categorize(
+  value: number,
+  ranges: { min: number; max: number; label: string }[],
+) {
+  if (value >= 0) {
+    for (const range of ranges) {
+      if (value >= range.min) {
+        if (value <= range.max) {
+          if (range.label !== "") {
+            return range.label;
+          } else {
+            return "unlabeled";
+          }
+        }
+      }
+    }
+  } else {
+    return "negative";
+  }
+  return "unknown";
+}
+
+// VIOLATION: no explicit return type on exported function,
+// bare process.env non-null assertion, hardcoded magic number
+export function getTimeout() {
+  const base = parseInt(process.env.TIMEOUT_MS!, 10);
+  return base + 5000;
+}
+
+// VIOLATION: unescaped shell interpolation with template literal
+import { execSync } from "child_process";
+
+export function runGitCommand(branch: string): string {
+  const output = execSync(`git checkout ${branch}`, { encoding: "utf8" });
+  return output.trim();
+}
+
+// VIOLATION: default import, not organized in 3 groups
+import path from "path";
+import { resolve } from "path";
+import { readFileSync } from "fs";
+
+// VIOLATION: type used for extendable object shape instead of interface
+export type UserConfig = {
+  name: string;
+  email: string;
+  settings: Record<string, unknown>;
+};
+
+// VIOLATION: TODO without author tag
+// TODO: refactor this later
+export function readUserConfig(configPath: string): UserConfig {
+  const full = path.join(resolve("."), configPath);
+  const raw = readFileSync(full, "utf8");
+  return JSON.parse(raw) as UserConfig;
+}
+
+// VIOLATION: logging sensitive data
+export function authenticate(token: string): boolean {
+  console.log(`Authenticating with token: ${token}`);
+  return token.length > 0;
+}