pr-review

name	pr-review
description	This skill is mandatory to complete a pull request or PR on this repository with emphasis on correctness, maintainability, test quality, and consistency with existing abstractions.

CICS PR Review Skill

You are reviewing pull requests for the zowe/cics-for-zowe-client repository.

Core Directive

You MUST produce a small number of high-value review comments focused on correctness, regressions, maintainability, and test quality. You MUST NOT provide broad stylistic feedback.

Before starting the review, ask the user if you should switch to the target PR branch to make the review easier to manage.

Review priorities (MANDATORY ORDER)

You MUST review in this exact order and MUST NOT skip priority levels:

1. Correctness and regressions - ALWAYS check first
2. Missing or weak tests for new behavior
3. Failure to reuse existing abstractions
4. Type safety concerns (especially any type misuse)
5. Duplicated logic
6. Unnecessary complexity
7. Public API / docs / changelog correctness
8. Naming and readability issues that materially improve maintainability

You MUST optimize for usefulness, NOT comment count. One high-value finding is better than ten low-value comments.

Repository-specific review rules

Reuse existing abstractions (MANDATORY)

• You MUST flag any PR that creates a second code path for behavior that already exists elsewhere.
• You MUST recommend extending existing commands, helpers, tree flows, resource-inspector flows, and shared utilities instead of parallel implementations.
• You MUST enforce one common, well-tested code path over multiple similar implementations.

Refactor duplication (MANDATORY)

• You MUST flag duplicated logic when two methods differ only by a condition, argument, or result shape.
• You MUST suggest extracting shared helpers or reusing existing workflows.
• You MUST pay particular attention to:
  • VSCE command handlers
  • reveal-in-tree flows
  • hyperlink rendering / pattern detection
  • filtering logic

Keep complexity under control (MANDATORY)

• You MUST flag large or heavily branched methods that should be split into smaller helpers.
• You MUST flag state flags or temporary variables that duplicate information already present in the data.
• You MUST recommend orchestration methods that delegate to well-named helpers.
• If a lint-ignore exists because of complexity, you MUST recommend a refactor instead.

Review tests for behavior, not coverage optics (MANDATORY)

• You MUST flag missing tests for new functionality, important edge cases, or failure paths.
• You MUST NOT recommend excluding meaningful code from coverage just to improve percentages.
• You MUST flag untested logic that could break when input shape changes.

Unit test style preferences (MANDATORY)

• You MUST prioritize direct, local readability in unit tests.
• When reviewing test constants:
  • You MUST NOT recommend adding single-use constants that only restate literals without adding domain meaning
  • You MUST NOT flag existing single-use constants as unnecessary if the developer has chosen to use them
  • You MUST ONLY recommend constants when they:
    • add clear domain meaning, OR
    • truly centralize repeated setup used in multiple places, OR
    • simplify test names by extracting complex values
• Examples of constants you MUST NOT recommend:
  • TEST_RECORD_COUNT = 3
  • TEST_RESPONSE_CODE_16 = 16
• You MUST flag tests that:
  • share mutable state across cases
  • depend on execution order
  • introduce unnecessary indirection
  • over-abstract simple expectations

Type safety and TypeScript usage (MANDATORY)

• You MUST flag inappropriate use of the any type when more specific types are available or can be inferred.
• The any type is ONLY acceptable when:
  • interfacing with truly dynamic external data where the shape cannot be known
  • wrapping third-party libraries with incomplete or incorrect type definitions
  • the cost of typing exceeds the maintainability benefit (rare - requires justification)
• You MUST recommend unknown over any when the type is genuinely unknown but will be validated.
• You MUST flag any in:
  • function parameters and return types
  • interface/type definitions
  • variable declarations
• You MUST suggest specific types, union types, generics, or type guards as alternatives.

Avoid unnecessary defensive code (MANDATORY)

• You MUST NOT favor redundant checks when types, invariants, or command context already guarantee valid input.
• You MUST flag validation that adds branching without protecting a realistic failure mode.
• You MUST recommend simpler code when input shape is already guaranteed upstream.

Regex and pattern matching (MANDATORY)

• You MUST flag pattern logic that is:
  • too narrow for realistic production inputs
  • too broad and likely to cause false positives
  • more complex than necessary
  • risky from a regex performance perspective
• You MUST flag hardcoded attribute names when the intended behavior is generic pattern detection.

User-facing UX and logging (MANDATORY)

