feat(lfx-review-guard): add self-review skill for common reviewer blockers

[manishdixitlfx]

Summary

• Add new /lfx-review-guard skill — a self-review checklist that catches the 9 most common reviewer blocker patterns
• Can be run after /develop and before /preflight, or on PRs already submitted
• Based on patterns consistently flagged by reviewers MRashad26 and asithade across 15+ PRs
• Update README with skill overview, details, architecture diagram, workflow, and project structure entries

9 Checks

1. Raw HTML elements — must use LFX wrappers (lfx-input-text, lfx-select, lfx-textarea, <p-skeleton>)
2. Dead code — unused providers, imports, methods, signals
3. Component responsibility — 4+ service injections flagged for discussion
4. Loading states — stats showing 0 during load, missing loading guards
5. Type safety in templates — no ! non-null assertions in templates
6. Error handling — no silent catchError without logging
7. Signal patterns — no BehaviorSubject for simple state, no cdr.detectChanges()
8. Upstream API alignment — parameter names match upstream contracts
9. PR description completeness — behavioral changes documented

JIRA

LFXV2-1312

Test plan

• Run ./install.sh and verify lfx-review-guard appears in installed skills list
• Open an LFX repo and run /review-guard to confirm the skill loads
• Verify tool-mapping comment is present in SKILL.md (vendor-neutral)
• Verify README additions render correctly on GitHub

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds a new /lfx-review-guard skill to run a report-only self-review checklist targeting common reviewer blockers, and documents it in the repository README alongside the existing skill workflow/architecture.

Changes:

• Add new skill definition at lfx-review-guard/SKILL.md implementing the 9-check review checklist and report format.
• Update README.md to include /lfx-review-guard in the skill list, architecture diagram, skill table, usage flows, and project structure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
lfx-review-guard/SKILL.md	Introduces the new review-guard skill instructions, checks, and reporting format.
README.md	Documents the new skill across the skills overview, architecture diagram, workflow snippets, and structure listing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The guidance that an effect() calling form.patchValue()/setValue()/reset() “needs allowSignalWrites: true” appears incorrect: allowSignalWrites is relevant when an effect writes to signals, not when it performs imperative form updates. As written, this will create false “blocker” reports; please narrow the rule to actual signal writes (e.g., someSignal.set(...)) or remove the allowSignalWrites requirement for form operations.

Suggested change

	- **`effect()` writing to forms** — any `effect()` that calls `patchValue()`, `setValue()`, or `reset()` on a form needs `allowSignalWrites: true` in the effect options.
	- **Signals not initialized inline** — per `component-organization.md`, simple `WritableSignal`s must be initialized directly (e.g., `loading = signal(false)`), not in the constructor.
	**Severity:** BLOCKER for BehaviorSubject misuse and missing `allowSignalWrites`. DISCUSS for ChangeDetectorRef (may be legacy code being modified).
	- **`effect()` writing to signals** — any `effect()` that calls `someSignal.set(...)`, `update(...)`, or otherwise writes to signals needs `allowSignalWrites: true` in the effect options. Imperative form updates like `form.patchValue()/setValue()/reset()` do **not** require `allowSignalWrites`.
	- **Signals not initialized inline** — per `component-organization.md`, simple `WritableSignal`s must be initialized directly (e.g., `loading = signal(false)`), not in the constructor.
	**Severity:** BLOCKER for BehaviorSubject misuse and missing `allowSignalWrites` when an effect writes to signals. DISCUSS for ChangeDetectorRef (may be legacy code being modified).

[Copilot]

This workflow line suggests /lfx-review-guard can “fix or proceed”, but the skill is documented as read-only/report-only (no Write/Edit). Please reword to indicate the user (or another skill) fixes blockers before proceeding.

Suggested change

	/lfx-review-guard → scans changed files → reports blockers, discussion items, and passes → fix or proceed
	/lfx-review-guard → scans changed files → reports blockers, discussion items, and passes → you or another skill fix any blockers before proceeding

[Copilot]

This section is about PR description completeness, but it says to flag changes missing from commit messages. Commit messages and PR descriptions are different artifacts; this check should compare against (or instruct updates to) the PR description instead of commit messages.

[Copilot]

The prompt asks whether the assistant should “fix the blockers”, but this skill is configured without Write/Edit tools and is described as report-only. As-is, the skill can’t actually apply fixes; either remove this prompt or add the necessary tools and update the mode/description accordingly.

[Copilot]

/develop is referenced as a way to “generate new code”, but this repo’s documented commands are the /lfx-* skills (and /develop isn’t listed in README). Consider pointing users to /lfx-coordinator and/or /lfx-ui-builder instead, to avoid suggesting a command that isn’t part of this skill set.

Suggested change

	- Generate new code (use `/develop` or `/lfx-ui-builder`)
	- Generate new code (use `/lfx-coordinator` or `/lfx-ui-builder`)

[Copilot]

