Skip to content

Adopt the interim review regime for material runtime decisions - #24

Merged
kikashy merged 4 commits into
mainfrom
adr-cross-vendor-review
Jul 28, 2026
Merged

Adopt the interim review regime for material runtime decisions#24
kikashy merged 4 commits into
mainfrom
adr-cross-vendor-review

Conversation

@kikashy

@kikashy kikashy commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Material-decision impact: documented-claim; review: see the ## Cross-vendor adversarial review comment on this PR

What this adopts

The runtime's adopting text for the interim review regime (spec RFC 0009 / GOVERNANCE.md, adopted in Judgment-Pack/judgment-pack-spec#16): material decisions — public surface, documented claim, conformance-relevant behavior, security posture, or dependency boundary, with or without an ADR — require a recorded cross-vendor adversarial model review with written maintainer dispositions, pinned to a reviewed SHA. Every PR in this repository (this one first) carries a one-line Material-decision impact: declaration; a PR template supplies it by default, and CONTRIBUTING.md points at the rule.

Until this merges, the runtime is under no obligation from RFC 0009 — this text is what creates it, commit-relatively, from the introducing PR onward.

The record

This PR's own review (per the rule it introduces) follows as a comment: OpenAI gpt-5.6-sol returned three majors and three minors, no blockers; all dispositioned, five accepted, one accepted in part (verbatim adoption of reviewer text keeps GOVERNANCE's explicit non-invalidation carve-out instead of requiring a third vendor — the reasons are in the disposition table).

🤖 Generated with Claude Code

kikashy and others added 4 commits July 27, 2026 19:10
…ec GOVERNANCE interim regime)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…C 0009 dispositions

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ance definition, Material-decision impact declaration on every PR

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wed-SHA pinning, commit-ancestry activation, PR template

Six findings from the cross-vendor review of the adopting text, all
accepted (finding 1 in part: verbatim adoption keeps the GOVERNANCE
carve-out rather than requiring a third vendor, now stated as an
explicit non-invalidation). CONTRIBUTING points at the rule; a PR
template carries the declaration by default.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kikashy

kikashy commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Cross-vendor adversarial review

  • Drafting models: Anthropic claude-fable-5 (session: direction, integration, dispositions) and Anthropic claude-opus-5 (drafting subagents), 2026-07-27.
  • Reviewing model: OpenAI gpt-5.6-sol (vendor: OpenAI) via codex-cli 0.145.0, 2026-07-27.
  • Reviewed SHA: 546089f. Context given: full working-tree checkout of this branch, shell access, instructed to diff against main; it could not read the spec repository and was told to review the text on its own terms; no MCP servers.
  • Runs: one on this PR. No run was discarded. The output below is the reviewing model's complete returned review text; the full session log including tool exploration is preserved locally.

Prompt (verbatim)

Adversarially review the documentation change on this branch of judgment-pack-runtime
(`git diff main...HEAD` — it touches only docs/adr/README.md). It adds a "Review of material
decisions" section: the runtime's adopting text for an interim review regime defined in the
specification repository (GOVERNANCE.md / RFC 0009 there — you cannot read that repo from here;
review this text as it stands, on its own terms). It requires cross-vendor adversarial model review
of material decisions, defines vendor/lineage and drafting assistance, uses a commit-relative scope
("the pull request merging this section and every pull request merged after it"), and requires a
one-line "Material-decision impact:" declaration on every pull request in this repository.

Attack:
1. Internal coherence: does the materiality definition, the commit-relative scope, the declaration
   requirement, and the review-attaches-to-the-PR rule form a consistent, followable procedure for
   a contributor reading only this file?
2. Honesty: the section states its own weak point (self-classified materiality). Does any sentence
   claim more auditability or protection than the mechanism provides?
3. Practicality: is the declaration line's grammar unambiguous (exactly one value; link required
   when non-none)? What does a PR that is pure refactoring declare? A dependency bump? A test-only
   change?
4. Conflicts with the rest of this README (ADR immutability, the ADR/RFC split, the index) or with
   the repo's existing contribution docs if any exist.

Severity (blocker/major/minor), exact text, concrete fix. If sound, say so plainly — do not invent
findings.

Output (complete, unedited)

The change is not fully sound yet. I found three major procedural gaps and three minor documentation issues; no blockers.

