# Katalon Code Review

> Katalon Code Review

- Skill: `nghiabui-kms/katalon-code-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add nghiabui-kms/katalon-code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/nghiabui-kms/katalon-code-review/raw
- Safety review: pending (external: skill-scanner PASS, skillspector PASS)
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: nghiabui-kms (https://skillmd.com/u/nghiabui-kms)
- Updated: 2026-09-22
- Page: https://skillmd.com/skills/nghiabui-kms/katalon-code-review

---


# Katalon Code Review

Review Groovy code in the **Katalon framework** (project `Katalon.UI.Tests`). The framework wraps Katalon with custom abstractions — `Perform.*` over `WebUI.*`, `Page.As()` factory, `locator()` Playwright-style selectors, `completeAllRequiredFields` data-driven form filling. Generic Katalon advice does not apply; review against the team's specific rules below.

## When to use this skill

Trigger in any of these situations:

- User shares a `.groovy` file from `Test Cases/Clients/` or `Keywords/katalon/`
- User shares a PR link, diff, or `git diff` output touching those folders
- User asks "review", "check", "audit", "feedback", "looks good?"
- User asks "review my changes", "review my work", "review what I have", or any review request **without naming a specific file** — default to reviewing current git working state
- Conversation already involves Katalon and the user shares any Groovy snippet

If the file is NOT in the Katalon codebase (e.g., generic Groovy script), defer to the user — confirm whether Katalon-specific rules apply.

## Procedure

1. **Determine the review target** — pick exactly one source:

   | Signal | Target |
   |---|---|
   | User pasted code inline | Review the pasted code |
   | User named a specific file/path | Read and review that file |
   | User shared a PR URL or PR number | Run `gh pr diff <N>` directly; only ask user to paste the diff if the command fails |
   | User said "review my changes today" or temporal phrase | Use `git log --since='1 day ago' --name-only --pretty=format:` then diff each file |
   | **Default — no target named** | **Review the full PR-equivalent scope (see below)** |

   **Default workflow — "PR-equivalent" scope:**

   The goal is to review *everything this PR would introduce when opened*: committed-but-not-yet-merged changes on the current branch PLUS any uncommitted local changes on top.

   ```bash
   # 1. Detect current branch name
   git rev-parse --abbrev-ref HEAD

   # 2. Determine base branch — prefer the real PR target over heuristics:
   gh pr view --json baseRefName -q .baseRefName 2>/dev/null   # use this if non-empty
   # If empty, fall back to detecting from remote HEAD:
   git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@'
   # If still empty, probe common names in order:
   for b in main master develop; do
       git rev-parse --verify origin/$b >/dev/null 2>&1 && echo "$b" && break
   done
   ```

   ```bash
   # 3. Once base branch is known (call it BASE), fetch to avoid stale ref, then gather scope:
   git fetch origin $BASE --quiet
   git log origin/$BASE..HEAD --oneline                         # commits on this branch
   git diff origin/$BASE...HEAD                                 # committed changes vs base (3 dots = since merge-base)
   git diff HEAD                                                # uncommitted on top
   git ls-files --others --exclude-standard '*.groovy'          # untracked .groovy files
   ```

   **Branch-state matrix** — pick scope based on what's found:

   | Current branch | Commits on branch | Uncommitted | Scope to review |
   |---|---|---|---|
   | Feature branch | Yes (≥1) | Yes | **Full PR equivalent**: `git diff origin/$BASE` + untracked |
   | Feature branch | Yes (≥1) | No | **Committed only**: `git diff origin/$BASE...HEAD` |
   | Feature branch | No | Yes | **Uncommitted only**: `git diff HEAD` + untracked |
   | main/master/develop | — | Yes | **Uncommitted only**: `git diff HEAD` + untracked |
   | main/master/develop | — | No | Reply: `No pending changes on default branch. Specify a file or check out a feature branch.` |
   | Detached HEAD / no remote | — | — | Fallback to uncommitted only, prefix warning: `Cannot detect PR scope (detached HEAD or no remote). Reviewing uncommitted changes only.` |

   Then for each affected file:
   - **Filter to in-scope only**: `.groovy` files inside `Test Cases/`, `Keywords/katalon/`, `Scripts/`, `Test Listeners/`. Skip generated paths (`build/`, `bin/`, `.cache/`, `.gradle/`), reports, data files.
   - **New files (untracked or added on branch)** → read full content
   - **Modified files** → focus on changed hunks + immediate context (±5 lines)
   - **Deleted files** → skip (nothing to review)

   **Edge cases for default mode:**
   - Pending changes all out-of-scope (no .groovy in scope folders) → `Pending changes are outside the Katalon scope: <list of files>. Confirm if you want them reviewed anyway.` Wait for user.
   - Very large scope (> 15 files or > 800 lines) → `Found <N> changed files / <M> lines across <K> commits. Review all, narrow to a folder, or limit to last commit (git diff HEAD~1)?`
   - Working directory is not a git repo → `Not in a git repository. Please specify a file or paste code to review.`
   - Base branch detection failed completely → ask user: `Cannot detect default branch. Is it 'main', 'master', 'develop', or other?`

2. **Identify the artifact type** (per file):
   - `Test Cases/Clients/<Client>/...` → test case mode
   - `Keywords/katalon/pages/common/*` → common page object mode
   - `Keywords/katalon/pages/<client>/*` → client page object mode
   - `Keywords/katalon/core/*` or `common/helpers/*` → framework mode (most strict)
   - PR/diff → review only changed lines + immediate context

3. **Determine mode**:
   - Default: **pragmatic** (flag 🔴 + 🟡 only)
   - Switch to **strict** if user wrote "strict", "thorough", or invoked `/katalon-code-review strict`

4. **Run checks in order**: hard-rule blockers → recurring-issue warnings → (strict only) suggestions

5. **Format per `## Output format`**, do not deviate

6. **End with a verdict**: `APPROVE` / `REQUEST CHANGES` / `COMMENT-ONLY`

## The 9 hard rules (🔴 Blockers)

### Rule 1: Form filling MUST use `completeAllRequiredFields`

**Why it matters:** Manual field-by-field calls couple test logic to UI layout — a renamed label or added field breaks the test silently and leaves no traceability back to the data source.

Manual sequences of `Perform.click` / `Perform.inputText` / `Perform.selectDropdown` to fill form fields are forbidden. All form data must flow through the data-driven dispatcher.

**Bad:**
```groovy
Perform.inputText(locator("label:text-is('First Name') + input"), "John")
Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
Perform.selectDropdown(locator("label:text-is('State') + select"), "NY")
```

**Good:**
```groovy
def data = DataHelper.JSON.render("DataStorage/Seeds/<Client>/<TICKET>.json", [:])
Page.As(SomePage).completeAllRequiredFields(data.SomeScreen.Fields)
```

### Rule 2: Page interaction MUST use `Page.As()`

**Why it matters:** Direct instantiation bypasses the factory's scope and lifecycle management, making cross-frame and client-context handling inconsistent and causing hard-to-reproduce failures.

Direct page instantiation or static factories are forbidden in test cases.

**Bad:**
```groovy
def loginPage = new LoginPage()
loginPage.logInBySimulator(creds, data)
```

**Good:**
```groovy
Page.As(LoginPage).logInBySimulator(creds, data)
```

### Rule 3: `WebUI.*` ONLY when `Perform.*` lacks a wrapper

**Why it matters:** `Perform.*` adds retry logic, uniform failure handling, and timeout normalization. Bypassing it via raw `WebUI.*` skips built-in waits and produces inconsistent failure messages, causing intermittent failures that are hard to diagnose.

If `Perform` has the operation, use it. Falling back to raw `WebUI.*` is only allowed for native operations Perform doesn't wrap.

**Known direct-WebUI exceptions** (acceptable): `WebUI.uploadFile`, `WebUI.switchToWindowIndex`, `WebUI.findWebElements`, browser/window-level navigation that Perform doesn't expose.

**Bad** (Perform has wrapper):
```groovy
WebUI.click(smartOR(btnSubmit))
WebUI.setText(smartOR(txtName), "John")
WebUI.verifyElementVisible(smartOR(lblTitle))
```

**Good:**
```groovy
Perform.click(btnSubmit)
Perform.inputText(txtName, "John")
Perform.verifyElement(lblTitle, Expectation.Visible, defaultTimeout, FailureHandling.STOP_ON_FAILURE)
```

### Rule 4: Inheritance chain — `CommonPage` → `ClientCommonPage` → `SpecificPage`

**Why it matters:** Skipping the client tier loses client-specific helpers and default configurations, forcing duplicated overrides and making page objects unable to access client context (e.g., client-specific frame handling, base URLs, shared selectors).

- Pages in `pages/common/*` extend `CommonPage`
- Client-common pages in `pages/<client>/<Client>CommonPage.groovy` extend `CommonPage`
- Specific pages in `pages/<client>/*` extend `<Client>CommonPage` (NOT `CommonPage` directly)

**Bad** (skip the client tier):
```groovy
// Keywords/katalon/pages/morganstanley/FundingPage.groovy
class FundingPage extends CommonPage { ... }
```

**Good:**
```groovy
class FundingPage extends MorganStanleyCommonPage { ... }
```

### Rule 5: Test case MUST have `"Step N:"` and `"VP:"` markers

Every interaction step needs `"Step <N>: <action>"`, every verification needs `"VP: <expectation>"`. These are mandatory for report readability.

**Bad:**
```groovy
Page.As(LoginPage).logInBySimulator(creds, data)
Page.As(SomePage).completeRequiredField(data.Fields)
```

**Good:**
```groovy
"Step 1: Log in to the system"
Page.As(LoginPage).logInBySimulator(creds, data)

"VP: Verify the screen opens"
Page.As(SomePage).verifyCurrentPageIsActive(data.PageName)

"Step 2: Complete required fields"
Page.As(SomePage).completeRequiredField(data.Fields)
```

Decimal steps (`"Step 3.1:"`) are accepted for sub-steps.

### Rule 6: Step numbering must be sequential, no gaps, in order

Flag `"Step 1"` → `"Step 3"` (missing 2) or `"Step 2"` → `"Step 1"` (out of order).

### Rule 7: No empty catch / swallowed exceptions

A `catch` block that does nothing or returns false silently masks failures.

**Bad:**
```groovy
try {
    Perform.click(btnSubmit)
} catch (Exception e) {
    // ignored
    return false
}
```

**Good:**
```groovy
try {
    Perform.click(btnSubmit)
} catch (Exception e) {
    KeywordMain.stepFailed("Failed to click Submit: ${e.message}", FailureHandling.STOP_ON_FAILURE)
}
```

### Rule 8: No infinite loop without exit condition

`while (true)` or `for(;;)` must have a time-based or counter-based exit.

**Bad:**
```groovy
while (true) {
    if (Perform.isVisible(lblSuccess)) break
}
```

**Good:**
```groovy
long startTime = System.currentTimeMillis()
while (System.currentTimeMillis() - startTime < GlobalVariable.LONG_TIMEOUT * 1000) {
    if (Perform.isVisible(lblSuccess)) break
    Thread.sleep(500)
}
```

### Rule 9: No hardcoded timeout literals

Use `GlobalVariable.DEFAULT_TIMEOUT` / `LONG_TIMEOUT` / `defaultTimeout` / `longTimeout`.

**Bad:** `Perform.click(btnSubmit, 30)`
**Good:** `Perform.click(btnSubmit, GlobalVariable.LONG_TIMEOUT)`

## The 12 recurring issues (🟡 Warnings)

| # | Issue | Why it matters | What to suggest |
|---|---|---|---|
| 1 | Missing parameter type | Confusing API, hard to autocomplete | Add explicit type: `String`, `Map`, `Frame`, `List<Map>` etc. |
| 2 | Step name doesn't match action | Reader misled; report misleading | Rename step to match what the next line actually does |
| 3 | Unclear logic | Hard to maintain | Refactor; split into smaller methods; rename for clarity |
| 4 | Page logic diverges from UI behavior with no note | Maintainer can't tell intent | Add `// Note: <why>` comment explaining the divergence |
| 5 | Retry without justification | Hides real bugs, makes test meaningless | Add `// Retry needed because: <reason>` or remove retry |
| 6 | New public method missing Groovydoc | Next developer doesn't know how to use it | Add `/** @param ... @return ... */` |
| 7 | Code "hack" to make test pass | May mask real bugs | Rewrite with proper logic, not workaround |
| 8 | Selector uses `text()=` | Brittle, breaks on whitespace | Use `:text-is('value')` or `:has-text('value')` |
| 9 | Missing `escapeSelector()` for user-provided values | Selector breaks on quote/backslash in data | Wrap input with `escapeSelector(value)` |
| 10 | Builder method doesn't `return this` | Breaks fluent chain | Add `return this` |
| 11 | `if (Perform.isVisible(x))` instead of `Perform.verifyElement` | Silent failure — test doesn't fail when it should | Use `Perform.verifyElement(x, Expectation.Visible, ..., FailureHandling.STOP_ON_FAILURE)` |
| 12 | Typo in identifier, comment, or test step name | Looks unprofessional; can break grep/search | Fix the typo |

For each warning issue, follow the same output pattern (file:line, brief why, diff fix).

## 🟢 Suggestions (strict mode only)

- Line > 200 chars (codenarc warning threshold)
- Method cyclomatic complexity > 15
- Missing region markers in page class (`//*** Page Objects ***`, `//*** Page Actions ***`, `//*** Page Assertions ***`, `//*** Helper Methods ***`)
- Variable could be renamed for clarity