The ASCII architecture diagram looks misaligned: the /lfx-setup (env) row is a single full-width cell, but the separator below it uses a split junction (┴) as if there were two columns. Also, /lfx-preflight (valid) is an unclear abbreviation compared to “validation” used elsewhere. Consider adjusting the box-drawing characters and label so the diagram renders consistently in a monospace block.

Suggested change

	│ /lfx-review-guard (self-review) │ /lfx-preflight (valid) │
	├──────────────────────────────────┴────────────────────────┤
	│ /lfx-setup (env) │
	├───────────────────────────────┴─────────────────────────┤
	│ /lfx-review-guard (self-review) │ /lfx-preflight (validation) │
	├──────────────────────────────────┴────────────────────────┤
	│ /lfx-setup (env) │
	├─────────────────────────────────────────────────────────┤

[Copilot]

Under “This skill DOES”, it says it will “Offer to fix blockers”, but the skill’s allowed tools don’t include Write/Edit. Please align capabilities and wording (either remove fix-offer language, or add write/edit capability and clarify that fixes are opt-in).

[Copilot]

The skill refers to running before/using /preflight, but the documented/installed command in this repo is /lfx-preflight. Using a different command name here will confuse users; please update these references to /lfx-preflight for consistency with README and the skill directory name.

Suggested change

	You are running a self-review checklist that catches the patterns most commonly flagged by reviewers before a PR is submitted. This skill runs **after development** and **before `/preflight`**.
	You are running a self-review checklist that catches the patterns most commonly flagged by reviewers before a PR is submitted. This skill runs **after development** and **before `/lfx-preflight`**.

[Copilot]

The dead-code check suggests flagging "public methods in services not called from any changed file" as unused. That’s not a reliable signal (unchanged files can still call the method), and it can lead to incorrect “dead code” findings. Consider limiting this to private methods within the file, or require a repo-wide reference search before reporting a public service method as unused.

Suggested change

	- **Unused methods** — private methods not called anywhere in the file; public methods in services not called from any changed file
	- **Unused methods** — private methods not called anywhere in the same file; public methods in services only if a repo‑wide reference search shows they are never called

[niravpatel27]

@manishdixitlfx After thinking through the architecture, I'd like us to fold these checks into /lfx-preflight instead of creating a separate skill. Here's the reasoning:

1. Single workflow — contributors already know code → /lfx-preflight → PR. Adding /lfx-review-guard as a separate step means another command to remember and another place where pre-PR checks live.
2. Auto-fix capability — preflight already has Write/Edit tools. Mechanical checks like raw HTML wrappers and dead imports can be auto-fixed, just like formatting and license headers. The current
   PR has a contradiction where the skill says "report only" but then offers to fix blockers — merging into preflight resolves this naturally.
3. No overlap confusion — two pre-PR skills creates a "which one do I run?" problem. One gate, one answer.

Preflight already detects Angular vs Go repos. These 15 checks would become new Angular-specific sections:

• Auto-fix checks: Raw HTML wrappers (Add LFX SKILL project structure and agent definitions #1), dead imports (Add MIT License title to LICENSE file #2), type safety (Add /lfx-pr-catchup skill for morning PR dashboard #5), signal patterns (feat: replace hardcoded ~/lf/ with dynamic linuxfoundation org discovery #7) — treated like formatting fixes
• Advisory checks (⚠ report only): Component size (Update README repo URL to linuxfoundation/skills #3), loading states (Add /lfx entry point and PM-friendly improvements #4), error handling (Add /lfx-test-journey skill for multi-branch journey testing #6), API alignment (feat: add /lfx-intercom skill for LFX Intercom integration #8), PR description (feat: add lfx-snowflake-access skill for Snowflake access requests #9), accessibility ([LFXV2-1303] Add indexed-data-types reference to lfx-coordinator skill #10), design tokens (feat(lfx-git-setup): add interactive Git DCO and GPG signing setup guide #11), N+1 patterns
  (fix(lfx-git-setup): address review comments — flags, hook safety, formatting #12), template completeness (feat(lfx-git-setup): add interactive Git DCO and GPG signing setup guide #13), stale data (Add a platform plugin for claude desktop #14), visitor gating (Make LFX Skills vendor-neutral #15) — reported as discussion items in the preflight output
• Skipped for Go repos — all 15 are Angular-specific, so they'd be gated behind the existing repo-type detection

Bugs to fix regardless of direction

• Check 7 (allowSignalWrites) — the rule about effect() + patchValue()/setValue()/reset() needing allowSignalWrites: true is incorrect. That flag is for writing to signals, not imperative form
  updates. This would generate false blocker reports.
• Naming — references /preflight and /develop instead of /lfx-preflight and /lfx-coordinator (or /lfx-ui-builder)
• Count mismatch — PR description says "9 checks" but SKILL.md has 15

Could you close this PR and open a new one that adds these checks as new sections in lfx-preflight/SKILL.md? The check definitions you've written are great — they just need a different home.

[manishdixitlfx]

Closing per Nirav's feedback — folding these checks into /lfx-preflight instead of a separate skill. See new PR coming.