---
name: review-and-fix
description: >
  Deep-review one open PR, apply the resulting fixes, verify each one live
  (build → local redeploy → Playwright + DB), commit, push, drive CI to
  green, then reply to the review threads. Use when asked to "fix the review
  findings on #N", "review and fix PR #N", "clean up #N before merge", or on
  a PR a prior merge-gate run left BLOCKED-REVIEW. It never merges, approves,
  or requests changes — it hands back a green, fixed PR ready for the user's
  final review and merge.
argument-hint: "<pr-number>"
---

# Review and Fix (HMIS)

Given one open PR: fresh deep code review → classify and discuss the
non-obvious findings → apply the fixes → verify each one live (build →
local redeploy → Playwright + DB, or browser-only for JSF-only changes) →
commit → push → **drive CI to green** → reply to the review threads.

**This skill never merges, approves, or requests changes.** It ends when the
PR is fixed, pushed, its review threads answered, and **CI is green on the
head commit** — ready for the user's final review and merge.

Invoking this skill is the explicit authorization for every commit / push /
thread-reply step below — do not re-ask before each one. The discussion
gate is step 3 (classify + discuss the non-obvious findings); that is the
only point where you pause for the user.

## Background

`merge-gate` deliberately **never fixes anything** — it finds blocking
issues and hands back a report. On its first substantial run against
contributor PRs (`#23348`, `#23229`, `#22041`) the gate did its job: it
found a footer column-alignment defect on `#23229` and four High-severity
correctness findings on `#22041` (`paidAt` overwritten on every row save,
implicit INNER-join row drops inside `SELECT NEW`, a date-only "To Date"
excluding the last selected day, `paidAmount` left stale when "Paid" is
unchecked). Handing that report back to the contributor to fix proved
slower and less reliable than fixing it directly, for two reasons:

1. **Project-convention knowledge.** One flagged "bug" on `#22041` — the
   "Invoice Approved" filter keying on `b.createdAt` instead of an approval
   timestamp — was a false positive: `b.createdAt` is the exact proxy the
   sibling `InwardReportController` uses, and outside-charge bills never
   populate `approveAt`, so "fixing" it would make the filter match
   nothing. Telling that apart from a real bug needs someone who can read
   the sibling report.
2. **A real verification loop.** Confidence that the `paidAt`-preservation
   fix held required rebuilding, redeploying locally, seeding test rows,
   driving the Update flow in Playwright, and checking the `bill` row in
   the DB before and after editing an unrelated field on an already-paid
   row.

No existing skill covers that middle step. This one does.

## Non-goals

- **Not a replacement for `merge-gate`.** `merge-gate` decides *whether* a
  batch of PRs is safe to merge and runs two fixed baseline regression
  checks unrelated to any PR's scope. This skill *fixes* one PR the gate
  (or the user) has already flagged.
- **Not a replacement for `review-pr`.** It *invokes* `review-pr` for the
  thread-reply step rather than re-implementing the cardinal rules.
- **Not a general test-automation framework.** It drives the existing
  `code-review`, `playwright-e2e`, and `review-pr` skills.
- **Not batch.** One PR per invocation — the verify loop needs focus;
  `merge-gate` already frames the batch.
- **Does not merge.** Final review and the merge button are always the
  user's.

## Where this sits among the review skills

| Skill | Fixes? | Fresh review? | Live verify? | CI-green loop? |
|---|---|---|---|---|
| `merge-gate` | No (by design) | Yes (`code-review --comment`) | Yes (E2E + 2 baselines) | No — reports outcome |
| `review-pr` | Yes | No — triages existing bot threads | No (`Read/Grep/Glob/Bash`) | Partial (checks green before replying) |
| `review-code` | No | Manual checklist | No | No |
| `code-review` (built-in) | `--fix` blind-applies | Yes | No | No |
| **`review-and-fix`** (this) | **Yes** | **Yes** | **Yes** | **Yes — hard exit condition** |

Composed with `merge-gate`:

```text
merge-gate #A #B #C          # gate a batch
  -> #A PASSED
  -> #B BLOCKED-REVIEW
  -> #C BLOCKED-REVIEW
review-and-fix #B            # fix + verify + push + CI-green #B
review-and-fix #C            # fix + verify + push + CI-green #C
merge-gate #B #C             # re-gate -> PASSED
# user merges
```

Each skill stays single-purpose; `merge-gate` keeps its "never touches
code" identity.

## Arguments

- `$0` — one **PR number** (not an issue number). If the number doesn't
  resolve to an open PR, say so and ask for the correct PR number rather
  than guessing which PR closes an issue.