## What NOT to flag

These patterns are intentional in this codebase — never flag:

- `if (true) { ... }` blocks — used to group steps for report readability
- Hungarian prefix on UI element variables — `ddl*`, `btn*`, `lbl*`, `txt*`, `lnk*`, `cbx*`, `rdo*` is the convention
- `def` for local variables — codenarc allows it (`NoDef` disabled)
- Missing JavaDoc on classes — codenarc allows (`ClassJavadoc`, `JavadocType` disabled)
- Jira-ticket-based test case naming — `MSAFRMFA-7848_AUTOKAT-794.groovy` is correct
- `"Step N: ..."` / `"VP: ..."` as bare Groovy string literals — this IS the convention
- Lines up to 200 chars
- Implicit return statements at end of method
- `smartOR(elem)` for single-element TestObject conversion
- `baseOR(elem)` for multi-element / count operations

## Output format

Two modes: **Markdown** (default) and **HTML** (when user asks to "xuất file", "save", "report", or "html").

### Markdown (default)

Respond using exactly this 3-part structure. Severity emojis only — no `BLOCKER`/`WARNING` words.

**Part (a) — Header (2 lines):**

```
SCOPE: <what was reviewed — see scope guide below>
VERDICT: REQUEST CHANGES | Risk: Medium | 3 issues (1🔴 2🟡)
```

