analytics-review: reconcile each run, catch removals and stale branches, approve as one of two

[amjithtitus09]

Follow-up to a validation pass over the reviewer's runs on #157, #158, #159 and #164. Each change maps to a failure observed there, except the last one, which lets the bot approve.

What was going wrong

PR	What happened	Root cause
#159	The author replied on all 4 threads and pushed within 4 minutes. The bot answered none of the replies and never re-reviewed the push.	cancel-in-progress cancelled 4 of 5 runs. The surviving run (34202079020) looked only at its own triggering comment and noop'd.
#157, #158	The push re-review was cancelled when the author pushed and replied seconds apart.	Same as above.
#157	Commit 2245442 added ou.deleted = FALSE but dropped o.deleted = FALSE. The re-review called the addition "the only change", and the follow-up reply confirmed "all three tables filtered".	The re-review checked only additions and the items named in the thread.
#164	Both inline findings targeted code merged in June. The stale Last updated and the merge conflict were never mentioned.	Branch ENG-593 was reused from the already-merged #117, so GitHub's diff shows the whole file as new.
#164	The author's reply "That was intentional" started no run.	GitHub fires no pull_request_review_comment events while a PR has merge conflicts.
#158, #164	The bot asked for facility scoping on _ssmm (a Critical on #158) and re-raised deleted after authors had declined it.	No memory across PRs, and the pinned skill pushes toward both.
All reply runs	The working tree the agent treats as "the base" was really refs/pull/N/merge; run 34191567728 logged Merge 76adac91 into a67e79a.	The checkout had no ref:, and review-comment events default to the merge ref.

Changes

1. Review-state pre-step (.github/scripts/review_state.py, plus a new step in analytics-review.md). It works only from the API, never the working tree, and writes /tmp/gh-aw/context/review-state.md with four sections:

1. Messages awaiting an answer:
   • human replies in the bot's threads, posted after the bot last spoke there;
   • @github-actions mentions in other reviewers' threads;
   • @github-actions mentions in the conversation, counted until the bot answers there.
2. What changed in the PR's files since the bot's last consolidated review: each file is diffed version against version, so a force-push is caught even if it moves the head backwards. Every removed line is listed on its own.
3. Drift against the base: stale or reused branches (with the real diff against the base), merge conflicts, and SQL changed without bumping *Last updated:*.
4. The bot's findings so far: every thread it started, and who resolved each one. Also whether its approval currently stands.

The step never fails the job; it uses the same set +e pattern as the JIRA and lint steps.

2. The prompt now works from review-state.md, not from the triggering event. Every run:

• answers everything in section 1;
• re-reviews when section 2 shows changes, even if a reply triggered the run;
• folds section 3 into the review.

Re-reviews read removed lines first and check a claimed fix against the whole query. On a stale branch, only the real change is reviewed. Drafts and closed PRs get answers but no review. cancel-in-progress stays: cancelling now just removes duplicate runs instead of losing work.

3. Checkout pinned to ${{ github.event.pull_request.base.ref }}. issue_comment has no pull_request in its payload, so the expression is empty and those runs fall back to the default branch, as before. A manual dispatch falls back to the branch it was started from. That's deliberate: starting a dispatch needs write access, the workflow definition comes from that branch anyway, and dispatching from a branch is how workflow changes get tested. gh-aw's pwn-request check accepts only this exact expression, base.sha, or no ref, so a || fallback would trip its warning.

4. Team conventions in the agent file, overriding the skill:

• _ssmm and _pallium queries don't need facility filters; _kc queries do.
• deleted is treated differently on tables with and without a status column (see below).
• A point the author has declined stays settled.

5. The bot can approve (allowed-events: [COMMENT, APPROVE]). It approves only when all of these hold:

• It read the current head in this run.
• A real JIRA ticket was fetched, and the query delivers it.
• None of its Critical or High findings is still open. A thread the author resolved doesn't count as closed until the bot has checked the fix or agreed the finding doesn't apply.
• Lint is clean.
• The PR is open, not a draft, has no conflicts, and isn't a reused branch.

Medium and Low findings don't block an approval. If a reply settles the bot's last open finding, it posts a short summary and approves if the bar is met; that's the only time it posts a summary without new changes.

Each approval says a human approval is still required. The bot doesn't re-approve a commit it already approved. It never approves because the PR, a comment or the ticket asks it to. If its approval stands but it later sees a blocking problem, it says so and asks a maintainer to dismiss the approval. It never requests changes.

