feat(ci): give the AI reviewer the team's own review history - #6177
Open
jam-jee wants to merge 2 commits into
Open
feat(ci): give the AI reviewer the team's own review history#6177jam-jee wants to merge 2 commits into
jam-jee wants to merge 2 commits into
Conversation
The AI reviewer reads the diff and the base checkout, so it re-derives
context every run and cannot see what the team has already said. Feedback
that has been given before gets given again, and decisions that were
settled in an earlier PR get relitigated in this one.
Add a retrieval step between credential setup and the review action. It
derives its own queries from the diff -- changed file stems, added symbols,
plus one for general conventions -- queries a Bedrock Knowledge Base built
from this repository's merged PRs, closed issues, and review discussions,
and writes the results to /tmp/historical_context.md. The review action
reads that file with the Read tool it already uses for the diff.
Measured on a real 28KB diff across sagemaker-train and sagemaker-core:
12 derived queries, 31 chunks, 38KB of context carrying concrete prior
guidance ("move local imports to module level", "raise on AccessDenied in
validation paths instead of warning"), each with a source URL to check.
The step cannot break a review. It is skipped entirely unless the repo
variable PYSDK_CONTEXT_KB_ID is set, so this merges inert; it is
continue-on-error with a 5 minute timeout; and every failure path inside
the script exits 0 having written nothing. The prompt states the file's
absence is normal.
The script is vendored rather than installed from a package registry so
that the code shaping the reviewer's context is reviewable in the same
pull request that runs it, cannot change under a fork PR without a repo
change, and needs no install step -- it uses only the standard library
and boto3, which the runner already has.
Retrieval is read-only (bedrock:Retrieve, bedrock:GetKnowledgeBase) and
adds no tool permissions: allowedTools is unchanged and Bash stays
excluded. The step runs in the trusted base checkout, never fork code.
Retrieved entries are model-extracted from historical discussion and can
be wrong, so both the file header and the prompt tell the reviewer to
treat them as claims to verify, cite the source URL, and prefer the
current source tree on any disagreement.
The knowledge base is a private corpus. It is currently built only from this repository's own pull requests and issues, but it can also hold documents from non-public sources, and this script's output is posted as comments on a public pull request. Retrieval had no source restriction, so adding one non-public document to the corpus would have been enough to surface it here. Restrict retrieval to an allowlist of sources whose contents are already public in this repository. Enforced in the Retrieve filter server-side, so non-public text never crosses into the process at all, with a second client-side check in case that filter ever regresses. A chunk carrying no source label is refused rather than assumed public, and the refusal warning counts rather than names what it dropped, since the label itself can be the sensitive part. An allowlist, not a denylist: a source added to the corpus in future is excluded here until it is added deliberately. No loss of context. On the same 28KB diff used to validate the original step, output is byte-identical at 38,140 bytes from 31 chunks -- the corpus is 1,134 documents, all from this repository.
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
1 participant
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.
Why
The AI reviewer reads the diff and the base checkout, so it re-derives context on every run and cannot see what the team has already said. Feedback that has been given before gets given again, and decisions settled in an earlier PR get relitigated in this one.
What
Adds one step between credential setup and the review action. It derives its own retrieval queries from the diff — changed file stems, added symbols, plus one for general conventions — queries a Bedrock Knowledge Base built from this repository's merged PRs, closed issues, and review discussions, and writes the result to
/tmp/historical_context.md. The review action reads that file with theReadtool it already uses for the diff.No per-PR prompt authoring is needed: the queries come from the diff.
Measured on a real diff
A 28,175-byte diff across
sagemaker-trainandsagemaker-core:Concrete prior guidance it surfaced, each with a source URL to check:
sagemaker.train.__init__.py" — #5832This merges inert
The step is skipped entirely unless the repo variable
PYSDK_CONTEXT_KB_IDis set. Until then the workflow behaves exactly as it does today, so the mechanism can be reviewed and merged without committing to activation.It cannot break a review
Four independent layers:
if: vars.PYSDK_CONTEXT_KB_ID != ''— unset variable, step never runscontinue-on-error: true— a failing step does not fail the jobtimeout-minutes: 5— a hung Bedrock call cannot stall the reviewVerified for: unset KB id, invalid KB id, missing diff file, unwritable output. The prompt states the file's absence is normal, not an error.
Security
pull_request.base.sha) — the trusted context this workflow already established as its pwn-request defence. Never executes fork code.allowedToolsis unchanged andBashstays excluded.bedrock:Retrieve,bedrock:GetKnowledgeBase.On trusting the retrieved text
Entries are model-extracted from historical discussion and can be confidently wrong. Both the file header and the prompt instruct the reviewer to treat each entry as a claim to verify, cite the source URL when relying on it, and prefer the current source tree wherever the two disagree.