**Part (b) — Summary table:**

| Severity | Location | Description |
|---|---|---|
| 🔴 | `FundingPage.groovy:42` | Form filled manually instead of `completeAllRequiredFields` |
| 🟡 | `FundingPage.groovy:58` | Missing Groovydoc on new public method |

**Part (c) — Detailed findings**: one block per issue, ordered by severity then file alphabetically. Each block consists of: severity emoji + `file:line — Rule/Issue name` on the first line; a 1-line reason on the second line; then a ` ```diff ` fence with `- bad` and `+ good` lines. See the **Example** section below for a fully rendered instance.

**Scope guide** (first line — tells reader what was reviewed):
- `SCOPE: PR equivalent on feature/AUTOKAT-794 — 5 commits + 1 uncommitted, 4 files, 142 lines` (default, feature branch with uncommitted)
- `SCOPE: branch commits only — 3 commits on feature/X vs origin/main, 2 files, 87 lines` (committed, no uncommitted)
- `SCOPE: uncommitted only — 2 files, 34 lines` (on main/master, or branch w/o commits yet)
- `SCOPE: <file path>` (single file specified)
- `SCOPE: pasted snippet (~40 lines)` (inline code)
- `SCOPE: PR #1234 diff (gh pr diff)` (PR review)

When reviewing multiple files at once (default mode), order issues by severity first, then by file alphabetically. Don't add per-file headers — the `file:line` prefix on each issue is enough.

