Review And Fix

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

Composed with merge-gate:

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

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:

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)

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):

$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 GETs 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).