- Optional second argument: a `merge-gate` status-comment URL (or the words
  "merge-gate findings"). When given, fix only what that prior gate flagged
  rather than re-reviewing from scratch — read the linked inline comments,
  skip step 2's fresh `code-review`, and go straight to step 3.

Process exactly one PR per invocation. `merge-gate` already handles the
batch framing; the verify loop here needs to stay focused on one branch.

## 1. Checkout

```bash
git fetch origin
git checkout -- src/main/resources/META-INF/persistence.xml
gh pr checkout <PR>
```

The `git checkout --` discards any leftover uncommitted local-JNDI edit
before the branch switch (safe no-op if there is none). Then restore
`persistence.xml` to local JNDI (`jdbc/coop` / `jdbc/ruhunuAudit`) per
CLAUDE.md, left **unstaged**. Note the exact JNDI names — you restore them
again after the push in step 6.

Record the PR's base branch and how far behind it is. Measure from the
checked-out working tree, not a `origin/<head>` ref — `gh pr checkout` does
not create one for a fork-backed PR:

```bash
git rev-list --count HEAD..origin/development
```

A branch more than a few hundred commits behind `development` is worth a
rebase note in the final report — a clean textual auto-merge can still hide
semantic drift in a helper the changed code calls.

## 2. Fresh review

Skip this step if the optional second argument pointed at a prior
`merge-gate` result — use those findings instead.

Otherwise invoke the `code-review` skill against this PR at **high** effort,
**without** `--comment` or `--fix` — you are going to fix and verify each
finding by hand, not annotate the PR or blind-apply a patch.

Collect the findings it returns with their categories.

## 3. Classify and discuss (the one discussion gate)

| Category | Handling |
|---|---|
| correctness, regression, business-rule violation | **Must fix.** |
| security, privacy, data-integrity, availability | **Must fix** — never treated as optional. |
| style, simplification, efficiency, reuse-only | **Optional.** List them; ask the user whether to include any. |

Before touching code, present the must-fix list and the non-obvious calls
to the user and get a nod. Non-obvious means: anything that could be
**project intent rather than a bug**. Check each candidate against the
codebase and the known false-positive patterns from `review-pr` /
`review-code` first:

- Null checks where lazy init already handles it (e.g.
  `getBillFinanceDetails()`).
- "Fixes" for intentional typos (`purcahseRate`) — database compatibility.
- Constructor-signature changes — CLAUDE.md forbids modifying existing
  constructors; only add new ones.
- Native-SQL suggestions where JPQL is adequate (JPQL-first rule).
- Bootstrap CSS classes where the project uses PrimeFaces.
- A filter/column that looks "wrong" but matches how a sibling report in
  the same module does it (verify against that sibling before "fixing").

For each candidate, state: **Valid — will fix** / **False positive —
<reason>** / **Discuss**. Wait for the user on anything marked Discuss;
don't burn a build guessing.

## 4. Apply the fixes

Apply the confirmed batch. Match the surrounding code's style, naming, and
comment density. Respect the HMIS hard rules (CLAUDE.md): JPQL-first, never
modify existing constructors, `findLongByJpql` for `COUNT`, no hospital-name
gating in `rendered`/conditionals, wire new report buttons into Report
Favorites, etc.

Group everything into **one logical commit** (drafted in step 6), not one
commit per finding.

If a fix adds or renames a persisted entity field, run the `generate-ddl`
skill before moving on (same as `dev-issue` §5a). Skip it for pure
business-logic / query / view fixes.

## 5. Verify each fix live

Do not trust "the code looks right." Every must-fix finding gets exercised.

### 5a. JSF-only changes (XHTML, no Java)

Local Payara serves the exploded WAR and picks up an edited `.xhtml` on the
next request, so a full `mvn package` / `redeploy` is usually unnecessary.
**Confirm the edit is actually live before asserting anything** — hard-reload
the page and check the changed markup is present in the DOM; if it isn't
(stale facelet cache, WAR not exploded), redeploy per §5b first. Then drive
the affected page via the `playwright-e2e` skill: login, select a relevant
department, **navigate to the page through the menus — never by URL** (see
`playwright-e2e` §2; a URL-loaded page renders against uninitialised
session state and produces false findings), reproduce the exact scenario the
finding was about, and confirm
the new behaviour with DOM assertions or a screenshot. Column-alignment,
`rendered` guards, AJAX-update targets, dialog wiring — all observable this
way once the edit is confirmed live.

### 5b. Java changes

Rebuild and redeploy to local Payara, per `playwright-e2e` §0a / `dev-issue`
§6 (tool paths in CLAUDE.md § Local build tools — verify against the
`reference_maven_path` memory; the paths hardcoded in some skill snippets
are stale for this machine):