**Passed checks line** — always present, one line at the very end:

```
✓ Passed: hard rules · 12 recurring · selector patterns · timeout usage
```

**Strict mode** adds 🟢 finding blocks (same format as Part c) between the detailed findings and the passed line.

Optional `NOTE:` line after the passed line, **only if there is non-obvious insight** (e.g., "Issue #1 is the only flaky risk; the other two are maintenance"). Skip if the issue list speaks for itself.

### HTML (when user requests export)

When the user asks to "xuất file", "save", "report", or "html", write a standalone `.html` file (e.g., `review-AUTOKAT-794.html`) that can be opened in any browser. Required structure:

- **Header**: verdict pill (green = APPROVE, amber = COMMENT-ONLY, red = REQUEST CHANGES) + scope string
- **Metric cards**: Blockers count · Warnings count · Files reviewed
- **Summary table**: same columns as Markdown Part (b)
- **Finding cards**: one card per issue; left border color red (🔴) or amber (🟡); contains rule/issue name, file:line, 1-line why, and a diff block with red background for `-` lines and green background for `+` lines
- Inline CSS only — no external dependencies

Default is always Markdown; generate HTML only when explicitly requested.

**Verdict guide:**
- `APPROVE` — zero blockers, at most 1 warning
- `COMMENT-ONLY` — zero blockers + exactly 2 warnings; OR reviewing legacy code at user's request; OR diff context is insufficient to judge
- `REQUEST CHANGES` — any 🔴 blocker, or 3+ warnings

**Risk guide:**
- `Low` — no flaky risk, only style/clarity
- `Medium` — possible flakiness or maintainability issue
- `High` — guaranteed flakiness (e.g., infinity loop, swallowed exception in core path)

## Example

