From 57e4992621eaf3699a05315382513b330c7d4e5c Mon Sep 17 00:00:00 2001 From: AdamK Date: Thu, 28 May 2026 09:24:12 +0200 Subject: [PATCH] Added code review skills. --- .../rezolve-oof-pr-auto-review/SKILL.md | 91 ++++++++ .../SKILL.md | 47 ++++ .../rzlv-avoid-breaking-changes/SKILL.md | 64 ++++++ .github/skills/rzlv-code-review/SKILL.md | 7 + .../steps/step-01-gather-changes.md | 110 +++++++++ .../steps/step-02-rzlv-review.md | 139 ++++++++++++ .../steps/step-03-general-review.md | 135 +++++++++++ .../steps/step-04-consolidate-and-report.md | 209 ++++++++++++++++++ .github/skills/rzlv-code-review/workflow.md | 49 ++++ .../SKILL.md | 76 +++++++ .../rzlv-explain-non-obvious-code/SKILL.md | 72 ++++++ .../rzlv-explicit-null-validation/SKILL.md | 77 +++++++ .../rzlv-extract-methods-for-clarity/SKILL.md | 74 +++++++ .../SKILL.md | 72 ++++++ .../skills/rzlv-make-errors-explicit/SKILL.md | 65 ++++++ .../SKILL.md | 69 ++++++ .../rzlv-test-observable-behavior/SKILL.md | 70 ++++++ .../SKILL.md | 74 +++++++ .../rzlv-use-descriptive-names/SKILL.md | 84 +++++++ .../skills/rzlv-use-named-constants/SKILL.md | 65 ++++++ 20 files changed, 1649 insertions(+) create mode 100644 .github/skills/rezolve-oof-pr-auto-review/SKILL.md create mode 100644 .github/skills/rzlv-algorithm-precision-handling/SKILL.md create mode 100644 .github/skills/rzlv-avoid-breaking-changes/SKILL.md create mode 100644 .github/skills/rzlv-code-review/SKILL.md create mode 100644 .github/skills/rzlv-code-review/steps/step-01-gather-changes.md create mode 100644 .github/skills/rzlv-code-review/steps/step-02-rzlv-review.md create mode 100644 .github/skills/rzlv-code-review/steps/step-03-general-review.md create mode 100644 .github/skills/rzlv-code-review/steps/step-04-consolidate-and-report.md create mode 100644 .github/skills/rzlv-code-review/workflow.md create mode 100644 .github/skills/rzlv-eliminate-redundant-operations/SKILL.md create mode 100644 .github/skills/rzlv-explain-non-obvious-code/SKILL.md create mode 100644 .github/skills/rzlv-explicit-null-validation/SKILL.md create mode 100644 .github/skills/rzlv-extract-methods-for-clarity/SKILL.md create mode 100644 .github/skills/rzlv-future-proof-configuration-defaults/SKILL.md create mode 100644 .github/skills/rzlv-make-errors-explicit/SKILL.md create mode 100644 .github/skills/rzlv-manage-state-dependencies-properly/SKILL.md create mode 100644 .github/skills/rzlv-test-observable-behavior/SKILL.md create mode 100644 .github/skills/rzlv-thread-safety-synchronization/SKILL.md create mode 100644 .github/skills/rzlv-use-descriptive-names/SKILL.md create mode 100644 .github/skills/rzlv-use-named-constants/SKILL.md diff --git a/.github/skills/rezolve-oof-pr-auto-review/SKILL.md b/.github/skills/rezolve-oof-pr-auto-review/SKILL.md new file mode 100644 index 0000000..cf33246 --- /dev/null +++ b/.github/skills/rezolve-oof-pr-auto-review/SKILL.md @@ -0,0 +1,91 @@ +--- +name: 'rezolve-oof-pr-auto-review' +description: 'Request GitHub Copilot review on one open PR in Bluedot-Innovation, and optionally generate a local standardized review report.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* + - atlassian-rovo/* +--- + +# Rezolve OOF PR Code Review + +## Primary Outcome + +Given a PR, request **GitHub Copilot review** so Copilot is added/review-requested and posts comments/suggestions directly in GitHub. + +## Setup + +0. Preflight MCP check (mandatory): + - Confirm GitHub MCP PR tools are available for this session before doing any PR work. + - Confirm Atlassian MCP issue search tools are available if Jira enrichment may be needed. + - If required MCP tools are missing or not configured, abort immediately and report the missing MCP capability. + - Do not use gh CLI or any terminal/API fallback for PR operations. + +1. Determine the target PR in `Bluedot-Innovation`: + - If user provides `{owner}/{repo}#{number}`, use it. + - Otherwise, list open PRs in `Bluedot-Innovation` and ask the user to pick one. +2. Fetch PR data via GitHub MCP: + - PR details (`get`) + - PR files (`get_files`) + - PR diff (`get_diff`) +3. Extract a Jira ticket key by checking the PR head branch name first (e.g. `BD-7425` from `BD-7425-some-change`). If no key is found in the branch, use the PR title as a fallback source. If a ticket key is found from either source, fetch issue fields with this exact sequence: + - Run `mcp_com_atlassian_getAccessibleAtlassianResources` to resolve cloudId and site URL. + - Run `mcp_com_atlassian_searchJiraIssuesUsingJql` with `jql: key = `, `fields: ["summary", "description", "status", "issuetype", "priority"]`, and `responseContentFormat: "markdown"`. + - If description is missing, retry with `responseContentFormat: "adf"`. + - Do not use transitions APIs for issue content. +4. If the PR body is empty or near-empty (blank or shorter than 40 non-whitespace characters) and a Jira ticket was found, update the PR main body by inserting the following `AI Development Checklist` section immediately before the `Jira Context` block, then request Copilot review. + - Template (insert as-is): + + ## AI Development Checklist + - [ ] Spec/requirement linked: + - [ ] AI tool used: + - [ ] Tests generated/refined with AI + - [ ] AI review pass completed + - [ ] Repo context files still accurate (update if not) + + - Preferred content: Jira summary, description excerpt, acceptance criteria bullets, Jira URL. + - If Jira description is unavailable, still add fallback context with ticket key and Jira URL (`{siteUrl}/browse/{TICKET_KEY}`). + - If Jira fetch fails entirely, still add fallback context and include the fetch failure note. + - Do not overwrite substantial existing body text; append context only if missing. +5. Trigger GitHub-native Copilot review: + - Validate the PR is open. + - Call `mcp_github_request_copilot_review` with `owner`, `repo`, and `pullNumber`. + - Confirm success and provide the PR URL. + - If Copilot review request fails, report the error clearly and stop. +6. Inspect the diff to understand what types of changes are involved. +7. Apply the **Doc Routing Table** in `.github/copilot-instructions.md` — use the change types you identified to determine which architecture docs to load. Use those docs as your source of truth throughout the review. +8. If `.github/copilot-instructions.md` is missing, use `docs/ai/resources/*.md` as fallback architecture guidance and explicitly mention this fallback in the review. +9. If the codebase contradicts the docs, the **docs represent the target state** — flag the discrepancy rather than normalising the legacy pattern. + +--- + +## Review + +Evaluate the changes across these areas. Let the loaded architecture docs govern the specific standards — do not invent rules not found there. + +1. **Code Quality** — single responsibility principle, clarity, naming, redundancy, complexity, consistency with documented patterns +2. **Regressions** — silent breakage, unexpected behaviour changes, altered logic outside of the stated scope of the ticket, downstream impact +3. **Security** — input validation, sensitive data handling, auth correctness +4. **Performance** — inefficient or unnecessarily expensive code, unnecessary re-renders, missing cleanups, over-importing +5. **Error handling** — async error paths covered, error messaging in place, graceful degradation, no swallowed errors +6. **Accessibility** — semantic HTML, ARIA attributes, keyboard navigation, focus management +7. **Testing** — coverage of new logic, edge cases, error paths; flag superfluous tests +8. **Documentation** — non-obvious logic commented; docs updated where needed, architecture docs updated if the change introduces a new pattern or deviates from existing ones + +--- + +## Output Format + +- First, report GitHub Copilot review request status: + - PR URL + - Review request result (success/failure) + - PR body enrichment result (updated/skipped + reason) + +- **Inline comments** for specific lines or blocks. +- **Top-level observations** for broad themes or praise. +- Prioritise by severity: 🔴 blocking / 🟡 important / 🔵 minor. +- Do **not** include your reasoning steps or to-do list. +- Close with a witty quatrain summarising the PR. 🎭 + +If local analysis is requested, save the review to `docs/code-reviews/-code-review.md` (use the full PR head branch name from Setup). Confirm the file has been written. diff --git a/.github/skills/rzlv-algorithm-precision-handling/SKILL.md b/.github/skills/rzlv-algorithm-precision-handling/SKILL.md new file mode 100644 index 0000000..16a3c53 --- /dev/null +++ b/.github/skills/rzlv-algorithm-precision-handling/SKILL.md @@ -0,0 +1,47 @@ +--- +name: 'rzlv-algorithm-precision-handling' +description: 'Review Flutter/Dart code for algorithm precision issues: edge cases, floating-point handling, boundary conditions, and correct mathematical formulas.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Algorithm Precision Handling + +## Purpose + +Review code changes for algorithm correctness, mathematical precision, and robust edge case handling. Flag subtle bugs caused by approximations, incorrect formulas, or unvalidated boundary conditions that surface only under specific conditions. + +## When to Apply + +Trigger this skill when a PR introduces or modifies: +- Mathematical calculations or formulas +- String parsing or splitting logic +- Division, rounding, or floating-point arithmetic +- Validation functions that check numeric constraints +- Coordinate or geometry calculations + +## Key Practices + +- **Use `substring()` over `split()` for string processing** when precise prefix control is needed — `currentLabel.substring(baseLabel.length)` avoids issues when the base pattern appears multiple times in the string. +- **Apply correct unit conversions** with mathematical constants — e.g., `event.rotation = (double)state->rotation * (M_PI / 180)` for degrees-to-radians conversion. +- **Handle floating-point precision in division** — use `lerpValue.round()` after render-level calculations rather than redundant division-based rounding. +- **Validate all edge cases**, especially zero values and boundary conditions — check for `metrics.physical_width == 0` and tight constraint relationships. +- **Fix mathematical formulas to use correct operations** — e.g., `(math.max(first.bottom, second.bottom) - math.min(first.top, second.top))` for proper area height calculation (`min` for top, not `max`). + +## Review Checklist + +- [ ] Are all numeric inputs validated before use (zero, negative, infinite)? +- [ ] Are mathematical formulas using the correct operations (min vs max, etc.)? +- [ ] Is string splitting done safely when the delimiter may appear multiple times? +- [ ] Are unit conversions explicit and correct (degrees → radians, etc.)? +- [ ] Are boundary conditions (empty, zero-size, max values) tested? + +## Output + +Raise findings as: +- 🔴 **Blocking** — incorrect formula that produces wrong output +- 🟡 **Important** — missing edge case that could cause runtime failure in production +- 🔵 **Minor** — precision improvement or safer alternative available + diff --git a/.github/skills/rzlv-avoid-breaking-changes/SKILL.md b/.github/skills/rzlv-avoid-breaking-changes/SKILL.md new file mode 100644 index 0000000..5d446ca --- /dev/null +++ b/.github/skills/rzlv-avoid-breaking-changes/SKILL.md @@ -0,0 +1,64 @@ +--- +name: 'rzlv-avoid-breaking-changes' +description: 'Review Flutter/Dart APIs for backward compatibility: flag breaking parameter changes, missing deprecations, and altered default values that would force consumers to update code.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Avoid Breaking Changes + +## Purpose + +Review API changes to ensure backward compatibility. Flag changes that would force developers to maintain separate code paths for different SDK versions, and recommend safer alternatives such as deprecation with migration paths or optional parameters with sensible defaults. + +## When to Apply + +Trigger this skill when a PR: +- Modifies a public method signature (adds/removes/reorders parameters) +- Changes an existing default value +- Removes or renames a public API +- Adds a required parameter to an existing method +- Changes the behavior of an existing API + +## Key Practices + +- **Make new parameters optional with defaults** instead of required: + ```dart + // Instead of breaking change: + void getHandleAnchor(TextSelectionHandleType type, double textLineHeight, {required double cursorWidth}) + + // Use optional with default: + void getHandleAnchor(TextSelectionHandleType type, double textLineHeight, {double cursorWidth = 2.0}) + ``` + +- **Use `@Deprecated` with a migration message** rather than direct removal: + ```dart + @Deprecated( + 'Use sendAnnouncement instead. ' + 'This API is incompatible with multiple windows. ' + 'This feature was deprecated after 3.35.0-0.1.pre.' + ) + static Future announce(String message) { ... } + ``` + +- **Never change existing default values** — this is a silent breaking change. Add new options while preserving existing defaults. + +- **Design new APIs to be explicit** — avoid implicit assumptions that would require breaking changes when those assumptions need updating. + +## Review Checklist + +- [ ] Does the PR add any required parameters to existing public methods? +- [ ] Are any existing default values changed? +- [ ] Are removed/renamed APIs replaced with properly annotated `@Deprecated` wrappers? +- [ ] Do deprecation messages include the target replacement and the version deprecated after? +- [ ] Are new parameters optional with sensible defaults? + +## Output + +Raise findings as: +- 🔴 **Blocking** — change breaks existing consumers without a migration path +- 🟡 **Important** — change is technically breaking but could be mitigated with deprecation +- 🔵 **Minor** — default value choice or API design suggestion for future-proofing + diff --git a/.github/skills/rzlv-code-review/SKILL.md b/.github/skills/rzlv-code-review/SKILL.md new file mode 100644 index 0000000..1e8ac88 --- /dev/null +++ b/.github/skills/rzlv-code-review/SKILL.md @@ -0,0 +1,7 @@ +--- +name: rzlv-code-review +description: 'Performs an extended code review of the current branch vs a target branch, applying all rzlv- coding standards as review lenses plus a general code review. Outputs all findings as actionable items to a Markdown report file. Invoke as: /skill:rzlv-code-review ' +--- + +Follow the instructions in ./workflow.md. + diff --git a/.github/skills/rzlv-code-review/steps/step-01-gather-changes.md b/.github/skills/rzlv-code-review/steps/step-01-gather-changes.md new file mode 100644 index 0000000..8af630e --- /dev/null +++ b/.github/skills/rzlv-code-review/steps/step-01-gather-changes.md @@ -0,0 +1,110 @@ +--- +target_branch: '' # set from invocation argument +current_branch: '' # set at runtime via git +diff_output: '' # set at runtime +changed_files: '' # set at runtime +report_path: '' # set at runtime +jira_ticket_key: '' # set at runtime if branch contains a ticket number (e.g. BD-7558) +jira_ticket_summary: '' # set at runtime from Jira API +jira_ticket_body: '' # set at runtime — full ticket description + acceptance criteria +jira_ticket_status: '' # set at runtime +--- + +# Step 1: Gather Changes + +## RULES + +- Do not modify any source files. This step is read-only. +- All git commands must be run in the project root. +- **ALWAYS** use `git --no-pager` for every git command. Never omit `--no-pager`. Git may invoke a pager (`less`, `more`) on any command that produces output, which will block the terminal indefinitely. + +## INSTRUCTIONS + +### 1. Confirm target branch + +- Set `{target_branch}` from the value parsed in workflow initialization. +- Run `git --no-pager branch -a | cat` to list branches, then check whether `{target_branch}` appears. + - If not found locally, also run `git --no-pager ls-remote --heads origin {target_branch} | cat` to check remote. + - If neither check finds the branch, HALT and tell the user: "Branch `{target_branch}` was not found. Please check the branch name and try again." + +### 2. Identify current branch + +- Run `git rev-parse --abbrev-ref HEAD` (no pager needed — single-line output) and store as `{current_branch}`. +- If the result is `HEAD` (detached HEAD state), HALT and warn the user. + +### 3. Produce the diff + +- Run: `git --no-pager diff {target_branch}...HEAD` + - This shows all commits on `{current_branch}` that are **not** on `{target_branch}` (three-dot diff = changes introduced by this branch). +- Store the full output as `{diff_output}`. +- If `{diff_output}` is empty, HALT and tell the user: "No changes found between `{current_branch}` and `{target_branch}`. Nothing to review." + +### 4. Build changed-files list and summary stats + +- Run: `git --no-pager diff --name-only {target_branch}...HEAD` +- Store as `{changed_files}` (newline-separated list). +- Run: `git --no-pager diff --stat {target_branch}...HEAD | tail -1` + - This produces the summary line only (e.g. `12 files changed, 340 insertions(+), 45 deletions(-)`). Using `tail -1` avoids piping the full per-file table. +- Present a short summary to the user: + > Reviewing `{current_branch}` vs `{target_branch}`: + > - **Files changed:** + > - **Lines added:** <+N> + > - **Lines removed:** <-N> + +### 5. Large diff warning + +- If `{diff_output}` exceeds approximately 3000 lines, warn the user: + > ⚠️ This diff is large (~N lines). The review will proceed but may take a while. Consider chunking if context limits are hit. +- Continue regardless — do not block. + +### 6. Detect Jira ticket number + +- Scan `{current_branch}` for a pattern matching a Jira ticket key: one or more uppercase letters, a hyphen, then one or more digits (regex: `[A-Z]+-\d+`). + - Examples: `ak/BD-7558` → `BD-7558` | `feature/PROJ-123-some-description` → `PROJ-123` +- If a match is found: + - Set `{jira_ticket_key}` to the matched value (e.g. `BD-7558`). + - Announce: "🎫 Jira ticket detected: `{jira_ticket_key}`. Fetching ticket details…" + - Proceed to instruction 6a. +- If no match is found: + - Set `{jira_ticket_key}` = `""` (empty). + - Announce: "ℹ️ No Jira ticket number found in branch name. Skipping Jira compliance review." + - Skip to instruction 7. + +#### 6a. Fetch Jira ticket via Atlassian MCP + +1. Call `mcp_atlassian_getAccessibleAtlassianResources` to discover the available Jira cloud ID. + - Use the first Jira resource returned and store its `id` as `{jira_cloud_id}`. + - If the call fails or returns no resources, note the failure, set `{jira_ticket_key}` = `""`, and skip to instruction 7. + +2. Call `mcp_atlassian_getJiraIssue` with: + - `cloudId`: `{jira_cloud_id}` + - `issueIdOrKey`: `{jira_ticket_key}` + - `responseContentFormat`: `"markdown"` + +3. If the call succeeds: + - Set `{jira_ticket_summary}` = the issue `summary` field. + - Set `{jira_ticket_status}` = the issue `status.name` field. + - Set `{jira_ticket_body}` = the full issue `description` field (markdown). If the issue has an `Acceptance Criteria` custom field or a section labelled "Acceptance Criteria" in the description, extract and preserve it verbatim — it will be a primary review lens. + - Announce: + > 🎫 **Jira ticket loaded:** [`{jira_ticket_key}`] {jira_ticket_summary} + > **Status:** {jira_ticket_status} + +4. If the call fails (ticket not found, permission denied, network error): + - Warn the user: "⚠️ Could not fetch Jira ticket `{jira_ticket_key}`: . Jira compliance review will be skipped." + - Set `{jira_ticket_key}` = `""`. + +### 7. Determine report output path + +- Set `{report_path}` = `.github/reviews/code-review-{current_branch}-{date}.md` + - Replace `/` characters in `{current_branch}` with `-` to keep it filesystem-safe. + - Format `{date}` as `YYYY-MM-DD`. + - If `{jira_ticket_key}` is non-empty, include it for clarity: + `.github/reviews/code-review-{jira_ticket_key}-{date}.md` +- Announce the output path to the user: + > 📄 Report will be written to: `{report_path}` + + +## NEXT + +Read fully and follow `./step-02-rzlv-review.md` + diff --git a/.github/skills/rzlv-code-review/steps/step-02-rzlv-review.md b/.github/skills/rzlv-code-review/steps/step-02-rzlv-review.md new file mode 100644 index 0000000..1a9360d --- /dev/null +++ b/.github/skills/rzlv-code-review/steps/step-02-rzlv-review.md @@ -0,0 +1,139 @@ +--- +rzlv_findings: [] # accumulated findings list — append throughout this step +--- + +# Step 2: rzlv Standards Review + +## RULES + +- Review `{diff_output}` through **every** rzlv- skill lens listed below. Do not skip any. +- For each lens: load the skill's `SKILL.md`, apply its guidelines to the diff, collect violations. +- A finding must reference a specific file and line range from the diff if possible. +- If a skill's guidelines are not violated by anything in the diff, record it as ✅ clean for that lens. + +## FINDING FORMAT + +Each finding must be structured as: + +``` +- **[]** `:` — + > +``` + +If no violations are found for a lens: +``` +- ✅ **[]** No violations found. +``` + + +## INSTRUCTIONS + +Work through each lens in sequence. For each lens: +1. Read the referenced `SKILL.md` file fully. +2. Scan `{diff_output}` for violations of the guidelines in that skill. +3. Append all findings (or the clean ✅ note) to `{rzlv_findings}`. + +Process lenses in this order: + +--- + +### Lens 1 — Algorithm Precision Handling +**Skill file:** `.github/skills/rzlv-algorithm-precision-handling/SKILL.md` + +Look for: incorrect mathematical formulas, missing edge case validation (zero, negative, infinite values), unsafe `split()` where `substring()` is needed, missing unit conversions (e.g. degrees → radians). + +--- + +### Lens 2 — Avoid Breaking Changes +**Skill file:** `.github/skills/rzlv-avoid-breaking-changes/SKILL.md` + +Look for: new required parameters added to existing public methods, changed default values, removed or renamed APIs without `@Deprecated` wrappers, deprecation annotations missing a migration message or version reference. + +--- + +### Lens 3 — Eliminate Redundant Operations +**Skill file:** `.github/skills/rzlv-eliminate-redundant-operations/SKILL.md` + +Look for: redundant null/validation checks duplicating what the callee already handles, unnecessary object allocations inside hot paths (message handlers, `build()`, frequent callbacks), expensive ancestor-chain lookups inside `build()`, duplicate calculations within a single execution path, dead fallback code made obsolete by API changes. + +--- + +### Lens 4 — Explain Non-Obvious Code +**Skill file:** `.github/skills/rzlv-explain-non-obvious-code/SKILL.md` + +Look for: magic numeric literals without explanatory comments, parameter constraints or interactions undocumented in dartdoc, implementation decisions that are not self-explanatory, platform-specific branches without annotation, assumed input invariants not documented. + +--- + +### Lens 5 — Explicit Null Validation +**Skill file:** `.github/skills/rzlv-explicit-null-validation/SKILL.md` + +Look for: `?? fallback` expressions masking unexpected nulls, nullable parameters interacting with predicates or invariants without explicit assertion, numeric inputs used in arithmetic without `isFinite`/`isNaN` checks, missing callbacks silently ignored instead of asserted. + +--- + +### Lens 6 — Extract Methods for Clarity +**Skill file:** `.github/skills/rzlv-extract-methods-for-clarity/SKILL.md` + +Look for: methods longer than ~35 lines, `if/else` branches each containing more than ~8 lines of complex logic, repeated initialization or computation patterns across multiple call sites, inline boolean conditions requiring significant parsing. + +--- + +### Lens 7 — Future-Proof Configuration Defaults +**Skill file:** `.github/skills/rzlv-future-proof-configuration-defaults/SKILL.md` + +Look for: non-negatable boolean flags, hardcoded numeric range limits or operational constants that should be configurable properties, ambiguous nullable config fields (null vs. explicitly-disabled), deprecated options missing a replacement and removal timeline. + +--- + +### Lens 8 — Make Errors Explicit +**Skill file:** `.github/skills/rzlv-make-errors-explicit/SKILL.md` + +Look for: silent default returns (e.g. `return 0`, `return null`) on unexpected code paths instead of assertions, unchecked results of `find()` / `firstWhere()` / map lookups before use, caught exceptions with no logging or re-throw, impossible states silently skipped instead of asserted or logged. + +--- + +### Lens 9 — Manage State Dependencies Properly +**Skill file:** `.github/skills/rzlv-manage-state-dependencies-properly/SKILL.md` + +Look for: `Overlay.of`, `Navigator.maybeOf`, or similar context lookups that don't establish a rebuild dependency, `setState` or state-mutating callbacks invoked during `build()` or `didChangeDependencies`, expensive or reactive context lookups not cached in `didChangeDependencies`. + +--- + +### Lens 10 — Test Observable Behavior +**Skill file:** `.github/skills/rzlv-test-observable-behavior/SKILL.md` + +Look for: tests asserting internal controller values or private state instead of rendered output, `@visibleForTesting` on production code added solely for test assertions, tests that would break on internal refactors while the user-facing behavior remains unchanged. + +--- + +### Lens 11 — Thread Safety & Synchronization +**Skill file:** `.github/skills/rzlv-thread-safety-synchronization/SKILL.md` + +Look for: writes to shared data structures without a mutex or equivalent lock, handlers/callbacks registered and not unregistered before object destruction, logically related multi-step operations split across different task runners/threads, missing `_destroyed` or `_invalidated` guards on async callbacks. + +--- + +### Lens 12 — Use Descriptive Names +**Skill file:** `.github/skills/rzlv-use-descriptive-names/SKILL.md` + +Look for: function names describing mechanism rather than purpose, variables using abbreviations or single letters, type names reflecting implementation details instead of semantics, missing `k` prefix on Dart/C++ constants, callback parameters missing `handle` prefix, anonymous `_` parameters where a name would clarify meaning. + +--- + +### Lens 13 — Use Named Constants +**Skill file:** `.github/skills/rzlv-use-named-constants/SKILL.md` + +Look for: string literals repeated more than once or used as identifiers/prefixes, magic numeric values whose purpose requires surrounding context to understand, file paths or resource names hardcoded inline, constants not following the language's naming convention (`k` prefix in Dart/C++, `ALL_CAPS` in Java/Kotlin). + +--- + +## COMPLETION + +Once all 13 lenses are processed, confirm: +> ✅ All 13 rzlv lenses applied. Proceeding to general review. + + +## NEXT + +Read fully and follow `./step-03-general-review.md` diff --git a/.github/skills/rzlv-code-review/steps/step-03-general-review.md b/.github/skills/rzlv-code-review/steps/step-03-general-review.md new file mode 100644 index 0000000..31049d1 --- /dev/null +++ b/.github/skills/rzlv-code-review/steps/step-03-general-review.md @@ -0,0 +1,135 @@ +--- +general_findings: [] # accumulated findings from general review — append throughout this step +jira_findings: [] # accumulated findings from Jira ticket compliance — append if ticket present +--- + +# Step 3: General Code Review + +## RULES + +- Review `{diff_output}` from a broad engineering perspective, independent of the rzlv lenses. +- Focus on issues that the individual rzlv lenses do not already cover. +- Avoid duplicate findings — if an issue is already captured in `{rzlv_findings}`, do not repeat it here. + +## FINDING FORMATS + +General finding: +``` +- **[GENERAL/]** `:` — + > + > **Severity:** Critical | High | Medium | Low +``` + +Jira compliance finding: +``` +- **[JIRA/{jira_ticket_key}/]** — + > + > **Severity:** Critical | High | Medium | Low +``` + +Use `` values: `MISSING` (requirement not implemented), `MISMATCH` (implemented differently than spec), `OUT_OF_SCOPE` (change not related to ticket), `INCOMPLETE` (partial implementation). + +Use these General categories: `LOGIC`, `ARCHITECTURE`, `PERFORMANCE`, `SECURITY`, `CORRECTNESS`, `MAINTAINABILITY`, `CONCURRENCY`, `TESTING`, `DOCUMENTATION`. + + +## INSTRUCTIONS + +### 0. Jira Ticket Compliance (run only if `{jira_ticket_key}` is non-empty) + +Skip this section entirely if `{jira_ticket_key}` is empty and proceed directly to section 1. + +You have the following ticket context: +- **Key:** `{jira_ticket_key}` +- **Summary:** `{jira_ticket_summary}` +- **Status:** `{jira_ticket_status}` +- **Description & Acceptance Criteria:** `{jira_ticket_body}` + +Evaluate `{diff_output}` against the ticket. Produce findings in `{jira_findings}` for each of the following: + +#### 0a. Requirements coverage +- Read every requirement, user story sentence, and acceptance criterion in `{jira_ticket_body}`. +- For each one: determine whether the diff contains code that implements it. +- If a requirement has **no corresponding change** in the diff, raise a `MISSING` finding. +- If a requirement is **partially addressed** (e.g. success path implemented but error path not), raise an `INCOMPLETE` finding. + +#### 0b. Implementation fidelity +- Where the diff does implement ticket requirements, verify the implementation matches the spec intent. +- If behaviour, data structures, API signatures, or logic deviate from what the ticket describes, raise a `MISMATCH` finding. + +#### 0c. Scope creep +- Identify changes in the diff that are **not** mentioned or implied by the ticket. +- If a change is unrelated to the ticket, raise an `OUT_OF_SCOPE` finding with justification. +- Minor refactors incidental to the main change do not need to be flagged unless they are substantial. + +#### 0d. Ticket status sanity check +- If `{jira_ticket_status}` indicates the ticket is `Done`, `Closed`, or `Cancelled`, warn: + > ⚠️ Ticket `{jira_ticket_key}` has status `{jira_ticket_status}`. Verify this is the correct ticket for these changes. + +After processing all sub-sections, summarise: +> ✅ Jira compliance review complete for `{jira_ticket_key}`. Found: MISSING, MISMATCH, OUT_OF_SCOPE, INCOMPLETE. + +--- + +Carefully scan `{diff_output}` and produce findings for each of the areas below. If an area has no issues, note that explicitly. Use your own judgment — this is a broad, experienced-engineer review. + +### 1. Correctness & Logic + +- Off-by-one errors, wrong operator precedence, incorrect conditionals +- Misuse of standard library functions +- Data mutations with unintended side effects +- Missing return values or unreachable code paths + +### 2. Architecture & Design + +- Violations of separation of concerns +- Tight coupling between widgets, services, or modules +- God classes / functions with too many responsibilities +- Breaking of established patterns visible in the codebase +- Widget responsibilities that belong in a state-management layer + +### 3. Security + +- Input validation gaps (unvalidated user/network input flowing into sensitive operations) +- Hardcoded credentials, tokens, or secrets +- Insecure data storage or logging of sensitive information +- Missing authentication/authorisation checks on new code paths +- Use of deprecated or insecure cryptographic methods + +### 4. Performance + +- Unnecessary object allocations inside `build()` or other hot paths +- Blocking calls on the main isolate (synchronous I/O, heavy computation without `compute()`) +- Missing stream subscription cancellation causing memory leaks +- Expensive operations (network calls, DB queries) triggered on every rebuild + +### 5. Concurrency & Thread Safety + +- Mutable shared state accessed from multiple isolates or platform threads without synchronisation +- Stream or `Future` subscription leaks (subscriptions started but never cancelled in `dispose()`) +- Race conditions between platform thread operations and the Dart UI thread +- Missing `mounted` checks before calling `setState` after an `await` + +### 6. Error Handling & Resilience + +- Swallowed exceptions (`catch (e) { }` with no action) +- Missing error propagation to callers +- Crash-prone code paths with no fallback (unguarded async I/O without try-catch) +- `Future` errors not surfaced to the user or logged + +### 7. Test Coverage + +- New logic paths not covered by accompanying tests +- Tests that verify internal implementation rather than observable behavior +- Tests that depend on execution order or share mutable state +- Missing widget tests for new UI components + +### 8. Documentation & Readability + +- Missing dartdoc (`///`) on new public APIs +- Misleading, stale, or contradictory comments +- TODO/FIXME comments introduced without a tracking issue reference + + +## NEXT + +Read fully and follow `./step-04-consolidate-and-report.md` diff --git a/.github/skills/rzlv-code-review/steps/step-04-consolidate-and-report.md b/.github/skills/rzlv-code-review/steps/step-04-consolidate-and-report.md new file mode 100644 index 0000000..6ce4fd4 --- /dev/null +++ b/.github/skills/rzlv-code-review/steps/step-04-consolidate-and-report.md @@ -0,0 +1,209 @@ +--- +--- + +# Step 4: Consolidate and Write Report + +## RULES + +- Write the report to `{report_path}`. Create parent directories if needed. +- Do not modify any source code files — this step is report-writing only. +- Every finding must be an actionable checklist item so the developer can work through them one by one. +- Deduplicate: if an rzlv finding and a general finding describe the same issue at the same location, merge them into a single item and note both sources. +- Include `{jira_findings}` in the deduplicated pool — a Jira finding and a general finding about the same gap should be merged. + +## INSTRUCTIONS + +### 1. Deduplicate findings + +- Combine `{rzlv_findings}`, `{general_findings}`, and `{jira_findings}` into one pool. +- For any finding that refers to the same file, line range, and root cause, merge into one entry. Mark its source as both (e.g., `[rzlv-explicit-null-validation + GENERAL/CORRECTNESS]`). +- Count totals: `{rzlv_count}`, `{general_count}`, `{jira_count}`, `{merged_count}`, `{total_count}`. + +### 2. Classify severity (if not already set) + +- For each finding without an explicit severity, assign one: + - **Critical** — crash risk, data loss, or security vulnerability + - **High** — significant correctness or performance issue + - **Medium** — maintainability, code-quality, or standards violation + - **Low** — style, naming, or minor nit + +### 3. Sort findings + +Order the final list as: +1. Critical (top priority) +2. High +3. Medium +4. Low +5. ✅ Clean lenses (informational, kept at the bottom) + +### 4. Write the report file + +Create the file at `{report_path}` with the following structure: + +--- + +```markdown +# Code Review Report + +**Branch:** `{current_branch}` +**Compared to:** `{target_branch}` +**Date:** {date} +**Files changed:** | **Lines:** + / - + +**Jira Ticket:** [{jira_ticket_key}] {jira_ticket_summary} *(Status: {jira_ticket_status})* + +--- + +## Summary + +| Category | Count | +|------------------|-------| +| 🔴 Critical | N | +| 🟠 High | N | +| 🟡 Medium | N | +| 🔵 Low | N | +| **Total issues** | **N** | +| ✅ Clean lenses | N | + +--- + +## Jira Ticket Compliance + + + +**Ticket:** [{jira_ticket_key}] {jira_ticket_summary} +**Status:** {jira_ticket_status} + +| Type | Count | +|----------------|-------| +| ❌ Missing | N | +| ⚠️ Mismatch | N | +| 🔵 Incomplete | N | +| 🟡 Out of scope | N | + +### Compliance Action Items + +- [ ] **[JIRA/{jira_ticket_key}/MISSING]** — + > <Detail> + +- [ ] **[JIRA/{jira_ticket_key}/MISMATCH]** — <Title> + > <Detail> + +*(repeat for all jira findings, grouped by type; omit types with zero findings)* + +--- + +## Action Items + +> Each item below is an independent task. Check it off once resolved. + +### 🔴 Critical + +- [ ] **[<SOURCE>]** `<file>:<line>` — <Title> + > <Detail> + +*(repeat for all Critical findings, or omit section if none)* + +--- + +### 🟠 High + +- [ ] **[<SOURCE>]** `<file>:<line>` — <Title> + > <Detail> + +*(repeat for all High findings, or omit section if none)* + +--- + +### 🟡 Medium + +- [ ] **[<SOURCE>]** `<file>:<line>` — <Title> + > <Detail> + +*(repeat for all Medium findings, or omit section if none)* + +--- + +### 🔵 Low + +- [ ] **[<SOURCE>]** `<file>:<line>` — <Title> + > <Detail> + +*(repeat for all Low findings, or omit section if none)* + +--- + +## rzlv Standards Coverage + +List every rzlv lens and its outcome: + +| Lens | Result | +|------|--------| +| rzlv-algorithm-precision-handling | ✅ Clean / ⚠️ N finding(s) | +| rzlv-avoid-breaking-changes | ✅ Clean / ⚠️ N finding(s) | +| rzlv-eliminate-redundant-operations | ✅ Clean / ⚠️ N finding(s) | +| rzlv-explain-non-obvious-code | ✅ Clean / ⚠️ N finding(s) | +| rzlv-explicit-null-validation | ✅ Clean / ⚠️ N finding(s) | +| rzlv-extract-methods-for-clarity | ✅ Clean / ⚠️ N finding(s) | +| rzlv-future-proof-configuration-defaults | ✅ Clean / ⚠️ N finding(s) | +| rzlv-make-errors-explicit | ✅ Clean / ⚠️ N finding(s) | +| rzlv-manage-state-dependencies-properly | ✅ Clean / ⚠️ N finding(s) | +| rzlv-test-observable-behavior | ✅ Clean / ⚠️ N finding(s) | +| rzlv-thread-safety-synchronization | ✅ Clean / ⚠️ N finding(s) | +| rzlv-use-descriptive-names | ✅ Clean / ⚠️ N finding(s) | +| rzlv-use-named-constants | ✅ Clean / ⚠️ N finding(s) | + +--- + +## General Review Coverage + +| Area | Result | +|-----------------------------|--------| +| Correctness & Logic | ✅ Clean / ⚠️ N finding(s) | +| Architecture & Design | ✅ Clean / ⚠️ N finding(s) | +| Security | ✅ Clean / ⚠️ N finding(s) | +| Performance | ✅ Clean / ⚠️ N finding(s) | +| Concurrency & Thread Safety | ✅ Clean / ⚠️ N finding(s) | +| Error Handling & Resilience | ✅ Clean / ⚠️ N finding(s) | +| Test Coverage | ✅ Clean / ⚠️ N finding(s) | +| Documentation & Readability | ✅ Clean / ⚠️ N finding(s) | + +--- + +*Generated by rzlv-code-review skill* +``` + +--- + +### 5. Confirm write + +After writing the file, announce: + +> ✅ **Code review complete.** +> +> **Report written to:** `{report_path}` +> +> | Severity | Issues | +> |-----------|--------| +> | 🔴 Critical | N | +> | 🟠 High | N | +> | 🟡 Medium | N | +> | 🔵 Low | N | +> | **Total** | **N** | +> +> <!-- Include if {jira_ticket_key} is non-empty --> +> **Jira compliance:** <N_MISSING> missing, <N_MISMATCH> mismatch, <N_INCOMPLETE> incomplete, <N_OOS> out-of-scope +> +> Open `{report_path}` to work through the action items. + +### 6. Offer next steps + +Present: + +> **What would you like to do next?** +> 1. **Fix issues now** — I will start applying fixes from the top of the action list +> 2. **Walk through findings** — I will explain each one before touching code +> 3. **Done** — the report is saved, I will stop here + +**HALT** — wait for the user's choice before taking any further action. + diff --git a/.github/skills/rzlv-code-review/workflow.md b/.github/skills/rzlv-code-review/workflow.md new file mode 100644 index 0000000..7b0a8c1 --- /dev/null +++ b/.github/skills/rzlv-code-review/workflow.md @@ -0,0 +1,49 @@ +--- +skills_root: '{project-root}/.github/skills' +report_output: '{project-root}/.github/reviews' +--- + +# rzlv-code-review Workflow + +**Goal:** Perform a comprehensive, multi-lens code review of the current branch vs a target branch. Each rzlv- coding standard is applied as an independent review lens. A general review pass is also performed. All findings are written to an actionable Markdown report. + +**Your Role:** You are an elite code reviewer with deep knowledge of Flutter and Dart best practices. You gather changes, systematically evaluate them through every rzlv coding-standard lens, perform a broad code review, consolidate all findings, and produce a clear, actionable report. No noise, no filler. + +**Invocation format:** `/skill:rzlv-code-review <target-branch>` +Example: `/skill:rzlv-code-review dev/18.1.0` + + +## WORKFLOW ARCHITECTURE + +This uses **step-file architecture** for disciplined execution: + +- **Sequential Enforcement**: Complete steps in order, no skipping +- **Just-In-Time Loading**: Only load the current step file when ready for it +- **Append-Only Building**: Build the findings list incrementally across steps + +### Step Processing Rules + +1. **READ COMPLETELY**: Read the entire step file before acting +2. **FOLLOW SEQUENCE**: Execute sections in order +3. **LOAD NEXT**: When directed, read fully and follow the next step file + +### Critical Rules (NO EXCEPTIONS) + +- **NEVER** load multiple step files simultaneously +- **ALWAYS** read entire step file before execution +- **NEVER** skip steps or change their sequence +- **ALWAYS** apply every rzlv- skill lens — do not skip any + + +## INITIALIZATION + +Parse the invocation text for the target branch argument: +- Extract the first token after `/skill:rzlv-code-review` (e.g., `dev/18.1.0`) +- Store as `{target_branch}` +- If no argument is present, HALT and ask: "Which branch should I diff against? (e.g., `dev/18.1.0`)" + + +## STEP SEQUENCE + +Read fully and follow `./steps/step-01-gather-changes.md` to begin. + diff --git a/.github/skills/rzlv-eliminate-redundant-operations/SKILL.md b/.github/skills/rzlv-eliminate-redundant-operations/SKILL.md new file mode 100644 index 0000000..c0d1de5 --- /dev/null +++ b/.github/skills/rzlv-eliminate-redundant-operations/SKILL.md @@ -0,0 +1,76 @@ +--- +name: 'rzlv-eliminate-redundant-operations' +description: 'Review Flutter/Dart code for unnecessary operations in hot paths: redundant checks, duplicate calculations, unnecessary allocations, and obsolete code paths that hurt performance.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Eliminate Redundant Operations + +## Purpose + +Identify and flag unnecessary operations that impact performance. Focus especially on hot paths such as widget `build()` methods, message handlers, and frequently called callbacks where repeated redundant work accumulates. + +## When to Apply + +Trigger this skill when a PR touches: +- Widget `build()` or `rebuild()` methods +- Message handlers or event listeners +- Methods called on every frame or user interaction +- Data structure initialization inside loops or callbacks +- Validation or guard checks before calling other functions + +## Key Practices + +1. **Remove redundant null/validation checks** that are already handled by the called function: + ```dart + // Avoid — parseFloat already handles NaN + if (parsed != null && !parsed.isNaN) { styleProperty = parsed; } + + // Better — let parseFloat handle it + styleProperty = parsed; + ``` + +2. **Avoid unnecessary allocations in hot paths** — don't create new objects (e.g., `List.from(...)`) on every message or event: + ```dart + // Avoid — allocates a new list on every message + final List<Handler> handlers = List<Handler>.from(_messageHandlers); + + // Better — use debug assertions to guard against modification during iteration + ``` + +3. **Move expensive operations out of `build()`** — ancestor chain walks (`Navigator.maybeOf`, `InheritedWidget` lookups) should live in `didChangeDependencies` or be cached: + ```dart + // Avoid — walks ancestor chain on every rebuild + if (Navigator.maybeOf(context, rootNavigator: true) != this) { ... } + + // Better — move to didChangeDependencies and cache the result + ``` + +4. **Avoid duplicate calculations** — restructure branches so each code path computes a value at most once: + ```dart + // Avoid — lerpValue computed then divided again + lerpValue = (lerpValue * widget.divisions!).round() / widget.divisions!; + + // Better — use if-else with a single computation per path + ``` + +5. **Remove redundant code paths** — delete fallback logic made obsolete by API changes. + +## Review Checklist + +- [ ] Are there null/type checks that duplicate what the called API already validates? +- [ ] Are new objects allocated inside loops or frequent callbacks? +- [ ] Are expensive lookups (ancestor walks, map lookups) happening inside `build()`? +- [ ] Is the same value computed more than once within a single execution path? +- [ ] Is any fallback code now dead after recent API changes? + +## Output + +Raise findings as: +- 🔴 **Blocking** — causes measurable jank or memory pressure in production hot paths +- 🟡 **Important** — redundant work that adds up over many calls +- 🔵 **Minor** — style-level simplification with minor perf benefit + diff --git a/.github/skills/rzlv-explain-non-obvious-code/SKILL.md b/.github/skills/rzlv-explain-non-obvious-code/SKILL.md new file mode 100644 index 0000000..ec9d419 --- /dev/null +++ b/.github/skills/rzlv-explain-non-obvious-code/SKILL.md @@ -0,0 +1,72 @@ +--- +name: 'rzlv-explain-non-obvious-code' +description: 'Review Flutter/Dart code for missing explanatory comments: magic numbers, implementation decisions, parameter relationships, edge cases, and platform-specific behavior that are not self-evident.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Explain Non-Obvious Code + +## Purpose + +Flag code that lacks sufficient explanatory comments. Non-obvious logic, magic numbers, implementation decisions, and parameter constraints should always be documented to aid future maintainers and reviewers. + +## When to Apply + +Trigger this skill when a PR introduces or modifies: +- Hard-coded numeric constants without explanation +- Complex conditional logic or bitwise operations +- Platform-specific workarounds or version guards +- API parameters with non-obvious constraints or interactions +- Algorithms whose correctness is not immediately apparent + +## Key Practices + +- **Document magic numbers and constants** — explain how the value was derived and its significance: + ```dart + // Bad: No explanation + TextSelectionHandleType.collapsed => const Offset(_kHandleSize / 2.18, -4.5), + + // Good: Explains origin + // These values were eyeballed to match a physical Pixel 9 running Android 16, + // which horizontally centers the anchor 1 pixel below the cursor. + TextSelectionHandleType.collapsed => const Offset(_kHandleSize / 2.18, -4.5), + ``` + +- **Document parameter relationships and edge-case behavior**: + ```dart + // Bad: Unclear parameter purpose + void hideToolbar([bool hideHandles = true, Duration? toggleDebounceDuration]); + + // Good: Fully documented + /// Hides the text selection toolbar. + /// + /// When [hideHandles] is false, the toolbar is hidden but handles remain visible. + /// When [toggleDebounceDuration] is non-null, a subsequent call to [toggleToolbar] + /// will not show the toolbar unless the duration threshold has been exceeded. + void hideToolbar([bool hideHandles = true, Duration? toggleDebounceDuration]); + ``` + +- **Clarify implementation decisions** — explain *why* a specific approach was chosen, not just *what* it does. + +- **Document assumed constraints** — if a method only supports a subset of theoretically possible inputs (e.g., `baseLabel` is always a prefix of `currentLabel`), say so explicitly. + +- **Note platform differences** — document behavior differences across platforms or SDK versions. + +## Review Checklist + +- [ ] Are all numeric literals either obvious or commented? +- [ ] Are parameter constraints and interactions documented in dartdoc? +- [ ] Is the *reason* for non-obvious logic explained (not just restated)? +- [ ] Are platform-specific branches annotated? +- [ ] Are assumptions about input values or invariants documented? + +## Output + +Raise findings as: +- 🔴 **Blocking** — absent comment makes the code dangerous to modify safely +- 🟡 **Important** — non-obvious logic that will confuse future maintainers +- 🔵 **Minor** — nice-to-have clarification that would improve readability + diff --git a/.github/skills/rzlv-explicit-null-validation/SKILL.md b/.github/skills/rzlv-explicit-null-validation/SKILL.md new file mode 100644 index 0000000..ea40d30 --- /dev/null +++ b/.github/skills/rzlv-explicit-null-validation/SKILL.md @@ -0,0 +1,77 @@ +--- +name: 'rzlv-explicit-null-validation' +description: 'Review Flutter/Dart code for silent null fallbacks: flag implicit null handling that should instead use assertions, explicit checks, or fast-fail patterns to surface programming errors early.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Explicit Null Validation + +## Purpose + +Flag silent null fallbacks and implicit null handling. When a null value indicates a programming error, the code should fail fast with a clear message rather than silently substituting a default value and masking the bug. + +## When to Apply + +Trigger this skill when a PR: +- Uses `?? SomeFallback()` where null would indicate a caller error +- Optionally calls a callback without documenting the null case +- Performs numeric operations without checking for `isFinite`, `isNaN`, or zero +- Adds nullable parameters that interact with non-null predicates or invariants + +## Key Practices + +- **Prefer `assert` over silent fallbacks** when null is unexpected: + ```dart + // Bad — silent fallback hides caller error + selectableDayPredicate?.call(initialDateTime ?? DateTime.now()) + + // Good — explicit assertion surfaces the issue + assert( + selectableDayPredicate == null || + initialDate == null || + selectableDayPredicate!(initialDate!), + 'Initial date must be selectable when predicate is provided' + ); + ``` + +- **Assert unexpected nulls before optional dispatch**: + ```dart + // Bad — silent no-op if callback is missing + final VoidCallback? callback = callbacks[actionId]; + if (callback != null) { callback(); } + + // Good — assert the expected state, then call + final VoidCallback? callback = callbacks[actionId]; + assert(callback != null, 'No callback registered for action: $actionId'); + callback?.call(); + ``` + +- **Validate numeric values explicitly**: + ```dart + // Bad — silently skips on invalid values + if (itemExtent > 0.0) { ... } + + // Good — asserts the expected precondition + assert(scrollOffset.isFinite && itemExtent.isFinite, + 'scrollOffset and itemExtent must be finite'); + if (itemExtent > 0.0) { ... } + ``` + +## Review Checklist + +- [ ] Are `?? fallback` expressions intentional, or are they masking unexpected nulls? +- [ ] Are nullable parameters that interact with predicates or invariants validated explicitly? +- [ ] Are numeric inputs checked for `isFinite`, `isNaN`, and zero before arithmetic? +- [ ] Are missing callbacks asserted rather than silently ignored? +- [ ] Do assertion messages provide actionable context? + +## Output + +Raise findings as: +- 🔴 **Blocking** — silent fallback masks a caller error that will be hard to debug in production +- 🟡 **Important** — null case is plausible but undocumented and unasserted +- 🔵 **Minor** — defensive check that would improve clarity or debuggability + diff --git a/.github/skills/rzlv-extract-methods-for-clarity/SKILL.md b/.github/skills/rzlv-extract-methods-for-clarity/SKILL.md new file mode 100644 index 0000000..01f5be8 --- /dev/null +++ b/.github/skills/rzlv-extract-methods-for-clarity/SKILL.md @@ -0,0 +1,74 @@ +--- +name: 'rzlv-extract-methods-for-clarity' +description: 'Review Flutter/Dart code for large methods and repeated patterns that should be broken into smaller, well-named private methods or getters to improve readability and testability.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Extract Methods for Clarity + +## Purpose + +Flag large methods, deeply nested conditionals, and repeated code patterns that reduce readability and make testing harder. Recommend well-named private methods or getters that express *what* is happening rather than *how*. + +## When to Apply + +Trigger this skill when a PR introduces or modifies: +- Methods longer than ~30–40 lines +- `if/else` blocks containing complex logic in both branches +- Repeated code patterns across multiple call sites +- Inline boolean conditions that require significant mental parsing +- C++ or Dart functions with duplicated initialization patterns + +## Key Practices + +1. **Extract large if/else branches into focused methods**: + ```dart + // Before: 20+ lines inline per branch + if (hasNewline) { /* complex logic */ } else { /* different complex logic */ } + + // After: Self-documenting extraction + final Path path = hasNewline + ? _createMultilineIndicatorPath() + : _createSingleLineIndicatorPath(); + ``` + +2. **Extract repeated initialization patterns into helper methods**: + ```cpp + // Before: Same parameters repeated + auto data_host_buffer = HostBuffer::Create(allocator, waiter, alignment); + auto indexes_host_buffer = needsPartition + ? HostBuffer::Create(allocator, waiter, alignment) + : data_host_buffer; + + // After: Single helper encapsulates the pattern + auto [data_host_buffer, indexes_host_buffer] = createHostBuffers(); + ``` + +3. **Extract inline boolean conditions into descriptive getters**: + ```dart + // Before: Requires reading two fields to understand intent + if (widget.separatorBuilder != null && index.isOdd) { ... } + + // After: Intent is immediately clear + bool get hasSeparators => widget.separatorBuilder != null; + bool get isSeparator => hasSeparators && index.isOdd; + ``` + +## Review Checklist + +- [ ] Are there methods longer than ~35 lines that could be split by responsibility? +- [ ] Do `if/else` branches each contain more than ~5–8 lines of logic? +- [ ] Is the same initialization or computation pattern repeated more than twice? +- [ ] Do inline boolean conditions require more than a single glance to understand? +- [ ] Would extracting a getter or method make the call site self-documenting? + +## Output + +Raise findings as: +- 🔴 **Blocking** — method is so large it is unmaintainable and hides logic errors +- 🟡 **Important** — extraction would significantly reduce cognitive load and enable unit testing +- 🔵 **Minor** — style-level extraction that would improve readability + diff --git a/.github/skills/rzlv-future-proof-configuration-defaults/SKILL.md b/.github/skills/rzlv-future-proof-configuration-defaults/SKILL.md new file mode 100644 index 0000000..f099d9c --- /dev/null +++ b/.github/skills/rzlv-future-proof-configuration-defaults/SKILL.md @@ -0,0 +1,72 @@ +--- +name: 'rzlv-future-proof-configuration-defaults' +description: 'Review Flutter/Dart configuration design for hardcoded values, non-negatable flags, ambiguous null vs empty states, and missing migration paths that would prevent safe default changes in future versions.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Future-Proof Configuration Defaults + +## Purpose + +Flag configuration design choices that will be difficult or breaking to change later. Ensure boolean flags are negatable, values are configurable rather than hardcoded, null and empty states are distinguishable, and deprecated options have clear migration paths. + +## When to Apply + +Trigger this skill when a PR: +- Adds new CLI flags or build configuration options +- Hardcodes range limits (min/max) or operational constants +- Introduces nullable configuration fields +- Deprecates or removes existing configuration options +- Adds boolean parameters where the default might reasonably change in future + +## Key Practices + +- **Make boolean flags negatable** so users can opt out when the default changes: + ```dart + // Good: negatable: true allows flipping the default later without a breaking change + addFlag('enable-gradle-managed-install', negatable: true) + ``` + +- **Replace hardcoded opinionated values with configurable properties**: + ```dart + // Bad: Hardcoded and impossible to override + setAttribute('aria-valuemin', "0") + + // Good: Configurable with sensible default + class ProgressBarConfig { + final double minValue; + final double maxValue; + } + ``` + +- **Distinguish null (not configured) from empty/false (explicitly disabled)**: + ```dart + if (config != null && config.enabled) { + // Feature explicitly enabled + } else if (config != null && !config.enabled) { + // Feature explicitly disabled + } else { + // config == null: use default behavior + } + ``` + +- **Provide clear deprecation messaging** with the replacement option and removal timeline when retiring configuration options. + +## Review Checklist + +- [ ] Are new boolean flags `negatable: true`? +- [ ] Are numeric constants (min, max, threshold) exposed as configurable properties rather than hardcoded? +- [ ] Is a nullable config field ambiguous between "not set" and "explicitly disabled"? +- [ ] Do deprecated options include a replacement and a "deprecated after X.Y.Z" message? +- [ ] Could the current default value need to change in a future release, and is that change safe? + +## Output + +Raise findings as: +- 🔴 **Blocking** — design forces a breaking change to update a default in future +- 🟡 **Important** — hardcoded value or non-negatable flag that limits future flexibility +- 🔵 **Minor** — missing deprecation messaging or ambiguous null state + diff --git a/.github/skills/rzlv-make-errors-explicit/SKILL.md b/.github/skills/rzlv-make-errors-explicit/SKILL.md new file mode 100644 index 0000000..b18ad1d --- /dev/null +++ b/.github/skills/rzlv-make-errors-explicit/SKILL.md @@ -0,0 +1,65 @@ +--- +name: 'rzlv-make-errors-explicit' +description: 'Review Flutter/Dart code for hidden error conditions: silent default returns, unchecked dereferences, swallowed exceptions, and missing error logging that obscure failures instead of surfacing them.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Make Errors Explicit + +## Purpose + +Flag error conditions that are silently swallowed, hidden behind default return values, or left unlogged. Errors should be visible during development and provide actionable context when they occur. + +## When to Apply + +Trigger this skill when a PR: +- Returns a default/sentinel value (e.g., `0`, `null`, `false`) on an unexpected code path +- Calls `find()`, `firstWhere()`, or similar without checking the result +- Catches exceptions without re-throwing or logging +- Silently ignores duplicate events or unexpected state transitions +- Uses conditional execution (`if (itemExtent > 0.0)`) to bypass a computation that should always be valid + +## Key Practices + +- **Replace silent default returns with assertions**: + ```dart + // Bad — hides error by returning 0 + if (itemExtent > 0.0) { + final double actual = scrollOffset / itemExtent; + if (!actual.isFinite) { + return 0; // Caller has no idea something went wrong + } + } + + // Good — asserts the precondition; crashes fast in debug mode + assert(scrollOffset.isFinite && itemExtent.isFinite, + 'scrollOffset and itemExtent must be finite'); + if (itemExtent > 0.0) { + final double actual = scrollOffset / itemExtent; + } + ``` + +- **Check results before dereferencing** — calls to `find()`, map lookups, or `firstWhere()` should be validated before use. + +- **Log unexpected conditions** — use `FML_LOG(ERROR)` (C++) or `FlutterError.reportError` (Dart) for states that should never occur. + +- **Throw on duplicate events** rather than silently ignoring them — duplicate pointer events, duplicate registrations, etc. indicate a logic error upstream. + +## Review Checklist + +- [ ] Are there `return defaultValue` statements on error paths instead of assertions? +- [ ] Are results of `find()`, `firstWhere()`, or map lookups checked before use? +- [ ] Are caught exceptions logged or re-thrown? +- [ ] Are "impossible" states (e.g., duplicate events) asserted or logged rather than silently skipped? +- [ ] Are preconditions for arithmetic (finite, non-zero) asserted explicitly? + +## Output + +Raise findings as: +- 🔴 **Blocking** — silent failure masks a correctness bug that will be invisible in production +- 🟡 **Important** — missing assertion or log makes debugging significantly harder +- 🔵 **Minor** — additional logging that would aid future diagnosis + diff --git a/.github/skills/rzlv-manage-state-dependencies-properly/SKILL.md b/.github/skills/rzlv-manage-state-dependencies-properly/SKILL.md new file mode 100644 index 0000000..c106709 --- /dev/null +++ b/.github/skills/rzlv-manage-state-dependencies-properly/SKILL.md @@ -0,0 +1,69 @@ +--- +name: 'rzlv-manage-state-dependencies-properly' +description: 'Review Flutter widget code for improper state dependency management: direct context lookups that bypass the dependency system, setState calls during build phases, and missing didChangeDependencies updates.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Manage State Dependencies Properly + +## Purpose + +Flag Flutter widget code that accesses inherited or external state without establishing a proper rebuild dependency, and identify `setState` calls that occur during a build phase and would cause "setState() or markNeedsBuild() called during build" errors. + +## When to Apply + +Trigger this skill when a PR: +- Calls `Overlay.of(context)`, `Navigator.maybeOf(context)`, or similar without using the dependency-establishing variant +- Reads `InheritedWidget` data directly instead of via `context.dependOnInheritedWidgetOfExactType` +- Calls `setState` or a state-mutating callback from `didChangeDependencies` or `build` +- Stores a result from a context lookup at construction time (bypassing reactive updates) + +## Key Practices + +- **Use dependency-establishing lookups** so widgets rebuild when inherited state changes: + ```dart + // Problematic: Overlay.of does not establish a dependency + final overlayState = Overlay.of(context); + + // Better: Use the InheritedWidget-based API that establishes a rebuild dependency + // Ensures the widget rebuilds when the overlay changes + ``` + +- **Defer state updates triggered during build** using `addPostFrameCallback`: + ```dart + void handleCloseRequest() { + if (widget.onCloseRequested != null) { + if (SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks) { + // Defer to avoid setState during build + SchedulerBinding.instance.addPostFrameCallback((_) { + widget.onCloseRequested!(); + }); + } else { + widget.onCloseRequested!(); + } + } + } + ``` + +- **Move expensive or reactive lookups to `didChangeDependencies`** rather than `build()`, and cache the result in state. + +- **Never mutate state inside `build()`** — any mutation triggered transitively from `build` will crash in debug mode. + +## Review Checklist + +- [ ] Are all `InheritedWidget` lookups using the dependency-establishing variants? +- [ ] Are there `setState` or callback invocations that could be triggered during a build phase? +- [ ] Are context lookups stored at widget construction time (where they won't update reactively)? +- [ ] Are expensive or potentially-null context reads guarded with `didChangeDependencies` caching? +- [ ] Does the code handle the case where the scheduler phase is `persistentCallbacks`? + +## Output + +Raise findings as: +- 🔴 **Blocking** — causes a crash (`setState during build`) or missed rebuilds in production +- 🟡 **Important** — stale data shown to user or unpredictable rebuild behavior +- 🔵 **Minor** — missed optimization or defensive guard that would improve robustness + diff --git a/.github/skills/rzlv-test-observable-behavior/SKILL.md b/.github/skills/rzlv-test-observable-behavior/SKILL.md new file mode 100644 index 0000000..4bea32e --- /dev/null +++ b/.github/skills/rzlv-test-observable-behavior/SKILL.md @@ -0,0 +1,70 @@ +--- +name: 'rzlv-test-observable-behavior' +description: 'Review Flutter test code for tests that check internal implementation details rather than observable user-facing behavior, and flag uses of @visibleForTesting that exist solely to enable white-box testing.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Test Observable Behavior + +## Purpose + +Flag tests that verify internal implementation details rather than user-observable outcomes. Tests should remain valid when the implementation changes, and should not require `@visibleForTesting` annotations on production code just to enable assertions. + +## When to Apply + +Trigger this skill when a PR: +- Tests internal controller values or private state directly +- Adds `@visibleForTesting` to production code solely for testing purposes +- Verifies that a specific internal method was called rather than what the user sees +- Tests the number of internal objects rather than the rendered output +- Bypasses the public API to set up or assert against widget state + +## Key Practices + +- **Test visual output, not controller internals**: + ```dart + // Bad: Tests the internal controller value + expect(controller.value, closeTo(0.5, 0.01)); + + // Good: Tests what the user actually sees + expect( + find.byType(LinearProgressIndicator), + paints + ..rect(rect: expectedBackgroundRect) + ..rect(rect: expectedProgressRect) + ); + ``` + +- **Avoid `@visibleForTesting` on production code**: + ```dart + // Bad: Exposes internals just for a test assertion + @visibleForTesting + Set<Color> distinctVisibleOuterColors() { ... } + + // Good: Test the observable rendering instead + expect(find.byType(FadeTransition), paints..opacity(0.5)); + ``` + +- **Prefer `paints` matchers** for visual widgets — they verify what is actually drawn. +- **Test interactions and their effects** — simulate user gestures and assert on the resulting UI state. +- **Test accessibility semantics** — verify `Semantics` labels and announcements rather than internal state. +- **Use public APIs** to set up test scenarios; use platform channel mocks for system integrations. + +## Review Checklist + +- [ ] Are tests verifying rendered output rather than internal state or controller values? +- [ ] Is `@visibleForTesting` used only for code that genuinely needs exposure, not to work around encapsulation? +- [ ] Are `paints` matchers used instead of private field checks for visual widgets? +- [ ] Do tests remain valid if the internal implementation is refactored? +- [ ] Are user interactions tested end-to-end rather than individual internal method calls? + +## Output + +Raise findings as: +- 🔴 **Blocking** — test will break on any internal refactor, giving false regression signals +- 🟡 **Important** — `@visibleForTesting` leak or internal assertion that should use public API +- 🔵 **Minor** — test could be more meaningful or resilient with a small change + diff --git a/.github/skills/rzlv-thread-safety-synchronization/SKILL.md b/.github/skills/rzlv-thread-safety-synchronization/SKILL.md new file mode 100644 index 0000000..c5ec8e5 --- /dev/null +++ b/.github/skills/rzlv-thread-safety-synchronization/SKILL.md @@ -0,0 +1,74 @@ +--- +name: 'rzlv-thread-safety-synchronization' +description: 'Review Flutter/C++ platform code for race conditions: unprotected access to shared data structures, missing mutex locks, callbacks received after object destruction, and multi-threaded operations that need consolidation.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Thread Safety & Synchronization + +## Purpose + +Flag race conditions, unprotected shared state, and multi-threaded coordination issues in Flutter platform code (C++, platform channels, isolates). Inconsistent UI state and hard-to-reproduce crashes are frequently caused by missing synchronization. + +## When to Apply + +Trigger this skill when a PR touches: +- C++ platform view or shell code that modifies shared data structures +- Object destruction sequences that may race with in-flight callbacks +- Platform channel message handlers that run on different threads +- Operations split across platform thread and raster/UI thread +- Any code that accesses or modifies shared state from multiple call sites + +## Key Practices + +1. **Protect shared data with mutexes**: + ```cpp + // Before: Unprotected access — race condition with other threads + expected_frame_constraints_.erase(view_id); + + // After: Scoped lock prevents concurrent modification + std::scoped_lock<std::mutex> lock(resize_mutex_); + expected_frame_constraints_.erase(view_id); + ``` + +2. **Unregister handlers before destroying objects** to prevent callbacks arriving during/after destruction: + ```dart + @override + void destroy() { + if (_destroyed) return; + + // Unregister BEFORE issuing the destroy, not after + _owner.removeMessageHandler(this); + _Win32PlatformInterface.destroyWindow(getWindowHandle()); + _destroyed = true; + _delegate.onWindowDestroyed(); + } + ``` + +3. **Consolidate multi-threaded operations** — when a logical operation requires multiple steps, post all steps to the same thread: + ```cpp + // Rather than split across platform and raster threads (race condition): + task_runners_.GetPlatformTaskRunner()->PostTask([&, jni_facade = jni_facade_]() { + HideOverlayLayerIfNeeded(); + jni_facade->applyTransaction(); + }); + ``` + +## Review Checklist + +- [ ] Is every write to a shared data structure protected by a mutex or equivalent? +- [ ] Are callbacks/handlers unregistered before the owning object is destroyed? +- [ ] Are logically related multi-step operations posted to the same task runner? +- [ ] Are there any `_destroyed` or `_invalidated` guards missing on async callbacks? +- [ ] Is the threading model of new APIs documented (which thread owns what)? + +## Output + +Raise findings as: +- 🔴 **Blocking** — race condition that can cause data corruption, crashes, or inconsistent UI in production +- 🟡 **Important** — potential race that is unlikely but possible under load or timing +- 🔵 **Minor** — missing guard or documentation that would improve safety and auditability + diff --git a/.github/skills/rzlv-use-descriptive-names/SKILL.md b/.github/skills/rzlv-use-descriptive-names/SKILL.md new file mode 100644 index 0000000..f21b241 --- /dev/null +++ b/.github/skills/rzlv-use-descriptive-names/SKILL.md @@ -0,0 +1,84 @@ +--- +name: 'rzlv-use-descriptive-names' +description: 'Review Flutter/Dart/C++ code for vague, abbreviated, or misleading names: functions, variables, constants, and types that do not clearly communicate their purpose, behavior, or data type.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Use Descriptive Names + +## Purpose + +Flag names that are too generic, abbreviated, or misleading. Names should clearly describe what a symbol represents or does — not how it is implemented, and not via shorthand that requires domain knowledge to decode. + +## When to Apply + +Trigger this skill when a PR introduces or renames: +- Function names that describe mechanism rather than purpose +- Variable names using abbreviations or single/double letters +- Type names that reflect implementation details instead of semantics +- Constants missing the `k` prefix (Dart/C++ convention) +- Callback parameters named `_` (anonymous) without good reason +- C++ identifiers using snake_case where camelCase is expected (or vice versa per language style) + +## Key Practices + +- **Name functions by what they do, not how**: + ```dart + // Bad: Generic, describes a check, not the action + void _checkOnCustomDaysDisplay() { ... } + + // Good: Clearly states the outcome + void _scrollToFirstSelectableDate() { ... } + ``` + +- **Use full words instead of abbreviations**: + ```cpp + // Bad + const double hsw = half_stroke_width; + + // Good + const double halfStrokeWidth = half_stroke_width; + ``` + +- **Reflect the actual data type or concept** in type and variable names: + ```dart + // Bad: Name implies a size but it holds constraints + Map<int, ExpectedFrameSize> expectedFrameConstraints; + + // Good + Map<int, ExpectedFrameConstraints> expectedFrameConstraints; + ``` + +- **Use semantic names over widget-layer names**: + ```dart + // Bad: 'overlayPortalParent' leaks widget implementation details + final overlayPortalParent = ...; + + // Good: Describes the traversal role + final traversalOwner = ...; + ``` + +- **Apply conventional prefixes consistently**: + - `k` prefix for constants: `kSystemToolbarToggleDebounceThreshold` + - `handle` prefix for callbacks: `handleSystemHideToolbar` + - `_` prefix for private members in Dart + +## Review Checklist + +- [ ] Do method names describe *what* they do rather than *how*? +- [ ] Are abbreviations expanded to their full forms? +- [ ] Do type names reflect actual data semantics, not implementation details? +- [ ] Are constants prefixed with `k` per Flutter/Dart conventions? +- [ ] Are callback parameters named `handle<Event>` for clarity? +- [ ] Do C++ identifiers follow the project's naming style guide? + +## Output + +Raise findings as: +- 🔴 **Blocking** — misleading name that inverts or obscures the meaning of critical logic +- 🟡 **Important** — ambiguous name that will confuse reviewers and future maintainers +- 🔵 **Minor** — abbreviation or style inconsistency that reduces readability + diff --git a/.github/skills/rzlv-use-named-constants/SKILL.md b/.github/skills/rzlv-use-named-constants/SKILL.md new file mode 100644 index 0000000..7648b28 --- /dev/null +++ b/.github/skills/rzlv-use-named-constants/SKILL.md @@ -0,0 +1,65 @@ +--- +name: 'rzlv-use-named-constants' +description: 'Review Flutter/Dart/Java code for hardcoded string literals and magic values that should be extracted into named constants to prevent typos, improve readability, and create a single source of truth.' +allowed-tools: + - edit/editFiles + - edit/createFile + - github/* +--- + +# rzlv: Use Named Constants + +## Purpose + +Flag hardcoded string literals, numeric magic values, and repeated raw values that should be extracted into named constants. A named constant prevents typos, makes the code self-documenting, and ensures all usages stay in sync when the value changes. + +## When to Apply + +Trigger this skill when a PR introduces or uses: +- String literals repeated more than once, or used as identifiers/prefixes +- Numeric literals with non-obvious meaning (e.g., buffer sizes, timeouts, offsets) +- String prefixes or suffixes used in string parsing/construction +- File paths or resource names hardcoded inline + +## Key Practices + +- **Extract string prefixes and resource names to constants**: + ```java + // Bad: Hardcoded literal used for string manipulation + String prefix = "--aot-shared-library-name="; + Path path = internalStorageDirAsPathObj.resolve(Paths.get("library.so")); + + // Good: Named constants that communicate intent + private static final String AOT_SHARED_LIBRARY_NAME_PREFIX = "--aot-shared-library-name="; + private static final String DEFAULT_LIBRARY_NAME = "library.so"; + + String prefix = AOT_SHARED_LIBRARY_NAME_PREFIX; + Path path = internalStorageDirAsPathObj.resolve(Paths.get(DEFAULT_LIBRARY_NAME)); + ``` + +- **Apply Flutter/Dart constant conventions**: + ```dart + // Good: k-prefixed constant with descriptive name + const double kSystemToolbarToggleDebounceThreshold = 0.5; + const String kDefaultFontFamily = 'Roboto'; + ``` + +- **Group related constants** in a dedicated constants file or class, rather than scattering them across implementations. + +- **Use constants for quantities that may change** — even if a value is used once, a named constant communicates intent and makes future changes safe. + +## Review Checklist + +- [ ] Are there string literals that appear more than once, or that function as identifiers? +- [ ] Are numeric values with non-obvious meaning extracted into named constants? +- [ ] Are file paths, resource names, or prefixes hardcoded inline? +- [ ] Do constant names follow the language's conventions (`k` prefix in Dart/C++, `ALL_CAPS` in Java)? +- [ ] Are related constants co-located for discoverability? + +## Output + +Raise findings as: +- 🔴 **Blocking** — duplicate literal where a mistake in one copy causes a silent bug +- 🟡 **Important** — magic value whose purpose requires reading surrounding code to understand +- 🔵 **Minor** — single-use literal that would benefit from a name for clarity +