diff --git a/.agents/skills/review-pull-request/SKILL.md b/.agents/skills/review-pull-request/SKILL.md new file mode 100644 index 0000000..a09ca97 --- /dev/null +++ b/.agents/skills/review-pull-request/SKILL.md @@ -0,0 +1,251 @@ +# Skill: Review Pull Request for explain-error-plugin + +## Description + +Step-by-step workflow for reviewing pull requests submitted to the `jenkinsci/explain-error-plugin` repository. Produces a structured, actionable review covering security, code quality, architecture, UI, testing, and dependency management — consistent with Jenkins hosting requirements. + +## Tools + +- `github-pull-request_openPullRequest` — fetch PR metadata and diff +- `github-pull-request_doSearch` — look up related issues or prior PRs +- `file_search` / `grep_search` / `read_file` — inspect source files for context +- `get_errors` — check for compile/lint errors +- `run_in_terminal` — run `mvn test` or `mvn checkstyle:check` to validate changes + +--- + +## Workflow + +### Step 1 — Fetch the PR + +Load the PR using the PR number or URL provided by the user. + +Collect: +- Title, description, linked issue(s) +- Changed files list +- Diff/patch of each changed file + +> If no PR number is given, ask: *"Which PR number or URL would you like me to review?"* + +--- + +### Step 2 — Understand the Intent + +Read the PR description and linked issue to understand **what problem is being solved**. + +Check: +- Is there a linked issue? Does the change match the issue scope? +- Is the change scoped (minimal, focused) or does it include unrelated refactoring? +- Is there a test included? + +Flag if the description is absent or too vague to evaluate intent. + +--- + +### Step 3 — Security Review + +For every changed `.java` file, check: + +#### API Keys / Secrets +- [ ] API keys use `Secret apiKey` (not `String`) +- [ ] Access uses `Secret.toString(apiKey)` only at usage point +- [ ] No secrets logged or printed anywhere + +#### Permission Checks +- [ ] All `doXxx()` form methods that change state or validate credentials use `@RequirePOST` (or `@POST`) +- [ ] `Jenkins.get().checkPermission(Jenkins.ADMINISTER)` present in sensitive operations + +#### Nullability +- [ ] Public API methods annotated with `@NonNull` or `@CheckForNull` (from `edu.umd.cs.findbugs.annotations`) +- [ ] No unguarded `.get()` calls on `Optional` or nullable values + +#### Input Validation +- [ ] User-supplied inputs validated at system boundary (form fields, pipeline step params) +- [ ] No SQL/XSS/command injection surface (no shell exec with user data, no raw HTML output) + +--- + +### Step 4 — Code Architecture Review + +#### Jenkins Extension Points +- [ ] New config classes use `@Extension` + `@Symbol` for CasC support +- [ ] `@DataBoundConstructor` present on constructors; optional fields use `@DataBoundSetter` +- [ ] Global config provides a static `get()` method + +#### AI Provider Pattern +- [ ] New providers extend `BaseAIProvider` and implement `ExtensionPoint` +- [ ] Provider descriptor extends `BaseProviderDescriptor` with `@Symbol` annotation +- [ ] `createAssistant()` used for LangChain4j integration (no direct HTTP/JSON) +- [ ] `isNotValid()` implemented to guard against empty config + +#### Action Management +- [ ] `addOrReplaceAction()` used (not `addAction()`) to avoid duplicate actions +- [ ] New UI actions use `TransientActionFactory` instead of `RunListener` + +#### Backward Compatibility +- [ ] If config fields are renamed or removed, a `readResolve()` migration method is present +- [ ] Old fields marked `@Deprecated` and `transient` if replaced + +#### Logging +- [ ] `java.util.logging.Logger` used (not SLF4J directly, not `System.out.println`) +- [ ] Trace/debug calls use `FINE`/`FINER`/`FINEST` +- [ ] `INFO` only for significant operational events +- [ ] No `e.printStackTrace()` — exceptions logged via `LOGGER.log(Level.WARNING, "...", e)` + +#### Error Handling +- [ ] Failures throw `ExplanationException` (not returned as strings) +- [ ] Exception messages are user-friendly and actionable + +--- + +### Step 5 — UI / Jelly Review + +For `.jelly` and `.js` files: + +#### Design System +- [ ] Uses Jenkins design library components (`l:card`, `jenkins-button`, `jenkins-!-*` spacing classes) +- [ ] No hard-coded colors — uses CSS variables (`var(--background)`, `var(--text-color)`) +- [ ] Icons use symbol library (`symbol-help`, etc.) not image paths + +#### CSP Compliance +- [ ] No inline `