review-and-fix

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:

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)

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 — / 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, 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).

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 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/.