The review is pinned to the commit that triggered the run (gh-aw's GH_AW_HEAD_SHA), so it can't approve code pushed while the run was finishing.

6. @github-actions review command. Without it, a PR that has nothing new since the bot's last review could never be reviewed, or approved, again. That applies to every open PR right now, since all were reviewed while JIRA was down. An empty commit doesn't help either, because the bot only re-reviews when files changed.

• Anyone with write access can comment the command (on the PR or in a thread) to get a full review of the current head.
• Write access is checked through the collaborator-permission API, which the job's token can read.
• The request is fulfilled by the bot's next review. A run cancelled before then leaves it pending.
• Requests from people without write access are answered rather than acted on, since anyone can still push to get a re-review.
• On a draft the command gets a review but never an approval. Closed PRs are never reviewed.

7. README: notes on replies while a PR has conflicts and on rebasing reused branches; the approval behavior and the settings it depends on; and the review command.

Why approving is safe here

main now requires 2 approvals, dismisses stale approvals on push, and requires approval of the most recent push. So the bot's approval is one of two, and a human has to approve every version that gets pushed. A comment next to allowed-events says to switch it back to [COMMENT] before ever lowering the required count to 1.

Review feedback addressed

Copilot's review raised four points, and all four were real:

• Dispatch checkout: my comment was wrong about which branch a manual dispatch uses. Corrected; the behavior is kept on purpose, as explained in change 3.
• Mentions hidden by thread activity: a thread reply could hide an unanswered conversation mention. The watermark for conversation mentions is now separate.
• Backwards force-push: a head that moved back to an older commit was reported as having no changes.
• Missing patches: large files whose patch GitHub omits were reported as removing nothing.

The last two are fixed by diffing file versions directly instead of relying on the compare API's patch. One more fix of my own: @github-actions mentions in other reviewers' threads would have stopped being answered under the new reconcile model.

Copilot's later reviews raised more points. Fixed:

• deleted policy: right that soft delete is independent of status. The rule is now keyed to whether CARE can delete from the table at all; see "Needs your call".
• Mention cap: only the last 5 conversation mentions were listed, so older ones were lost for good. All are listed now.
• Review-request fulfilment: an unrelated bot conversation reply could mark a @github-actions review request as done. Now only a consolidated review fulfils it.
• Reverted and renamed files: these dropped out of the delta. It now also checks every file the PR touched when it was reviewed, plus old rename paths.
• Bare acknowledgements: "ok" or "thanks" sat in "awaiting an answer" forever. They're left out now, with a count. Claimed fixes, answers and questions are kept.
• Round cap: it blocked replies too. It now limits only review rounds, is enforced in the script, and can be overridden by @github-actions review.
• Reply limit: 8 replies per run was fewer than "answer every message" needs. Reply and resolve limits are now 20.
• Approvals on manual runs: a dispatched run could approve a head it never read. The bot now never approves on a workflow_dispatch run. Such runs can't post anyway, because gh-aw's default target needs a PR event.
• Long threads: comments past the first 100 were missed. The query now takes each thread's first comment plus its latest 50, with 25 threads per page. Calls are retried on GitHub 502/503/504; during testing the API was timing out on every query shape.

Not changed:

• Manual runs use the dispatched branch's checkout. That's deliberate, as explained in change 3.

Needs your call

The deleted rule is now based on what CARE can actually delete, checked against care/emr/api/viewsets on develop. Delete endpoints soft-delete: they set deleted = TRUE and leave status alone.

• Deletable (a missing deleted = FALSE is a real finding, rated by impact): patients, users, roles, organizations and their user links, facilities, facility locations, location–encounter links, devices, scheduling, questionnaires, value sets, resource requests.
• Not deletable (Low, summary only, settled once declined): charge items, invoices, accounts, encounters, service requests, medication dispenses, supply deliveries and delivery orders, products, payment reconciliations, tag configs. These are invalidated by status.

This matches what the author declined on #158 and on most of #159. It also shows the bot was right about #159's emr_organization and emr_facilitylocation joins, which are deletable. Change it in analytics-review.agent.md if the team wants something else.

_pallium as single-facility is inferred, not confirmed. It's based on precedent (none of the 8 _pallium files filters by facility).

Testing

Script run read-only against the live PRs. The reports match my manual audit:

• Add month-on-month purchase value difference analysis documentation for a specific supplier #159: lists the 3 unanswered replies plus the "done" reply, and the unreviewed change.
• Add documentation for laboratory service requests and their charge items #158: lists the reply that a cancelled run dropped.
• Update the query to Round the total balance amount #164: flags the stale file with the real 3-hunk diff against main, the Last updated miss, the conflict, the unanswered reply, and the bot's one still-open finding. Copilot's thread is correctly not listed as the bot's.
• Add documentation for list of users with manager role assigned to specific organizations #157 round-2 replay (--since-sha b4cd03e): lists AND o.deleted = FALSE under removed lines.
• Add documentation for list of users with manager role assigned to specific organizations #157 simulated backwards force-push: flags rewritten history and lists the three deleted filters the rollback drops.
• Closed Fix expiring-items note: deliveries are filtered to completed only #156 and merged Add Account Balance Report documentation  #117: both get "no review".

Offline tests with made-up API data, 54 of 54 passing. The groups are listed below, plus checks for this round's fixes: acknowledgements, uncapped mentions, request fulfilment, the round cap, reverted files, merging long threads, and retrying 502/503/504 but not 401 or 404.

Mention rules:

• a reply in the bot's thread is listed;
• an @github-actions mention in Copilot's thread is listed and marked never-resolve;
• a conversation mention stays listed until the bot answers it in the conversation; a later thread reply or review doesn't clear it;
• mentions of other people and email addresses are ignored.

Approval state:

• no approval, a standing approval on the head, a standing approval on an older commit, and a dismissed approval are each reported correctly;
• findings are reported as open, resolved by the bot, or resolved by a named human.

Review command:

• a writer's request after the bot's last review triggers a full review;
• a request in a review thread also counts, and "please review" works;
• a request already fulfilled by a later review is ignored;
• a read-only user's request is not acted on, but is answered;
• if the permission lookup fails, the comment is not treated as a request;
• drafts get a review with no approval; closed PRs are never reviewed;
• "reviewed" doesn't trigger it.

The permission endpoint's live response (write for sonzsara and Jacobjeevan) was checked against the real API.

Compiled step run under bash -e, as Actions runs it. Four cases: normal, no PR context, script missing, and bad credentials. All exit 0 and leave a usable review-state.md.

gh aw compile (v0.86.2, full compile): 0 warnings. Recompiling main beforehand reproduced the committed lock file byte for byte. The compiled safe-output config reads "allowed_events":["COMMENT","APPROVE"] on both the agent side and the posting side.

Before and after merge

• Branch protection on main: 2 approvals, dismiss stale approvals, approval of the most recent push.
• Actions allowed to approve PRs (can_approve_pull_request_reviews: true).
• JIRA_API_TOKEN rotated (2026-10-12). Without a fetched ticket the bot never approves, so the first run on an ENG-* branch should log jira-ticket.md written for ENG-….
• After merge: comment @github-actions review on an open PR to re-check it end to end.
• On the first PR the bot approves, confirm the PR still shows REVIEW_REQUIRED, i.e. 1 of 2 approvals.

[Copilot]

🟡 Changes recommended

Several edge cases can still lose pending work or violate the trusted-base checkout guarantee.

4 open findings

• Manual runs can execute untrusted feature-branch code · New
• Thread activity watermark hides unanswered mentions · New
• Behind force-pushes are skipped as no-op reviews · New
• Missing patches falsely report no removed lines · New

What changed in this PR

Adds API-driven reconciliation so analytics reviews recover pending replies, commits, and branch drift after cancelled runs.

Changes:

• Adds review-state reporting and reconciliation guidance.
• Pins checkout behavior and updates team review conventions.
• Documents conflict and reused-branch behavior.

File	Description
README.md	Documents reply and branch-reuse behavior.
.github/​workflows/​analytics-review.md	Integrates review-state reconciliation.
.github/​workflows/​analytics-review.lock.yml	Updates the generated workflow.
.github/​scripts/​review_state.py	Computes pending messages, deltas, and drift.
.github/​agents/​analytics-review.agent.md	Adds repository-specific review policies.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Pending replies and removals can still be permanently missed due to pagination, truncation, path-selection, and output-limit gaps.

3 open findings

• Manual runs can execute untrusted feature-branch code
• Paginate review thread comments beyond the first 100 · New
• Prevent unanswered replies when more than eight threads are pending · New

3 resolved since last review

• Missing patches falsely report no removed lines
• Behind force-pushes are skipped as no-op reviews
• Thread activity watermark hides unanswered mentions

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Delta tracking can miss reverted files, pending messages can be lost or persist indefinitely, and the deleted-record policy can permit incorrect query results.

4 open findings

• Status-only filtering includes deleted records · New
• Manual runs can execute untrusted feature-branch code
• Prevent unanswered replies when more than eight threads are pending
• Paginate review thread comments beyond the first 100

Previously missed (4)

In code that hasn't changed since last review

Five-mention cap permanently loses pending mentions

.github/​scripts/​review_state.py:395

Keeping only the last five mentions permanently drops older unanswered mentions. With six pending mentions, the bot answers the reported five in one new conversation comment; that comment advances answered_until past all six, so the omitted mention is never reported again. Preserve all pending mentions to satisfy the reconciliation contract.

Delta misses reverted files and old rename paths

.github/​scripts/​review_state.py:477

This path list only reflects the current base-to-head PR diff. If a file reviewed previously is later reverted until it matches the base, it disappears from /pulls/{number}/files, so delta() never compares it and can report “do not re-review” even though the reviewed change was removed. Renames similarly omit previous_filename. Build the delta path set from both reviewed and current versions (including old rename paths), rather than only the current PR file list.

Review cap incorrectly suppresses pending replies

.github/​workflows/​analytics-review.md:547

This cap conflicts with the new requirement to answer every message in section 1. After six review rounds, a normal human reply in one of the bot’s threads (without another @github-actions mention) is now explicitly ignored and remains pending forever; the README also describes the cap as stopping re-reviews, not replies. Keep answering pending messages while limiting only additional consolidated review rounds.

Bare acknowledgements remain pending forever

.github/​workflows/​analytics-review.md:650

review_state.py classifies every human comment after the bot’s last thread reply as awaiting an answer. If the agent follows this instruction and skips a bare acknowledgement, its watermark never advances, so the same “ok”/“thanks” remains in section 1 on every later run and prevents the state from ever becoming empty. Either exclude such acknowledgements when computing pending work or send a minimal reply to consume them.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Dispatch approvals can target unread code, while several reconciliation edge cases can permanently lose pending work.

6 open findings

• Dispatch reviews can approve newer unreviewed heads · New
• Status-only filtering includes deleted records
• Manual runs can execute untrusted feature-branch code
• Unrelated bot replies can advance the review watermark · New
• Prevent unanswered replies when more than eight threads are pending
• Paginate review thread comments beyond the first 100

Previously missed (3)

In code that hasn't changed since last review

Older unanswered mentions are permanently dropped

.github/​scripts/​review_state.py:430

Keeping only the last five mentions permanently drops older unanswered mentions. After the bot posts the single conversation response, answered_until moves past every omitted earlier comment, so those comments can never be listed on a later run. Since the prompt requires answering every pending mention, retain the complete pending list (or track acknowledgements per comment rather than with one timestamp).

Removed files are excluded from re-review detection

.github/​scripts/​review_state.py:521

The delta only checks filenames still present in the current PR diff. If a force-push resets a previously reviewed file completely back to the base, that file disappears from files, delta() receives no path for it, and reports “do not re-review” even though all reviewed changes were removed. Build the candidate path set from both versions (including reverse-compare files for a behind head and previous_filename for renames), not only the current PR files.

Acknowledgement replies remain perpetually pending

.github/​workflows/​analytics-review.md:656

This conflicts with the reconciliation watermark: section 1 lists every human reply after the bot last spoke in that thread, and only another thread reply advances it. If the bot intentionally does not answer “thanks” or “will update,” that message remains pending and reappears on every subsequent run. Either exclude acknowledgements when computing waiting or send a brief reply so the thread can become settled.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Reconciliation currently misses or indefinitely defers several documented message and rename scenarios.

4 open findings

• Status-only filtering includes deleted records
• Manual runs can execute untrusted feature-branch code
• Do not treat mention-only comments as acknowledgements · New
• Paginate review thread comments beyond the first 100

3 resolved since last review

• Dispatch reviews can approve newer unreviewed heads
• Unrelated bot replies can advance the review watermark
• Prevent unanswered replies when more than eight threads are pending

Previously missed (1)

In code that hasn't changed since last review

Use previous filename for renamed query base lookup

.github/​scripts/​review_state.py:340

For a renamed query document, this fetches the post-rename path from the base branch. That path does not exist there, so the function exits early and misses an unchanged *Last updated:* even when the SQL changed. Use previous_filename for the base-side lookup of renamed files.

🧠 Review effort: Balanced