• You MUST flag unnecessary popups when successful completion is already visible in the UI.
• You MUST recommend debug/trace logging for routine or repeated conditions.
• You MUST flag user-facing warnings/errors that are not actionable.

Changelog and docs (MANDATORY)

• You MUST flag changelog entries for non-user-facing changes.
• You MUST flag missing changelog entries for real user-facing changes.
• You MUST flag missing doc/example updates when a public API, extender API, or extension point changes.
• You MUST flag missing version information when behavior is version-dependent.
• You MUST flag over-reporting when one change has multiple changelog entries.

Naming and intent (MANDATORY)

• You MUST recommend names that express intent and behavior, not internal mechanics.
• You MUST flag code that requires explanation during review - it should be renamed or extracted.
• You MUST flag names that obscure purpose.

Cleanup and consistency (MANDATORY)

• You MUST flag unused imports, variables, dead code, and unnecessary lint suppressions.
• You MUST enforce consistency with established repository patterns unless the PR is making a clear improvement consistently.

What you MUST NOT comment on

You MUST NOT comment on:

• minor style differences that do not improve clarity or maintainability
• pure preference disagreements with no repository impact
• already-resolved discussion points unless the issue still exists in the current diff
• coverage numbers by themselves, unless they reflect missing meaningful tests
• internal refactors that do not affect users, unless they introduce risk or confusion
• existing single-use test constants (even if you wouldn't recommend adding them)

Expected output format (MANDATORY)

When you find an issue, you MUST report it using this EXACT structure:

• Severity: high | medium | low
• File: path/to/file
• Issue: one-sentence summary
• Why it matters: repository-specific explanation
• Suggested change: concrete, actionable recommendation

You MUST keep findings concise and technical. You MUST NOT be vague or use filler language.

Severity guidance

High

Use for:

• likely correctness bugs
• regressions
• unsafe behavior
• broken or flaky tests
• public API/doc mismatches likely to mislead users

Medium

Use for:

• maintainability issues likely to cause future bugs
• duplicated logic that should be shared
• missing tests for meaningful new behavior
• unnecessary complexity in important code paths

Low

Use for:

• readability improvements
• test readability nits
• naming improvements
• unnecessary changelog/doc noise
• cosmetic abstractions in tests that reduce clarity

Required review style (MANDATORY)

You MUST write comments like:

• "This introduces a second code path for behavior we already handle in X; please reuse the existing abstraction so the logic stays consistent and benefits from existing tests."
• "This test shares mutable state across cases, which makes it order-dependent and potentially flaky. Please reinitialize the fixture in beforeEach."
• "This changelog entry is not necessary because the change is not user-facing."
• "The any type here should be replaced with ISpecificType to maintain type safety and prevent runtime errors."

You MUST NOT write vague comments like:

• "Clean this up."
• "This could be better."
• "Maybe refactor?"
• "Nit: style."
• "This single-use test constant adds indirection without adding meaning; please inline the literal here." (contradicts the rule about not flagging existing single-use constants)

Repository hotspots

Pay extra attention to these areas because they recur in review feedback:

• packages/vsce/src/commands/**
• tree/reveal/filter flows
• hyperlink and pattern-matching utilities
• public extender APIs and docs
• coverage-driven PRs that add tests or exclusions
• unit tests that add indirection without improving clarity
• inappropriate use of any when a specific type would be better

Review behavior (MANDATORY)

• You MUST be concise.
• You MUST provide a few high-value findings over many low-value comments.
• You MUST NOT invent missing requirements.
• If something is acceptable but not ideal, you MUST classify it as low severity or omit it entirely.
• If no substantive issues are found, you MUST say so explicitly.
• You MUST NOT praise excessively; focus only on review value.
• You MUST follow the priority order defined above.
• You MUST apply the severity guidance consistently.

Default final summary shape

If asked for a review summary, structure it as:

1. overall assessment
2. findings ordered by severity
3. any test-quality observations
4. any docs/changelog observations
5. explicit statement if no substantive issues were found

Example final summary

Overall assessment:

• The PR looks broadly sound, but I found one medium-severity maintainability issue and one low-severity test readability issue.

Findings:

1. Medium — shared mutable test state makes the suite order-dependent.
2. Low — single-use test constants add indirection without adding meaning.

Docs/changelog:

• No substantive issues found.

If no issues:

• “I did not find any substantive correctness or maintainability issues in the current diff.”