test: add review guidelines skill for testing bot guideline adherence

[factory-nizar]

Purpose

Adds a .factory/skills/review-guidelines/SKILL.md to this repo so we can test whether the review bot picks up and enforces custom guidelines.

Guidelines included

The file contains a deliberate mix of conventions the bot should flag:

• Naming: boolean prefix rules (is/has/should/can), no single-letter vars, SCREAMING_SNAKE_CASE constants
• Error handling: no silent catches, explicit async error handling, contextual error logging
• TypeScript: interface over type for object shapes, no any, explicit return types on exports, prefer readonly
• Structure: 50-line function limit, max 3 nesting levels, early return pattern
• Comments: must start capitalized, no trailing periods, TODO must have author tag
• Imports: 3-group ordering, prefer named imports, no barrel imports
• Security: no logging sensitive data, no unescaped shell interpolation, no bare process.env.X!

Several of these rules are intentionally strict or unusual (e.g., no periods in comments, no single-letter loop vars) to make it easy to tell if the bot is actually reading and applying the guidelines vs just doing its default review.

[factory-droid]

Droid finished @factory-nizar's task —— View job

---

✅ Review complete — no issues found.

This PR adds a .factory/skills/review-guidelines/SKILL.md file with repository-specific review conventions. No functional code changes; nothing to flag.

[factory-droid]

This PR adds a review guidelines skill file (.factory/skills/review-guidelines/SKILL.md) containing repository-specific conventions for the review bot to enforce. No functional code changes are included — only a Markdown documentation file — so there are no bugs or issues to flag.

[factory-nizar]

@droid review

[factory-droid]

Droid finished @factory-nizar's task —— View job

---

---

🔍 Review complete — 6 issues found (3× P0, 3× P1).

Priority	File	Issue
P0	src/utils/example-violations.ts:27	Arbitrary code execution via dynamic require(path)
P0	src/utils/example-violations.ts:78	Command injection via execSync with interpolated branch name
P0	src/utils/example-violations.ts:104	Sensitive token leakage to logs
P1	.github/workflows/droid-review.yml:28	Mutable branch ref for GitHub Action
P1	src/utils/example-violations.ts:8	Unsafe test fixture in production source tree
P1	src/utils/example-violations.ts:97	Path traversal risk in readUserConfig()

Pin the workflow action to an immutable ref and relocate the unsafe fixture out of src/ so it cannot be shipped or accidentally imported at runtime.

[factory-droid]

This PR is primarily about adding review-guideline scaffolding for bot testing, but it also changes CI to use a mutable action ref and adds an intentionally unsafe TypeScript fixture under src/. Pin the workflow action to an immutable ref and relocate/contain the unsafe fixture so it cannot be shipped or accidentally imported at runtime.

[factory-nizar]

@droid review

[factory-droid]

Droid finished @factory-nizar's task —— View job

---

---

🔍 Review complete — 1 issue found (1× P1).

Priority	File	Issue
P1	src/utils/example-violations.ts:70	Unvalidated TIMEOUT_MS can yield NaN

The getTimeout() function silently returns NaN when TIMEOUT_MS is unset — validate the env var or provide a safe default.

[factory-droid]

.

[factory-droid]

Droid finished @factory-nizar's task —— View job

---

🔍 Review validated — 2 issues approved.

Both are concrete TypeScript typecheck breaks in src/utils/example-violations.ts under the repo’s current tsconfig.json (noUnusedLocals + no esModuleInterop).

[factory-droid]

[P0] Typecheck break: unused catch binding under noUnusedLocals

This file is included by tsconfig ("include": ["src/**/*"]) and the repo enables "noUnusedLocals": true; the catch (e) binding is never read, so tsc --noEmit will fail with an unused-local error. Fix by removing the binding (catch { ... }) or by using/renaming it (e.g., catch (_error) { ... }).

[factory-droid]

[P0] Typecheck break: default import from Node builtin without esModuleInterop

tsconfig.json does not enable esModuleInterop/allowSyntheticDefaultImports, so import path from "path" will fail typechecking (Node builtins are export = modules in @types/node). Use import * as path from "node:path" or switch to named imports (e.g., import { join, resolve } from "node:path") to keep the file typecheckable.

[factory-droid]

[P0] Typecheck break: unused catch binding under noUnusedLocals

This file is included by tsconfig ("include": ["src/**/*"]) and the repo enables "noUnusedLocals": true; the catch (e) binding is never read, so tsc --noEmit will fail with an unused-local error. Fix by removing the binding (catch { ... }) or by using/renaming it (e.g., catch (_error) { ... }).

[factory-droid]

[P0] Typecheck break: default import from Node builtin without esModuleInterop

tsconfig.json does not enable esModuleInterop/allowSyntheticDefaultImports, so import path from "path" will fail typechecking (Node builtins are export = modules in @types/node). Use import * as path from "node:path" or switch to named imports (e.g., import { join, resolve } from "node:path") to keep the file typecheckable.