fix: add a waive disposition and a closure contract to finding-triage - #171
Open
ultimatile wants to merge 16 commits into
Open
fix: add a waive disposition and a closure contract to finding-triage#171ultimatile wants to merge 16 commits into
ultimatile wants to merge 16 commits into
Conversation
A finding can be valid, its fix understood, and still be closed with no fix and no follow-up, because making the fix would cost more than the defect. None of the six dispositions covered that: `defer` requires a record, and `false-positive` requires the finding to be wrong. `waive` is that slot — the fix is not made, no follow-up issue is filed, and only the user may close it, carrying the reasoning that states what the fix would cost. A stated precedence keeps the three local-fix branches single-valued: the fix is made (`actionable`), or it is not and a follow-up issue is filed (`defer`), or it is not and none is (`waive`). The follow-up issue is the discriminator because it is observable — a note would land where a waiver's reasoning lands and leave the two indistinguishable. A new Closure section states what discharges each disposition, who may, and which four are terminal against which three are transient. The gates that each stated their own waiver rule — done-check, gh-body-check, file-issue, file-pullreq — now reference it, so the authority is the user everywhere instead of unstated at three sites. done-check and gh-body-check widen their concern triage from a two-way actionable / false-positive split to the full catalogue. copilot-review's reply rule moves out of a bash comment into prose, and `acknowledged` — which absorbed five distinct dispositions — is replaced by an enumeration keyed to the catalogue. review-telemetry gains `waive` in its slug list and a schema bump to 4, so the boundary the added value marks is one a query can name. Closes #155 Closes #166
…tifact The precedence classified an unfixed finding as `defer` "if a follow-up issue is filed", while Closure made filing that issue the act which discharges an already-`defer` finding. Between the decision and the filing the issue does not exist, so the precedence selected `waive` for a finding nobody had waived. Selection and discharge are now separate. The precedence turns on what the user directs — carry the finding past this run, or not — which the run witnesses rather than predicts. Filing the issue and recording the reasoning stay where they were, as what discharges each disposition. done-check's unjudgeable row is `uncertain-validity` rather than a row with no disposition: the re-dispatch is that disposition's investigation, and a second clause-less return means it cannot come back, which is when the user decides whether the audit proceeds with the row open. copilot-review's deferred reply links the follow-up issue, matching the carrier the catalogue names.
Closure forbade proceeding with a transient disposition, while done-check let the user proceed with an unjudgeable row by waiving the gate. The escape is now stated once, in Closure: where the investigation cannot return, only the user lets the gate proceed, and that decision is about the gate rather than a disposition of the finding. done-check points at it instead of claiming its row is the only case, which the external-system rule already contradicted. The dismissal bars still named the discharge artifacts a disposition produces rather than the decision that selects it; they now name the decision. `waive` referred to a bar its own text did not state. Its reasoning now has to account for a `critical` finding's happy-path error, which is the predicate the response-selection sentence was pointing at.
…cal fixes `defer` and `waive` already turned on what the run decides; `actionable` still turned on whether the edit had landed, which at triage time it never has. All three now turn on the decision, and the paragraph says so once instead of per branch: the edit, the filed issue and the recorded reasoning are what Closure discharges each with, not what selects it. `waive` had lost the locality its precedence assumes, so a finding with a understood but non-local fix that the user declined to carry matched both it and `opens-a-question`.
… gates `waive` stated a cost in its reasoning requirement without requiring that cost to exceed the defect, so any reasoned decline reached it. The bar is now a condition on the disposition; it is not a rival discriminator, since what selects the branch is the user's decision. gh-body-check claimed a math-scan concern closes only by fixing the body, which no other consumer may say of a shared catalogue. It takes a disposition like any other, and `defer` drops out on its own terms rather than on a consumer's carve-out: the body being drafted is the current task. Both gates stop restating what `false-positive` means and keep only their own handling of it.
…e-out Requiring the stated cost to exceed the defect left a valid local finding the user declines to carry, at a cost that does not clear the bar, accepted by no disposition at all. The cost is what the reasoning must disclose, not a threshold the disposition tests — the catalogue classifies decisions rather than preventing thin ones, and a thin waiver is visible in its own reasoning. gh-body-check ruled `defer` out on the ground that the body being drafted is the current task, which is not the scope test the catalogue applies: a concern in pre-existing body text can have a local fix outside an append-only task's scope. The carve-out is gone and the catalogue applies unrestricted.
The precedence selects `defer` from the user's direction to carry a finding past this run, while `defer` itself still required the finding to be out of scope. An in-scope finding the run will not fix and the user wants tracked satisfied neither, nor `waive`, which is the carry-nothing branch. Scope is now what usually motivates carrying it, and the definition mirrors `waive`'s shape: same preconditions, opposite decision. gh-body-check blocked on any unclosed ⚠ without the leave Closure grants when an investigation cannot return, so a finding resting on an unavailable authoritative check could never clear that gate.
…handling The no-fix branches turn on a decision the user has not made when a consumer first classifies, so a finding it will not fix held no disposition until the user answered. It starts at `actionable`, which is what puts the fix in front of the user, and the decline is the re-triage that lands it elsewhere — the mechanism the catalogue already permits and Response selection already names. The leave granted when an investigation cannot return let a gate proceed with the finding open, which no caller's "until every concern is closed" predicate admits. It now closes that gate's handling of the finding while leaving the finding transient, so the callers need no exception of their own.
…the test "Do not fix this", with nothing said about tracking, answered half the question the no-fix branches turn on, and matched neither. It is an incomplete answer rather than a third outcome, so the finding stays `actionable` until the user supplies the rest — which is also what asks for it. `opens-a-question` read as though needing investigation were its condition and non-locality only a description, leaving a fix that is exactly understood but reaches outside this run with nowhere to go. Non-locality is the test; the three reasons are how it usually arises.
… of the user done-check derived a closed row's verdict from its disposition alone, so a concern dismissed because the item does not apply, or fixed by an edit that removed the item's trigger, still rendered as a pass. The verdict comes from the item's own criterion against what the diff now holds; the disposition only says when the row reaches that point. `false-positive` requires its reasoning to be stated to the user, and gh-body-check placed it only in the report it hands back, which its callers present to no one. The gate now surfaces it.
There was a problem hiding this comment.
Pull request overview
Updates the repo’s review-triage single source of truth (finding-triage) to add a waive disposition and formalize “Closure” semantics, then aligns multiple consuming skills to reference the SSOT rather than restating closure/waiver/deferral rules.
Changes:
- Add
waive, precedence among local-fix dispositions, and a new Closure section tofinding-triage. - Update gate skills (e.g.,
done-check,gh-body-check,review-pipeline,file-issue/file-pullreq,research) to gate on “closed” per Closure rather than “no unresolved ⚠”. - Extend
review-telemetrydisposition normalization and bump plugin version.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/review-telemetry/SKILL.md | Bumps telemetry record schema to 4 and documents waive compatibility semantics. |
| skills/review-pipeline/SKILL.md | Aligns pipeline gating language to finding-triage Closure rules. |
| skills/research/SKILL.md | Updates gh-body-check laundering gate wording to rely on Closure semantics. |
| skills/quality-list/items/escape-hatch-necessity.md | Repoints deferral/closure wording to finding-triage Closure. |
| skills/land-via-integration-branch/SKILL.md | Replaces “deferred” phrasing with explicit finding-triage disposition tracking. |
| skills/implement/SKILL.md | Repoints done-check gating language to finding-triage Closure. |
| skills/gh-body-check/SKILL.md | Reframes triage from a 2-way split to full finding-triage disposition catalog + Closure gating. |
| skills/finding-triage/SKILL.md | Adds waive, precedence rules, and a new Closure section; updates related wording. |
| skills/file-pullreq/SKILL.md | Changes PR-body gating from “unresolved ⚠” to “⚠ not closed” per Closure. |
| skills/file-issue/SKILL.md | Changes issue-body gating from “unresolved ⚠” to “⚠ not closed” per Closure. |
| skills/done-check/SKILL.md | Updates concern triage and the step-5 gate to close concerns per Closure rules. |
| skills/copilot-review/SKILL.md | Moves reply guidance into prose and aligns reply outcomes with SSOT concepts (without printing slugs). |
| skills/codex-review/SKILL.md | Removes bespoke “waived findings stop counting” clause in favor of SSOT re-triage semantics. |
| skills/codex-plan-review/SKILL.md | Removes local restatement of defer meaning from plan review triage list. |
| skills/codex-contract-test-review/SKILL.md | Removes local restatement of defer meaning from contract-test review triage list. |
| skills/code-review-gate/SKILL.md | Clarifies that waiver here is of the gate itself, not a per-finding disposition. |
| README.md | Updates skill table descriptions for gh-body-check and finding-triage (adds waive). |
| .claude-plugin/marketplace.json | Bumps plugin metadata version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
"Closed on the same terms" carried the referent as a pronoun across a subsection boundary, so nothing linked the derived-body gate to the rule it gates on. finding-triage rests its own single-statement-plus-references shape on a broken reference costing one grep to find; a pronoun costs a reader.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
skills/done-check/SKILL.md:204
- Step 5 now allows closing concerns via
defer/waive, which can intentionally leave the defect in place. The instruction “State concretely what will change.” is misleading in those cases (nothing changes, but a closure record/justification is still required). Reword to cover both “what changes” and “what is recorded” closures.
5. Close every concern before proceeding, on `finding-triage`'s **Closure** terms — which disposition closes, and who may close it, is stated there. State concretely what will change. A row left holding a transient disposition cannot close, step 4's clause-less row among them: its re-dispatch was the investigation, and a second return without a clause means none can come back. Closure states what happens then — only the user lets this gate proceed with such a row open.
skills/gh-body-check/SKILL.md:84
- This step refers to an “rg hit”, but the procedure doesn’t run ripgrep; step 2 runs
${CLAUDE_SKILL_DIR}/body-math-scan.sh. Referring to a non-existentrgstep makes the gating instructions harder to follow.
Combine the rg hit (if any) and the cold-reader report into a single status. A math-scan hit step 2 judged ⊘ N/A is not a finding; one it judged ⚠ takes a disposition like any other. Decide a disposition in main context under the `finding-triage` SSOT, applying each per its definition there, for every ⚠ this check produced. A cold-reader concern most often lands on one of two:
…ame the scan that produced the hit Step 5 was reframed from "if any ⚠ remains, fix" to closing each concern by its disposition, and "state concretely what will change" survived the move. It now mis-frames three of the four terminal dispositions, which change nothing and record instead — and the step already states, per disposition, what each one moves. Covering both in that line would copy the paragraph beside it. The math scan reaches the executor as a script, so a hit belongs to it, not to the tool inside it — which the sentence added next to this one already called a math-scan hit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/gh-body-check/SKILL.md:84
- Step 2 (math scan) does not itself “judge” hits as ⊘ N/A vs ⚠; the text just above says that judgment is made by main-context inspection. Referring to “a math-scan hit step 2 judged …” is internally inconsistent and could mislead callers about where the classification happens.
Combine the math-scan hit (if any) and the cold-reader report into a single status. A math-scan hit step 2 judged ⊘ N/A is not a finding; one it judged ⚠ takes a disposition like any other. Decide a disposition in main context under the `finding-triage` SSOT, applying each per its definition there, for every ⚠ this check produced. A cold-reader concern most often lands on one of two:
Owner
Author
|
The third Copilot review raised one suppressed comment on Dismissed as wrong. Step 2 is where that judgment is specified — its own text reads "a hit inside a fenced code block, an inline code span, or prose that merely names the construct → ⊘ N/A, judged by main-context inspection", and then "so judge intent in main context". The step is where the instruction lives; main context is what executes it, and the two are not alternatives. Attributing a step's instructed action to the step is this repository's usual register: Recorded here because a suppressed comment carries no thread to reply on. |
The telemetry sentence described the query block it sits above as aggregates over dispositions, left ungated. Of the seven queries there, three read no disposition at all and four already carry a schema gate. What is true of all of them is narrower: none gates on the new boundary. The closure gate's "such a row" had the paragraph's opening subject and its worked example both available as antecedents, and they differ in what the gate may do — one lets the user pass any unresolved transient concern, the other only one whose investigation cannot come back. The catalogue grants the leave for the second alone. Two transient dispositions bar carrying an unresolved instance past the point a fix would be committed, and the leave lets a gate immediately preceding a commit proceed with exactly such a finding. Neither stated precedence over the other, which left the leave either overriding the bars silently or being nullified one step later.
This was referenced Jul 31, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/gh-body-check/SKILL.md:84
- Step 2 (Math scan) doesn’t itself “judge ⊘ N/A” vs “⚠” — it returns hits and main context classifies them per the Step 2 rules. The current sentence reads as if the script performs that judgment, which contradicts the Step 2 description.
Combine the math-scan hit (if any) and the cold-reader report into a single status. A math-scan hit step 2 judged ⊘ N/A is not a finding; one it judged ⚠ takes a disposition like any other. Decide a disposition in main context under the `finding-triage` SSOT, applying each per its definition there, for every ⚠ this check produced. A cold-reader concern most often lands on one of two:
Closure typed what discharges each disposition, and a finding whose investigation can never return has no discharge to name. The section answered that anyway, with a user leave and a precedence over the bars two dispositions place on surviving to a committed fix. Neither was asked for by the work this change implements, and settling it is a design choice rather than a local edit, so Closure now states no closure for that case and says the gate states one. The two gates that close on Closure's terms keep the waiver each had before, scoped to the case Closure cannot close and marked as closing no disposition, so it does not overlap the disposition of the same name. The bar added to opens-a-question goes with the leave it was colliding with. Five sites restated gh-body-check's exit condition and now delegate to it, which is what the two skills that gate on done-check already do. The remaining edits are consistency: locality is what separates defer from opens-a-question, so the two places still calling that separation a matter of unknown resolution now say so; severity belongs in a waiver's reasoning, which only waive requires; and an incomplete decline re-triages nothing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/gh-body-check/SKILL.md:84
- Step 2 (the math scan) doesn’t itself “judge ⊘ N/A” vs “⚠”; step 2 only reports hits and the file explicitly says N/A vs ⚠ is decided by main-context inspection. Wording here currently attributes the judgment to step 2, which is inconsistent with the Step 2 description.
Combine the math-scan hit (if any) and the cold-reader report into a single status. A math-scan hit step 2 judged ⊘ N/A is not a finding; one it judged ⚠ takes a disposition like any other. Decide a disposition in main context under the `finding-triage` SSOT, applying each per its definition there, for every ⚠ this check produced. A cold-reader concern most often lands on one of two:
…ter reads Closure is where each disposition's discharge is stated, so waive's own bullet stops restating that recording the reasoning is what closes it. The workaround item likewise stops restating the defer / opens-a-question discriminator it had already delegated in the same sentence. Replacing the telemetry slug list with a pointer left the slug itself unpinned: one catalogue entry's heading carries a routing arrow the field value must not, so the pointer now says which part to read. Attributing the math scan's classification to the step rather than to its rules read as though the script performed it. The verdict a closed row moves to now says which verdicts main context may assign, since the item bodies that would decide the rest are closed to it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.claude-plugin/marketplace.json:9
metadata.versionshould roll over to the current month per the repo’s CalVer convention; with the current date in August 2026, a2026.7.xversion looks stale. CLAUDE.md specifies resetting the patch counter when the month rolls over (CLAUDE.md:36–49).
"version": "2026.7.37"
skills/done-check/SKILL.md:196
- This sentence implies step 5 "closes" concerns even when they re-triage to a transient disposition, but step 5 later states a transient disposition cannot close. Reword to make it explicit that transient dispositions remain open until they resolve to a terminal one (or the gate’s own waiver applies).
Any other disposition applies on its own terms. A concern that re-triages to a transient one is not closed by that re-triage — step 5 closes each concern by whichever disposition it then holds.
CalVer resets the patch counter when the month rolls over. The bump was made in July and the squash merge now lands in August, so it becomes the first patch of the new month rather than a July version arriving late. "For a row Closure cannot close" read as any transient row, since the clause before it had just said transient rows cannot close. Naming the case it means — an investigation that cannot return — leaves one reading. The check's gate carried the same wording and the same ambiguity. The reply rule said writing the reply is what discharges a disposition, which would let a run close by itself one that only the user may close. The reply is where the reasoning lands; the decision stays with whoever the disposition names. The triage step said it closes each concern by whichever disposition it holds, promising closure for the three its own closing step says close nothing. It binds them instead. The check's description asserted that an unclosed concern blocks its caller, which the waiver path in its own gate falsifies. The skill table carries that sentence too.
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.
Summary
#155 records that
finding-triage's catalogue of dispositions had no entry for a finding that is valid, whose fix is understood, and which is closed with no fix and no follow-up. #166 records thatdone-checkandgh-body-checknamed two of those entries where the catalogue holds more.This adds a
waiveentry, a stated precedence amongactionable,deferandwaive, and a Closure section giving, per entry, what discharges it and who may. The skills that had grown their own wording for closing a finding unfixed now reference that section instead.Closes #155 Closes #166
Changes
finding-triage:waive— valid finding, understood local fix, not made in this run, no follow-up issue filed; only the user may, and the waiver's reasoning states what the fix would cost.actionable; the user's decision to carry it past this run moves it todefer, the decision not to moves it towaive. Filing the issue and recording the reasoning are what discharge those two, and are stated under Closure rather than in the precedence, so nothing is classified by an artifact that does not exist yet.actionable,false-positive,deferandwaivecan close a finding at a gate;uncertain-validity,invariant-premise-checkandopens-a-questioncannot until the investigation each names returns. Where that investigation cannot return at all, Closure states no closure either, and what a gate may do then stays the gate's own — see Notes.Consumers, each replacing wording of its own with a reference:
done-check,gh-body-check— their triage of a concern namedactionableandfalse-positive; it now names the catalogue. Their gates close a concern on Closure's terms, andgh-body-check's frontmatter description changes with its gate. Each keeps a waiver of its own for a concern Closure cannot close, marked there as closing no disposition.file-issue,file-pullreq,research,review-pipeline— delegate togh-body-check's gate instead of restating its exit condition, and the first two drop a per-caller waiver clause besides. Five sites carried a restatement of their own; one statement now serves them.codex-review— drops a clause stating that a waived finding stops counting as open; a complete decline re-triages the finding out ofactionable, which the loop's existing exit condition already reads, while an incomplete one leaves it there. Its definition of a clean review points at that exit condition rather than restating it.copilot-review— the rule for what a reply says moves out of a comment inside a bash block into prose, andacknowledgedis replaced by an enumeration of outcomes: fixed, dismissed as wrong, deferred with the issue linked, waived with the reasoning, or still under investigation.codex-plan-review,codex-contract-test-review— drop their restatements of whatdefermeans.quality-list'sescape-hatch-necessityitem,implement,land-via-integration-branch,review-pipeline's done-check gate — drop or repoint wording that stated an entry's terms locally.code-review-gate— gains a clause saying its waiver takes the gate as its object, not an individual finding, which now has a disposition of its own.review-telemetry— itsdispositionfield defers to the SSOT's slug set instead of carrying a copy, and the record schema goes to 4, marking where the widened set begins. The skill states no gating rule for that boundary — see Notes.README— thefinding-triagerow lists the entries and gainswaive, and thegh-body-checkrow takes the same closure wording as that skill's own description..claude-plugin/marketplace.json— version.Test plan
None of this compiles, and the tree's one test script covers
gh-body-check's math scanner, which this change does not touch; the change is text an agent executes.pre-commit run --all-filesruns the tree's one hook,mdformat, and reports itPassed.jqparses.claude-plugin/marketplace.jsonand prints the bumpedmetadata.version, in the working tree and again in a freshgit worktreecheckout of the branch tip, which reads committed content only.Notes
What this does not do:
actionablefindings, and eight sites acrosscodex-review,review-pipeline,codex-contract-test-reviewandcodex-plan-reviewturn on that count. A finding sitting at one of the three entries that cannot close passes them.uncertain-validityis separately barred from surviving to a committed fix, which narrows but does not close the gap. Reviewer loop exits pass a finding whose disposition is unsettled #169 covers this.review-pipelinereplies to a Copilot finding before the fix that reply reports lands, andcopilot-reviewallows one reply per thread, so a classification that changes afterwards goes unreported. copilot-review: a reply lands before its fix, with no second turn #170 covers this.review-telemetrydocuments gates on it, because gating its disposition aggregates on schema 4 would discard every earlier run rather than correct it.uncertain-validityandinvariant-premise-checkhave overlapping entry conditions and no stated precedence, so which an unsettled finding takes is underdetermined. That predates this change and is untouched by it. finding-triage: two overlapping dispositions, no stated precedence #172 covers this.done-checkandgh-body-checkeach keep a waiver of their own for that case, carried over from before this change and marked as closing no disposition. Such a waiver can pass a row holdinguncertain-validity, which that disposition bars from surviving to a committed fix. Widening a gate's triage to the whole catalogue, which done-check, gh-body-check: concern triage names two of six finding-triage dispositions #166 asks for, is what brings the two into contact; finding-triage: nothing closes a permanently unresolved finding #173 covers what the waiver is.