fix(#3864): preserve prior review remediations - #1232
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoPreserve verified review remediations during re-review
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
d3c0a56 to
885e712
Compare
Treat changes that directly address app-verified prior findings as expected remediation during re-review, while keeping unrelated changes subject to scope checks. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
885e712 to
3a4b0a2
Compare
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on 3a4b0a2 — no approval or change request implied, and I have not touched assignment or labels.
Nine findings; seven are anchored inline. The three below have no line inside the diff, so they are here with explicit paths.
HIGH — Dispatch-examples table still omits intent-coherence, contradicting the new always-dispatch rule
skills/pr-review/SKILL.md:381-382
The PR rewrites lines 336-341 to "intent-coherence — always re-qualifies when changed_since_prior is non-empty". Both re-review rows of the Dispatch examples table are unchanged and still list agent sets without intent-coherence:
- line 381 —
Re-review after fix (prior findings in correctness only) | correctness (full scope), style-conventions (trivial scope), challenger - line 382 —
Re-review after fix (prior findings in security only) | correctness (full scope), security (normal scope), style-conventions (trivial scope), challenger
Verified both against head 3a4b0a2. Those rows were added by cc6af96 alongside the old narrowed rule and were not updated here. Every re-review-after-fix has a non-empty delta, so both rows now describe dispatch sets the rule 45 lines above forbids. An orchestrator reading the concrete examples (the more actionable of the two) will skip intent-coherence — the exact sub-agent that owns the new remediation exemption this PR exists to add — so the fix silently does nothing on the canonical scenario.
Suggestion: Add intent-coherence to both re-review rows (with whatever scope constraint the rule ends up carrying), and grep the rest of the table for any other row describing a re-review path.
MEDIUM — Step 3d context-package field list not updated for the two new fields
skills/pr-review/SKILL.md:637-659
The step 4 prompt template gained two new sections at lines 791-795 (### Prior-finding remediation candidates and ### Prior review provenance), but the step 3d "Prepare context packages" field list was not touched: it still enumerates only diff, pr_head, head_sha, repo_full_name, changed_files, prior_findings, prior_review_sha, changed_since_prior, pr_metadata, issue_context, cross_repo_context, scope_constraint.
3d is the step that says what to assemble; step 4 only renders it. An orchestrator implementing 3d literally never computes the candidates or the provenance value, so the template renders "none" on every re-review and the exemption never fires.
Suggestion: Add remediation_candidates (intent-coherence only, per 3a-2) and prior_review_provenance to the 3d field list so assembly and rendering agree.
MEDIUM — agents/review.md provenance enum omits bot-verified and unverifiable-wrong-user
agents/review.md:40-47
SKILL.md now declares (lines 204-206) that trusted provenance is "app-verified (GitHub) or bot-verified (GitLab)", and the new test asserts both literals. I verified bot-verified is real: fullsend's GitLab scaffold internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:451 sets PRIOR_REVIEW_PROVENANCE="bot-verified" after an author.id check (the comment at :396 notes it is a weaker check), emits unverifiable-wrong-user on mismatch, and scaffold_gitlab_test.go:237 asserts it.
But agents/review.md:40-47 — the runner-facing documentation of that same env var — still lists only none, app-verified, unverifiable-no-app, unverifiable-wrong-app. The skill and the agent contract doc now disagree on the value vocabulary, so a reader of the contract treats bot-verified as an unknown (therefore untrusted) value. This file is not in the diff, hence the body placement.
Suggestion: Add bot-verified (GitLab, author.id-based, weaker than the GitHub app check) and unverifiable-wrong-user to the enum in agents/review.md so the contract matches the scaffold and the skill.
Materialize and route the prior-review delta, preserve bounded intent review, and add a re-review evaluation fixture. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
|
Addressed the three body-only findings in 04144a0: the re-review dispatch examples include bounded intent-coherence; step 3d now assembles remediation candidates, provenance, and the incremental-diff path; and agents/review.md documents GitLab bot-verified and unverifiable-wrong-user provenance values. The protected-path item remains subject to human merge review. |
Summary
Fixes fullsend-ai/fullsend#3864.
Verification
bash scripts/pr-review-remediation-test.shbash eval/lint-cases.sh reviewmake check-bundlemake script-testmake lintA prepared single-case live functional eval was not started because this machine has no Vertex AI credentials; no temporary GitHub repo or PR was created.