**Input code** (`Test Cases/Clients/Acme/ACME-100_AUTOKAT-200.groovy`, new file):

```groovy
"PREPARATION"
def creds = DataHelper.JSON.render("DataStorage/Seeds/Acme/ACME-100.json", [:])
def data  = DataHelper.JSON.render("DataStorage/Seeds/Acme/ACME-100.json", [:])

"TEST STEPS"
Page.As(LoginPage).logInBySimulator(creds, data)

Perform.inputText(locator("label:text-is('First Name') + input"), "John")
Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
Perform.selectDropdown(locator("label:text-is('State') + select"), "CA")
Perform.click(btnSubmit)

Page.As(ConfirmationPage).verifyCurrentPageIsActive(data.PageName)
```

**Review output:**

SCOPE: pasted snippet (~14 lines)
VERDICT: REQUEST CHANGES | Risk: Medium | 3 issues (2🔴 1🟡)

| Severity | Location | Description |
|---|---|---|
| 🔴 | snippet:8–10 | Form filled manually instead of `completeAllRequiredFields` |
| 🔴 | snippet:6–13 | Missing `"Step N:"` and `"VP:"` markers throughout |
| 🟡 | snippet:8–10 | Check if field values from `data.*` need `escapeSelector()` |

🔴 snippet:8–10 — Rule 1: completeAllRequiredFields
   Three manual Perform calls replace what the data-driven dispatcher should own; breaks when a field is added or renamed in the form.

```diff
- Perform.inputText(locator("label:text-is('First Name') + input"), "John")
- Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
- Perform.selectDropdown(locator("label:text-is('State') + select"), "CA")
- Perform.click(btnSubmit)
+ Page.As(ApplicantPage).completeAllRequiredFields(data.ApplicantScreen.Fields)
```

🔴 snippet:6–13 — Rule 5: Missing Step/VP markers
   None of the actions or the verification are wrapped in `"Step N:"` / `"VP:"` literals; Katalon report will show blank step names.

```diff
+ "Step 1: Log in to the system"
  Page.As(LoginPage).logInBySimulator(creds, data)
+ "Step 2: Complete applicant fields"
  Page.As(ApplicantPage).completeAllRequiredFields(data.ApplicantScreen.Fields)
+ "VP: Verify confirmation page is shown"
  Page.As(ConfirmationPage).verifyCurrentPageIsActive(data.PageName)
```

🟡 snippet:8–10 — Issue 9: escapeSelector check
   Hardcoded string literals here are safe, but if these selectors reference dynamic `data.*` values in the real implementation, wrap them with `escapeSelector()` to guard against quotes or backslashes in data.

```diff
- locator("label:text-is('First Name') + input")
+ locator("label:text-is('" + escapeSelector(data.firstName) + "') + input")
```

✓ Passed: hard rules (3,4,6–9) · 12 recurring (1,2,3,4,5,6,7,8,10,11,12) · timeout usage

## Edge cases

- **Diff too narrow** — if ±3 lines around a change is all that's visible, don't speculate about hidden code. Note: `Cannot verify <X> without seeing the full method`.
- **Test case file is short** — if the file only has `"PREPARATION"` and `"TEST STEPS"` markers with one step, this is likely a draft. Note `Draft state — incomplete` and only flag structural blockers.
- **Generated/recorder code** — if locators are clearly auto-generated (huge absolute XPath, `.css-1a2b3c`), flag for refactor before merge.
- **Framework code** (`Keywords/katalon/core/*`) — apply stricter scrutiny: param types mandatory, Groovydoc mandatory, no `def`, no unjustified retry.
- **Legacy file assessment** — if user says "this is legacy, just assess", use `COMMENT-ONLY` verdict and focus on flaky-risk only.

## What this skill does NOT do

- Does not auto-fix code — only suggests; user applies changes.
- Does not run tests — for execution, use `katalon-execute-and-fix`.
- Does not analyze failure logs — use `katalon-failure-analyzer`.
- Does not review non-Katalon Groovy (e.g., generic scripts outside this project).

## References

- Data location: `DataStorage/Seeds/<Client>/<TICKET>.json` for input data; `DataStorage/Import/<TICKET-ID>/` for files to upload.
- Timeouts: `GlobalVariable.DEFAULT_TIMEOUT`, `GlobalVariable.LONG_TIMEOUT`, plus `defaultTimeout` / `longTimeout` shortcuts on `CommonPage`.
- Selector helpers: `smartOR(elem)` for single TestObject conversion; `baseOR(elem)` for multi-element/count operations.
- For deeper convention details, see the `katalon-convention-linter` skill if available.