feat(code-review): approve a reviewed head - #82
Merged
Conversation
…val floor A manifest cannot be validated against a vocabulary it cannot see, and the approval floor is a rung a repo names. The ladder therefore lives in internal/review, where the manifest is parsed; internal/reviewrun keeps its Severity as an alias, so a finding's severity and a manifest's floor are rungs of one ladder rather than two vocabularies that happen to agree. The floor defaults to AMBER. RED and AMBER are both defects and differ in the strength of the claim rather than in what they oblige, so "at or above the floor" reads as "a defect rather than a remark"; a repo that wants only RED to oblige a fix says so. A floor off the ladder is refused when the manifest is read, because an unrecognised severity ranks below every real one and would quietly oblige nothing — silently, in the repo that took the trouble to set it.
Nothing is persisted between runs — ids are per-run, and the pull request is the only record — so a posted review carries a marker naming the commit it was made against, whether the run reached a verdict, every surviving finding by fingerprint and severity, and which of those `agtk` could give no thread to. Written and parsed in one file, for the reason the fingerprint marker already gives: two definitions of one format drift, and here the failure is worse than silent, because a marker that does not parse is a review nothing downstream can see. The verdict is all three of the judge answering, every reviewer answering, and the thread list being readable. A run missing any of them found less than it would have, and "found nothing" is the one thing that must never be read into that. Findings are grouped by what a reader can do about one rather than by why it is not inline. A finding with no line, or one whose line the diff does not add, now hangs off the whole file: its own request after the review, because subject_type is not a field a review's draft comments carry. One that fails costs its own thread and nothing else, and is reported. A finding naming a path the change does not touch, or no path at all, still reaches the body alone — GitHub refuses a comment there. AddedLines keys every path the diff touches, so a file the change only deletes from is a path a file-level comment can hang off. It answers two questions now: whether an inline comment may name a line, and whether the diff names a path.
`agtk code-review approve --pr N` posts one review with event APPROVE, as the App, bound to the pull request's current head. A person types it. Four conditions hold or it refuses: the head carries a review by this installation, that review reached a verdict, every finding it reports at or above the floor is marked a false positive on its thread, and every comment thread is resolved. There is no --force. A flag that approves anyway makes each of those a checklist rather than a control, and the person who would type it is the one the gate exists to slow down. Both ways past a finding are acts on the pull request: change the code, so the next review does not report it, or reply on its thread saying it is not a defect. Each is attributable, visible and reversible. A prompt-injection finding agtk could attach to nothing refuses regardless — the material addresses the reviewer, there is nothing to reply to, and the deadlock is the point. A false positive is a reply opening a line with `agtk: false positive` and carrying a reason, from an account that can push. Resolution is not enough: it says the conversation is finished, which is a different claim from "this is not a defect". Write access rather than anyone who can comment, because a finding its own author could dismiss is one an injected instruction can dismiss too. internal/reviewapprove owns the event and the call that sends it. Three guards replace the walk that banned the literal everywhere: it is named in no other package; no package a model's output passes through can reach it in the import graph, which holds even if somebody spells the event differently; and it joins the credential surface. A fourth is about the grant rather than the act — the App holds contents: write only so its approvals count, so no file under internal/ may name an endpoint that writes contents, refs, trees, blobs or a merge. See ADR 0009. The injection clause now also says that none of a reviewer's bars narrows it: a prompt-injection finding is filed whether or not the change touches the line, whether or not a concrete failure follows, and whether or not the reviewer is sure what the author meant. The quote is the whole of the evidence.
|
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.
agtk code-review approve --pr Nposts one GitHub review with eventAPPROVE, as the App, bound to the pull request's current head. A person types it; nothing in a review run reaches it. It exists because a solo author cannot satisfy a required approval alone.Four conditions, and nothing overrides any of them:
A
security:prompt-injectionfindingagtkcould attach to nothing refuses regardless.There is no
--forceA flag that approves anyway makes every row above a checklist rather than a control, and the person who would type it is the one the gate exists to slow down. Both ways past a finding are acts on the pull request — change the code, or reply on its thread saying it is not a defect — and each is attributable to an account, visible to anybody reading the change, and reversible. This reverses an earlier consequence of ADR 0006.
How approval learns what the last review found
Nothing is persisted, so a posted review carries a marker in its own body: the commit, the verdict, every surviving finding by fingerprint and severity, and which of those
agtkcould give no thread to. Written and parsed in one file, for the reasonmarker.goalready gives.The last part is what keeps the gate honest in both directions. A finding with no thread blocks nothing — a gate with no remedy is a deadlock rather than a control — and a prompt-injection finding with no thread blocks everything.
Every blocking finding is answerable
A finding that carries no line, or whose line the diff does not add, now hangs off the whole file: its own request after the review, because
subject_typeis not a field a review's draft comments carry.AddedLinesgrew a second question — whether the diff names a path at all — so a file the change only deletes from is somewhere a comment can hang.False positives
A reply opening a line with
agtk: false positiveand carrying a reason, from an account that can push. Resolution is not enough: it says the conversation is finished, which is a different claim from "this is not a defect". Write access rather than anyone who can comment, because a finding its own author could dismiss is one an injected instruction can dismiss too.What guards it
internal/reviewapproveowns the event and the call. Three assertions replace the walk that banned the literal everywhere: the literal is named in no other package; no package a model's output passes through can reachinternal/reviewapprovein the import graph, which holds even if somebody spells the event differently; and it joins the credential surface. A fourth is about the grant rather than the act — no file underinternal/may name an endpoint that writes contents, refs, trees, blobs or a merge, because the App holdscontents: writeonly so that its approvals count (ADR 0009).Also here
approval.floorin the review manifest, read at the base ref, defaulting to AMBER. The severity ladder moves tointernal/reviewso a manifest can be validated against it;reviewrun.Severityis now an alias.ADRs 0006 and 0007 are amended; ADR 0009 is new; CONTEXT.md gains False positive and Review marker.