feat(bedrock-pr-review): FinalWalk review lifecycle, adjudication, on-demand triggers - #23
Closed
aprilb wants to merge 2 commits into
Closed
feat(bedrock-pr-review): FinalWalk review lifecycle, adjudication, on-demand triggers#23aprilb wants to merge 2 commits into
aprilb wants to merge 2 commits into
Conversation
…-demand triggers Brings the Bedrock reviewer in line with the FinalWalk standard (digital-analytics-len/FinalWalk, ADR-004). Positional comment identity is what makes an AI reviewer noisy: any edit earlier in a file shifts every finding below it, so on most pushes the same finding fails to match its old comment and gets deleted and re-posted, taking any acknowledgement or resolution with it. Review lifecycle: - Comment identity is now content-based: sha256 of the path plus the literal source text of the anchored line, read from the checked-out merge tree, plus an occurrence index for files that repeat a line. Line numbers are excluded, and the model's own prose is never part of identity, so a reworded finding keeps its comment. - Resolving a thread dismisses that finding permanently, including a nearby reworded restatement (same path, within 10 lines, claim-token Jaccard >= 0.35). Thread state comes from one GraphQL reviewThreads query. - A finding the model does not re-emit is deleted as fixed only when the code it anchors on changed since the last reviewed commit. On unchanged code, non-reemission is sampling noise. Without incremental data at all the comment is kept, never deleted on a guess. - A restatement of a claim already open on a nearby thread (within 40 lines) reconciles with that thread instead of duplicating it. - A thread a human replied to is never deleted, on a move or a rescan. - The summary is one persistent comment updated in place, stamped with the commit it reviewed. That stamp replaces parsing per-run review bodies for idempotency. Precision and cost: - An adjudication pass sends every candidate back with the surrounding source and drops what that source refutes, plus duplicates, unmechanised speculation, and anything contradicting the repo's review guide. Defaults to Sonnet 4.6 regardless of the finding model: precision is model-insensitive once verification is on, but the judgment's recall is not. Fail-open, so an adjudicator error keeps the full candidate set. - min_confidence drops low-confidence findings; an absent confidence field counts as medium so an omitted field never silently drops a finding. - max_files skips oversized PRs rather than reviewing them badly. - review_guide_path feeds .github/review-guide.md to both passes. - Dependabot PRs no-op: they run without secrets, so the role assumption cannot succeed and a red check is the only possible outcome. On-demand triggers: @bedrock-review re-reviews the current HEAD (overriding the already-reviewed and draft skips), @bedrock-review dismiss deletes every finding this reviewer posted. Comment bodies are passed through env, never interpolated into the shell. Also drops the cross-repo checkout of this repo for the shared prompt and embeds it as PROMPT_B64, matching codex-pr-review.yml. A consumer's job token can resolve a reusable workflow in an internal repo but cannot fetch its contents, so the checkout was a latent break for every consumer and an active one for the product-org-len mirror. check-prompt-embed.yml now guards both copies. Tests: tests/reconcile.test.mjs extracts the workflow's PURE LOGIC region from the YAML at test time, so the workflow file is the only copy. 38 tests covering identity stability, dismissal and variant matching, the confidence and severity floors, sticky threads, marker ownership, and thread preservation. scripts/validate-workflows.py loads every workflow as YAML and parses each embedded bash, Python and JavaScript block, since a reusable workflow only fails when a consumer dispatches it.
Self-review of the previous commit surfaced six defects. All are fixed here, each with a test that fails when the fix is reverted. 1. Nothing before the model call may red the advisory check. The OIDC role assumption, Python setup, SDK install, caller write and the post step had no continue-on-error, so a repo missing from the reviewer role's trust policy red-Xed every PR in it, contradicting the fail-soft guarantee this workflow states in its own header. All are fail-soft now, a Check setup step names what failed, and the run summary reports it instead of letting it surface as a confusing model error. 2. On-demand and dismiss now require write access. Both are privileged: dismiss deletes findings, and on-demand spends against the caller's Bedrock role while deliberately bypassing the debounce and already-reviewed gates. Anyone can comment on a PR, so a drive-by commenter could previously delete every finding or drive unbounded spend. Gated on the comment's author_association being OWNER, MEMBER or COLLABORATOR, with a ::notice:: naming a denied association so it is visible rather than silent. 3. The adjudicator can re-grade what it rescues. The confidence floor runs after adjudication, and a verdict could not change confidence, so a hedged finding the judge confirmed and rewrote into an assertion was then discarded by the default medium floor: precisely the candidate the adjudication pass exists to rescue. A verdict may now carry a confidence, and a rewritten candidate is floored at medium on the grounds that the rewrite IS the assertion. 4. max_files uses the exact changed-file count. It was derived from `gh pr view --json files`, which resolves through GraphQL `files(first: 100)` and saturates at 100, so any cap above that never fired: a 600-file PR reported 100 and sailed through a cap of 150. Read `.changed_files` from the REST pulls resource instead, and fail the guard open on a malformed value. 5. Comment events dedupe by PR. They were isolated by run_id, so two `@bedrock-review` comments ran at once, neither saw the other's posts, both opened a thread per finding, and the loser was unreachable forever after because the reconcile pass indexes one comment per fingerprint. Now keyed on the PR plus an event kind, so command runs dedupe with each other while still never cancelling a push review. 6. The model's line number is coerced once at ingest. A string "42" failed the commentable-line Set lookup and demoted the finding to a summary bullet even when its line was in the diff, and would have failed the strict equality that decides whether a finding moved, delete-and-reposting an unmoved comment on every run. A finding with an unusable line is now dropped with a warning rather than silently mishandled. Tests: 89, up from 38. Two new suites, both extracting the code under test from the workflow YAML so there is still only one copy: tests/trigger.test.mjs covers the author-association gate and that a hostile comment body cannot reach the shell, and tests/adjudication.test.mjs covers the verdict applier's keep, drop, rewrite, re-grade and fail-open paths. reconcile.test.mjs gains workflow-shape guards for the concurrency group, the fail-soft chain and the exact file count. Mutation-checked: reverting any one of the six fixes fails at least one test.
Member
Author
|
Second review round.
Findings 1 and 3 are the ones I would not have wanted to ship. 1 is the same Tests are 89, up from 38. Two new suites, both extracting the code under test
Mutation-checked, reverting each fix in turn:
|
Member
Author
|
Closing unmerged. The Spec Tool repos are moving to GitHub Copilot code review, Nothing here is lost. The two commits are recoverable at |
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.
Refs modsy/digitalops-platform#176
Brings the Bedrock reviewer in line with the FinalWalk standard
(
digital-analytics-len/FinalWalk, ADR-004).FinalWalk itself is not callable from
modsy: it is aninternalrepo in thelennar-emuenterprise andmodsyis a Team-plan org outside it, so a modsy jobtoken cannot resolve it and the
finalwalk-aiApp cannot install on modsy repos.This ports the behaviors instead.
Why
Positional comment identity is what makes an AI reviewer noisy. Identity was
path:line, so any edit earlier in a file shifted every finding below it: thesame finding failed to match its old comment, got deleted, and got re-posted.
Because the new comment is a different object, a reviewer's acknowledgement or
resolution did not survive the next push. There was also no notion of a resolved
thread, so a dismissed finding came straight back.
Review lifecycle
sha256(path + literal source text of the anchored line + occurrence index), read from the checked-out merge tree. Line numbersare excluded and the model's own prose is never part of identity, so a reworded
finding keeps its comment and an unrelated edit above it does not move it. The
occurrence index disambiguates files that repeat a line (common in JSON/YAML/SQL).
reviewThreadsquery. The exact finding is suppressed on every later run, and sois a nearby reworded restatement (same path, within 10 lines, claim-token Jaccard
when the code it anchors on changed since the last reviewed commit. On unchanged
code, non-reemission is sampling noise: the code cannot have been fixed by not
changing. With no incremental data at all (first review, force-push, unreachable
prior SHA) the comment is kept, never deleted on a guess.
thread (within 40 lines) reconciles with that thread instead of duplicating it.
reviewed. That stamp replaces parsing per-run review bodies for idempotency.
Precision and cost
surrounding source, which drops what that source refutes (a "missing check" whose
excerpt contains the check), plus duplicates, speculation with no named mechanism,
and anything contradicting the repo's review guide. Defaults to Sonnet 4.6
regardless of the finding model: precision is model-insensitive once verification
is on, but the judgment's recall is not, and a weak judge silently drops real
hedged findings. Fail-open, so an adjudicator error keeps the full candidate set
rather than losing findings. Disable with
adjudicate: false.min_confidence(defaultmedium) drops low-confidence findings. An absentconfidencefield counts asmedium, so an omitted field never silently drops one.max_files(default 50) skips oversized PRs rather than reviewing them badly.review_guide_pathfeeds.github/review-guide.mdto both passes.assumption cannot succeed and a red check is the only possible outcome.
On-demand triggers
@bedrock-review@bedrock-review dismissComment bodies are passed through
env, never interpolated into the shell.These need a caller change to fire. The triggers live in the caller, not the
reusable workflow.
examples/pr-review-bedrock.ymlis updated; the 7 Spec Toolcallers need the two extra events added, tracked as a sub-task on
modsy/digitalops-platform#176.
Prompt loading (bug fix in passing)
Dropped the cross-repo
actions/checkoutof this repo for the shared prompt andembedded it as
PROMPT_B64, matchingcodex-pr-review.yml. A consumer's job tokencan resolve a reusable workflow in an internal repo but cannot
git fetchitscontents, so this was a latent break here (this repo is public today) and an active
one for
product-org-len/ci-workflows, which is internal and still carries thecheckout. It is the same failure https://github.com/product-org-len/ci-workflows/pull/8
fixed for the Codex reviewer.
check-prompt-embed.ymlnow guards both embeddedcopies, so the two cannot silently diverge from the prompt file.
Removing that step also removes the only localized line between the two
ci-workflowsrepos (repository: modsy/ci-workflows), so the file can now bebyte-identical in both.
Self-review findings, fixed in this PR
/code-reviewon my own diff surfaced three defects, all fixed here with tests:reconcile dropped the new candidate to avoid a duplicate but did not mark the
thread it matched as still asserted, so the same run's orphan sweep could delete
that thread. A real finding vanished on a run where nothing was fixed.
bedrock-review(?::...)doesnot match
<!-- bedrock-reviewer -->. Dismiss removed the summary only byaccident, via the separate
reviewed-shastamp that sits beside it, so an emptyhead_shaor a marker rename would have silently broken it.went to "Minor notes" and never reached the reconcile set, orphaning its thread.
Fixed at the class level rather than per case: any finding still asserted this run
keeps its thread, whichever presentation bucket it lands in (inline, minor,
non-diff, over the cap, or dropped by a floor). Only a claim the model did not
restate at all is a deletion candidate, and even then only if its code changed.
A fourth, found while testing the changed-ranges pipeline:
+++ /dev/null(adeleted file) did not match the
b/prefix, sopathkept the previous file'svalue and the deleted file's hunk was attributed to it. Verified against a real
diff, pre-fix vs post-fix:
Testing
tests/reconcile.test.mjsextracts the workflow'sPURE LOGICregion from theYAML at test time and runs it, so the workflow file is the only copy and there is
nothing to drift. Coverage: identity stability across line shifts and rewording,
anchor fallbacks, occurrence indexing, claim tokenization and stopwords, Jaccard,
variant matching inside and outside both windows, the confidence and severity
floors including the fail-open on a missing grade, sticky-thread windows, marker
ownership (including that it does not claim the Codex reviewer's comments,
which post under the same
github-actions[bot]login), and thread preservation.Mutation-checked, so the assertions are not vacuous:
scripts/validate-workflows.pyloads every workflow here as YAML and parses eachembedded bash, Python and JavaScript block. A reusable workflow only fails when a
consumer dispatches it, so a syntax error in a block scalar is otherwise invisible
until it breaks somebody else's PR. It earned its place immediately: a raw NUL byte
in the embedded JS parsed fine as JavaScript and made the whole file invalid YAML.
Both checks run in CI via the new
tests.yml.Not covered by local testing
The end-to-end review has not run against a live PR: it needs the OIDC role, and
a reusable workflow only executes when a consumer dispatches it. Everything above
is static validation plus unit tests on the reconcile logic. The reviewer is
advisory and fail-soft throughout (
continue-on-erroron both model calls, andevery API mutation individually guarded), so a runtime failure degrades to no
review rather than a red check, but the first live run is the real proof and is
worth watching.
Known transition gap
Inline comments posted by the previous version of this workflow carry no
fingerprint marker, so this version will neither update nor delete them: they stay
as orphaned threads on PRs that are already open. Claiming unmarked bot comments is
not an option, because the Codex reviewer posts under the same
github-actions[bot]login and we would delete its threads. Self-limiting to PRsopen at merge time.
UI / visual proof
N/A, no user interface. The user-visible surface is PR comments, which cannot be
captured before the first live run for the reason above.