```powershell
$env:JAVA_HOME="<JDK 11 path>"
& "<mvn.cmd>" clean package -DskipTests
& "<asadmin.bat>" [--port <admin-port>] redeploy --name <app> "<project-root>\target\rh-3.0.0.war"
```

Check `server.log` for deployment errors before touching the browser. If
`mvn clean package` or `asadmin redeploy` fails, fix the compile/deploy
problem before continuing — a stale WAR verifies nothing.

Then, via `playwright-e2e`: log in, select a department the feature
touches, and exercise the **specific** changed behaviour with real records.
Verify the result in the local DB with read-only `mysql` queries
(credentials: `local_mysql_credentials` memory / `C:\Credentials\`).

If the local DB lacks data to exercise the finding, in order of preference:

1. Use an **existing** record that fits (read-only navigation / API `GET`s
   to find one).
2. Create it **through the app** — the normal billing/admission/report
   workflow — so it is a real, consistent entity.
3. Only if both are blocked (e.g. the UI path 500s on an unrelated
   pre-existing bug), **ask the user** before seeding anything by direct
   SQL. If they approve, keep the insert minimal and schema-valid (real
   enum names, required FKs), scope it to this one verification, and
   **delete it in the same session** — it is throwaway test scaffolding,
   never left behind. This is not "mock data in business logic" (which
   CLAUDE.md forbids); it is a disposable fixture for one browser check.

Never fall back to "code looks correct" as the evidence.

**Local Payara connection-pool note:** a long-idle local domain can start
throwing `EJBTransactionRolledbackException: Client's transaction aborted`
on unrelated queries (patient allergies, favourite reports). Flush the
pools (`asadmin flush-connection-pool poolCoop`,
`... poolRuhunuAuditLocal`) or `restart-domain` — it is not a bug in the
fix. See the `stale_audit_connection_pool_local` memory.

Capture a screenshot / query output for each verified finding into the
project `tmp/` folder. Redact patient identifiers, credentials, and tokens
**as it is written** — `tmp/` is on disk in the project tree, so raw
sensitive evidence must not land there even transiently. Crop/mask
screenshots before saving; select only non-sensitive columns in the
verification query. Remove the `tmp/` artifacts at the end (step 6).

## 6. Commit and push

`persistence.xml` holds a local JNDI name for the duration of this skill and
must end back that way **no matter how this step exits**. Treat the restore
as a `finally`: if the commit or push fails, or you abort here for any
reason, your very next action is to put the local JNDI names back and leave
that change unstaged. Never walk away from this step with `${JDBC_DATASOURCE}`
in the working tree.

1. Check `src/main/resources/META-INF/persistence.xml` — if
   `<jta-data-source>` holds a local JNDI name, note both values, then swap
   both units to `${JDBC_DATASOURCE}` / `${JDBC_AUDIT_DATASOURCE}` with
   `Edit`.
2. `git add` the intended source/doc files plus `persistence.xml` (now
   holding placeholders).
3. Commit with the
   [Commit Conventions](../../../developer_docs/git/commit-conventions.md)
   format — imperative subject, Co-Authored-By trailer. Body: one line per
   finding fixed, each naming the file:line and how it was verified; a
   final short paragraph for any finding deliberately **not** fixed (a
   false positive) and why. **If the commit fails → do 6.5 below and stop.**
4. `git push`. **If the push fails → do 6.5 below and stop.**
5. (6.5) Restore `persistence.xml` to the local JNDI names from 6.1 with
   `Edit`, left **unstaged**. Then `grep` the file to confirm both units
   read `jdbc/...` and not `${...}` before moving on. This restore runs on
   every exit from §6 — success or failure.

Then clean up the `tmp/` evidence.

## 7. Drive CI to green — the skill does not end until it is

`developer_docs/git/pr-review-workflow.md` is explicit that CI must be green
**before** replying to review threads and that there is exactly **one**
re-review request, at the very end. So the reply-in-full step (8) runs after
CI is green — not here. This step only reaches a green head commit, applying
review fixes reply-only along the way.

Wait for **every** check on the head commit: `validate-compilation`,
`validate-jdbc-data-sources`, CodeRabbit, and anything else the PR runs.

- `pending` is **not** a stopping point. Poll it out — `ScheduleWakeup`
  ~270s (same cadence as `dev-issue` §14) and recheck; don't block with
  `gh pr checks --watch` past a couple of minutes.
- On a **check failure**: read the failing job's log, fix the cause, commit,
  push (step 6's `finally` rule for `persistence.xml` applies to every push),
  go back to the top of this step.
- **New CodeRabbit / Codex comments on the fix commit** → loop back to step 3
  for those (classify → fix → verify → commit → push). For each thread you
  acted on, post a **reply-only** note now (`/replies` endpoint,
  `gh api .../pulls/<PR>/comments/<id>/replies`) — "Fixed in `<sha>`: `<what
  changed>`" or "Dismissed because: `<reason>`". Do **not** run the full
  `review-pr` skill here and do **not** re-request review yet — those happen
  once in step 8.

### Loop bounds

- At most **3 review→fix cycles**. If CodeRabbit is still raising new
  substantive findings after the third, stop and ask the user.
- At most **~40 minutes** of wall-clock polling for a stuck `pending` check
  (CodeRabbit is frequently slow / rate-limited on this repo). Past that,
  stop: report which check is stuck and that the two `validate-*` checks are
  green, and let the user decide whether CodeRabbit is a blocker.
- A check that goes **red and stays red** after a fix attempt → stop, report
  exactly which check, the failure, and everything tried. Never hand back a
  half-green PR silently.

Only a **fully green head commit** (or an explicit user decision that a
stuck-pending non-required check is acceptable) lets you proceed to step 8.

## 8. Reply to the review threads (once, after CI is green)

Run the `review-pr` skill for the same PR number. It owns the cardinal
rules — `/replies` endpoint only, never a new top-level thread, no "please
resolve" wording (it triggers a CodeRabbit-Chat auto-PR against a stale
snapshot), self-review items live in the commit message, **one** re-review
request at the end. The fixes are applied, pushed, and CI-verified by now,
so `review-pr`'s reply text describes what was done — "Fixed in
`<head-commit-sha>`: `<what changed>`" for the findings you fixed,
"Dismissed because: `<reason>`" for any false positive — not what a reviewer
should do next. Threads you already answered reply-only in step 7 don't need
a second reply; `review-pr` covers whatever remains and issues the single
re-review request.

If this PR came from a `merge-gate` run, also post one new top-level status
comment recording the fixes applied (commit SHA, one line per finding, and
what was verified live) — this is the same carved-out exception `merge-gate`
uses for its own outcome comments, so a merger who wasn't in the session can
see the gate's findings were addressed.

## 9. Report

Give the user:

- The PR link and the head-commit SHA.
- One line per finding: what it was, how it was fixed (or why dismissed),
  and how it was verified live.
- The CI state — say "green" only when every check on the head commit is
  actually green; if a non-required check is stuck pending and the user
  accepted that per step 7, say so explicitly instead.
- Any rebase caveat from step 1 (branch far behind `development`).
- "Ready for your final review and merge."

**Never merge, approve, or request changes** — that is always the user's
call (matching `dev-issue` §15, `merge-gate`, `review-pr`).

## Definition of done

The skill has **not** completed until **all** of these hold:

- every must-fix finding is fixed **and** verified live (JSF-only: exercised
  in the browser after confirming the edit is live);
- a false positive is left unfixed only with its reasoning recorded in the
  commit body and the final report;
- the fixes are committed and pushed (one logical commit for the review
  batch; additional small commits for any follow-up review-loop fixes are
  fine);
- every review thread has a threaded reply (fixed / dismissed-with-reason) —
  intermediate loop threads answered reply-only in step 7, the rest via
  `review-pr` in step 8, with its single re-review request;
- **CI is fully green on the head commit** — not pending, not "probably
  fine", green — OR the user has explicitly accepted a stuck-pending
  non-required check per step 7's loop bounds;
- `persistence.xml` is back to local JNDI, unstaged; `tmp/` evidence
  removed; working tree otherwise clean.

Stopping after the push, or after replying to threads, or with CI still
pending / red (and no explicit user sign-off on it), is a bug in the skill
— that is the exact failure mode that motivated it.

## Hygiene

- `persistence.xml` discarded and restored to local JNDI around checkout
  (step 1) and again right after **every** push — including the review-loop
  pushes in step 7 — as a `finally`, never only on the success path. Always
  left unstaged, and `grep`-confirmed to read `jdbc/...` afterwards.
- Temporary screenshots and query output go to the project `tmp/` folder,
  redacted of patient / sensitive data **as they are written**, and removed
  at the end.
- Never `git push --force` or skip hooks.

## Not mirrored to `.codex/skills/`

This skill drives `code-review`, `playwright-e2e`, and `review-pr`, and uses
the `Agent`, `mcp__playwright__*`, and `ScheduleWakeup` tools — the same
Claude-only dependency set as `dev-issue`, `dev-issue-unattended`, and
`merge-gate`, none of which are present under `.codex/skills/`.