feat(review): port the agentic review-and-merge pipeline (LFXV2-2276)#103
Open
josep-reyero wants to merge 11 commits into
Open
feat(review): port the agentic review-and-merge pipeline (LFXV2-2276)#103josep-reyero wants to merge 11 commits into
josep-reyero wants to merge 11 commits into
Conversation
Land the LFX agentic review-and-merge flow in lfx-mcp: - The four deterministic workflows (agentic-escalation, agentic-conductor, agentic-apply, agentic-gate) ported byte-for-byte from the production reference in lfx-v2-newsletter-service, per the porting doctrine (harness verbatim, brains repo-specific). - Seven Copilot skill brains under .github/skills/: the reviewer router (copilot-code-reviewer) and its two method skills (mcp-code-review, mcp-security-review), the escalation judge (needs-human-escalation) and its guidelines (escalation-guidelines, carrying the fork-replay escalation retune: escalate on the boundary a diff moves, not on visible-surface reshaping), the pr-conductor, and the shared agentic-comment-format contract. Brains are written against lfx-mcp's real threat model (registration-time gating, two-class upstream authorization, token plumbing, structured-output exposure) and were validated on a 20-PR fork-replay evaluation. - .github/copilot-instructions.md rewritten as the pure agentic-review router (the newsletter shape); the PR #101 repo-guide content it carried lives on as the house-standards distillate inside mcp-code-review, where the reviewer loads it, with AGENTS.md still the canonical source. The gate runs fail-soft until deployment wiring lands (lfx-reviewer PAT via OIDC/Secrets Manager, branch protection, CODEOWNERS): without the PAT every privileged job skips green and nothing is posted. The NEEDS_HUMAN_UNLABELERS allowlist in agentic-gate.yml is ported as-is and needs a maintainer decision before arming. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Extending NEEDS_HUMAN_UNLABELERS takes a reviewed PR to this file by design; requested by the repo maintainer during the port review. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Ports the agentic PR review pipeline into lfx-mcp, separating model judgment from deterministic GitHub state changes.
Changes:
- Adds escalation, conductor, apply, and approval-gate workflows.
- Adds repository-specific review, security, escalation, and reconciliation skills.
- Replaces Copilot guidance with task routing.
Blocking issues remain: clean statuses are not creator-authenticated, and incomplete agent ledgers can incorrectly produce a clean result. Reviewed with /copilot-code-reviewer, /mcp-code-review, and /mcp-security-review.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/agentic-gate.yml |
Evaluates gate inputs and manages automated approval. |
.github/workflows/agentic-escalation.yml |
Starts per-head human-escalation judgments. |
.github/workflows/agentic-conductor.yml |
Coordinates review rounds and reconciliation. |
.github/workflows/agentic-apply.yml |
Parses verdicts and applies labels/statuses. |
.github/skills/pr-conductor/SKILL.md |
Defines finding reconciliation behavior. |
.github/skills/needs-human-escalation/SKILL.md |
Defines human-escalation decisions. |
.github/skills/mcp-security-review/SKILL.md |
Adds the repository security-review method. |
.github/skills/mcp-code-review/SKILL.md |
Adds code-quality review guidance. |
.github/skills/escalation-guidelines/SKILL.md |
Documents escalation boundaries. |
.github/skills/copilot-code-reviewer/SKILL.md |
Defines reviewer behavior and severities. |
.github/skills/agentic-comment-format/SKILL.md |
Defines machine-readable verdict formats. |
.github/copilot-instructions.md |
Routes agentic review tasks to skills. |
| # created_at has only second precision, so a same-second | ||
| # success/failure pair would tie and sort by state instead. | ||
| CLEAN="$(gh api "repos/$REPO/commits/$HEAD/statuses" --paginate \ | ||
| --jq ".[] | select(.context==\"agentic-review/clean\" and ((.description // \"\") | contains(\"pr=#$PR:\"))) | [(.id|tostring), .state] | @tsv" \ |
Contributor
Author
There was a problem hiding this comment.
This is a real observation, but it is a documented, deliberately accepted trust boundary of the pipeline, not a defect this PR introduces — and the actor it requires is already inside a stronger documented boundary.
The header of agentic-apply.yml (lines 17–28) documents that any write-access insider can already forge the verdict comments themselves through the repo-wide MCP credential: they can emit a clean: true agentic-check as the trusted identity, which the apply step will then convert into exactly the status this finding worries about. Creator-filtering the status read would therefore not close the bypass — the same actor class forges one layer up. The boundary is documented as "accepted for the pilot: it is an insider-level act, fully auditable … and reversible", with per-task credential isolation named as the platform ask.
| # Full status history, newest stamp for THIS PR by numeric status | ||
| # id — see Evaluate gate. | ||
| NOW_CLEAN="$(gh api "repos/$REPO/commits/$HEAD/statuses" --paginate \ | ||
| --jq ".[] | select(.context==\"agentic-review/clean\" and ((.description // \"\") | contains(\"pr=#$PR:\"))) | [(.id|tostring), .state] | @tsv" \ |
Contributor
Author
There was a problem hiding this comment.
Same boundary as the previous thread: the racing actor needs commit-status write access, and that actor class can already forge the verdict comment itself through the documented shared-credential boundary in agentic-apply.yml (accepted for the pilot; per-task credential isolation is the platform ask). Creator-filtering here would not remove the insider capability.
| ROWS_FULL=$(printf '%s\n' "$BLOCK" | grep -E '^[[:space:]]*-[[:space:]]*id:' || true) | ||
| ROWS=$(printf '%s\n' "$ROWS_FULL" | sed -E 's/reason:.*//' || true) | ||
| BLOCKING_N=0; INVALID_N=0; UNKNOWN_N=0 | ||
| if [ -n "$ROWS_FULL" ]; then |
Contributor
Author
There was a problem hiding this comment.
The ledger's completeness is deliberately owned by the trusted-agent layer, not re-derived in apply, and the gap this describes is narrower than it looks:
- The baseline (first-round) check is authored deterministically by
agentic-conductor.ymlitself — it enumerates every AI thread mechanically (fail-closed: unprefixed findings default to should-fix), so no thread can be missing from round one. - Later rounds come from the conductor agent under the carry-forward invariant (every previously-blocking row must reappear until genuinely cleared), and only a comment authored by the trusted
lfx-revieweridentity is parsed. An agent silently omitting a thread is the trusted-agent-misbehaving case, which sits inside the same documented insider/trusted-identity boundary as the apply header describes — an actor who can shape that comment can also just writeclean: true. - Re-enumerating threads in apply would also need apply to re-derive nit-vs-blocking severity from thread text (PR-author-influenced content), moving judgment into the deterministic layer — the design keeps judgment in the agent and mechanical validation in apply.
Strengthening the validator (e.g. requiring every AI thread id present in the ledger, severity from the finding prefix) is a reasonable hardening direction, noted for a follow-up.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
.github/workflows/agentic-apply.yml:215
- [high]
clean: trueis accepted when the agent omits every ledger row. This check only rejects blocking/invalid/foreign rows that are present; withROWS_FULLempty, a later-round comment can silently drop a previously outstanding finding despite the conductor contract requiring carry-forward, and this publishes the success status that drives the CODEOWNERS approval. Validate that every prior blocking thread and every current non-nit AI finding is represented before acceptingclean: true(and reject duplicate IDs), rather than relying solely on model instructions.
if [ "$CLEAN" = "true" ] && [ $((BLOCKING_N + INVALID_N + UNKNOWN_N)) -gt 0 ]; then
echo "::warning::agentic-check block inconsistent (blocking=$BLOCKING_N invalid=$INVALID_N foreign=$UNKNOWN_N rows); failing closed"
CLEAN=false
Comment on lines
+201
to
+202
| NEEDS_HUMAN="$(gh pr view "$PR" --repo "$REPO" --json labels \ | ||
| --jq 'any(.labels[].name; . == "needs-human")')" |
Comment on lines
+522
to
+524
| NOW_LABEL="$(gh pr view "$PR" --repo "$REPO" --json labels \ | ||
| --jq 'any(.labels[].name; . == "needs-human")')" | ||
| [ "$NOW_HEAD" = "$HEAD" ] && [ "$NOW_CLEAN" = "success" ] && [ "$NOW_LABEL" != "true" ] && esc_ok && threads_ok |
Comment on lines
+421
to
+423
| if [ "${already:-0}" -gt 0 ]; then | ||
| echo "lfx-reviewer already approved $HEAD; no-op." >> "$GITHUB_STEP_SUMMARY" | ||
| else |
Comment on lines
+46
to
+48
| "on": | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize] |
Comment on lines
+41
to
+43
| "on": | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize] |
The agentic gate approves as lfx-reviewer; for that approval to satisfy code-owner branch protection the machine user must be a code owner, paired with a human maintainer (emsearcy), mirroring the lfx-v2-newsletter-service pattern. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
| -H "X-GitHub-Api-Version: 2026-03-10" --method POST \ | ||
| -f model=gpt-5.5 -F create_pull_request=false \ | ||
| -f base_ref="$BASE_REF" -f head_ref="$HEAD_REF" \ | ||
| -f prompt="Reconcile the AI-reviewer threads on pull request #$PR in $REPO against head $HEAD_SHA and any developer replies, following the repository instructions exactly. Immediately before posting, re-read the PR head: if it no longer equals $HEAD_SHA, post nothing — a newer round owns the verdict. Post your agentic-check verdict comment (head: $HEAD_SHA) using the add_issue_comment tool." \ |
Contributor
Author
There was a problem hiding this comment.
The window exists but is narrower than described, and what leaks through it is bounded by the SHA-binding design:
- A verdict is bound to the head SHA it judged, and on an A→B→A force-push the old task's verdict describes byte-identical code — SHA A is tree A. The stale verdict cannot vouch for different code; at worst it reports thread adjudications from before replies that arrived during the B interlude, and those replies are re-adjudicated at the next push round (the same guarantee the pipeline gives for every reply between rounds).
- The scenario also requires the PR author to force-push back to an already-reviewed SHA — the driver-side documentation treats reused SHAs as a known hazard with explicit anchoring rules, and force-pushing is not part of the flow's normal operation.
- A per-round nonce is a reasonable hardening direction, noted for a follow-up.
Comment on lines
+556
to
+558
| if [ -z "${GH_TOKEN:-}" ]; then | ||
| echo "lfx-reviewer PAT unavailable (OIDC role or secret not wired); nothing dismissed." >> "$GITHUB_STEP_SUMMARY" | ||
| exit 0 |
Contributor
Author
There was a problem hiding this comment.
Real observation, but the exposure is bounded to one sweep interval and requires a compound failure:
- The gate re-evaluates every open PR on a 10-minute scheduled sweep (plus every status/label/comment trigger). A transient OIDC/Secrets Manager failure on one withheld run leaves the stale approval standing only until the next sweep run succeeds and dismisses it — a bounded, self-healing window, not a persistent bypass.
- Exploiting the window requires the regression (needs-human added / clean failed / thread untidy), the transient PAT failure, and a human clicking merge inside those minutes. The companion branch-protection control ("dismiss stale reviews on push") independently covers the most common regression, a new push.
- Failing closed here has a real cost the design chose against: the PAT fetch is
continue-on-errorso that unwired repos (exactly this repo's state until deployment mechanics land) and transient AWS hiccups don't turn every sweep into a red X on the default branch's Actions history. - "Dismiss must fail loud once armed" is a fair hardening direction, noted for a follow-up.
Comment on lines
+46
to
+50
| - **License header**: every tracked source/config file (Go, YAML, shell, | ||
| Makefile, etc.) begins with the two-line LF/MIT header | ||
| (`// Copyright The Linux Foundation and contributors.` / | ||
| `// SPDX-License-Identifier: MIT`; `#` syntax for YAML/shell/Makefile). | ||
| Missing headers are the #1 cause of CI failure |
Contributor
Author
There was a problem hiding this comment.
Fixed in 413c401: the distillate now documents both wordings by file type (Go vs YAML/shell/Makefile) and tells the reviewer to match the surrounding convention.
The repo uses 'and contributors.' in Go files but 'and each contributor to LFX.' in YAML/shell/Makefile; prescribing the Go wording universally would make the reviewer flag correct headers as drift. Raised by Copilot review on PR #103. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/agentic-gate.yml:469
- [high] The last-moment approval check repeats the second-precision verdict ordering, so two comments created in the same second can make a newer malformed verdict lose to an older
no. That lets this recheck authorize the approval despite the latest verdict being invalid. Sort by the numeric comment ID here as well, then remove that field before the existing timestamp/value parsing.
VRAW="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \
--jq ".[] | select(.user.login==\"lfx-reviewer\" and (.body | contains(\"<!-- agentic:needs-human v1 -->\")) and (.body | contains(\"<!-- head: $HEAD -->\"))) | [.created_at, (if ((.body | contains(\"<!-- needs-human: yes -->\")) and (.body | contains(\"<!-- needs-human: no -->\"))) then \"malformed\" elif (.body | contains(\"<!-- needs-human: yes -->\")) then \"yes\" elif (.body | contains(\"<!-- needs-human: no -->\")) then \"no\" else \"malformed\" end)] | @tsv" \
2>/dev/null)" || return 1
V="$(printf '%s\n' "$VRAW" | grep -v '^$' | sort | tail -1 || true)"
Comment on lines
+267
to
+272
| if VRAW="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \ | ||
| --jq ".[] | select(.user.login==\"lfx-reviewer\" and (.body | contains(\"<!-- agentic:needs-human v1 -->\")) and (.body | contains(\"<!-- head: $HEAD -->\"))) | [.created_at, (if ((.body | contains(\"<!-- needs-human: yes -->\")) and (.body | contains(\"<!-- needs-human: no -->\"))) then \"malformed\" elif (.body | contains(\"<!-- needs-human: yes -->\")) then \"yes\" elif (.body | contains(\"<!-- needs-human: no -->\")) then \"no\" else \"malformed\" end)] | @tsv" \ | ||
| 2>/dev/null)"; then | ||
| V_OK=true | ||
| V="$(printf '%s\n' "$VRAW" | grep -v '^$' | sort | tail -1 || true)" | ||
| fi |
Contributor
Author
There was a problem hiding this comment.
Fixed in df61205: both the Evaluate site and the pre-approval recheck now order same-head verdicts by the numeric comment id (monotonic, unique) instead of second-precision created_at, mirroring how this file already orders statuses and timeline events by numeric id for exactly this tie problem.
Two hardening fixes from Copilot review on PR #103, both real defects in the shared harness (also being routed upstream to the reference implementation in lfx-v2-newsletter-service): - The conductor's wait-poll matched reviewer logins with an unanchored regex, so any account whose login merely contains 'copilot-pull-request-reviewer' could satisfy the wait and let the round fire before the real bot posted. Anchor it to the exact login with an optional [bot] suffix, matching the exact-match identity checks used elsewhere in the same workflow. - The gate ordered same-head escalation verdicts by created_at, which has only second precision: a same-second pair would tie and fall through to sorting by verdict value, letting an older 'no' beat a newer 'malformed'. Order by the numeric comment id (monotonic, unique) at both the Evaluate site and the pre-approval recheck, mirroring how the same file already orders statuses and timeline events by numeric id for exactly this reason. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/agentic-gate.yml:498
- [critical] The last-moment recheck repeats the unauthorized-unlabel bypass: a
noverdict newer than a non-allowlisted removal returns success. This defeats the manual hold immediately before approval even though the comments state that the most recent unlabel must be authorized regardless of the judge's answer. Require the latest removal to be absent or allowlisted here as well.
[ -z "$LAST_TS" ] || [ -n "$AUTH_UNLABEL" ] || [ "${V%% *}" \> "$LAST_TS" ]
…nlabel
The gate's escalation condition let a per-head 'no' verdict prevail over
needs-human unlabel events older than itself, on the reasoning that the
later judge round was fresher evidence. But the judge adjudicates the
DIFF, never label actors: a manual hold can be raised for reasons
outside the diff entirely, so a non-allowlisted triager removing it just
before a routine push would have the ensuing 'no' clear the hold without
any authorized human — defeating the manual-override invariant the same
step documents ('the most recent unlabel overall must be authorized').
Drop the verdict-recency exception at both the Evaluate site and the
pre-approval recheck: a 'no' now stands only when no unlabel ever
happened or the newest one is allowlisted. Recovery from an unauthorized
removal is an allowlisted human re-adding and removing the label.
Raised by Copilot review on PR #103; routed upstream to the reference
harness in lfx-v2-newsletter-service alongside the other findings.
Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Comment on lines
+64
to
+67
| The `<!-- head: ... -->` line binds your verdict to the exact head you judged: read | ||
| the PR's current head SHA immediately before posting and write all 40 characters. | ||
| The gate only honors a verdict whose `head:` equals the head it is about to | ||
| approve, so a stale verdict from an earlier push can never vouch for newer commits. |
Contributor
Author
There was a problem hiding this comment.
Fixed in 192f15d. Agreed this was the sharpest finding of the review — an author-level path (no insider access) to a diff nobody reviewed. Rather than reshaping the comment contract (base-binding every verdict would break the verdict formats and the deterministic parsers), the fix closes it at the round-triggering layer: escalation and conductor now also fire on edited events narrowed to actual base changes (github.event.changes.base), so a retarget starts a fresh review round for the head. The new round's pending stamp immediately outranks the stale clean status under the gate's newest-id-wins ordering, and the fresh escalation verdict re-binds to the (unchanged) head — restoring exactly the "every reviewed artifact reflects the current diff" property.
| -H "X-GitHub-Api-Version: 2026-03-10" --method POST \ | ||
| -f model=gpt-5.5 -F create_pull_request=false \ | ||
| -f base_ref="$BASE_REF" -f head_ref="$HEAD_REF" \ | ||
| -f prompt="Run the needs-human escalation for pull request #$PR in $REPO, following the repository instructions exactly. Post your machine-readable needs-human verdict comment using the add_issue_comment tool." \ |
Contributor
Author
There was a problem hiding this comment.
Fixed in 192f15d: the escalation prompt now pins the event head (against head $HEAD_SHA), instructs the task to re-read the PR head immediately before posting and post nothing if it moved, and to stamp head: $HEAD_SHA — the same guard the conductor's reconcile prompt already carried. Agreed this was an inconsistency between the two task prompts with a real stale-verdict consequence.
Comment on lines
+178
to
+181
| if ! gh api "repos/$REPO/pulls/$PR/commits" --paginate --jq '.[].sha' | grep -qxF "$TARGET_SHA"; then | ||
| echo "::error::head: $TARGET_SHA is not a commit of PR #$PR; refusing to act on this verdict" | ||
| exit 1 | ||
| fi |
Contributor
Author
There was a problem hiding this comment.
Fixed in 192f15d: the membership check now captures the complete paginated commit listing into a variable before matching, as the escalation workflow does for labels. Agreed — under pipefail an early-page match could SIGPIPE gh and falsely reject a legitimate stale-head verdict.
… avoid SIGPIPE Three fixes from Copilot review round 5 on PR #103, all real defects (routed upstream to the reference harness alongside the earlier ones): - Base retargeting changes the reviewed diff materially while keeping the same head SHA, so no synchronize event fires and the head's stale verdicts and clean status keep vouching for a diff nobody reviewed — an author-level bypass. Trigger escalation and conductor on 'edited' events narrowed to actual base changes: a retarget now starts a fresh round, whose pending stamp immediately outranks the stale clean status under the gate's newest-id-wins ordering. - The escalation task prompt carried no expected head, so a stale task surviving a cancellation could stamp a judgment made from an older workspace onto a newer head. Pin the prompt to the event head and require the task to post nothing when the head has moved — the same guard the conductor's reconcile prompt already carries. - The apply step's PR-membership check piped a paginated listing into an early-exiting grep -q under pipefail: a match on an early page kills gh with SIGPIPE mid-pagination and falsely rejects a commit that belongs to the PR. Capture the complete listing before matching. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
.github/workflows/agentic-gate.yml:473
- [critical] The last-moment recheck has the same base-retarget gap as evaluation: because it selects verdicts only by unchanged
HEAD, an old-basenoremains valid during or after the new-base round. Apply the same base/round binding here so the pre-approval check cannot re-authorize a stale escalation verdict.
VRAW="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \
--jq ".[] | select(.user.login==\"lfx-reviewer\" and (.body | contains(\"<!-- agentic:needs-human v1 -->\")) and (.body | contains(\"<!-- head: $HEAD -->\"))) | [(.id|tostring), .created_at, (if ((.body | contains(\"<!-- needs-human: yes -->\")) and (.body | contains(\"<!-- needs-human: no -->\"))) then \"malformed\" elif (.body | contains(\"<!-- needs-human: yes -->\")) then \"yes\" elif (.body | contains(\"<!-- needs-human: no -->\")) then \"no\" else \"malformed\" end)] | @tsv" \
.github/workflows/agentic-gate.yml:440
- [should-fix] This comment still documents the removed verdict-recency exception: the code below accepts a
noonly when the latest unlabel is absent or authorized, regardless of whether an unauthorized unlabel is older than the verdict. Update this load-bearing security comment so future edits do not restore the invalid rule.
# Same rule as Evaluate gate: an explicit per-head "no" (which
# stands only if the latest needs-human unlabel is authorized,
# absent, or older than the verdict — the label is also a
# manual override surface and the allowlist invariant applies
# regardless of the judge's answer); or a
| # complete lookup that returns nothing takes that branch. | ||
| V_OK=false; V="" | ||
| if VRAW="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \ | ||
| --jq ".[] | select(.user.login==\"lfx-reviewer\" and (.body | contains(\"<!-- agentic:needs-human v1 -->\")) and (.body | contains(\"<!-- head: $HEAD -->\"))) | [(.id|tostring), .created_at, (if ((.body | contains(\"<!-- needs-human: yes -->\")) and (.body | contains(\"<!-- needs-human: no -->\"))) then \"malformed\" elif (.body | contains(\"<!-- needs-human: yes -->\")) then \"yes\" elif (.body | contains(\"<!-- needs-human: no -->\")) then \"no\" else \"malformed\" end)] | @tsv" \ |
Contributor
Author
There was a problem hiding this comment.
Fixed in 527211f. Correct — the trigger fix alone left the window you describe: the gate's verdict lookup was head-keyed and a retarget keeps the head. Both the Evaluate site and the pre-approval recheck now discard same-head verdicts older than the newest base_ref_changed timeline event (the retarget-triggered escalation re-judges the current diff and re-posts), and a failed timeline read withholds outright — a hidden retarget can never validate a stale verdict. This binds verdict validity to the base's history without reshaping the comment contract.
Comment on lines
+158
to
+160
| done_n=$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate \ | ||
| --jq ".[] | select((.user.login|test(\"^$rv(\\\\[bot\\\\])?$\";\"i\")) and .commit_id==\"$HEAD_SHA\") | .id" \ | ||
| 2>/dev/null | grep -c . || true) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 527211f. Correct — reviews are commit-bound and a retarget keeps the commit, so the old-base review satisfied the poll. The wait-poll now counts only reviews submitted after the newest base_ref_changed timeline event, so the re-requested review for the new diff must actually post before the round proceeds (and its findings become fresh threads the reconcile adjudicates). A failed timeline read fails the round closed; no retarget history degrades to the previous behavior exactly.
…gate Completes the base-retarget closure from the previous commit (round 5): that fix started a fresh round on retarget, but the round's inputs were still head-keyed, and a retarget keeps the head SHA — so old-base artifacts could satisfy the new round's checks in the window before (or forever if) the fresh round's outputs land. Raised by Copilot review round 6 on PR #103. - The conductor's wait-poll now counts only reviews submitted AFTER the newest base_ref_changed timeline event: the old base's same-commit review can no longer complete the poll before the re-requested review for the new diff posts. A failed timeline read fails the round (closed), and an empty retarget history degrades to the previous behavior. - The gate (both the Evaluate site and the pre-approval recheck) discards same-head escalation verdicts older than the newest base_ref_changed event: the retarget-triggered escalation re-judges the new diff and re-posts. Without a complete timeline read no verdict is trusted at all — a hidden retarget would otherwise validate a stale one. - The recheck's esc_ok comment still documented the verdict-recency exception removed two commits ago; align the load-bearing comment with the code so the invalid rule is not restored by a future edit. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
.github/workflows/agentic-escalation.yml:194
- [critical] A task started before a base retarget survives workflow cancellation, and this head-only guard still passes because retargeting preserves
HEAD_SHA. If that old task postsnoafter the retarget, the gate sees a comment timestamp newer thanbase_ref_changedand accepts a judgment made against the previous diff. Pin the expected base too and require both the current head and base to match immediately before posting (or bind verdicts to an immutable round).
-f prompt="Run the needs-human escalation for pull request #$PR in $REPO against head $HEAD_SHA, following the repository instructions exactly. Immediately before posting, re-read the PR head: if it no longer equals $HEAD_SHA, post nothing — a newer push owns the next verdict. Post your machine-readable needs-human verdict comment (head: $HEAD_SHA) using the add_issue_comment tool." \
.github/workflows/agentic-conductor.yml:329
- [critical] The reconcile task also survives cancellation but validates only the head. After a base retarget, an old task can therefore post
clean: truefor the previous diff after the new round's pending status; apply will stamp that later success because agentic-check artifacts have no retarget binding. Require the task to verify the current base as well as the head before posting, or round-bind the ledger/status.
-f prompt="Reconcile the AI-reviewer threads on pull request #$PR in $REPO against head $HEAD_SHA and any developer replies, following the repository instructions exactly. Immediately before posting, re-read the PR head: if it no longer equals $HEAD_SHA, post nothing — a newer round owns the verdict. Post your agentic-check verdict comment (head: $HEAD_SHA) using the add_issue_comment tool." \
Comment on lines
+230
to
+233
| NOW_HEAD="$(gh api "repos/$REPO/pulls/$PR" --jq '.head.sha')" | ||
| if [ "$NOW_HEAD" != "$HEAD_SHA" ]; then | ||
| echo "::notice::head moved ($HEAD_SHA -> $NOW_HEAD) after the poll; aborting this round" | ||
| exit 0 |
Contributor
Author
There was a problem hiding this comment.
Fixed in e336bf5 (plus the base guard in the pre-post check): the deterministic baseline now embeds the round id from its own pending stamp, its pre-post guard re-reads head AND base, and apply validates the round: against the newest pending stamp — so a stale baseline racing cancellation posts a superseded round id and is rejected; it can never become the newest success.
Implements round-bound artifacts — the structural closure of the retarget/stale-task family Copilot surfaced across review rounds 5-7 on PR #103 — while keeping every existing guard in place (the r5-r7 point-patches remain as belt-and-braces): - The conductor's round-start pending stamp now doubles as the ROUND ID: its numeric status id is repo-global, monotonic, and minted by the deterministic layer, so agents cannot forge one. The baseline block and the reconcile task's prompt carry it, and the agentic-check contract gains a 'round:' line. - agentic-apply accepts an agentic-check verdict only when its round: equals the NEWEST pending stamp for that head+PR. A verdict from a superseded round — a task surviving cancellation across a push, base retarget, or re-run — is rejected mechanically whenever it posts. Missing/malformed round: or a failed stamp lookup rejects (closed). - needs-human verdicts now carry a 'base:' marker naming the base branch the judge actually saw (the escalation prompt names it), and the gate's verdict lookups (Evaluate + pre-approval recheck) honor only verdicts matching the PR's CURRENT base — killing the task-post TOCTOU: what matters is what the task judged, not when it posted. Verdicts predating the marker fail closed; the retarget/push-triggered escalation re-posts with it. - Conductor pre-fire and baseline pre-post guards now check the base as well as the head; both task prompts pin head AND base. - /agentic-comment-format, /pr-conductor, and /needs-human-escalation document the new markers as load-bearing. Behavioral delta on the happy path: none — round ids and base markers are minted, threaded, and validated automatically; every new check can only reject artifacts that today would be wrongly accepted. All failure modes are fail-closed (head stays pending; next push retries). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/agentic-gate.yml:573
- [high]
recheckruns underif ! recheck, so Bash suppresseserrexitinside the function. If this label lookup fails,NOW_LABELis empty and the subsequent!= "true"test passes, allowing an approval without confirming thatneeds-humanis absent. Return failure explicitly when the lookup fails so both the pre-POST and compensating checks fail closed.
NOW_LABEL="$(gh pr view "$PR" --repo "$REPO" --json labels \
--jq 'any(.labels[].name; . == "needs-human")')"
Comment on lines
+44
to
+48
| # reviewed diff materially while keeping the same head SHA, so no | ||
| # synchronize fires; without a fresh round the head's stale verdicts and | ||
| # clean status would keep vouching for a diff nobody reviewed. The job | ||
| # condition below narrows edited events to actual base changes. | ||
| types: [opened, reopened, synchronize, edited] |
Contributor
Author
There was a problem hiding this comment.
Already addressed: the PR description no longer makes the byte-for-byte claim — it describes the workflows, brains, and router on their own terms, and the review-hardening commits (this trigger included) are part of this PR's own history rather than attributed to another source. No description/code mismatch remains.
…ookup recheck() runs under 'if ! recheck', which suppresses errexit inside the function: a failed label lookup left NOW_LABEL empty, and the subsequent != "true" test read that as label-absent — the one fail-open lookup in the function. Return failure explicitly on lookup error and require a literal true/false, so the compensating check withholds like every other input read. Raised by Copilot review on PR #103. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.github/workflows/agentic-gate.yml:200
- [critical] The selected clean status is bound to the PR and SHA, but not to the current base-specific round. After a base retarget, the old base's
successremains authoritative until the conductor creates its new pending stamp; if that run fails before line 135, escalation can still post a current-baseno(or a skip marker), and this gate can approve the materially different diff using the old clean result. Require a pending round created after the newestbase_ref_changedevent and accept only a success newer than that round, both here and in the pre-approval recheck.
CLEAN="$(gh api "repos/$REPO/commits/$HEAD/statuses" --paginate \
--jq ".[] | select(.context==\"agentic-review/clean\" and ((.description // \"\") | contains(\"pr=#$PR:\"))) | [(.id|tostring), .state] | @tsv" \
2>/dev/null | sort -n | tail -1 | cut -f2 || true)"
.github/workflows/agentic-gate.yml:208
- [high] This current-base lookup has the same stale-approval failure mode: an API error exits the evaluation before
approve=falseis written, preventing the PAT-backed dismissal step from running. Track lookup success explicitly and force the final verdict to withheld on failure rather than aborting the step.
CUR_BASE="$(gh pr view "$PR" --repo "$REPO" --json baseRefName --jq .baseRefName)"
.github/workflows/agentic-gate.yml:202
- [high] This label lookup is unhandled under
set -e. A transient read failure abortsEvaluate gatebefore it emitsapprove=false, so the stale-approval dismissal step is skipped and an existing automated approval remains active while the gate state is unknown. Convert lookup failure into an explicit withheld result so execution reaches the dismissal backstop, as the thread-listing failure path already does.
NEEDS_HUMAN="$(gh pr view "$PR" --repo "$REPO" --json labels \
--jq 'any(.labels[].name; . == "needs-human")')"
josep-reyero
marked this pull request as ready for review
July 24, 2026 09:32
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
AI reviewers re-review the whole diff every round, so they can loop: re-raising an issue that already has a thread, re-raising one already adjudicated, or flagging code the PR never touched. Add a de-duplication section to the pr-conductor skill: - one issue, one blocking thread: the original thread keeps the authoritative status; duplicates get obsolete rows naming the original, never a second blocking row for the same defect; - a re-raise of an adjudicated issue is re-verified against the current code first — it stays settled unless it carries new substance or the code regressed; - findings on code the diff does not touch are pre-existing issues: reported in the summary as follow-ups, never blockers. Judgment stays code-grounded (identity/scope named in each reason, 'when in doubt, not a duplicate'), and the mechanics are untouched: duplicates still get rows and replies, only their status changes. Uses the existing obsolete status, so the comment contract and the apply parser are unchanged; the carry-forward invariant keeps the original thread blocking until genuinely cleared. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
.github/workflows/agentic-gate.yml:200
- [critical]
CLEANis still bound only to head+PR, not to the current base/round. After a base retarget, if the conductor run fails or is delayed before writing its new pending stamp (the workflow explicitly fail-softs on PAT setup), the old base'ssuccessremains newest; a new base-bound escalationnocan then satisfy the other predicates and approve a diff that never received a current-base review. Require evidence of a pending round created after the newestbase_ref_changedevent and accept only a success produced from that round, or carry and validate the base/round on the clean status itself.
CLEAN="$(gh api "repos/$REPO/commits/$HEAD/statuses" --paginate \
--jq ".[] | select(.context==\"agentic-review/clean\" and ((.description // \"\") | contains(\"pr=#$PR:\"))) | [(.id|tostring), .state] | @tsv" \
2>/dev/null | sort -n | tail -1 | cut -f2 || true)"
.github/workflows/agentic-gate.yml:571
- [critical] The last-moment recheck repeats the head+PR-only clean lookup. A base can change without changing
HEAD; if the new conductor has not stamped its round but a new base-bound escalation verdict has arrived, this recheck can accept the previous base's success and post the approval. Revalidate that the selected success belongs to a round started after the latest base retarget here as well.
NOW_CLEAN="$(gh api "repos/$REPO/commits/$HEAD/statuses" --paginate \
--jq ".[] | select(.context==\"agentic-review/clean\" and ((.description // \"\") | contains(\"pr=#$PR:\"))) | [(.id|tostring), .state] | @tsv" \
2>/dev/null | sort -n | tail -1 | cut -f2 || true)"
.github/workflows/agentic-conductor.yml:180
- [critical] Filtering by
submitted_at > RETARGET_TSdoes not prove this review analyzed the new base. A Copilot review already in flight for the old base can finish after the retarget (and the re-request above may fail because that review is still pending), yielding the same head SHA and a post-retarget timestamp; this poll then advances before any review of the new diff. The round needs a review that is provably initiated after the retarget—for example, wait out any pre-retarget pending request, issue a fresh request, and correlate its completion—rather than inferring provenance from submission time alone.
done_n=$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate \
--jq ".[] | select((.user.login|test(\"^$rv(\\\\[bot\\\\])?$\";\"i\")) and .commit_id==\"$HEAD_SHA\" and ((.submitted_at // \"\") > \"$RETARGET_TS\")) | .id" \
2>/dev/null | grep -c . || true)
.github/workflows/agentic-conductor.yml:215
- [should-fix] This “fresh baseline” test misses reviews of the same head against an earlier base. If a retarget cancels the old run after its review but before its check comment, both the old-base and new-base reviews have
commit_id == HEAD_SHA, so this selects the deterministic path and marks stale old-base non-nit threads outstanding. Route this case to the reconcile agent by treating pre-retarget reviews/threads as prior state; it can then mark findings that no longer apply as obsolete.
EARLIER="$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate \
--jq ".[] | select((.user.login | test(\"^(copilot(-pull-request-reviewer)?(\\\\[bot\\\\])?|github-actions(\\\\[bot\\\\])?)$\"; \"i\")) and .commit_id != \"$HEAD_SHA\") | .id")"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the LFX agentic review-and-merge pipeline to lfx-mcp (rollout epic LFXV2-2276): four deterministic workflows, repo-specific review brains, and the Copilot task router.
Workflows
.github/workflows/agentic-escalation.yml— fires the needs-human judge per head (skips while the sticky label is set, recording a per-head marker status).github/workflows/agentic-conductor.yml— waits for the reviewers each round, derives the baseline check deterministically, fires the reconcile task from round two.github/workflows/agentic-apply.yml— the sole GitHub-write actor from agent output: parses lfx-reviewer verdict comments, sets the stickyneeds-humanlabel and theagentic-review/cleanstatus.github/workflows/agentic-gate.yml— the only merge-verdict actor, no model runs in it; approves (never merges) iff clean status on the current head, label absent, escalation satisfied per head, and every thread answered; fail-closed, dismisses stale approvalsBrains (
.github/skills/, written against lfx-mcp's threat model)copilot-code-reviewer— reviewer identity, method, severities; anchored to the two properties that drive review judgment here (registration-time tool gating is the permission model; two-class upstream authorization)mcp-code-review— quality lens + the house-standards distillate (license headers, package doc comments, the tool-registration pattern and itsdefaultToolspitfall, repo conventions);AGENTS.mdstays canonicalmcp-security-review— diff-aware, low-false-positive method over the server's durable threat anchors (inbound credential verification, registration gating, the two-class model, token plumbing, write tools, output-schema exposure, secrets)needs-human-escalation+escalation-guidelines— the escalation judge and its boundary guidelines, carrying the fork-replay retune: escalate on the boundary a diff moves (write-tool exposure, structured-output internal-field exposure, pinned-LFX-module bumps, access-checks), not on visible-surface reshapingpr-conductor— adjudicates AI-reviewer threads with the carry-forward ledger; thread UI state is never authorityagentic-comment-format— the machine-readable verdict-comment contract the apply step parses.github/copilot-instructions.md— rewritten as the router that dispatches each Copilot task to its skill; its previous repo-guide content lives on insidemcp-code-reviewValidation
The brains were calibrated on a 20-PR fork-replay evaluation against this repo's real history: our reviewer 32 findings / 0 false positives, 15 important issues caught that had shipped to merged main; escalation judge 11/11 sensitive recall with 0–1/8 routine over-escalation after the retune this PR carries.
What this PR does NOT do (deployment mechanics, human/devops)
The pipeline is inert until wired — every privileged job fail-soft-skips (green) without the PAT, and nothing posts, labels, or approves:
/cloudops/managed-secrets/cloud/github/linuxfoundation/lfx-reviewer-pat), reachable via the repo's OIDC roleNEEDS_HUMAN_UNLABELERSallowlist inagentic-gate.yml— ported as-is (josep-reyero niravpatel27, extended withemsearcyin-PR); confirm the final list@emsearcy @lfx-reviewer); branch protection wiring (require code-owner review, dismiss stale approvals on push) still human/devopsThe author-side PR-driver skill (
.claude/skills/) is deliberately not included in this PR.