Findings

  1. Major — adopting reviewer-written text can invalidate the required vendor separation

    docs/adr/README.md:43 defines drafting assistance to include “generated or revised text that survives in the merged artifact,” but line 48 merely says that “adopting reviewer-generated text verbatim is noted in the record.”

    Under the definition, verbatim adoption makes the reviewer a drafting assistant. That reviewer is therefore no longer from a different vendor than every drafting model. Merely noting this does not cure the conflict, and the contributor is not told whether another review is required.

    Concrete fix:

    Adopting reviewer-generated text verbatim makes that model a drafting assistant. Its review then no longer satisfies the cross-vendor requirement, and a model from another vendor must review the resulting decision. Applying a finding in the maintainer’s own words does not have that effect.

  2. Major — the declaration grammar is not closed or unambiguous

    docs/adr/README.md:60 shows only a category value, while lines 64–65 require every non-none value to “carry a link.” It does not specify whether the category must be hyperlinked, whether the URL follows the category, or whether the link may appear elsewhere.

    “Exactly one value” also cannot accurately represent overlapping impacts. A security dependency upgrade may be both security and dependency; an API conformance change may be both public-surface and conformance. Multiple material decisions or multiple review records are likewise undefined.

    Define the complete line grammar, for example:

    Material-decision impact: none
    Material-decision impact: security, dependency; review: https://github.com/...#issuecomment-...
    

    Then state:

    none must appear alone. Otherwise list one or more applicable categories, followed by review: and one or more permanent links to a review record covering every listed material decision.

    If exactly one category is intentional, add a multiple value or define a precedence rule.

  3. Major — the review is attached to an evolving PR, not to an identifiable reviewed revision

    docs/adr/README.md:39 requires a review “on the pull request,” but does not require the record to identify the commit or artifact reviewed. Material changes can therefore be pushed after the review without the record showing whether the merged decision was covered.

    This also makes the claim at line 71 that the classification is “dated, attached to the diff” stronger than the procedure warrants.

    Concrete fix:

    The review record identifies the reviewed commit SHA. Any later change that materially alters the reviewed decision requires an updated review and dispositions before merge.

    Without that requirement, replace “attached to the diff” with the weaker and accurate “recorded on the pull request.”

  4. Minor — the activation boundary for the universal declaration is implicit

    Lines 52–58 explicitly scope review to the adopting PR and later material PRs, then say “Every pull request in this repository” must declare an impact. Literally, that includes historical PRs; alternatively, “This applies” could be read as excluding later non-material PRs, defeating the none declaration.

    “This section was merged 2026-07-27” is also fragile until the PR actually merges.

    Concrete replacement:

    The review obligation applies to the pull request that introduces this section and every later pull request that makes a material decision. Beginning with the introducing pull request, every pull request—material or not—must carry the declaration below. “Later” is determined by commit ancestry from the commit introducing this section, not by a calendar date.

  5. Minor — “What it removes is silence” overstates an unenforced declaration rule

    Lines 68–72 candidly acknowledge self-classification, but “What it removes is silence” claims an outcome the mechanism does not guarantee. There is no PR template or automated check, and an author who omits a review can also omit or misclassify the declaration.

    Concrete fix:

    It makes silence a policy violation: when followed, the declaration leaves an explicit classification that can be contested on the pull request.

  6. Minor — the repo-wide rule is absent from contributor guidance, and the ADR summary is now misleading

    CONTRIBUTING.md:6 is the natural place contributors look before opening a PR, but it does not mention the mandatory declaration or review. This particularly affects Dependabot PRs generated from .github/dependabot.yml, whose descriptions will need manual amendment or automation.

    Also, docs/adr/README.md:9 still characterizes runtime ADRs as “Lightweight; no comment-before-commit process.” That is at least misleading once material decisions require adversarial PR review and written dispositions.

    Add the declaration and a link to this section under “Before opening a pull request,” preferably with a PR template. Change the ADR summary to something like:

    Lightweight; no public RFC comment period, although material decisions require the PR-level review described below.

Practical classifications under the current definition

  • A genuinely behavior-preserving refactor: none.
  • A test-only change that only improves coverage or restructures tests: none. A change to the conformance oracle, fixtures, or what CI treats as conforming could be conformance, but the text should say so.
  • Adding, removing, or replacing a dependency boundary: dependency.
  • A routine version bump of an existing dependency: indeterminate. It arguably remains none if no listed property changes; a vulnerability fix is security; a boundary-changing upgrade is dependency. The overlap exposes the single-category problem above.

The materiality definition itself, the anti-evasion rule, vendor/API-host distinction, broad drafting-assistance definition, and disclosure of self-classification are otherwise candid and coherent. I found no direct conflict with ADR immutability or the index: the review living on the PR is compatible with immutable accepted ADRs, and the index still indexes ADR files rather than process prose.


Dispositions

# Finding Disposition Reason Implemented by
1 Verbatim adoption of reviewer text invalidates vendor separation Accept in part The internal incoherence was real — the definition made the reviewer a drafter while the note pretended neutrality. But the proposed cure (a third-vendor review) is rejected: the spec GOVERNANCE text deliberately carves verbatim adoption under an accepted finding out of retroactive invalidation, and a third-vendor spiral is impractical with two major vendors in play. The carve-out is now explicit here and answers the contributor's question (no further review on that account). docs/adr/README.md, the drafting-assistance sentence (5db68a4)
2 Declaration grammar not closed (single-value cannot express overlapping impacts; link placement undefined) Accept Real ambiguity; a security dependency bump is two categories. Closed grammar: none alone, else comma-separated categories + ; review: <link>; worked classifications for test-only, refactor, and dependency-bump cases; bot-PR handling (5db68a4)
3 Review attaches to an evolving PR, not an identifiable revision Accept The spec regime pins a reviewed SHA; the runtime text omitted it. Reviewed-SHA requirement + fresh-review-on-material-change with the disposition-implementing exception, mirrored from GOVERNANCE; "attached to the diff" replaced by the now-accurate recorded-with-SHA wording (5db68a4)
4 Activation boundary implicit; merge-date sentence fragile Accept Commit ancestry is the regime's own test; the calendar date was a leftover. Rewritten scope paragraph: obligation from the introducing PR by commit ancestry; declaration on every PR from the introducing PR onward (5db68a4)
5 "What it removes is silence" overstates an unenforced rule Accept The mechanism makes silence a violation; it does not make it impossible. Reworded to "make silence a policy violation: when the rule is followed…"; a PR template now supplies the declaration by default (5db68a4)
6 Rule absent from CONTRIBUTING; ADR summary ("no comment-before-commit") now misleading Accept Both true. CONTRIBUTING.md gains the declaration + link; the ADR/RFC split bullet now says material decisions require the PR-level review; .github/pull_request_template.md added (5db68a4)

The changes in 5db68a4 implement dispositioned findings of this review and are covered by the exception the text itself states; no fresh review is required on their account.

@kikashy
kikashy merged commit 2784f64 into main Jul 28, 2026
4 checks passed
@kikashy
kikashy deleted the adr-cross-vendor-review branch July 28, 2026 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant