How much do Modelplane review bot findings vary between runs? (modelplaneai/modelplane#622)

README.md

How much do reviews vary between runs?

This reviews each of four Modelplane PRs five times with the review bot, without posting, and measures each run's share of the distinct findings that the five runs raised between them. It then judges whether a maintainer would want each finding posted, and traces where each run lost the findings it didn't post. See modelplaneai/modelplane#622 for the results.

Files

• review-eval.yml is the workflow that runs the reviews. It's the review job from review.yml at 7f50acb, changed to run every entry in runs.json in parallel and to post nothing. Each run reviews the whole PR, which can be in another repo. It fetches each PR's context once, before any run starts, and leaves the bot's own reviews, and replies to them, out of it. The runs reported in the issue used an earlier version that fetched context in each job, so a re-run job could see a newer PR description.
• runs.json lists the 20 runs, each a PR and commit with a run number.
• collect.sh downloads and extracts every run's review and transcripts, and prints each run's cost.
• group-prompt.md is the prompt an agent used to group one PR's comments into distinct findings. The agent that grouped #625 ran without its last paragraph, and followed that rule anyway.
• analyze.py computes how much runs overlap, from the grouped findings.
• extract.sh pulls each run's reviewer reports and main agent notes out of its transcripts.
• trace-prompt.md is the prompt an agent used to classify, for each finding a run didn't post, whether a reviewer raised it and the main agent dropped it, or no reviewer found it.
• judge-prompt.md is the prompt an agent used to judge whether each finding was true, and whether a maintainer would want it posted.
• judged.py combines the judged and traced findings. maintainer-ratings.json holds my ratings of the findings the judge called marginal, which replace the judge's.
• clusters-<PR>.json, trace-<PR>.json and judge-<PR>.json are the results.
• reviews-<PR>.md are all five reviews of each PR.

Running it again

1. In a fork of modelplane, set the REVIEW_MODEL_API_KEY secret. REVIEW_MODEL and REVIEW_MODEL_API work as they do for review.yml.
2. Branch from main. Add review-eval.yml to .github/workflows/ and runs.json to hack/review-eval/, and set the workflow's on.push.branches to the branch's name. Each push to the branch runs every entry in runs.json. The reviewer's agents and skills come from the branch, so to evaluate a change to them, make it on the branch.
3. When the workflow run finishes, re-run its failed jobs with gh run rerun <run> --failed. In my run, about a third of jobs landed on runners without KVM, and failed in their first minute.
4. In an empty directory, run collect.sh <fork> <run>, then extract.sh.
5. For each PR, give an agent group-prompt.md with its placeholders filled in: {DIR} is the directory from step 4, {CLONE} a clone of modelplane, and {PR}, {RUNS} and {SHA} the PR, its run numbers, and the commit reviewed. Then run analyze.py clusters-*.json.
6. For each PR, give a fresh agent trace-prompt.md, and another judge-prompt.md, with the same placeholders. {CHECKOUT} is a worktree of the clone at the commit reviewed. Then run judged.py with the PR numbers.

I used Claude Opus 5.5 for every agent, a fresh one for each PR and step. The workflow is on negz/modelplane@take-five.

analyze.py

#!/usr/bin/env python3
"""Summarise how much review findings vary between runs.

Reads the clusters-<PR>.json files that grouping wrote, and prints a markdown
table. A run's share is the fraction of the distinct findings the PR's runs
raised between them that it raised, averaged over the runs. Two runs' share
counts the findings either run raised, averaged over every pair. Blocking
findings are those the bot presented as blocking, and bugs are those that
claim something breaks.

Usage: analyze.py clusters-*.json
"""

import itertools
import json
import sys


def share(findings, runs, k):
    if not findings:
        return None
    combos = list(itertools.combinations(runs, k))
    total = sum(
        sum(1 for f in findings if set(f["runs"]) & set(c)) / len(findings)
        for c in combos
    )
    return total / len(combos)


def pct(v):
    return "n/a" if v is None else f"{v * 100:.0f}%"


print(
    "| PR | Distinct findings | In all runs | In 1 run "
    "| One run's share | Two runs' share "
    "| Blocking findings and bugs | One run's share of those "
    "| Two runs' share of those |"
)
print("|---|---|---|---|---|---|---|---|---|")
for path in sys.argv[1:]:
    c = json.load(open(path))
    runs, findings = c["runs"], c["findings"]
    important = [
        f for f in findings if f["severity"] == "blocking" or f["kind"] == "bug"
    ]
    every = sum(1 for f in findings if len(f["runs"]) == len(runs))
    one = sum(1 for f in findings if len(f["runs"]) == 1)
    print(
        f"| #{c['pr']} | {len(findings)} | {every} | {one} "
        f"| {pct(share(findings, runs, 1))} | {pct(share(findings, runs, 2))} "
        f"| {len(important)} | {pct(share(important, runs, 1))} "
        f"| {pct(share(important, runs, 2))} |"
    )

clusters-607.json

{
  "pr": 607,
  "runs": [1, 2, 3, 4, 5],
  "verdicts": {
    "1": "Close to mergeable; approach follows #603, remaining questions are about how stages read in edge cases.",
    "2": "Close, with a few questions to settle first, chiefly what fail() reports on a running cluster.",
    "3": "Close to mergeable; stage design follows #603, a few questions on how the stages read.",
    "4": "Close, with a few questions about stages reading Pending/Provisioning wrongly; design not challenged.",
    "5": "Close to mergeable; main questions are stages that misreport what's happening."
  },
  "findings": [
    {
      "id": "F1",
      "title": "fail() marks a running cluster's later stages Pending when an Accepted check fails",
      "kind": "bug",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-607-1/x/review/comments/01.md",
        "review-607-2/x/review/comments/01.md",
        "review-607-3/x/review/comments/01.md",
        "review-607-4/x/review/comments/01.md",
        "review-607-5/x/review/comments/01.md",
        "review-607-1/x/review/summary.md",
        "review-607-2/x/review/summary.md",
        "review-607-3/x/review/summary.md",
        "review-607-4/x/review/summary.md",
        "review-607-5/x/review/summary.md"
      ],
      "notes": "Every summary names this as the most important question."
    },
    {
      "id": "F2",
      "title": "PR description wrongly says a Ready cloud cluster XR without the new conditions counts as having finished both stages; drop the sentence",
      "kind": "process",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-607-1/x/review/summary.md",
        "review-607-2/x/review/summary.md",
        "review-607-3/x/review/summary.md",
        "review-607-4/x/review/summary.md",
        "review-607-5/x/review/summary.md"
      ],
      "notes": "Raised only in summaries."
    },
    {
      "id": "F3",
      "title": "Cloud cluster XR status.nodePools read as an untyped dict instead of through generated models",
      "kind": "design",
      "severity": "issue",
      "runs": [1, 3, 4, 5],
      "comments": [
        "review-607-1/x/review/comments/02.md",
        "review-607-3/x/review/comments/09.md",
        "review-607-4/x/review/comments/07.md",
        "review-607-5/x/review/comments/03.md"
      ],
      "notes": "Run 3 marked it Nit; runs 1, 4, 5 presented it as a normal issue. Could arguably be classed nit (style)."
    },
    {
      "id": "F4",
      "title": "_pending docstring 'Pending while the stage after waits on is False' doesn't parse and ignores Unknown",
      "kind": "doc",
      "severity": "nit",
      "runs": [1, 3, 5],
      "comments": [
        "review-607-1/x/review/comments/03.md",
        "review-607-3/x/review/comments/10.md",
        "review-607-5/x/review/comments/05.md"
      ],
      "notes": ""
    },
    {
      "id": "F5",
      "title": "Shared gpu-observer makes one GPU pool's readiness depend on others (adding a pool flips Ready pools to Provisioning)",
      "kind": "bug",
      "severity": "issue",
      "runs": [1, 2, 4, 5],
      "comments": [
        "review-607-1/x/review/comments/04.md",
        "review-607-2/x/review/comments/04.md",
        "review-607-4/x/review/comments/06.md",
        "review-607-5/x/review/comments/07.md",
        "review-607-2/x/review/summary.md"
      ],
      "notes": "Run 2 also extends it to Nebius's shared gpu-cluster resource."
    },
    {
      "id": "F6",
      "title": "ServingStackReady never reports Failed; a broken Helm release in the serving stack reads Provisioning forever",
      "kind": "design",
      "severity": "issue",
      "runs": [1],
      "comments": [
        "review-607-1/x/review/comments/05.md"
      ],
      "notes": "Related to F7 (Failed only from LastAsyncOperation in cloud functions) and F15 (docs overpromise Failed), but this one is about the ServingStack XR reporting its own failure. Kept separate."
    },
    {
      "id": "F7",
      "title": "Failed only derives from LastAsyncOperation; failures shown only on Synced (connect errors, Helm releases, Objects) read Provisioning forever",
      "kind": "design",
      "severity": "issue",
      "runs": [3, 4],
      "comments": [
        "review-607-3/x/review/comments/04.md",
        "review-607-4/x/review/comments/03.md",
        "review-607-3/x/review/summary.md"
      ],
      "notes": "Run 3's example is a ProviderConfig connect error, run 4's is failing Helm releases/Objects in the cloud XR; both propose Synced=False for non-reference reasons. Run 3 summary mentions 'which failures can reach Failed at all'."
    },
    {
      "id": "F8",
      "title": "Docs say 'the cloud provider' sets LastAsyncOperation; should be 'the Crossplane provider'",
      "kind": "doc",
      "severity": "nit",
      "runs": [1],
      "comments": [
        "review-607-1/x/review/comments/06.md"
      ],
      "notes": ""
    },
    {
      "id": "F9",
      "title": "XRD status.nodePools description says it covers each pool in spec.nodePools, but role: System pools are excluded",
      "kind": "doc",
      "severity": "nit",
      "runs": [1, 3, 5],
      "comments": [
        "review-607-1/x/review/comments/07.md",
        "review-607-3/x/review/comments/08.md",
        "review-607-5/x/review/comments/06.md"
      ],
      "notes": "Run 5 didn't mark it Nit."
    },
    {
      "id": "F10",
      "title": "Serving stack composition waits on node pools (cloud XR Ready) rather than ClusterReady, so one stuck GPU pool blocks the stack",
      "kind": "design",
      "severity": "issue",
      "runs": [2],
      "comments": [
        "review-607-2/x/review/comments/02.md",
        "review-607-2/x/review/summary.md"
      ],
      "notes": "Same code area as F11 but a different problem: this questions the compose gate, F11 the status reported in a window."
    },
    {
      "id": "F11",
      "title": "ServingStackReady reads Provisioning before the stack is composed (NodePoolsReady True while ClusterReady still False)",
      "kind": "bug",
      "severity": "issue",
      "runs": [3, 4],
      "comments": [
        "review-607-3/x/review/comments/02.md",
        "review-607-4/x/review/comments/02.md",
        "review-607-3/x/review/summary.md",
        "review-607-4/x/review/summary.md"
      ],
      "notes": "Run 4's fix (wait on ClusterReady/cloud XR Ready) overlaps F10's suggestion but the problem described differs."
    },
    {
      "id": "F12",
      "title": "Coordinate stage contract with #594 (UpCloud function would read Provisioning/Pending indefinitely)",
      "kind": "process",
      "severity": "issue",
      "runs": [2],
      "comments": [
        "review-607-2/x/review/comments/03.md",
        "review-607-2/x/review/summary.md"
      ],
      "notes": "Could be classed design (documenting what a cloud cluster XR must report)."
    },
    {
      "id": "F13",
      "title": "Docs wrongly cite IAM role as a resource that stays Provisioning on failure; upjet IAM role is async and would report Failed",
      "kind": "doc",
      "severity": "issue",
      "runs": [2, 3, 4],
      "comments": [
        "review-607-2/x/review/comments/05.md",
        "review-607-3/x/review/comments/05.md",
        "review-607-4/x/review/comments/04.md"
      ],
      "notes": ""
    },
    {
      "id": "F14",
      "title": "Docs condition table omits the FatalError reason relayed for ClusterReady/NodePoolsReady",
      "kind": "doc",
      "severity": "nit",
      "runs": [2, 3],
      "comments": [
        "review-607-2/x/review/comments/06.md",
        "review-607-3/x/review/comments/07.md"
      ],
      "notes": ""
    },
    {
      "id": "F15",
      "title": "build-the-platform.md promises a stage reads Failed on errors, but Helm release/Object failures stay Provisioning",
      "kind": "doc",
      "severity": "issue",
      "runs": [3],
      "comments": [
        "review-607-3/x/review/comments/06.md"
      ],
      "notes": "Related to F6/F7 (underlying limitation) but this asks only for a doc fix."
    },
    {
      "id": "F16",
      "title": "Comment/docs describing when a stage is Pending ('only until it's composed') don't match NodePoolsReady/ServingStackReady rules",
      "kind": "doc",
      "severity": "nit",
      "runs": [2, 4],
      "comments": [
        "review-607-2/x/review/comments/07.md",
        "review-607-4/x/review/comments/05.md"
      ],
      "notes": "Run 2 targets the fn.py:79 comment (marked Nit); run 4 targets inference-cluster.md:125 and explicitly names the same fn.py:79 comment, presented as a normal issue. Severity is split."
    },
    {
      "id": "F17",
      "title": "_pending returns a tuple though only fail() chains; a single-condition helper would be simpler",
      "kind": "nit",
      "severity": "nit",
      "runs": [2, 4, 5],
      "comments": [
        "review-607-2/x/review/comments/08.md",
        "review-607-4/x/review/comments/08.md",
        "review-607-5/x/review/comments/04.md"
      ],
      "notes": ""
    },
    {
      "id": "F18",
      "title": "#603 asks for strict stage order, but NodePoolsReady can be True while ClusterReady isn't; update #603 to match",
      "kind": "process",
      "severity": "issue",
      "runs": [3],
      "comments": [
        "review-607-3/x/review/comments/03.md"
      ],
      "notes": "Asks to change the issue rather than the code; could be classed design."
    },
    {
      "id": "F19",
      "title": "PR description credits moving reconcile checks to Accepted to the wrong commit",
      "kind": "process",
      "severity": "nit",
      "runs": [3],
      "comments": [
        "review-607-3/x/review/summary.md"
      ],
      "notes": "Raised in passing in the summary only."
    },
    {
      "id": "F20",
      "title": "Test fixtures for observed managed resources built two different ways across the six cloud functions",
      "kind": "nit",
      "severity": "nit",
      "runs": [4],
      "comments": [
        "review-607-4/x/review/comments/09.md"
      ],
      "notes": ""
    },
    {
      "id": "F21",
      "title": "Accepted=True reported before node pool checks run while provider activation is pending (and ClusterReady Provisioning with no message)",
      "kind": "bug",
      "severity": "issue",
      "runs": [5],
      "comments": [
        "review-607-5/x/review/comments/02.md",
        "review-607-5/x/review/summary.md"
      ],
      "notes": "Comment also notes ClusterReady reads Provisioning with no message while no cloud XR exists; folded in as the suggested fix covers both."
    },
    {
      "id": "F22",
      "title": "_first should be named for what it returns (a summary message)",
      "kind": "nit",
      "severity": "nit",
      "runs": [5],
      "comments": [
        "review-607-5/x/review/comments/08.md"
      ],
      "notes": ""
    }
  ]
}

clusters-619.json

{
  "pr": 619,
  "runs": [1, 2, 3, 4, 5],
  "verdicts": {
    "1": "Close on approach; accuracy of several claims and copyable steps for the TLS gateway stands between it and merging.",
    "2": "Approach fits the API, but the page needs another look before merging because a reader following it in order hits broken steps.",
    "3": "Close, but a few steps won't work as written and need another look before merge.",
    "4": "Close to mergeable once steps work on the TLS gateway and managed-settings advice stops putting everyone under one caller.",
    "5": "Close; a few claims the code or docs contradict stand between it and merging."
  },
  "findings": [
    {
      "id": "F1",
      "title": "kubectl status.endpoints snippet yields empty ANTHROPIC_BASE_URL for the guide's own TLS gateway",
      "kind": "bug",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/03.md",
        "review-619-2/x/review/comments/02.md",
        "review-619-3/x/review/comments/02.md",
        "review-619-4/x/review/comments/02.md",
        "review-619-5/x/review/comments/04.md",
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": "Summaries 1 and 4 allude vaguely to steps not holding for the TLS gateway; not listed, but they are consistent with this and F2."
    },
    {
      "id": "F2",
      "title": "Pointer to Messages API guide's port-forward doesn't work for a TLS gateway (:80 redirects, :443 cert mismatch)",
      "kind": "bug",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/05.md",
        "review-619-2/x/review/comments/03.md",
        "review-619-3/x/review/comments/05.md",
        "review-619-4/x/review/comments/03.md",
        "review-619-5/x/review/comments/06.md",
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F3",
      "title": "Distributing the client block via managed settings gives everyone the same per-person key/caller",
      "kind": "bug",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/04.md",
        "review-619-2/x/review/comments/04.md",
        "review-619-3/x/review/comments/04.md",
        "review-619-4/x/review/comments/01.md",
        "review-619-5/x/review/comments/05.md",
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md",
        "review-619-4/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": "Severity blocking in runs 2-4 summaries; run 5 summary frames it as a lesser question. All five comments also raise F4's point (shell exports rather than the JSON env block #597 asked for); they are credited there too."
    },
    {
      "id": "F4",
      "title": "Page shows shell exports, not the managed-settings JSON env block #597 asks for and the PR description/commit promise",
      "kind": "process",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/summary.md",
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md",
        "review-619-4/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": "Every summary raises the PR description/commit mismatch. The line-102 comments in all runs (1/04, 2/04, 3/04, 4/01, 5/05, filed under F3) also ask for the env block, so this overlaps F3; separated because summaries frame it as a description-accuracy/scope issue rather than the shared-key problem."
    },
    {
      "id": "F5",
      "title": "Wrong reason for naming services after the Claude ID: Claude Code checks the model ID it sends, not the response",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/02.md",
        "review-619-2/x/review/comments/01.md",
        "review-619-3/x/review/comments/01.md",
        "review-619-4/x/review/comments/04.md",
        "review-619-5/x/review/comments/02.md",
        "review-619-3/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F6",
      "title": "Recommending gateway 429 retries for a single shared key is wrong (same key, no backoff/retry-after, multiplies with Claude Code retries)",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/06.md",
        "review-619-2/x/review/comments/06.md",
        "review-619-3/x/review/comments/06.md",
        "review-619-4/x/review/comments/05.md",
        "review-619-5/x/review/comments/07.md",
        "review-619-3/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": "1/06 and 4/05 also raise outlier ejection of the only endpoint (F7); credited there."
    },
    {
      "id": "F7",
      "title": "Passive health check can eject the only endpoint after five 5xx (e.g. 529), failing requests; page should mention",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 4],
      "comments": [],
      "notes": "Raised inside 1/06 and 4/05, which are listed under F6. Could be seen as part of F6, but it's a distinct failure mode."
    },
    {
      "id": "F8",
      "title": "Example usage record doesn't match what the gateway logs (endpoint name/namespace, missing response_model, types)",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/07.md",
        "review-619-2/x/review/comments/07.md",
        "review-619-3/x/review/comments/07.md",
        "review-619-4/x/review/comments/06.md",
        "review-619-5/x/review/comments/08.md",
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md",
        "review-619-4/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": "Runs 3 and 5 additionally note counts/status should be JSON numbers not strings; kept in this finding as part of the same 'record shape is wrong' problem."
    },
    {
      "id": "F9",
      "title": "input_tokens includes cache writes as well as reads; page should say so",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/08.md",
        "review-619-2/x/review/comments/08.md",
        "review-619-3/x/review/comments/08.md",
        "review-619-4/x/review/comments/07.md",
        "review-619-5/x/review/comments/09.md",
        "review-619-4/x/review/summary.md",
        "review-619-5/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F10",
      "title": "Claim that a mixed service serves both OpenAI and Anthropic clients contradicts the XRD (OpenAI requests to Anthropic endpoint fail)",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 5],
      "comments": [
        "review-619-1/x/review/comments/01.md",
        "review-619-2/x/review/comments/10.md",
        "review-619-5/x/review/comments/01.md"
      ],
      "notes": ""
    },
    {
      "id": "F11",
      "title": "Page doesn't say a Claude ID with no service (via /model, new releases, or background built-in IDs) gets a 404",
      "kind": "doc",
      "severity": "issue",
      "runs": [1, 2, 3],
      "comments": [
        "review-619-1/x/review/comments/09.md",
        "review-619-2/x/review/comments/05.md",
        "review-619-3/x/review/comments/03.md"
      ],
      "notes": "2/05 and 3/03 lead with background sub-calls using built-in IDs; 1/09 leads with /model and new model releases. All ask the page to state the 404 and link #596, so grouped, but the emphasis differs."
    },
    {
      "id": "F12",
      "title": "Unused modelplane.ai/external-provider label on the endpoints",
      "kind": "nit",
      "severity": "nit",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/10.md",
        "review-619-2/x/review/comments/13.md",
        "review-619-3/x/review/comments/11.md",
        "review-619-4/x/review/comments/09.md",
        "review-619-5/x/review/comments/11.md"
      ],
      "notes": ""
    },
    {
      "id": "F13",
      "title": "1800s timeout reasoning (and other manifest-comment explanations) repeated in several places",
      "kind": "nit",
      "severity": "nit",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-619-1/x/review/comments/11.md",
        "review-619-2/x/review/comments/12.md",
        "review-619-3/x/review/comments/10.md",
        "review-619-4/x/review/comments/08.md",
        "review-619-5/x/review/comments/03.md"
      ],
      "notes": "1/11, 2/12 and 5/03 also raise the sentence fragment (F14); credited there."
    },
    {
      "id": "F14",
      "title": "'Setting the request timeout to 1800s here...' is a sentence fragment with no main verb",
      "kind": "nit",
      "severity": "nit",
      "runs": [1, 2, 5],
      "comments": [],
      "notes": "Raised inside 1/11, 2/12 and 5/03, which are listed under F13."
    },
    {
      "id": "F15",
      "title": "Proxy drain timeout fixed at 300s cuts turns the 1800s request timeout allows during a rollout",
      "kind": "doc",
      "severity": "issue",
      "runs": [3, 5],
      "comments": [
        "review-619-3/x/review/comments/09.md",
        "review-619-5/x/review/comments/12.md"
      ],
      "notes": ""
    },
    {
      "id": "F16",
      "title": "Guide references cluster gw-gcp-us it never creates, and doesn't say where the TLS Secret goes",
      "kind": "bug",
      "severity": "blocking",
      "runs": [2],
      "comments": [
        "review-619-2/x/review/comments/09.md",
        "review-619-2/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F17",
      "title": "ANTHROPIC_API_KEY doesn't change the background model when ANTHROPIC_DEFAULT_HAIKU_MODEL is pinned",
      "kind": "nit",
      "severity": "nit",
      "runs": [2],
      "comments": [
        "review-619-2/x/review/comments/11.md"
      ],
      "notes": "Factual doc accuracy point, but marked Nit."
    },
    {
      "id": "F18",
      "title": "Closing sentence promises future content and will go stale",
      "kind": "nit",
      "severity": "nit",
      "runs": [5],
      "comments": [
        "review-619-5/x/review/comments/10.md"
      ],
      "notes": ""
    },
    {
      "id": "F19",
      "title": "docs-vale reports 13 errors on the new page (PR description says Vale passes)",
      "kind": "process",
      "severity": "issue",
      "runs": [2, 3],
      "comments": [
        "review-619-2/x/review/summary.md",
        "review-619-3/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F20",
      "title": "Commit trailer uses 'Part of #597.' instead of CONTRIBUTING's 'Towards #597.'",
      "kind": "process",
      "severity": "nit",
      "runs": [3],
      "comments": [
        "review-619-3/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F21",
      "title": "'Generated with Claude Code' AI attribution line should be removed",
      "kind": "process",
      "severity": "issue",
      "runs": [1],
      "comments": [
        "review-619-1/x/review/summary.md"
      ],
      "notes": "Unclear whether this is in the PR description or commit; summary doesn't say."
    },
    {
      "id": "F22",
      "title": "Draft #633 removes one of the subscription blockers the last section names; section will need an edit",
      "kind": "doc",
      "severity": "nit",
      "runs": [4],
      "comments": [
        "review-619-4/x/review/summary.md"
      ],
      "notes": "Presented as a heads-up for later, not a merge blocker."
    }
  ]
}

clusters-625.json

{
  "pr": 625,
  "runs": [1, 2, 3, 4, 5],
  "verdicts": {
    "1": "Needs a step back on the API: drainTimeout addresses #618, but the rest of spec.proxy repeats Envoy Gateway or doesn't take effect.",
    "2": "Needs rework on everything but the drainTimeout pass-through; grace-period patch and rollout fields are redundant or broken, and the default differs from #618.",
    "3": "Needs another pass on its approach before the details matter.",
    "4": "Needs a step back on its API; most of what it adds beyond the drain timeout could go.",
    "5": "Needs another look at what the function composes from spec.proxy, since the rollout settings wouldn't reach the proxy Deployment."
  },
  "findings": [
    {
      "id": "F1",
      "title": "terminationGracePeriodSeconds patch is redundant: Envoy Gateway v1.8.4 already sets it to drainTimeout + 300s, so the docstring's 'inert' claim is false; just pass drainTimeout through",
      "kind": "design",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-625-1/x/review/comments/06.md",
        "review-625-2/x/review/comments/01.md",
        "review-625-3/x/review/comments/01.md",
        "review-625-4/x/review/comments/01.md",
        "review-625-5/x/review/comments/04.md",
        "review-625-1/x/review/summary.md",
        "review-625-2/x/review/summary.md",
        "review-625-3/x/review/summary.md",
        "review-625-4/x/review/summary.md"
      ],
      "notes": "Runs 1-4 also raise the _seconds ms-as-minutes bug (F2) inside these same comments. Run 1's summary raises it only implicitly ('repeats what Envoy Gateway already does')."
    },
    {
      "id": "F2",
      "title": "_seconds regex matches 'm' before 'ms', so 500ms parses as 500 minutes",
      "kind": "bug",
      "severity": "issue",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-625-5/x/review/comments/05.md"
      ],
      "notes": "Runs 1-4 raise this inside their F1 comment (listed under F1, since each file appears once). Only run 5 gives it a separate comment, and it adds that ms is weighted 0 in the unit table."
    },
    {
      "id": "F3",
      "title": "maxSurge/maxUnavailable are composed flat under envoyDeployment.strategy instead of strategy.rollingUpdate, so they're rejected or pruned; the test expects the same wrong layout",
      "kind": "bug",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-625-1/x/review/comments/07.md",
        "review-625-2/x/review/comments/02.md",
        "review-625-3/x/review/comments/02.md",
        "review-625-4/x/review/comments/02.md",
        "review-625-5/x/review/comments/01.md",
        "review-625-1/x/review/summary.md",
        "review-625-3/x/review/summary.md",
        "review-625-4/x/review/summary.md",
        "review-625-5/x/review/summary.md"
      ],
      "notes": "Runs disagree on the consequence: runs 2 and 5 say pruned (silently keeps 25%/25%), runs 3 and 4 say SSA rejects the whole EnvoyProxy, run 1 says either. Same underlying problem. Run 2's summary says 'broken as written' without specifics, so it isn't counted here."
    },
    {
      "id": "F4",
      "title": "maxSurge/maxUnavailable typed as string: Deployment rejects non-percent strings like \"1\", and the reference manifest's unquoted integers fail the docs-manifests check; use x-kubernetes-int-or-string",
      "kind": "bug",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-625-1/x/review/comments/04.md",
        "review-625-2/x/review/comments/03.md",
        "review-625-3/x/review/comments/03.md",
        "review-625-4/x/review/comments/03.md",
        "review-625-5/x/review/comments/02.md",
        "review-625-1/x/review/summary.md",
        "review-625-3/x/review/summary.md"
      ],
      "notes": "Run 1 splits it: the inline comment covers Deployment rejecting \"1\", the summary covers docs-manifests failing. Runs 2-5 put both halves in one comment. Treated as one finding because the fix is the same."
    },
    {
      "id": "F5",
      "title": "maxSurge and maxUnavailable can both be 0, which Kubernetes rejects; add a CEL rule",
      "kind": "bug",
      "severity": "issue",
      "runs": [1, 3, 5],
      "comments": [
        "review-625-1/x/review/comments/05.md",
        "review-625-3/x/review/comments/06.md",
        "review-625-5/x/review/comments/03.md"
      ],
      "notes": ""
    },
    {
      "id": "F6",
      "title": "Rollout fields add nothing at the fixed 2 replicas: Kubernetes' 25%/25% default already rounds to surge 1, unavailable 0; drop them or justify",
      "kind": "design",
      "severity": "blocking",
      "runs": [1, 2, 3, 4],
      "comments": [
        "review-625-1/x/review/comments/02.md",
        "review-625-2/x/review/comments/05.md",
        "review-625-3/x/review/comments/07.md",
        "review-625-4/x/review/comments/04.md",
        "review-625-1/x/review/summary.md",
        "review-625-2/x/review/summary.md",
        "review-625-4/x/review/summary.md"
      ],
      "notes": "Run 3's comment is on the docs example and asks the docs to say the example matches the default (or pick a different example), rather than to drop the fields; same observation, different fix. Run 2's comment also floats the strategy.rollingUpdate layout (F9) in its last sentence."
    },
    {
      "id": "F7",
      "title": "drainTimeout defaults to a fixed 300s instead of #618's 'longest route timeout the gateway serves', so raising a ModelService timeout brings the cut back",
      "kind": "design",
      "severity": "blocking",
      "runs": [1, 2, 3, 4, 5],
      "comments": [
        "review-625-1/x/review/comments/03.md",
        "review-625-2/x/review/comments/04.md",
        "review-625-3/x/review/comments/05.md",
        "review-625-4/x/review/comments/05.md",
        "review-625-5/x/review/comments/06.md",
        "review-625-1/x/review/summary.md",
        "review-625-2/x/review/summary.md",
        "review-625-3/x/review/summary.md",
        "review-625-4/x/review/summary.md"
      ],
      "notes": "Fixes differ: run 2 proposes deriving from ModelRoutes; run 4 suggests raising the fixed default to 3600s; runs 3 and 5 ask the PR to document the reason (derived default rolls the proxy on every timeout change). Severity is blocking in runs 1-4's summaries, issue in run 5."
    },
    {
      "id": "F8",
      "title": "Cluster gateway's drain timeout (and rollout) isn't covered by this PR, #554 or #626; decide where it lives / track it",
      "kind": "design",
      "severity": "issue",
      "runs": [1, 2],
      "comments": [
        "review-625-1/x/review/comments/01.md",
        "review-625-2/x/review/summary.md"
      ],
      "notes": "Run 3's summary and run 5's summary mention the cluster gateway reusing these fields, but as a reason to settle layout now (F9) or as approval, not as a coverage gap, so they're not counted here."
    },
    {
      "id": "F9",
      "title": "Rollout fields should mirror upstream DeploymentStrategy as proxy.strategy.rollingUpdate.{maxSurge,maxUnavailable}, and the layout should be settled now since #554 will reuse it",
      "kind": "design",
      "severity": "issue",
      "runs": [2, 3],
      "comments": [
        "review-625-3/x/review/comments/04.md",
        "review-625-3/x/review/summary.md"
      ],
      "notes": "Run 2 raises the same nesting suggestion in the last sentence of review-625-2/x/review/comments/05.md, which is listed under F6. Distinct from F3 (the composed output's nesting) since this is about the XRD shape."
    },
    {
      "id": "F10",
      "title": "proxy should take default: {} so the 300s default applies without writing proxy:, removing the duplicate _DRAIN_TIMEOUT fallback in the function",
      "kind": "design",
      "severity": "issue",
      "runs": [4],
      "comments": [
        "review-625-4/x/review/comments/06.md"
      ],
      "notes": "Could also be read as a nit; the bot didn't mark it as one."
    },
    {
      "id": "F11",
      "title": "docs/content/models/model-service.md still says in-flight requests get five minutes on gateway restart; should point at spec.proxy.drainTimeout",
      "kind": "doc",
      "severity": "issue",
      "runs": [4],
      "comments": [
        "review-625-4/x/review/comments/07.md"
      ],
      "notes": ""
    },
    {
      "id": "F12",
      "title": "Docs should warn that changing drainTimeout itself rolls the proxy, and the first rollout still drains on the old 300s",
      "kind": "doc",
      "severity": "issue",
      "runs": [5],
      "comments": [
        "review-625-5/x/review/comments/07.md"
      ],
      "notes": "Run 5's F7 comment uses the same command-line mechanism as an argument about the default, but this is a separate docs problem."
    },
    {
      "id": "F13",
      "title": "Commit lacks Signed-off-by and issue trailer; PR description should open with 'Part of #618'",
      "kind": "process",
      "severity": "issue",
      "runs": [2, 3, 5],
      "comments": [
        "review-625-2/x/review/summary.md",
        "review-625-3/x/review/summary.md",
        "review-625-5/x/review/summary.md"
      ],
      "notes": "Run 5 mentions only Signed-off-by; runs 2 and 3 also mention the issue trailer; only run 2 mentions the PR description opener."
    },
    {
      "id": "F14",
      "title": "Commit message falsely credits the grace-period patch with keeping the pod alive through draining",
      "kind": "process",
      "severity": "issue",
      "runs": [3, 5],
      "comments": [
        "review-625-3/x/review/summary.md",
        "review-625-5/x/review/summary.md"
      ],
      "notes": "Same false premise as F1 (whose comments call out the docstring), but about the commit message text, so kept separate. Could reasonably be merged into F1."
    },
    {
      "id": "F15",
      "title": "PR description says nix run .#build and nix flake check weren't run",
      "kind": "process",
      "severity": "issue",
      "runs": [1],
      "comments": [
        "review-625-1/x/review/summary.md"
      ],
      "notes": "Run 1 pairs this with the docs-manifests failure, which is counted under F4."
    }
  ]
}

clusters-632.json

{
  "pr": 632,
  "runs": [
    1,
    2,
    3,
    4,
    5
  ],
  "verdicts": {
    "1": "Needs a maintainer's call on direction (hostPath for ML teams, escape hatch vs #630/#72 node storage) before details matter.",
    "2": "Needs a step back on whether ModelDeployment authors get hostPath and how it fits #630 before it's close to merging.",
    "3": "Close to mergeable, with one design question first: arbitrary hostPath on shared GPU nodes.",
    "4": "Close, with design questions to settle before merge; main one is how fixed dshm/model-cache reservations fit #574.",
    "5": "Needs a step back: the PR description and commit disagree on which volume sources (hostPath) are allowed."
  },
  "findings": [
    {
      "id": "F1",
      "title": "Any ModelDeployment author can mount any hostPath on shared nodes; node paths belong to platform teams and nothing restricts it",
      "kind": "design",
      "severity": "blocking",
      "runs": [
        1,
        2,
        3,
        4,
        5
      ],
      "comments": [
        "review-632-1/x/review/comments/01.md",
        "review-632-2/x/review/comments/01.md",
        "review-632-3/x/review/comments/01.md",
        "review-632-4/x/review/comments/04.md",
        "review-632-5/x/review/comments/01.md",
        "review-632-1/x/review/summary.md",
        "review-632-2/x/review/summary.md",
        "review-632-3/x/review/summary.md",
        "review-632-4/x/review/summary.md"
      ],
      "notes": "Lead/blocking concern in runs 1, 2 and 3 (run 5 leads with F18, which overlaps); run 4 lists it as a secondary open question. Run 1 comment 01 also raises hostPath tying deployments to node layout (credited to F2). Run 5 comment 01 frames it partly through the PR description mismatch (credited to F16)."
    },
    {
      "id": "F2",
      "title": "hostPath ties a deployment to a node layout and is invisible to placement; use emptyDir plus #630's device selection instead",
      "kind": "design",
      "severity": "issue",
      "runs": [
        1,
        2
      ],
      "comments": [
        "review-632-2/x/review/comments/02.md"
      ],
      "notes": "Run 1 raises this as the second half of comment 01 (listed under F1): replica on a cluster without /mnt/nvme never starts, suggests emptyDir/ephemeral plus #630 device selector. Close to F3 (both cite #630) but F3 is about the abstraction/escape-hatch direction and init-container staging, not placement."
    },
    {
      "id": "F3",
      "title": "Documented hostPath + init-container weight staging gets ahead of #630/#72/ModelCache abstractions; escape hatch vs Modelplane-owned storage",
      "kind": "design",
      "severity": "blocking",
      "runs": [
        1,
        2
      ],
      "comments": [
        "review-632-1/x/review/comments/02.md",
        "review-632-2/x/review/comments/03.md"
      ],
      "notes": "Run 1 presents it as part of its blocking direction question (summary). Run 2 comment 03 frames it as presenting init containers generically and leaving weight staging to ModelCache; run 2 summary's 'how that fits with the node-local storage #630 provisions' could equally map here or F2."
    },
    {
      "id": "F4",
      "title": "Volume source schema: fixed allowlist of 11 sources but each is free-form inside, so typos or hostPath: {} pass admission",
      "kind": "design",
      "severity": "issue",
      "runs": [
        1
      ],
      "comments": [
        "review-632-1/x/review/comments/03.md"
      ],
      "notes": ""
    },
    {
      "id": "F5",
      "title": "PVC/configMap/secret/csi volumes need their objects or drivers on every target InferenceCluster; document that contract",
      "kind": "doc",
      "severity": "issue",
      "runs": [
        1
      ],
      "comments": [
        "review-632-1/x/review/comments/04.md"
      ],
      "notes": ""
    },
    {
      "id": "F6",
      "title": "design/design.md (and user-authored-routing.md) out of date: template contents don't list volumes, init containers, reserved names/paths",
      "kind": "doc",
      "severity": "issue",
      "runs": [
        1,
        2,
        3,
        4
      ],
      "comments": [
        "review-632-1/x/review/comments/05.md",
        "review-632-2/x/review/comments/04.md",
        "review-632-3/x/review/comments/07.md"
      ],
      "notes": "Run 4 raises it inside comment 02 (listed under F17): 'design.md still lists only containers and imagePullSecrets'. Run 3 additionally names user-authored-routing.md."
    },
    {
      "id": "F7",
      "title": "'Passes them through as written' / 'written as in a Kubernetes pod spec' is inaccurate: init containers and mounts are a pruned subset",
      "kind": "doc",
      "severity": "issue",
      "runs": [
        1,
        2,
        3,
        5
      ],
      "comments": [
        "review-632-1/x/review/comments/06.md",
        "review-632-2/x/review/comments/08.md",
        "review-632-3/x/review/comments/05.md",
        "review-632-5/x/review/comments/06.md"
      ],
      "notes": "Run 1 targets the XRD description, others the docs page; run 1 and run 5 mention both."
    },
    {
      "id": "F8",
      "title": "Mounts referencing undeclared volumes (typos, model-cache without modelCacheRef) pass admission and fail only on the workload cluster",
      "kind": "bug",
      "severity": "issue",
      "runs": [
        1,
        2,
        3,
        4,
        5
      ],
      "comments": [
        "review-632-1/x/review/comments/07.md",
        "review-632-2/x/review/comments/06.md",
        "review-632-3/x/review/comments/02.md",
        "review-632-4/x/review/comments/03.md",
        "review-632-5/x/review/comments/03.md",
        "review-632-4/x/review/summary.md"
      ],
      "notes": "Run 4 covers only the model-cache-without-modelCacheRef case, not arbitrary typos; judged the same underlying gap. Run 5 comment 03 also notes the PR description promises this rule (credited to F16)."
    },
    {
      "id": "F9",
      "title": "Python models edited by hand; regenerate with nix run .#build (missing docstring, stale fs://apis hash in .lock.json)",
      "kind": "process",
      "severity": "nit",
      "runs": [
        1,
        2,
        3,
        4
      ],
      "comments": [
        "review-632-1/x/review/comments/08.md",
        "review-632-2/x/review/comments/07.md",
        "review-632-3/x/review/comments/08.md",
        "review-632-4/x/review/comments/07.md"
      ],
      "notes": "Only run 1 marks it Nit; runs 2-4 present it as a plain request. Could be classed as issue. Kind could arguably be bug (stale generated code)."
    },
    {
      "id": "F10",
      "title": "Commit message says models were edited by hand; drop that note from history",
      "kind": "process",
      "severity": "issue",
      "runs": [
        1,
        4,
        5
      ],
      "comments": [
        "review-632-1/x/review/summary.md",
        "review-632-4/x/review/summary.md",
        "review-632-5/x/review/summary.md"
      ],
      "notes": "Summary-only. Closely tied to F9 in runs 1 and 4 (fix by regenerating); kept separate because it is about the commit message rather than the code."
    },
    {
      "id": "F11",
      "title": "Reserved mountPath rule compares exact strings, so a trailing slash (/dev/shm/, /mnt/models/) bypasses it and shadows Modelplane's mount",
      "kind": "bug",
      "severity": "issue",
      "runs": [
        2,
        3,
        4,
        5
      ],
      "comments": [
        "review-632-2/x/review/comments/05.md",
        "review-632-3/x/review/comments/03.md",
        "review-632-4/x/review/comments/05.md",
        "review-632-5/x/review/comments/04.md",
        "review-632-2/x/review/summary.md",
        "review-632-4/x/review/summary.md"
      ],
      "notes": ""
    },
    {
      "id": "F12",
      "title": "Init container field descriptions copied from the engine container don't apply (VLLM_HOST_IP env example, engine mount restriction, resources message)",
      "kind": "doc",
      "severity": "nit",
      "runs": [
        2,
        3,
        4
      ],
      "comments": [
        "review-632-2/x/review/comments/09.md",
        "review-632-3/x/review/comments/04.md",
        "review-632-4/x/review/comments/06.md"
      ],
      "notes": "Run 3 doesn't mark it Nit (issue-level there)."
    },
    {
      "id": "F13",
      "title": "Docs example starts at a bare template:, which readers may put under spec.template.spec where volumes don't exist",
      "kind": "doc",
      "severity": "nit",
      "runs": [
        3
      ],
      "comments": [
        "review-632-3/x/review/comments/06.md"
      ],
      "notes": ""
    },
    {
      "id": "F14",
      "title": "Add a compose-model-deployment test that copying the template into the ModelReplica keeps volumes, mounts and init containers",
      "kind": "design",
      "severity": "issue",
      "runs": [
        3
      ],
      "comments": [
        "review-632-3/x/review/comments/09.md"
      ],
      "notes": "Test coverage request; 'design' is the nearest kind."
    },
    {
      "id": "F15",
      "title": "template_pod_fields helper bundles init containers and pull secrets and is poorly named; split into init_containers(member)",
      "kind": "nit",
      "severity": "nit",
      "runs": [
        3,
        4
      ],
      "comments": [
        "review-632-3/x/review/comments/10.md",
        "review-632-4/x/review/comments/08.md"
      ],
      "notes": ""
    },
    {
      "id": "F16",
      "title": "Hardcoded dshm/model-cache reservations conflict with #574/#281 per-cache volume names and paths; decide who owns collision checks",
      "kind": "design",
      "severity": "blocking",
      "runs": [
        4,
        5
      ],
      "comments": [
        "review-632-4/x/review/comments/01.md",
        "review-632-5/x/review/comments/02.md",
        "review-632-4/x/review/summary.md"
      ],
      "notes": "Run 4's main concern (summary). Run 5 comment 02 also raises F17 (names become a contract) and F18 (missing modelplane- prefix rule from the description)."
    },
    {
      "id": "F17",
      "title": "Docs make dshm and model-cache public API by inviting users to mount them by name; renaming later breaks deployments",
      "kind": "design",
      "severity": "issue",
      "runs": [
        4,
        5
      ],
      "comments": [
        "review-632-4/x/review/comments/02.md"
      ],
      "notes": "Unsure whether this is separate from F16; both cite #574/#281. Run 4 splits them into two comments; run 5 comment 02 (listed under F16) raises both. Run 4 comment 02 also raises design.md being out of date (credited to F6)."
    },
    {
      "id": "F18",
      "title": "PR description and commit disagree: description excludes hostPath/nfs/image and promises name rules (modelplane- prefix, mount-name check) the commit lacks",
      "kind": "process",
      "severity": "blocking",
      "runs": [
        5
      ],
      "comments": [
        "review-632-5/x/review/summary.md"
      ],
      "notes": "Run 5's lead concern. Also raised inside run 5 comments 01 (F1), 02 (F16) and 03 (F8). Overlaps F1 in substance (which volume sources to allow) but the stated problem is the mismatch."
    },
    {
      "id": "F19",
      "title": "If hostPath goes, disk-backed emptyDir only lands on NVMe on EKS after #630 and restages on every pod replacement; docs should say what other clusters get",
      "kind": "doc",
      "severity": "issue",
      "runs": [
        5
      ],
      "comments": [
        "review-632-5/x/review/comments/05.md"
      ],
      "notes": "Related to F2 (emptyDir + #630 instead of hostPath) but from the opposite side: questions the adequacy of the emptyDir alternative rather than proposing it."
    }
  ]
}

collect.sh

#!/usr/bin/env bash
# Download every review from a Review Eval workflow run, extract it, and print
# each run's exit status, comment count and model cost in USD.
#
# Usage: collect.sh <owner/repo> <workflow run ID>
set -euo pipefail
repo=$1
run=$2
for name in $(gh api --paginate "repos/$repo/actions/runs/$run/artifacts" -q '.artifacts[].name'); do
  [ -d "$name/x" ] && continue
  gh run download "$run" -R "$repo" -n "$name" -D "$name" >/dev/null
  mkdir -p "$name/x"
  tar -xf "$name/out.tar" -C "$name/x"
done
for d in review-*/; do
  d=${d%/}
  cost=$(jq -s '[.[].info.cost] | add' "$d"/x/transcript/*.json)
  echo "$d exit=$(cat "$d/x/exit") comments=$(ls "$d/x/review/comments" 2>/dev/null | wc -l) cost=$cost"
done

extract.sh

#!/usr/bin/env bash
# For each run collect.sh extracted, write reports.md, the four reviewers'
# final reports to the main agent, and main.md, the main agent's own text and
# reasoning. Run it in the directory collect.sh used.
set -euo pipefail
for d in review-*/; do
  d=${d%/}
  : > "$d/reports.md"
  for agent in review-design review-correctness review-craft review-prose review; do
    for f in "$d"/x/transcript/*.json; do
      [ "$(jq -r .info.agent "$f")" = "$agent" ] || continue
      if [ "$agent" = review ]; then
        jq -r '.messages[] | select(.type=="assistant") | .content[] | select(.type=="text" or .type=="reasoning") | .text // empty' "$f" > "$d/main.md"
      else
        { echo "# $agent"; echo; jq -r '[.messages[] | select(.type=="assistant")][-1] | .content[] | select(.type=="text") | .text' "$f"; echo; } >> "$d/reports.md"
      fi
    done
  done
done

group-prompt.md

You're measuring how much an AI code review bot's findings vary between runs. The bot reviewed the same pull request, at the same commit, several times independently. No run saw another's output. Your job is to work out which findings recur across runs and which don't.

Each run's review is in a directory under {DIR} named review-<PR>-<run>, at x/review/summary.md (the review summary) and x/review/comments/*.md (one inline comment each, with front matter giving its file and line). Review PR #{PR}. Its runs are: {RUNS}. You can read the PR's code in {CLONE} (a clone of the repo) at commit {SHA}, with git -C {CLONE} fetch -q origin {SHA} first, if you need it to tell whether two comments are about the same problem.

1. Read every comment and summary from every run.
2. Group them into distinct findings. Two comments are the same finding when they're about the same underlying problem, even if they're worded differently, sit on different lines, or suggest different fixes. Two comments on the same line about different problems are different findings. A summary that raises a problem counts as raising it. Be strict: "both touch the autoscaler" is not the same finding. When you're unsure whether two comments match, say so in the notes rather than forcing it.
3. Classify each finding:
   • kind: bug (claims something breaks or behaves wrongly), design (questions the approach, API shape or scope), doc (docs or comments that are untrue or misleading), nit (marked Nit, or minor style, naming or readability), or process (commits, PR description, issue linkage).
   • severity as the bot presented it: blocking (it's a reason the PR can't merge as is), issue, or nit.

Write your result as JSON to {DIR}/clusters-{PR}.json, in this shape:

{
  "pr": 607,
  "runs": [1, 2, 3, 4, 5],
  "verdicts": {"1": "one-line paraphrase of the summary's opening verdict"},
  "findings": [
    {
      "id": "F1",
      "title": "short description of the problem",
      "kind": "bug",
      "severity": "issue",
      "runs": [1, 3, 4],
      "comments": ["review-607-1/x/review/comments/02.md", "review-607-3/x/review/summary.md"],
      "notes": "anything uncertain about the grouping"
    }
  ]
}

Every comment file from every run must appear in exactly one finding's comments list, except a summary, which appears only where it raises a problem. Then reply with a short table: finding, kind, severity, and which runs raised it, plus any grouping you were unsure of. Don't judge whether findings are right. Don't edit anything outside {DIR}.

A comment that raises two problems counts toward both findings' runs, though its file is listed under only one finding's comments. Say so in that finding's notes.

judge-607.json

{
  "pr": 607,
  "findings": [
    {
      "id": "F1",
      "title": "fail() marks a running cluster's later stages Pending when an Accepted check fails",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "fail() now sets ClusterReady/NodePoolsReady/ServingStackReady Pending alongside Accepted=False, and fail() runs every reconcile (e.g. a deleted class or a new pool with a typo'd className), while Crossplane v2.4.0 keeps conditions a fatal result sets (reconciler.go L790-803) and the cluster keeps serving. The commit message says this is deliberate, but it contradicts the PR's own docs ('Once a stage has started, it reports its own state') and the fn.py:77-81 comment, so a question is warranted."
    },
    {
      "id": "F2",
      "title": "PR description wrongly says a Ready cloud cluster XR without the new conditions counts as having finished both stages; drop the sentence",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "_relay only reads the relayed condition type; when it's absent it returns Provisioning (or Pending), and nothing in derive_conditions reads the cloud XR's Ready. The description's claim is false; a one-sentence summary note asking to drop it fits the skill's guidance for PR-description findings."
    },
    {
      "id": "F3",
      "title": "Cloud cluster XR status.nodePools read as an untyped dict instead of through generated models",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "relayed_node_pools does struct_to_dict(...).get('status')...get('nodePools'), while _observed_cluster_cache_class already validates the cloud XR via a passed model type and every cloud model now has status.nodePools. python-code-factoring explicitly calls a .get() chain on reads a defect, including in files that already contain dicts."
    },
    {
      "id": "F4",
      "title": "_pending docstring 'Pending while the stage after waits on is False' doesn't parse and ignores Unknown",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "The docstring is _relay's (fn.py ~307), reads garbled, and the code tests after.status != 'True', so Unknown also yields Pending. A valid but very small docstring nit."
    },
    {
      "id": "F5",
      "title": "Shared gpu-observer makes one GPU pool's readiness depend on others (adding a pool flips Ready pools to Provisioning)",
      "true": "partly",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Vultr pool_resources adds the single gpu-observer Object to every GPU pool, and its readiness CEL requires numberReady == desiredNumberScheduled on the cluster-wide validator DaemonSet, so a new GPU pool's (or autoscaled) nodes turn every Ready GPU pool Provisioning. The Nebius extension (run 2) is weaker: a shared gpu-cluster stays Ready when another pool is added."
    },
    {
      "id": "F6",
      "title": "ServingStackReady never reports Failed; a broken Helm release in the serving stack reads Provisioning forever",
      "true": "yes",
      "clears_bar": false,
      "fails": "Request to do more: the docs table deliberately lists only Pending/Provisioning for ServingStackReady; adding Failed means the ServingStack XR reporting failures, which is new scope.",
      "worth_posting": "no",
      "evidence": "derive_conditions only produces Available/Pending/Provisioning for ServingStackReady, and the docs say so. #603 makes Failed optional ('optionally Failed if an error is propagated')."
    },
    {
      "id": "F7",
      "title": "Failed only derives from LastAsyncOperation; failures shown only on Synced (connect errors, Helm releases, Objects) read Provisioning forever",
      "true": "partly",
      "clears_bar": false,
      "fails": "Author would want to change it / request to do more: the limitation is deliberate and documented in code, commit message and docs, and the suggested narrower signal isn't readily available.",
      "worth_posting": "no",
      "evidence": "True that only LastAsyncOperation is read. But crossplane-runtime marks reference-resolution and connect failures with the same ReconcileError reason (reconciler.go L1079, L1099), so 'Synced=False for a reason other than reference resolution' would need message matching; the author already weighed and rejected Synced."
    },
    {
      "id": "F8",
      "title": "Docs say 'the cloud provider' sets LastAsyncOperation; should be 'the Crossplane provider'",
      "true": "partly",
      "clears_bar": false,
      "fails": "Author would want to change it: the user-facing sentence is about the cloud's error being reported, which is accurate enough for readers.",
      "worth_posting": "no",
      "evidence": "The doc says the cloud provider 'reports that its last create... failed'; the error does originate from the cloud API and upjet records it. Wording quibble with no misleading consequence."
    },
    {
      "id": "F9",
      "title": "XRD status.nodePools description says it covers each pool in spec.nodePools, but role: System pools are excluded",
      "true": "partly",
      "clears_bar": false,
      "fails": "It can happen: compose-inference-cluster passes only non-system pools and the cloud function injects the system pool outside spec.nodePools, so a System pool in a cloud XR's spec.nodePools only arises from a hand-written cloud XR.",
      "worth_posting": "marginal",
      "evidence": "_stage_pools filters role != 'System', and the cloud XRDs' spec.nodePools description still mentions System pools, so the description is imprecise. In the composed path the description matches what's reported."
    },
    {
      "id": "F10",
      "title": "Serving stack composition waits on node pools (cloud XR Ready) rather than ClusterReady, so one stuck GPU pool blocks the stack",
      "true": "yes",
      "clears_bar": false,
      "fails": "This PR introduced it: gating the ServingStack on the cloud XR's Ready predates the PR; changing it is a request to do more.",
      "worth_posting": "no",
      "evidence": "compose_<cloud> composes the stack via resolve_*_backend_secrets(…_ready=…), the same gate as before the PR; the PR only reports it as 'Waiting for NodePoolsReady'."
    },
    {
      "id": "F11",
      "title": "ServingStackReady reads Provisioning before the stack is composed (NodePoolsReady True while ClusterReady still False)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Cloud derive_conditions sets NodePoolsReady=True once all pools are Ready regardless of ClusterReady, while the stack is composed only once the cloud XR is Ready; derive_conditions' Pending branch tests only pools_ready, so in that window ServingStackReady reads Provisioning with nothing composed. Plausible on a new cluster whose add-ons or Helm releases lag the GPU node groups."
    },
    {
      "id": "F12",
      "title": "Coordinate stage contract with #594 (UpCloud function would read Provisioning/Pending indefinitely)",
      "true": "yes",
      "clears_bar": false,
      "fails": "This PR introduced it: it's a cross-PR merge concern, not a defect in this PR's code.",
      "worth_posting": "marginal",
      "evidence": "#594 (open, by a different author) adds compose-upcloud-cluster and edits compose-inference-cluster; with this PR's _relay, a cloud XR that doesn't report ClusterReady reads Provisioning forever. A useful heads-up, but more of a note for #594 than a review finding."
    },
    {
      "id": "F13",
      "title": "Docs wrongly cite IAM role as a resource that stays Provisioning on failure; upjet IAM role is async and would report Failed",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "upjet's DefaultResource sets UseAsync: true (pkg/config/common.go), and provider-upjet-aws's namespaced iam/role zz_controller.go uses NewTerraformPluginSDKAsyncConnector, so a failed Role sets LastAsyncOperation=False. The docs (and commit message) say a failed IAM role stays Provisioning, which is untrue."
    },
    {
      "id": "F14",
      "title": "Docs condition table omits the FatalError reason relayed for ClusterReady/NodePoolsReady",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "_relay passes through the Unknown/FatalError conditions Crossplane sets when the cloud XR's function fails (the PR tests this), and the table's 'Reasons while not True' column omits it. Minor doc completeness nit."
    },
    {
      "id": "F15",
      "title": "build-the-platform.md promises a stage reads Failed on errors, but Helm release/Object failures stay Provisioning",
      "true": "no",
      "clears_bar": false,
      "fails": "It's true: the sentence already limits Failed to errors the cloud provider reports, which excludes Helm releases and in-cluster Objects, and links to the detailed page.",
      "worth_posting": "no",
      "evidence": "The sentence reads 'a stage reads Failed while the cloud provider reports an error on one of its resources'; a Helm release failure isn't a cloud-provider error, so it doesn't overpromise."
    },
    {
      "id": "F16",
      "title": "Comment/docs describing when a stage is Pending ('only until it's composed') don't match NodePoolsReady/ServingStackReady rules",
      "true": "partly",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "EKS composes node groups immediately, yet NodePoolsReady stays Pending while ClusterReady isn't True and no pool is Ready, so the fn.py:79 comment ('Pending ... only until it's composed') is untrue for that stage. The docs line 125 sentence is broadly right; the comment is the real inaccuracy."
    },
    {
      "id": "F17",
      "title": "_pending returns a tuple though only fail() chains; a single-condition helper would be simpler",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "Two of three callers pass a one-element tuple and take [0]; `after` is a str in _pending and a Condition in _relay. A legitimate readability nit that removes code."
    },
    {
      "id": "F18",
      "title": "#603 asks for strict stage order, but NodePoolsReady can be True while ClusterReady isn't; update #603 to match",
      "true": "yes",
      "clears_bar": false,
      "fails": "Author would want to change it: the deviation is deliberate and explained in the commit message and docstring; editing the issue is not a code change.",
      "worth_posting": "no",
      "evidence": "Cloud derive_conditions lets NodePoolsReady leave Pending once any pool is Ready, as its docstring and commit aa9b5d2 explain. The issue is closed by the PR anyway."
    },
    {
      "id": "F19",
      "title": "PR description credits moving reconcile checks to Accepted to the wrong commit",
      "true": "no",
      "clears_bar": false,
      "fails": "It's true: the description doesn't clearly attribute the Accepted move to the first commit.",
      "worth_posting": "no",
      "evidence": "The sentence about checks reporting on Accepted follows the first-commit sentences in the same paragraph but reads as a separate statement about the PR, not a claim about that commit."
    },
    {
      "id": "F20",
      "title": "Test fixtures for observed managed resources built two different ways across the six cloud functions",
      "true": "yes",
      "clears_bar": false,
      "fails": "Author would want to change it: the functions are self-contained packages, so 'share one helper' isn't possible; aligning shapes is low value.",
      "worth_posting": "no",
      "evidence": "AKS/Civo/Nebius/Vultr add _observed_managed_resource(ready, failure); EKS/GKE add _observed_resource + _observed_failed. Inconsistent, but test-only and per-package."
    },
    {
      "id": "F21",
      "title": "Accepted=True reported before node pool checks run while provider activation is pending (and ClusterReady Provisioning with no message)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "compose_<cloud> skips compose_<cloud>_cluster (where InvalidNodePool checks run) until activation is ready, yet still calls derive_conditions, which sets Accepted=True; it then flips False once activation completes. Real and introduced here, but only in a short first-activation window with an invalid pool."
    },
    {
      "id": "F22",
      "title": "_first should be named for what it returns (a summary message)",
      "true": "partly",
      "clears_bar": false,
      "fails": "Author would want to change it: the docstring ('The first failure, noting how many more there are') makes the name adequate.",
      "worth_posting": "no",
      "evidence": "_first returns the first failure plus an '(and N more)' suffix; the name is defensible given the docstring."
    }
  ]
}

judge-619.json

{
  "pr": 619,
  "findings": [
    {
      "id": "F1",
      "title": "kubectl status.endpoints snippet yields empty ANTHROPIC_BASE_URL for the guide's own TLS gateway",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "write_status in compose-inference-gateway/fn.py sets status.endpoints only when `address and not self.xr.spec.tls`, and the guide's `public` gateway sets spec.tls, so the snippet exports an empty base URL. The next paragraph does correct it, so this is a reordering fix rather than a blocker."
    },
    {
      "id": "F2",
      "title": "Pointer to Messages API guide's port-forward doesn't work for a TLS gateway (:80 redirects, :443 cert mismatch)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The Messages API guide forwards 8080:80 and uses http://localhost:8080/anthropic. With spec.tls set, compose-model-route binds routes to the https listener (fn.py:394) and compose_redirect 301s everything on :80 to HTTPS, so 'works the same way' is false."
    },
    {
      "id": "F3",
      "title": "Distributing the client block via managed settings gives everyone the same per-person key/caller",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The page tells readers to distribute a block containing ANTHROPIC_AUTH_TOKEN=sk-modelplane-dennis-... through managed settings. Anthropic's rollout guide puts only shared values in the managed env block and gets each person's key from apiKeyHelper or their own secrets process, so as written every record would be attributed to one caller."
    },
    {
      "id": "F4",
      "title": "Page shows shell exports, not the managed-settings JSON env block #597 asks for and the PR description/commit promise",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Both the commit body and the PR description say the client settings are given 'as an env block for managed settings', and #597 item 2 asks for one, but the page has only bash exports. This belongs in one summary sentence, and it overlaps F3's fix."
    },
    {
      "id": "F5",
      "title": "Wrong reason for naming services after the Claude ID: Claude Code checks the model ID it sends, not the response",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Anthropic's errors page says unrecognized_model fires on the model ID Claude Code sends, 'the model string as you configured it'. The endpoint rewrites the upstream model to claude-opus-5-5 (ModelEndpoint XRD), so the response would name the Claude ID whatever the service is called. The page's stated mechanism is false in both the prose and model-service.yaml."
    },
    {
      "id": "F6",
      "title": "Recommending gateway 429 retries for a single shared key is wrong (same key, no backoff/retry-after, multiplies with Claude Code retries)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The BackendTrafficPolicy in fn.py retries 429/503 up to 3 times and sets no backoff or retry-after handling. With one endpoint each retry goes back to the same key, and Anthropic documents that Claude Code itself retries 429s up to 10 times. The author's own draft #633 says 'a retried 429 spends more of a person's limit', so the 'usually what you want' endorsement contradicts his own view."
    },
    {
      "id": "F7",
      "title": "Passive health check can eject the only endpoint after five 5xx (e.g. 529), failing requests; page should mention",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "fn.py sets consecutive5XxErrors 5, maxEjectionPercent 100, baseEjectionTime 30s and panicThreshold 0, so with one external endpoint five 5xx responses (529 included) eject it and the service refuses requests for 30s. This is true, but it is pre-existing gateway behaviour that the author already flags in #633. Adding it to the doc is borderline a request to do more."
    },
    {
      "id": "F8",
      "title": "Example usage record doesn't match what the gateway logs (endpoint name/namespace, missing response_model, types)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "_USAGE_RECORD logs endpoint as the AIServiceBackend in the mirrored mp-<ns>-<hash> namespace named child_name(route, endpoint), and it includes response_model. e2e/tests/test_metering.py and design/fleet-gateway.md show the counts and status as JSON integers. The example's `anthropic/modelroute-...` and string counts are wrong, and readers will build queries from it."
    },
    {
      "id": "F9",
      "title": "input_tokens includes cache writes as well as reads; page should say so",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "In AI Gateway v1.1.0 (the version pinned in compose-serving-stack common.py), ExtractTokenUsageFromExplicitCaching sums input + cache_creation + cache_read. The page mentions only reads, while #597 explicitly asked for 'reads and writes', which matters for pricing."
    },
    {
      "id": "F10",
      "title": "Claim that a mixed service serves both OpenAI and Anthropic clients contradicts the XRD (OpenAI requests to Anthropic endpoint fail)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The ModelEndpoint XRD's api.schema description and model-service.md 'Alternate APIs' both say an OpenAI request routed to an Anthropic endpoint fails and warn against mixing. The page's 'serves both client shapes on one name' contradicts that."
    },
    {
      "id": "F11",
      "title": "Page doesn't say a Claude ID with no service (via /model, new releases, or background built-in IDs) gets a 404",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Anthropic's rollout page says 'some background sub-calls request a built-in ID regardless of the override', and routes match only the exact <namespace>/<service> (#596 notes the 404). A reader following this guide will hit it, and the page doesn't warn them outside the subscriptions section."
    },
    {
      "id": "F12",
      "title": "Unused modelplane.ai/external-provider label on the endpoints",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "The services select only on modelplane.ai/claude-model, and nothing in the guide uses external-provider. It looks carried over from concepts/model-endpoint.yaml, where it is the selector. True, but a low-stakes nit."
    },
    {
      "id": "F13",
      "title": "1800s timeout reasoning (and other manifest-comment explanations) repeated in several places",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "The 300s/1800s reasoning appears in the 'One service per model' prose, the model-service.yaml header comment (rendered on the page) and 'Timeouts and retries'. The schema and auth explanations also repeat between prose and manifest comments. This is a valid style nit."
    },
    {
      "id": "F14",
      "title": "'Setting the request timeout to 1800s here...' is a sentence fragment with no main verb",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Line 66 reads 'Setting the request timeout to 1800s here, over the 300s default, because ...', which has no finite verb. It is a cheap nit that Vale doesn't catch, and is best folded into F13's comment."
    },
    {
      "id": "F15",
      "title": "Proxy drain timeout fixed at 300s cuts turns the 1800s request timeout allows during a rollout",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "_DRAIN_TIMEOUT = \"300s\" in fn.py is commented as allowing a response to finish 'unless its service allows longer', so a proxy rollout cuts 1800s turns. The behaviour is pre-existing, and the author already has draft PR #625 for it. A one-line caveat next to the 1800s advice is reasonable but optional."
    },
    {
      "id": "F16",
      "title": "Guide references cluster gw-gcp-us it never creates, and doesn't say where the TLS Secret goes",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change it (weakly); not blocking as claimed",
      "worth_posting": "marginal",
      "evidence": "gw-gcp-us appears nowhere else and the page doesn't create it, unlike the Messages API guide, which ships inference-cluster.yaml. However, the manifest comment says it is the InferenceCluster the gateway runs on, and the InferenceGateway XRD documents where certificateRefs Secrets live. That makes it a prerequisite note at most, not a blocker."
    },
    {
      "id": "F17",
      "title": "ANTHROPIC_API_KEY doesn't change the background model when ANTHROPIC_DEFAULT_HAIKU_MODEL is pinned",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "Anthropic's llm-gateway-protocol table scopes the background-model change to 'unless ANTHROPIC_DEFAULT_HAIKU_MODEL pins one', and the page pins it. The clause is accurate in general but not for this setup, so this is a small nit."
    },
    {
      "id": "F18",
      "title": "Closing sentence promises future content and will go stale",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change it",
      "worth_posting": "no",
      "evidence": "'This page will cover it once that lands' mirrors #597's own plan ('the same page can cover people's own subscriptions'). It is harmless and intended, so the issue is taste, not correctness."
    },
    {
      "id": "F19",
      "title": "docs-vale reports 13 errors on the new page (PR description says Vale passes)",
      "true": "yes",
      "clears_bar": false,
      "fails": "CI already enforces it (skill: don't spend comments on Vale)",
      "worth_posting": "no",
      "evidence": "Running checks.aarch64-linux.docs-vale on the PR tree fails with 13 errors, all in claude-code-gateway.md (ai-tells ConsequenceClause/Figurative* and Google.Units). The description's 'Vale passes' claim is therefore false, but the flake check will report it once CI runs."
    },
    {
      "id": "F20",
      "title": "Commit trailer uses 'Part of #597.' instead of CONTRIBUTING's 'Towards #597.'",
      "true": "yes",
      "clears_bar": false,
      "fails": "author would want to change it",
      "worth_posting": "no",
      "evidence": "CONTRIBUTING lists `Towards #92.` as an example trailer, and the last 300 commits on main have 20 'Towards #' and 0 'Part of #'. The deviation is real but trivial, not worth a review note."
    },
    {
      "id": "F21",
      "title": "'Generated with Claude Code' AI attribution line should be removed",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change it",
      "worth_posting": "no",
      "evidence": "The PR description does end with that line, and the repo's writing-style skill says not to add AI attribution. But 14 of the last 60 merged PRs carry the same line and 117 recent commits on main have Co-Authored-By: Claude, so maintainers evidently accept it."
    },
    {
      "id": "F22",
      "title": "Draft #633 removes one of the subscription blockers the last section names; section will need an edit",
      "true": "yes",
      "clears_bar": false,
      "fails": "this PR didn't introduce it",
      "worth_posting": "no",
      "evidence": "Draft #633 adds spec.auth.apiKey.header, which addresses the first blocker. It is a heads-up about a future, unmerged PR, and the page already says it will be updated when #596 lands."
    }
  ]
}

judge-625.json

{
  "pr": 625,
  "findings": [
    {
      "id": "F1",
      "title": "terminationGracePeriodSeconds patch is redundant: Envoy Gateway v1.8.4 already sets it to drainTimeout + 300s, so the docstring's 'inert' claim is false; just pass drainTimeout through",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Envoy Gateway v1.8.4 (the version compose-serving-stack pins) sets the pod's grace period to drainTimeout + 300s in expectedTerminationGracePeriodSeconds (internal/infrastructure/kubernetes/proxy/resource_provider.go:623-633). So the PR's patch writes the same value EG already sets, and the docstring's claim that without it a raised drainTimeout would be inert is false. The fix removes code (_seconds, _GRACE_MARGIN, the deepcopy)."
    },
    {
      "id": "F2",
      "title": "_seconds regex matches 'm' before 'ms', so 500ms parses as 500 minutes",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "re.findall(r\"([0-9]+)(h|m|s|ms)\", \"500ms\") returns [('500','m')], and the XRD pattern admits ms. It's a real bug, but a millisecond drain timeout is unlikely, a too-large grace period is only an upper bound, and F1's fix deletes _seconds. It's best raised inside F1, as runs 1-4 did."
    },
    {
      "id": "F3",
      "title": "maxSurge/maxUnavailable are composed flat under envoyDeployment.strategy instead of strategy.rollingUpdate, so they're rejected or pruned; the test expects the same wrong layout",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "fn.py puts maxSurge and maxUnavailable directly under strategy, and the ProxyDrainAndRollout test expects {type, maxSurge, maxUnavailable}. The EnvoyProxy CRD types strategy as appsv1.DeploymentStrategy, with maxSurge and maxUnavailable only under rollingUpdate and no preserve-unknown-fields. So the fields never reach the Deployment. I didn't check whether they get pruned with a warning or rejected outright, but neither outcome applies the setting."
    },
    {
      "id": "F4",
      "title": "maxSurge/maxUnavailable typed as string: Deployment rejects non-percent strings like \"1\", and the reference manifest's unquoted integers fail the docs-manifests check; use x-kubernetes-int-or-string",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "Kubernetes' ValidatePositiveIntOrPercent rejects a String-typed IntOrString that isn't a percent (apps/validation.go:486-499), so a string \"1\" breaks the Deployment once F3 is fixed. The generated Proxy model is constr, and pydantic 2.12 rejects int 1 ('Input should be a valid string'), so the reference manifest's `maxSurge: 1` fails validate_manifests.py. The API server would also reject that docs example."
    },
    {
      "id": "F5",
      "title": "maxSurge and maxUnavailable can both be 0, which Kubernetes rejects; add a CEL rule",
      "true": "yes",
      "clears_bar": false,
      "fails": "author would want to change: a guard for a hypothetical misconfiguration, and moot if F6 drops the fields",
      "worth_posting": "marginal",
      "evidence": "ValidateRollingUpdateDeployment rejects maxSurge 0 together with maxUnavailable 0 ('may not be 0 when maxSurge is 0'). The rule would sit at admission, the right place for it, but nobody has hit this. It adds a guard for a nonsense input to fields F6 argues should go."
    },
    {
      "id": "F6",
      "title": "Rollout fields add nothing at the fixed 2 replicas: Kubernetes' 25%/25% default already rounds to surge 1, unavailable 0; drop them or justify",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "_PROXY_REPLICAS is fixed at 2, and EG's default strategy is a bare RollingUpdate (api/v1alpha1/kubernetes_helpers.go:21), so Kubernetes' 25%/25% applies. With 2 replicas that's ceil(0.5)=1 surge and floor(0.5)=0 unavailable, which is exactly the PR's headline example. The docs imply the default adds unavailability, which is wrong. Dropping the fields removes code and makes F3, F4, F5 and F9 moot."
    },
    {
      "id": "F7",
      "title": "drainTimeout defaults to a fixed 300s instead of #618's 'longest route timeout the gateway serves', so raising a ModelService timeout brings the cut back",
      "true": "partly",
      "clears_bar": false,
      "fails": "this PR introduced it: the 300s cut is pre-existing, and the PR's description states the 300s default deliberately",
      "worth_posting": "marginal",
      "evidence": "#618 does ask for a derived default, but it also says 'defaulting to today's values'. A derived default has a real cost: drainTimeout goes into the pod's preStop command (EG resource.go:287-292), so every ModelService timeout change would roll the proxy, which is the very cut it tries to avoid. A question about why the PR departs from #618 is fair. Asking it to derive the default is not."
    },
    {
      "id": "F8",
      "title": "Cluster gateway's drain timeout (and rollout) isn't covered by this PR, #554 or #626; decide where it lives / track it",
      "true": "partly",
      "clears_bar": false,
      "fails": "requests to do more; also the gap stays tracked because the PR is only 'Part of #618' and #618 lists the cluster gateway",
      "worth_posting": "no",
      "evidence": "#554 and draft PR #626 cover replicas, PDB and spread, not drain, so the commit's '#554 tracks it' is inaccurate. But #618 stays open with the cluster-gateway bullet, so the gap is tracked. Asking this PR to decide the other proxy's API is scope creep."
    },
    {
      "id": "F9",
      "title": "Rollout fields should mirror upstream DeploymentStrategy as proxy.strategy.rollingUpdate.{maxSurge,maxUnavailable}, and the layout should be settled now since #554 will reuse it",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change: an API-shape preference for an abstraction, moot if F6 drops the fields; the premise that #554 will reuse it is unsupported",
      "worth_posting": "no",
      "evidence": "Upstream does nest these under strategy.rollingUpdate. But Modelplane's XRD is an abstraction and isn't obliged to mirror it, and #626 (the #554 PR) adds no rollout fields."
    },
    {
      "id": "F10",
      "title": "proxy should take default: {} so the 300s default applies without writing proxy:, removing the duplicate _DRAIN_TIMEOUT fallback in the function",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change: behaviour is correct either way; low value",
      "worth_posting": "marginal",
      "evidence": "modelservices' timeouts uses default: {}, so the pattern exists. But compose-model-service still keeps an `or v1alpha1.Timeouts()` fallback, because the generated type stays Optional. So default: {} wouldn't remove the function's fallback as claimed. At most it's a nit about surfacing the default in the stored object."
    },
    {
      "id": "F11",
      "title": "docs/content/models/model-service.md still says in-flight requests get five minutes on gateway restart; should point at spec.proxy.drainTimeout",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "model-service.md:187 says 'A restarting gateway lets requests already in flight run for five minutes', which this PR makes a default only. That page is where readers set long timeouts, so the link closes the loop the PR exists for. The change is cheap."
    },
    {
      "id": "F12",
      "title": "Docs should warn that changing drainTimeout itself rolls the proxy, and the first rollout still drains on the old 300s",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "EG puts --drain-timeout in the preStop command and --ready-timeout in the shutdown-manager args (resource.go:268-292), so a change rolls the pods. The old pods terminate with their old 300s preStop, so poc-2's adoption still needs one maintenance window. It's useful for operators but is a docs addition rather than a defect."
    },
    {
      "id": "F13",
      "title": "Commit lacks Signed-off-by and issue trailer; PR description should open with 'Part of #618'",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The commit has only Co-Authored-By, with no Signed-off-by and no Towards/Fixes trailer. The PR body puts 'Part of #618' last. CONTRIBUTING.md:157-186 requires all three. The skill allows one closing summary sentence for this."
    },
    {
      "id": "F14",
      "title": "Commit message falsely credits the grace-period patch with keeping the pod alive through draining",
      "true": "yes",
      "clears_bar": false,
      "fails": "duplicate of F1; the message changes when F1 is addressed",
      "worth_posting": "no",
      "evidence": "The commit says the grace period is set 'so the API server doesn't kill the pod before Envoy finishes draining', which is the same false premise F1 covers, since EG already derives it. A separate note adds nothing."
    },
    {
      "id": "F15",
      "title": "PR description says nix run .#build and nix flake check weren't run",
      "true": "yes",
      "clears_bar": false,
      "fails": "CI already enforces it (nix flake check, incl. docs-manifests); the concrete failure is F4",
      "worth_posting": "no",
      "evidence": "The PR body does say the nix checks weren't run, and it's a draft. CI runs them, and the real consequence (the docs-manifests failure) is already raised as F4."
    }
  ]
}

judge-632.json

{
  "pr": 632,
  "findings": [
    {
      "id": "F1",
      "title": "Any ModelDeployment author can mount any hostPath on shared nodes; node paths belong to platform teams and nothing restricts it",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The volumes item schema accepts hostPath as a preserve-unknown-fields object, so {path: /} passes; containers can't otherwise set securityContext, so this is new node access, and base.py's modelexpress_security_context confirms Modelplane sets no Pod Security labels. design.md gives 'what's available' to platform teams, and the author's later description drops hostPath for this reason."
    },
    {
      "id": "F2",
      "title": "hostPath ties a deployment to a node layout and is invisible to placement; use emptyDir plus #630's device selection instead",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "A hostPath with type: Directory fails to mount on a node without /mnt/nvme, leaving the pod in ContainerCreating with nothing at admission, and #630's description says nodeadm moves kubelet state onto the striped NVMe, so a disk-backed emptyDir lands there. True, but it's the alternative half of F1 and is best folded into that comment rather than posted separately."
    },
    {
      "id": "F3",
      "title": "Documented hostPath + init-container weight staging gets ahead of #630/#72/ModelCache abstractions; escape hatch vs Modelplane-owned storage",
      "true": "partly",
      "clears_bar": false,
      "fails": "author would want to change it: a direction question for maintainers that overlaps F1. #631 already frames the pass-through as a stepping stone toward #72, so it's close to asking for a different architecture",
      "worth_posting": "marginal",
      "evidence": "#630's question 4 does say 'ModelDeployment has no volumes or volumeMounts, so a KVOffloadTier or ModelCache would add the mount', and the docs do present hostPath plus a staging init container as the pattern. But the same author wrote #630, #631 and this PR, and #631 states the intent explicitly. The actionable part, dropping hostPath from the docs, is F1."
    },
    {
      "id": "F4",
      "title": "Volume source schema: fixed allowlist of 11 sources but each is free-form inside, so typos or hostPath: {} pass admission",
      "true": "yes",
      "clears_bar": false,
      "fails": "author would want to change it: the commit message deliberately makes each source free-form and leaves validation to the workload cluster, and typing 11 sources adds a lot of schema for a hypothetical typo",
      "worth_posting": "no",
      "evidence": "Every source is type: object with x-kubernetes-preserve-unknown-fields, and the CEL only checks has(), so hostPath: {} passes. The commit message states this trade-off ('any field ... passes through, and the workload cluster validates it'), and the later description keeps it."
    },
    {
      "id": "F5",
      "title": "PVC/configMap/secret/csi volumes need their objects or drivers on every target InferenceCluster; document that contract",
      "true": "yes",
      "clears_bar": false,
      "fails": "author would want to change it: defensive documentation of a contract that already applies to secret refs (and isn't in the user docs for those either), so the gap predates this PR",
      "worth_posting": "no",
      "evidence": "design.md already says secret references 'must exist on every InferenceCluster the deployment may target'. The user docs don't say this for existing secret refs either, so the gap isn't new, and it's ordinary Kubernetes behaviour for a referenced object."
    },
    {
      "id": "F6",
      "title": "design/design.md (and user-authored-routing.md) out of date: template contents don't list volumes, init containers, reserved names/paths",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "design.md lines 452-456 list only the engine's image, command, args, env, ports and imagePullSecrets, and user-authored-routing.md:516 says 'Containers can't mount volumes.', which this PR makes untrue. The skill asks for a comment on the change that makes a doc untrue."
    },
    {
      "id": "F7",
      "title": "'Passes them through as written' / 'written as in a Kubernetes pod spec' is inaccurate: init containers and mounts are a pruned subset",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The initContainers items define only name, image, command, args, env, resources and volumeMounts, and mounts only name, mountPath, subPath and readOnly, with no preserve-unknown-fields, so securityContext, workingDir or mountPropagation are rejected under strict validation or pruned otherwise. This is the same failure #631 reports. Some runs overstate 'without an error', since kubectl's default strict field validation does error, but the docs claim is still false."
    },
    {
      "id": "F8",
      "title": "Mounts referencing undeclared volumes (typos, model-cache without modelCacheRef) pass admission and fail only on the workload cluster",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The only new CEL rules are the reserved-names rule, the engine mountPath rule and the init/container name clash rule. Nothing ties a mount's name to volumes, dshm or model-cache, and pod_volumes adds model-cache only when cache_mounts returns it, so a model-cache mount without modelCacheRef reaches the workload cluster and fails there. modelCacheRef sits at spec.template.spec, above the members, so a CEL rule can see both, and this is the kind of admission rule the skill prefers."
    },
    {
      "id": "F9",
      "title": "Python models edited by hand; regenerate with nix run .#build (missing docstring, stale fs://apis hash in .lock.json)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The commit message says the models were hand-edited. schemas/.lock.json isn't in the diff even though apis/ changed, and the ModelReplica InitContainer.resources (v1alpha1.py:181) lacks the 'CPU and memory requests and limits.' docstring that the XRD (definition.yaml:705) and the ModelDeployment model carry. I didn't run the generator myself, but four runs report the same output."
    },
    {
      "id": "F10",
      "title": "Commit message says models were edited by hand; drop that note from history",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "The commit body says 'The Python models are edited by hand to match the XRDs, since the generator runs through nix', which CONTRIBUTING calls development story rather than what the code does. It goes away once F9 is fixed, so it's worth only the one closing summary sentence the skill allows."
    },
    {
      "id": "F11",
      "title": "Reserved mountPath rule compares exact strings, so a trailing slash (/dev/shm/, /mnt/models/) bypasses it and shadows Modelplane's mount",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The CEL uses `m.mountPath in ['/dev/shm', '/mnt/models']`, and Kubernetes' duplicate-mountPath check is an exact-string set (validation.go v1.34.0 lines 3032-3048), so /dev/shm/ passes both. The shadowing at runtime rests on two runs' kind experiments, which I didn't repeat. It breaks the invariant the PR's base.py comment and docs assert."
    },
    {
      "id": "F12",
      "title": "Init container field descriptions copied from the engine container don't apply (VLLM_HOST_IP env example, engine mount restriction, resources message)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "yes",
      "evidence": "The initContainers env description cites VLLM_HOST_IP, the init container volumeMounts description repeats 'The engine container can't mount at /dev/shm or /mnt/models', and the resources CEL message talks about a member's device requests, all copied from the engine container. These are user-facing schema descriptions that say something misleading about the field. Nit-level."
    },
    {
      "id": "F13",
      "title": "Docs example starts at a bare template:, which readers may put under spec.template.spec where volumes don't exist",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "The page opens by describing spec.template and spec.template.spec, and every earlier example starts at engines:, so the new example's bare `template:` is ambiguous between the two levels. A real but small readability nit."
    },
    {
      "id": "F14",
      "title": "Add a compose-model-deployment test that copying the template into the ModelReplica keeps volumes, mounts and init containers",
      "true": "yes",
      "clears_bar": false,
      "fails": "author would want to change it: a test request for a path that works today. Pydantic's default extra='ignore' makes it plausible, but no failure is observed",
      "worth_posting": "marginal",
      "evidence": "fn.py:503 copies the template with mrv1alpha1.Template.model_validate(member.template.model_dump(...)), and pydantic ignores unknown fields by default, so a missing model field would drop silently. The PR updates both models, so nothing is broken now, and the existing sidecar test is the precedent to follow."
    },
    {
      "id": "F15",
      "title": "template_pod_fields helper bundles init containers and pull secrets and is poorly named; split into init_containers(member)",
      "true": "yes",
      "clears_bar": true,
      "fails": "",
      "worth_posting": "marginal",
      "evidence": "base.template_pod_fields returns a dict of imagePullSecrets and initContainers that callers spread with **, unlike sidecar_containers, which returns a list. It's a legitimate factoring nit, but small."
    },
    {
      "id": "F16",
      "title": "Hardcoded dshm/model-cache reservations conflict with #574/#281 per-cache volume names and paths; decide who owns collision checks",
      "true": "partly",
      "clears_bar": false,
      "fails": "it can happen: #574 keeps the model-cache name and /mnt/models path, so the conflict is only with unscheduled multi-cache future work (#281)",
      "worth_posting": "marginal",
      "evidence": "#574's design doc does say multiple caches 'would need its own volume name and its own path', but #574 itself publishes `name: model-cache` (_MOUNT_VOLUME = 'model-cache'), so these rules hold after it merges. The modelplane- prefix idea is a cheap hedge, but the finding rests on a hypothetical feature."
    },
    {
      "id": "F17",
      "title": "Docs make dshm and model-cache public API by inviting users to mount them by name; renaming later breaks deployments",
      "true": "partly",
      "clears_bar": false,
      "fails": "backward compatibility: the skill says not to raise future-rename breakage pre-1.0, and mounting model-cache by name is the point of #631's staging use case",
      "worth_posting": "no",
      "evidence": "The docs do invite mounting dshm and model-cache by name. But the harm is breakage if a later release renames them, which is a compatibility concern the skill excludes, and #631 needs the init container to name the cache volume."
    },
    {
      "id": "F18",
      "title": "PR description and commit disagree: description excludes hostPath/nfs/image and promises name rules (modelplane- prefix, mount-name check) the commit lacks",
      "true": "partly",
      "clears_bar": false,
      "fails": "it can happen / introduced by this PR: the mismatch is an artifact of judging the first commit against a description the author rewrote for later commits",
      "worth_posting": "no",
      "evidence": "The PR's description edit history shows the original 00:14 description matched f688619 (hostPath example, only reserved-name rules). It was rewritten at 01:18-01:30 alongside commits 3f63026 and c3f54f7. At the time this commit was current, the two agreed."
    },
    {
      "id": "F19",
      "title": "If hostPath goes, disk-backed emptyDir only lands on NVMe on EKS after #630 and restages on every pod replacement; docs should say what other clusters get",
      "true": "partly",
      "clears_bar": false,
      "fails": "introduced by this PR: it critiques a design (no hostPath) that this commit doesn't implement, and it asks for more docs about a hypothetical",
      "worth_posting": "no",
      "evidence": "At f688619 hostPath is allowed and the docs example uses it, so there's no emptyDir-only contract to document. The point only applies to the later description's design, and #630 is an unmerged draft."
    }
  ]
}

judge-prompt.md

You're judging the findings an AI code review bot raised on Modelplane PR #{PR}, to learn which of them deserved to be posted. The bot reviewed the PR several times. {DIR}/clusters-{PR}.json lists the distinct findings those reviews raised. Each finding's comments points to the comments that raised it, under {DIR}/review-{PR}-/x/review/. Where a finding lists no comment file of its own, its notes say which comment raised it.

The PR is checked out at {CHECKOUT}, at commit {SHA}. Its base is origin/main. Use git -C {CHECKOUT} merge-base HEAD origin/main to find where it branched, then diff against that to see what the PR changed. Read the PR and its linked issues with gh pr view {PR} -R modelplaneai/modelplane and gh issue view <n> -R modelplaneai/modelplane, but don't read the PR's review threads or comments: judge each finding yourself.

The bot posts a finding only if it clears the bar in {CLONE}/.agents/skills/reviewing-pull-requests/SKILL.md, with examples in its references/calibration.md. Read both first. In short, a finding must be true and verified, introduced by this PR, able to happen in a deployment someone would run, and something the author would want to change. The skill also lists things not to raise at all.

For each finding:

1. Check whether it's true. Read the code it's about, and the upstream source or docs for any claim about how another tool behaves. You can clone upstream repos under {DIR}/upstream, and run tests with nix run .#test -- <function> from the checkout if that's the only way to tell. For a design question, judge whether its premise is factually right.
2. Judge whether it clears the bar, and if not, which test it fails.
3. Say whether a maintainer would want it posted, given everything you found.

Write JSON to {CHECKOUT}.json:

{
  "pr": {PR},
  "findings": [
    {
      "id": "F1",
      "title": "copied from clusters",
      "true": "yes | partly | no | unverified",
      "clears_bar": true,
      "fails": "which test it fails, or empty",
      "worth_posting": "yes | marginal | no",
      "evidence": "one or two sentences: what you checked and what it showed"
    }
  ]
}

Then reply with a short table of finding, true, clears_bar and worth_posting, and note any finding you couldn't verify. Don't edit the checkout. Do scratch work under {DIR}/scratch-{PR}.

judged.py

#!/usr/bin/env python3
"""Summarise judged findings and where runs lost them.

Reads clusters-<PR>.json, judge-<PR>.json and trace-<PR>.json for each PR
given, and prints a markdown table of how many findings were wanted, the share
of those one run raised, and totals across PRs: how the findings only one run
raised were rated, and how often a run dropped or never found a finding.

A maintainer's rating in maintainer-ratings.json, if present, replaces the
judge's. Excludes the findings listed in EXCLUDE.

Usage: judged.py 625 619 632 607
"""

import collections
import json
import os
import sys

# A re-run reviewed #632's commit against a description rewritten for later
# commits. These two findings came from that mismatch.
EXCLUDE = {("632", "F18"), ("632", "F19")}

RATINGS = "maintainer-ratings.json"


def load(kind, pr):
    return {f["id"]: f for f in json.load(open(f"{kind}-{pr}.json"))["findings"]}


def share(findings, runs):
    if not findings:
        return 0
    return sum(
        sum(1 for f in findings if r in f["runs"]) / len(findings) for r in runs
    ) / len(runs)


ratings = json.load(open(RATINGS)) if os.path.exists(RATINGS) else {}

print(
    "| PR | Distinct findings | Wanted | Marginal | Unwanted "
    "| One run's share of wanted findings |"
)
print("|---|---|---|---|---|---|")
one_run = collections.Counter()
lost = collections.Counter()
for pr in sys.argv[1:]:
    clusters = json.load(open(f"clusters-{pr}.json"))
    runs = clusters["runs"]
    judge, trace = load("judge", pr), load("trace", pr)
    findings = [f for f in clusters["findings"] if (pr, f["id"]) not in EXCLUDE]

    def rating(f):
        return ratings.get(pr, {}).get(f["id"], judge[f["id"]]["worth_posting"])

    counts = collections.Counter(rating(f) for f in findings)
    wanted = [f for f in findings if rating(f) == "yes"]
    print(
        f"| #{pr} | {len(findings)} | {counts['yes']} | {counts['marginal']} "
        f"| {counts['no']} | {share(wanted, runs) * 100:.0f}% |"
    )
    for f in findings:
        if len(f["runs"]) == 1:
            one_run[rating(f)] += 1
        for status in trace[f["id"]]["runs"].values():
            lost[(status["status"], rating(f))] += 1

print()
print("Findings only one run raised:", dict(one_run))
for status in ("posted", "dropped", "missed"):
    print(
        f"Times a run {status} a finding:",
        {r: lost[(status, r)] for r in ("yes", "marginal", "no")},
    )

maintainer-ratings.json

{
  "625": {"F2": "marginal", "F12": "yes"},
  "619": {"F7": "yes", "F12": "yes", "F13": "yes", "F15": "yes", "F17": "yes"},
  "632": {"F2": "marginal", "F10": "marginal", "F13": "yes", "F15": "yes"},
  "607": {"F4": "yes", "F14": "yes", "F16": "yes", "F17": "yes", "F21": "yes"}
}

review-eval.yml

name: Review Eval

# Reviews the PRs listed in hack/review-eval/runs.json, several times each,
# without posting anything, to measure how much reviews of one PR vary. Each run
# reviews the whole PR, and none sees another's output or the review bot's.
#
# This runs in a maintainer's fork, and reviews PRs in the repo each run names.
# It needs only the model API key, not the reviewer's GitHub App. Each run's
# review and transcripts are an artifact named review-<pr>-<run>.
#
# The review job matches review.yml's, which explains the VM and its network.
on:
  push:
    branches: [take-five]

permissions: {}

jobs:
  # Every run of a PR gets the same context, fetched once here. A run re-run
  # later would otherwise see a description or threads the PR gained since.
  runs:
    runs-on: ubuntu-24.04
    permissions:
      contents: read
      pull-requests: read
      issues: read
    outputs:
      runs: ${{ steps.runs.outputs.runs }}
    steps:
      - name: Checkout
        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
        with:
          persist-credentials: false

      - name: Read the runs
        id: runs
        run: echo "runs=$(jq -c . hack/review-eval/runs.json)" >> "$GITHUB_OUTPUT"

      # The review bot's own reviews, and the replies to its comments, would
      # tell each run what an earlier review found, so they're left out.
      - name: Fetch each PR's context
        env:
          GH_TOKEN: ${{ github.token }}
          BOT: modelplane-review[bot]
        run: |
          jq -r '[.[] | "\(.repo) \(.pr)"] | unique[]' hack/review-eval/runs.json | while read -r repo pr; do
            dir="context/$pr"
            mkdir -p "$dir"
            gh pr view "$pr" --repo "$repo" \
              --json number,title,body,author,baseRefName,headRefOid,commits,closingIssuesReferences,files \
              > "$dir/pr.json"
            gh api --paginate "repos/$repo/pulls/$pr/reviews" \
              | jq -s --arg bot "$BOT" 'add | map(select(.user.login != $bot))' > "$dir/reviews.json"
            gh api --paginate "repos/$repo/pulls/$pr/comments" \
              | jq -s --arg bot "$BOT" 'add | (map(select(.user.login == $bot) | .id)) as $ids | map(select(.user.login != $bot and ((.in_reply_to_id // 0) as $r | $ids | index($r) | not)))' \
              > "$dir/review-comments.json"
            gh api --paginate "repos/$repo/issues/$pr/comments" \
              | jq -s --arg bot "$BOT" 'add | map(select(.user.login != $bot))' > "$dir/issue-comments.json"
          done

      - name: Upload the context
        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
        with:
          name: context
          path: context

  review:
    needs: runs
    strategy:
      fail-fast: false
      # Every run's reviewers share one API key and its rate limit.
      max-parallel: 10
      matrix:
        include: ${{ fromJSON(needs.runs.outputs.runs) }}
    runs-on: ubuntu-24.04
    timeout-minutes: 60
    permissions:
      contents: read
      pull-requests: read
      issues: read

    env:
      REPO: ${{ matrix.repo }}
      PR: ${{ matrix.pr }}
      SHA: ${{ matrix.sha }}
      CADDY_IMAGE: caddy@sha256:36c24ad91a7ff7bc4c87fe22bfa8df3c4b18da0773d73fc12ee2c52b98c0bb7e

    steps:
      - name: Checkout the reviewer
        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
        with:
          path: reviewer
          persist-credentials: false

      - name: Checkout the PR
        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
        with:
          repository: ${{ matrix.repo }}
          ref: ${{ matrix.sha }}
          path: pr
          fetch-depth: 0
          persist-credentials: false

      - name: Install the reviewer's config
        run: |
          for f in .opencode .agents .claude .vale .vale.ini opencode.json opencode.jsonc AGENTS.md CLAUDE.md CONTRIBUTING.md; do
            rm -rf "pr/$f"
            if [ -e "reviewer/$f" ]; then cp -r "reviewer/$f" "pr/$f"; fi
          done

      - name: Download the context
        uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
        with:
          name: context
          path: all-context

      - name: Pick the PR's context
        run: mv "all-context/$PR" context

      - name: Allow the runner user to use KVM
        run: |
          echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules
          sudo udevadm control --reload-rules
          sudo udevadm trigger --name-match=kvm
          if ! test -w /dev/kvm; then
            ls -l /dev/kvm || true
            echo "::error::This runner has no usable /dev/kvm, so it can't run the review VM. Re-run the failed jobs."
            exit 1
          fi

      - name: Install Nix
        uses: cachix/install-nix-action@v31

      - name: Create the VM's network
        run: |
          sudo ip tuntap add vm0 mode tap user "$USER"
          sudo ip addr add 192.168.100.1/30 dev vm0
          sudo ip link set vm0 up
          sudo sysctl -qw net.ipv4.ip_forward=1
          uplink=$(ip route show default | awk '{print $5; exit}')
          sudo iptables -t nat -A POSTROUTING -s 192.168.100.0/30 -o "$uplink" -j MASQUERADE
          sudo iptables -I FORWARD 1 -i vm0 -o "$uplink" -j ACCEPT
          sudo iptables -I FORWARD 1 -o vm0 -m conntrack --ctstate ESTABLISHED,RELATED -j ACCEPT
          for net in 10.0.0.0/8 172.16.0.0/12 192.168.0.0/16 100.64.0.0/10 169.254.0.0/16 168.63.129.16/32; do
            sudo iptables -I FORWARD 1 -i vm0 -d "$net" -j DROP
          done
          sudo iptables -I INPUT 1 -i vm0 -j DROP
          sudo iptables -I INPUT 1 -i vm0 -p tcp --dport 8787 -j ACCEPT
          sudo sysctl -qw net.ipv6.conf.vm0.disable_ipv6=1
          sudo ip6tables -I INPUT 1 -i vm0 -j DROP
          sudo ip6tables -I FORWARD 1 -i vm0 -j DROP

      - name: Start the model proxy
        env:
          MODEL_API_KEY: ${{ secrets.REVIEW_MODEL_API_KEY }}
          MODEL_API: ${{ vars.REVIEW_MODEL_API || 'anthropic' }}
          MODEL_URL: ${{ vars.REVIEW_MODEL_URL || 'https://api.anthropic.com/v1' }}
        run: |
          case "$MODEL_API" in
            anthropic)
              endpoint=/v1/messages
              auth='x-api-key {env.MODEL_API_KEY}'
              ;;
            openai)
              endpoint=/v1/chat/completions
              auth='Authorization "Bearer {env.MODEL_API_KEY}"'
              ;;
            *)
              echo "::error::REVIEW_MODEL_API must be anthropic or openai, not $MODEL_API."
              exit 1
              ;;
          esac
          origin=$(printf '%s' "$MODEL_URL" | sed -E 's#^(https?://[^/]+).*#\1#')
          host=${origin#*://}
          prefix=${MODEL_URL#"$origin"}
          prefix=${prefix%/}
          cat > Caddyfile <<EOF
          {
            admin off
          }
          http://192.168.100.1:8787 {
            bind 192.168.100.1
            @model {
              method POST
              path $endpoint
            }
            route @model {
              uri strip_prefix /v1
              rewrite * $prefix{uri}
              reverse_proxy $origin {
                header_up Host $host
                header_up $auth
                flush_interval -1
              }
            }
            respond 403
          }
          EOF
          docker run -d --name proxy --network host \
            -e MODEL_API_KEY \
            -v "$PWD/Caddyfile:/etc/caddy/Caddyfile:ro" \
            "$CADDY_IMAGE"

      - name: Build the VM
        run: nix build ./reviewer/hack/review-vm#nixosConfigurations.agent.config.microvm.declaredRunner -o vm

      - name: Prepare the VM's disks
        env:
          GH_TOKEN: ${{ github.token }}
          MODEL_API: ${{ vars.REVIEW_MODEL_API || 'anthropic' }}
          MODEL: ${{ vars.REVIEW_MODEL || 'claude-opus-5-5' }}
        run: |
          prompt="Review PR #$PR in $REPO at commit $SHA. The PR's context is in /work/context. Write the review to the directory _output/review."
          printf '%s' "$prompt" > prompt
          printf '%s' "$GH_TOKEN" > gh-token
          jq -n --arg api "$MODEL_API" --arg model "$MODEL" '{api: $api, model: $model}' > model.json

          sudo mkdir -p /mnt/vm
          sudo chown "$USER" /mnt/vm
          tar -cf /mnt/vm/in.tar pr context prompt gh-token model.json
          rm gh-token
          truncate -s 1G /mnt/vm/out.tar

      - name: Review
        working-directory: /mnt/vm
        run: timeout 50m "$GITHUB_WORKSPACE/vm/bin/microvm-run" < /dev/null

      - name: Upload the VM's output
        if: always()
        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
        with:
          name: review-${{ matrix.pr }}-${{ matrix.run }}
          path: /mnt/vm/out.tar
          if-no-files-found: ignore

reviews-607.md

Run 1

Summary

This looks close to mergeable. The stage model follows #603 and the committed designs, and what's left is a few questions about how the stages read in edge cases rather than about the approach.

The question with the most weight is what a fatal Accepted check reports for stages that are already running.

The PR description says a Ready cloud cluster XR that doesn't report the new conditions yet counts as having finished both stages, but _relay doesn't read Ready and no test covers that case. Could the description drop that sentence?

01: functions/compose-inference-cluster/function/fn.py:896

Will this show a running cluster's stages as Pending whenever a fatal check fails the reconcile? If a user adds a pool whose InferenceClass doesn't exist yet, nothing changes on the cluster, but all three stages read Pending. inference-cluster.md says a stage that has started reports its own state. Could we set Pending only for stages that haven't started? Or could we leave them out? Crossplane marks any condition a fatal result doesn't set as Unknown (reconciler.go).

02: functions/compose-inference-cluster/function/fn.py:1356

This reads the cloud cluster XR's status.nodePools as an untyped dict. Elsewhere this file reads the cloud XRs through their generated models, such as vultrv1alpha1.VultrCluster.model_validate(...) in observed_vultr_secrets. Could each compose_<cloud> read status.nodePools through its typed model and pass the list in? python-code-factoring prefers typed reads over .get() chains.

03: functions/compose-inference-cluster/function/fn.py:308

Nit: "Pending while the stage after waits on is False" doesn't parse, and the code tests != "True", so Unknown counts too. Something like "Pending while the stage it waits on isn't True" would match.

04: functions/compose-vultr-cluster/function/fn.py:423

Will this make one GPU pool's state depend on another's? gpu-observer is one Object shared by every GPU pool, and it's ready only when numberReady == desiredNumberScheduled on the validator DaemonSet, which spans all GPU nodes. Adding pool b to a cluster where pool a is serving would raise the DaemonSet's desired count, so a would go from Ready back to Provisioning while b's nodes come up. Could the pool states leave out gpu-observer, and NodePoolsReady count it as a whole? Or is that trade-off acceptable here?

05: docs/content/platform/inference-cluster.md:121

Is it deliberate that ServingStackReady never reports Failed? #603 is about a failure looking like slow provisioning, and a broken Helm release in the stack would still read Provisioning indefinitely. The ServingStack is Modelplane's own XR, so it could report a failure that this function relays, as the cloud cluster XRs now do. If that's out of scope for this PR, could the PR description say it's left for later?

06: docs/content/platform/inference-cluster.md:137

Nit: "the cloud provider" reads as AWS or GCP, but it's the Crossplane provider that sets LastAsyncOperation, and only on resources upjet creates asynchronously. Could this paragraph say "the Crossplane provider"? The function comment ("upjet sets this condition False ...") gets this right. The same phrase appears on line 133 and in build-the-platform.md.

07: apis/eksclusters/definition.yaml:259

Nit: this list leaves out every role: System entry in spec.nodePools (_stage_pools), but the description says it covers each pool in spec.nodePools. The system pool Modelplane adds isn't in spec.nodePools at all, so the second sentence doesn't explain the gap. Could it say System pools count toward ClusterReady? (Here and in the other five cloud XRDs.)

Run 2

Summary

I think this is close, with a few questions to settle first. The one I'd most want answered is what fail() reports on a cluster that's already running.

The stages, the relay from each cloud cluster XR and the ServingStackReady rename follow #603 and the rename asked for in its thread. The other questions cover where the serving stack belongs in the stage order, how resources shared between pools affect each pool's state, and keeping the stage contract consistent with #594, which adds an UpCloud cluster function.

The PR description says a Ready cloud cluster XR without the new conditions counts as having finished both stages, but _relay doesn't do that. Since AGENTS.md rules out compatibility handling, could that sentence come out of the description?

01: functions/compose-inference-cluster/function/fn.py:896

Won't this set every stage to Pending on a cluster that's up and serving? The docstring says fail() runs on existing clusters, such as after someone edits a node pool. Crossplane applies a fatal result's conditions. So adding a pool whose class doesn't exist yet would turn ClusterReady, NodePoolsReady and ServingStackReady to Pending while Ready stays True. That contradicts the docs, which say a stage that's started reports its own state.

Could we relay the observed cloud cluster XR's conditions here, or use Pending only before Crossplane observes the cloud cluster XR?

02: functions/compose-inference-cluster/function/fn.py:1392

Should the serving stack wait on the node pools? The stack still waits for the cloud cluster XR's Ready, which includes the pools, and this PR now reports that as "Waiting for NodePoolsReady". On a new cluster, one GPU pool stuck Failed on capacity, the case #603 opens with, keeps the stack from installing while the other pools are Ready. The cloud functions say the system pool runs the serving stack and belongs to ClusterReady.

Could the stack wait on the relayed ClusterReady instead, or is waiting on the pools deliberate?

03: functions/compose-inference-cluster/function/fn.py:315

How should this and #594 merge? #594 adds compose-upcloud-cluster and also changes this file. A cloud cluster XR that doesn't report ClusterReady reads Provisioning here indefinitely, and its pools read Pending. So whichever PR merges second needs the stage logic in the UpCloud function too. Could we note that on #594, or write down what a cloud cluster XR has to report?

04: functions/compose-vultr-cluster/function/fn.py:423

Won't the shared gpu-observer turn a Ready GPU pool back to Provisioning when someone adds another GPU pool? It waits for the nvidia-operator-validator DaemonSet across the whole cluster. The new pool's nodes hold it unready, and NodePoolsReady would read "0 of 2 node pools ready". Nebius shares its gpu-cluster-<fabric> between pools, so the same would happen there. Could the shared resource count toward the cluster, as EKS does with its EFA security group, or is this deliberate?

05: docs/content/platform/inference-cluster.md:138

Is an IAM role really excluded? upjet's DefaultResource sets UseAsync: true (common.go), and I couldn't find provider-upjet-aws overriding it for IAM. So a failed Role would set LastAsyncOperation=False and read Failed. Helm releases and provider-kubernetes Objects do stay Provisioning. Could the sentence name only those?

06: docs/content/platform/inference-cluster.md:119

Nit: ClusterReady and NodePoolsReady can also have the reason FatalError, which the table doesn't list. When the cloud cluster function fails, Crossplane marks the cloud cluster XR's conditions Unknown with that reason. The EKSClusterFunctionFailed test case in compose-inference-cluster shows this PR relaying it. Could the page say what it means?

07: functions/compose-inference-cluster/function/fn.py:79

Nit: "Pending on an earlier one only until it's composed" doesn't match either stage. ServingStackReady stays Pending until Crossplane observes the ServingStack, not until it's composed. The cloud functions hold NodePoolsReady Pending while no pool is Ready, even though the pools are already composed. Could this comment describe each stage's rule as their derive_conditions docstrings do?

08: functions/compose-inference-cluster/function/fn.py:290

Nit: only fail() needs a chain of conditions. The other two callers wrap one stage in a tuple and take [0]. A _pending(stage, waiting_on) that returns one condition would read more simply, with fail() building its chain itself. It would also stop after meaning a string here and a Condition in _relay.

Run 3

Summary

This looks close to mergeable. The stage design follows #603, and what's left is a few questions on how the stages read.

The ones that matter most are fail() marking a running cluster's stages Pending, ServingStackReady reading Provisioning before the stack exists, and which failures can reach Failed at all. The rest are docs and nits.

The PR description says a Ready cloud cluster XR that doesn't report the new conditions yet counts as having finished both stages, but _relay reports Provisioning until the cloud cluster XR reconciles. Dropping the sentence seems right, rather than adding the fallback, which AGENTS.md rules out. The description also credits the move of the reconcile checks to Accepted to the first commit, which only touches the ClusterProviderConfig.

01: functions/compose-inference-cluster/function/fn.py:896

Won't this mark a running cluster's later stages Pending too? If someone adds a pool with a typo'd className to a serving cluster, fail() sets ClusterReady, NodePoolsReady and ServingStackReady to Pending / "Waiting for Accepted", while the fatal result keeps every resource and Ready remains True. That seems to contradict the comment at L77-81 and the docs' "Once a stage has started, it reports its own state". Once the cloud cluster XR exists, could fail() relay its stages as derive_conditions does, or set only Accepted? I don't mind which.

02: functions/compose-inference-cluster/function/fn.py:1391

Can this report ServingStackReady=Provisioning before we compose the serving stack? The cloud functions let NodePoolsReady go True while ClusterReady is still False, once every pool is Ready. On EKS a cluster's only GPU node group can go Ready while the EFS CSI add-on or the autoscaler release is still coming up. The cloud cluster XR isn't Ready then, so we don't compose the stack, but pools_ready is True, so this returns Provisioning. Could this branch check the desired ServingStack instead of pools_ready?

03: functions/compose-eks-cluster/function/fn.py:1910

#603 asks for each stage to be True only once all the stages before it are. Here NodePoolsReady stops waiting on ClusterReady once any pool is Ready, so a running cluster can show ClusterReady=Provisioning beside NodePoolsReady=True. The derive_conditions docstring says this is so pools report their own state on a running cluster. Could #603 note that the strict order applies only to first provisioning, so the issue and the implementation agree? (Same in the other five cloud functions.)

04: functions/compose-eks-cluster/function/fn.py:350

Will a failure that only shows on Synced ever reach Failed? upjet sets LastAsyncOperation only from its async callback (api.go). A connect error, such as credentials.name naming a missing ProviderConfig, only sets Synced=False with ReconcileError (reconciler.go). That cluster would read Provisioning forever, the case #603 describes. Could a ReconcileError count as Failed too when it isn't about resolving a reference, or is that a follow-up?

05: docs/content/platform/inference-cluster.md:138

Is this right for IAM roles? upjet defaults UseAsync to true for every resource (common.go), and provider-upjet-aws doesn't turn it off for IAM, so I'd expect a failed IAM role to set LastAsyncOperation=False and report Failed. Helm releases and Objects read Provisioning because provider-helm and provider-kubernetes don't set LastAsyncOperation. Could this say that instead?

06: docs/content/getting-started/build-the-platform.md:263

A failed Helm release or in-cluster Object leaves its stage Provisioning, as the page this links to says, so this sentence promises more than the stages report. Could it say the stage reads Failed for errors on cloud resources, such as the control plane or a node group?

07: docs/content/platform/inference-cluster.md:119

Nit: ClusterReady and NodePoolsReady can also read Unknown with the reason FatalError, which Crossplane sets when the cloud cluster XR's function fails and this relays (the EKSClusterFunctionFailed test case covers it). A user would see FatalError in the CLUSTER or POOLS column, and nothing on this page explains it. Could the table or the text below it mention it?

08: apis/eksclusters/definition.yaml:259

Nit: _stage_pools drops every role: System pool in spec.nodePools, not only the system pool Modelplane adds, so "each node pool in spec.nodePools" overstates it. Could the description say that every System pool counts toward ClusterReady instead? (Here and in the other five cloud XRDs.)

09: functions/compose-inference-cluster/function/fn.py:1356

Nit: this reads the cloud cluster XR as a dict, while the same Composer validates each cloud cluster XR into its generated model to read secrets (see observed_eks_secrets and its siblings). Per the python-code-factoring skill, reads use typed models too. Could this validate the cloud cluster XR and read status.nodePools from it?

10: functions/compose-inference-cluster/function/fn.py:308

Nit: "Pending while the stage after waits on is False" is hard to parse, since after reads as an English word rather than the parameter. The code also checks after.status != "True", which includes Unknown. Something like "Pending while after isn't True" would read more plainly.

Run 4

Summary

I think this is close, with a few questions left about stages reading Pending on a cluster that's already running, and Provisioning before they've started.

The approach is what #603 asked for, including the ServingStackReady name, so none of the questions challenge the design.

The PR description says a Ready cloud cluster XR that doesn't report the new conditions yet counts as having finished both stages, but _relay never reads Ready. Since nothing needs migrating, I'd drop that sentence rather than add code that reads Ready.

01: functions/compose-inference-cluster/function/fn.py:896

Won't this report a running cluster's stages as Pending whenever an Accepted check fails? The checks run every reconcile, so a typo'd className on a new pool sets ClusterReady, NodePoolsReady and ServingStackReady to "Waiting for ...". The docs say a started stage reports its own state.

Could fail() set only Accepted? Crossplane would then set the stages fail() leaves out to Unknown (reconciler.go), and _relay already passes Unknown through from the cloud XRs.

02: functions/compose-inference-cluster/function/fn.py:1391

Can this report ServingStackReady=Provisioning before the function composes the serving stack? The cloud functions set NodePoolsReady=True once every pool is Ready, even while ClusterReady is still False, as when an EKS cluster's release-cluster-autoscaler or EFS add-on finishes. The function composes the stack only once the cloud XR is Ready, so in that window nothing is provisioning.

Could the Pending branch also wait on ClusterReady, or on the cloud XR's Ready, which is the gate the compose methods use?

03: functions/compose-eks-cluster/function/fn.py:350

Was any signal besides LastAsyncOperation considered for Failed? #603's second complaint is that a failure looks the same as slow provisioning. With only this condition, a failing Helm release (the cluster autoscaler, the network operator) or Object holds ClusterReady at Provisioning indefinitely. The docs acknowledge this.

As the comment above says, Synced=False alone is too noisy while references resolve. Could something narrower work, such as Synced=False for a reason other than reference resolution? (Here and in the other five cloud functions.)

04: docs/content/platform/inference-cluster.md:138

Is the IAM role example right? provider-upjet-aws builds Role with the async connector (zz_controller.go), so I'd expect a failed role to set LastAsyncOperation=False and report Failed. Could the example name only the Helm releases and Objects, which never set that condition?

05: docs/content/platform/inference-cluster.md:125

Does this match what NodePoolsReady does? EKS composes its node groups straight away, but the cloud functions keep NodePoolsReady Pending while ClusterReady isn't True and no pool is Ready. So while a new cluster provisions, the POOLS column reads Pending even though status.nodePools shows each pool Provisioning. The comment at compose-inference-cluster/function/fn.py:79, "Pending on an earlier one only until it's composed", has the same gap. Could both describe the rule in derive_conditions in the cloud functions?

06: functions/compose-vultr-cluster/function/fn.py:423

Won't adding a GPU pool flip every Ready GPU pool back to Provisioning? gpu-observer is one Object counted in every GPU pool, and its readiness needs numberReady == desiredNumberScheduled on the validator DaemonSet. When pool B's nodes join, desiredNumberScheduled rises before their validator pods are ready, so pool A reads Provisioning and NodePoolsReady reads "0 of 2". Is that acceptable for Vultr, or could a pool's readiness rest on its own node pool alone?

07: functions/compose-inference-cluster/function/fn.py:1356

This reads the cloud XR's status through .get() chains, where the rest of the file validates observed XRs with their generated models, as observed_nebius_secrets does. Please use a model here too, so a typo in a field name fails type checking. The method takes the resource key as a parameter, so it can't pick a model itself, but each compose_<cloud> could pass its model class or its validated status.nodePools.

08: functions/compose-inference-cluster/function/fn.py:290

Nit: two of the three callers wrap one stage in a tuple and take [0] from the result. Only fail() needs each stage to wait on the one before it. A helper returning one Condition, with fail() listing its stages, would read more simply.

09: functions/compose-aks-cluster/tests/test_fn.py:320

Nit: this PR builds the same fixture two ways. AKS, Civo, Nebius and Vultr add _observed_managed_resource(*, ready, failure), while EKS and GKE add _observed_resource(*, ready) and _observed_failed(*, message). Could all six share one helper?

Run 5

Summary

I think this is close to mergeable. The stage conditions and status.nodePools match #603 and the ServingStackReady rename. The main questions concern stages that misreport what's happening, such as on a running cluster that fails acceptance or a new one waiting on provider activation.

The PR description says a Ready cloud cluster XR that doesn't report the new conditions yet counts as having finished ClusterReady and NodePoolsReady, but _relay never reads Ready. A running cluster reads Provisioning until its cloud cluster XR next reconciles. Could the description drop that sentence rather than the code adding the fallback? AGENTS.md rules out backward compatibility.

01: functions/compose-inference-cluster/function/fn.py:896

Should a running cluster that fails acceptance report its later stages as Pending? Say someone adds a pool with a typo'd className to a serving cluster. Crossplane keeps the conditions a fatal result sets (reconciler.go), so all three later stages read Pending while the cluster keeps serving. The provisioning status section of inference-cluster.md says that once a stage has started, it reports its own state. Could fail() set only Accepted, or relay the observed stages as derive_conditions does?

02: functions/compose-inference-cluster/function/fn.py:1400

Won't this report Accepted=True before the function has checked the node pools? compose_<cloud>_cluster runs the InvalidNodePool and zone checks, and it doesn't run while provider activation is pending. A cluster whose class lacks a GKE block reads Accepted=True, then flips to False once activation is ready. ClusterReady also reads Provisioning with no message while no cloud cluster XR exists. Could the activation wait leave Accepted unset or False, and the later stages Pending, which would cover both?

03: functions/compose-inference-cluster/function/fn.py:1356

Could this read the cloud cluster XR through its typed model rather than .get() chains on a dict? _observed_cluster_cache_class already validates the observed cloud cluster XR with its model, and every cloud model now has status.nodePools. Per the python-code-factoring skill, reads should use the typed model as writes do.

04: functions/compose-inference-cluster/function/fn.py:290

Nit: two of _pending's three callers pass a one-element tuple and take [0], so only fail() uses the chaining. A helper that returns one condition, with fail() listing its three stages, would read more simply and drop _STAGES.

05: functions/compose-inference-cluster/function/fn.py:307

Nit: "or Pending while the stage after waits on is False" doesn't parse, and the code checks != "True", which includes Unknown. Something like "Pending while the stage before it isn't True" would say what the code does.

06: apis/eksclusters/definition.yaml:260

This says it reports "each node pool in spec.nodePools", but _stage_pools leaves out every role: System pool in spec.nodePools, and the spec requires at least one. The system pool Modelplane adds isn't in spec.nodePools at all. Could it say it covers the pools that aren't role: System? (Here and in the other five cloud XRDs.)

07: functions/compose-vultr-cluster/function/fn.py:420

Won't this mark every Ready GPU pool Provisioning whenever one GPU pool changes? There's one gpu-observer for the cluster, and it goes unready until the validator DaemonSet rolls out on every GPU node. After adding a second GPU pool, or the autoscaler adding a GPU node, NodePoolsReady would read "0 of N node pools ready". Could the observer count toward NodePoolsReady without belonging to each pool's status.nodePools entry?

08: functions/compose-eks-cluster/function/fn.py:353

Nit: _first builds a summary message ("X (and N more)") rather than returning the first failure. A name like _failure_message would say what it returns. (Here and in the other five cloud functions.)

reviews-619.md

Run 1

Summary

This looks close on approach: it documents a setup that works on the current API without changing it, and defers the subscription case to #596 as its issue asks. What stands between it and merging is accuracy. A few of the page's claims and copyable steps don't hold for the TLS gateway it builds.

I checked the gateway behaviour against compose-inference-gateway, compose-model-route and the XRDs on this commit. The auth, key stripping, header pass-through, route naming and timeout defaults it describes all match.

The client settings aren't yet the managed-settings env block that #597 asks for and the PR description promises, and the closing "Generated with Claude Code" line is AI attribution that the writing-style guide asks us to leave out.

01: docs/content/guides/claude-code-gateway.md:52

Does a mixed service serve both OpenAI and Anthropic clients? The ModelEndpoint XRD says "an Anthropic backend serves only Anthropic callers, and an OpenAI request routed to it fails" (definition.yaml). In a mixed service, an OpenAI request sent to the Anthropic endpoint fails outright rather than only losing a feature. Since the guide never builds a mixed service, could we cut this to the header pass-through sentence?

02: docs/content/guides/claude-code-gateway.md:63

Does Claude Code read the model ID from the response? The endpoint rewrites the request to claude-opus-5-5, and per the XRD the response gives the model that answered, so Claude Code gets claude-opus-5-5 back whatever the service name. #596's evidence (unrecognized_model with {"model":"anthropic/opus[1m]"}) points at the name Claude Code sends, from ANTHROPIC_DEFAULT_*_MODEL. The advice holds, but could we give the model name Claude Code sends as the reason, here and in the model-service.yaml header comment?

03: docs/content/guides/claude-code-gateway.md:77

Won't this set ANTHROPIC_BASE_URL to an empty string for the guide's own gateway? write_status publishes status.endpoints only when spec.tls is unset (fn.py), and the guide's public gateway sets it. Could we lead with https://<name>/anthropic and drop this snippet, or keep it only as the plain-HTTP case?

04: docs/content/guides/claude-code-gateway.md:102

Would distributing this block through managed settings give every person the same identity? Managed settings are one file for the organization, so pushing ANTHROPIC_AUTH_TOKEN=sk-modelplane-dennis-... makes every usage record say dennis. It's also shell rather than the JSON env block #597 asks for, and it leaves out ANTHROPIC_BASE_URL. Could the example be an env block holding the base URL and model variables, with each person's key coming from apiKeyHelper or their shell?

05: docs/content/guides/claude-code-gateway.md:108

Does the Messages API guide's port-forward work for this gateway? That one forwards 8080:80 to a plain-HTTP gateway. With spec.tls set, compose_redirect sends everything on :80 except /healthz to HTTPS (fn.py), and forwarding 443 to localhost won't match the certificate. Could we drop the sentence or say what changes under TLS?

06: docs/content/guides/claude-code-gateway.md:123

Is keeping the gateway's retry usually what you want with one shared key? The 429 retry exists so a full tier overflows to another endpoint (fn.py). With one endpoint, Envoy resends to the same key on its default backoff, ignoring retry-after. Anthropic's overload is a 529, which isn't retried, and with panicThreshold: 0, five consecutive 5xx eject the only endpoint, so requests fail until it returns. Could we describe the retry and ejection behaviour and drop the recommendation?

07: docs/content/guides/claude-code-gateway.md:137

Is this what endpoint looks like? The value is the route's AIServiceBackend in the mirrored namespace, named child_name(<route>, <endpoint>) (fn.py), so something like mp-anthropic-<hash>/claude-opus-5-5-public-..., as in design/fleet-gateway.md. The record also has a response_model field this leaves out. Could we match _USAGE_RECORD in that file?

08: docs/content/guides/claude-code-gateway.md:150

Does input_tokens also include cache writes? AI Gateway adds both cache-creation and cache-read tokens to input (metrics.go), which is what #597 asks the page to say. Since Anthropic prices the two differently and the record doesn't split them, could we say it counts both?

09: docs/content/guides/claude-code-gateway.md:92

Should the page say what happens to a Claude ID with no service? #596 notes it gets a 404, which a person hits by picking another model with /model, or when Anthropic releases a model before the platform team adds its endpoint and service. One sentence saying each new Claude model needs an endpoint, a service and a client setting until #596 lands would cover it.

10: docs/manifests/guides/claude-code-gateway/model-endpoint.yaml:11

Nit: nothing in the guide selects on modelplane.ai/external-provider, since each service selects on modelplane.ai/claude-model. Could we drop it? If a label stays, the reader's own domain as prefix, like example.org/ in concepts/inference-gateway.yaml, avoids implying Modelplane gives it meaning.

11: docs/content/guides/claude-code-gateway.md:66

Nit: "Setting the request timeout to 1800s here..." has no main verb. The timeout is also explained in the model-service.yaml comment and again under "Timeouts and retries", so it could live in one place.

Run 2

Summary

The guide's approach fits the current API, but I think the page needs another look before merging, because a reader following it in order would hit several broken steps.

The base URL and port-forward steps don't work for the TLS gateway the page builds, and the gateway refers to a cluster the page never creates. Beyond those, the managed-settings advice would give everyone the same key, and the usage record example doesn't match what the gateway logs. The rest are smaller accuracy points and nits.

Contrary to the PR description, the page has no managed-settings env block. docs-vale also reports 13 errors on the page.

01: docs/content/guides/claude-code-gateway.md:63

Is this the reason the service name matters? The gateway rewrites the model to the endpoint's model, so Anthropic's response names claude-opus-5-5 whatever you call the service. If Claude Code read the response, the name wouldn't matter. Anthropic's errors page ties unrecognized_model to the model ID Claude Code sends, which here is the service name from ANTHROPIC_DEFAULT_*_MODEL. Could this, and the same reason in model-service.yaml, say Claude Code picks its defaults from the model ID it sends?

02: docs/content/guides/claude-code-gateway.md:77

Won't this set ANTHROPIC_BASE_URL to an empty string for this page's gateway? compose-inference-gateway publishes status.endpoints only when spec.tls is unset, and the gateway above sets it. The next paragraph says as much, but someone following the page in order runs this first. Could the TLS form, https://<name>/anthropic, be the instruction, with the plain-HTTP case left to the Messages API guide?

03: docs/content/guides/claude-code-gateway.md:108

Does that port-forward work with TLS? The Messages API guide forwards 8080:80. With spec.tls set, the AIGatewayRoutes bind only to the HTTPS listener, and :80 answers with a 301 to HTTPS. Anthropic warns a redirect can drop the body or credential. Forwarding :443 would fail the certificate check on localhost. Could this drop the pointer, or say what works for a TLS gateway?

04: docs/content/guides/claude-code-gateway.md:102

Wouldn't distributing this block through managed settings give every machine the same key? It hard-codes ANTHROPIC_AUTH_TOKEN, so everyone would show up as dennis in the usage records. Anthropic's rollout guide puts the base URL in the managed env block and gets each person's key from an apiKeyHelper or their own secrets process. Could the page show a managed-settings env block with ANTHROPIC_BASE_URL and the model variables, and leave the key out of it?

05: docs/content/guides/claude-code-gateway.md:98

Will every request Claude Code makes use these names? The rollout guide says some background sub-calls request a built-in model ID regardless of the override. A route only matches <namespace>/<service>, so a bare claude-haiku-... gets a 404, as does any Claude ID someone picks with /model. Could the page say so and link #596 here too, as the subscriptions section does?

06: docs/content/guides/claude-code-gateway.md:123

Is this usually right for a shared key? The failover policy retries a 429 up to three times, and with one endpoint each retry goes back to the same rate-limited key, without reading retry-after. That turns one Claude Code attempt into up to four upstream requests before Claude Code's own retries start. The retry policy applies to the whole InferenceGateway, so a reader can't tune it per service. Could the page state it as a cost of the current defaults rather than recommend it?

07: docs/content/guides/claude-code-gateway.md:137

Is this the endpoint a reader would see? The field references the AIServiceBackend, which lives in the mirrored namespace mp-anthropic-<hash>. For these manifests that's roughly mp-anthropic-<hash>/claude-opus-5-5-public-<hash>-anthropic-claude-opus-5-5-<hash>. A query filtering on namespace anthropic wouldn't match any record. The record also logs response_model, which the example leaves out. Could the example match _USAGE_RECORD in functions/compose-inference-gateway/function/fn.py?

08: docs/content/guides/claude-code-gateway.md:150

Doesn't input_tokens include cache writes too? AI Gateway's Anthropic translator sets it to raw input plus cache reads plus cache creation (anthropic_anthropic.go). Anthropic bills writes at a different rate. Could the sentence say it counts both cache reads and cache writes?

09: docs/manifests/guides/claude-code-gateway/inference-gateway.yaml:14

The page never creates gw-gcp-us, so a reader applying the manifests in order gets a gateway that never becomes ready. The other guides include their cluster manifest, as anthropic-messages-api/inference-cluster.yaml does. Please add one, or state the prerequisite and link the InferenceCluster page. The page should also say where llm-example-com-tls goes: a TLS Secret in modelplane-system.

10: docs/content/guides/claude-code-gateway.md:52

Does a mixed service serve both OpenAI and Anthropic clients? The ModelEndpoint XRD says an OpenAI request routed to an Anthropic endpoint fails, so OpenAI clients would get errors on whatever share of requests the weights send to Anthropic. Since the guide doesn't build a mixed service, could this paragraph shrink to the forwarded anthropic-* headers, which is the part this setup relies on?

11: docs/content/guides/claude-code-gateway.md:89

Nit: with ANTHROPIC_DEFAULT_HAIKU_MODEL pinned, as this page does, ANTHROPIC_API_KEY doesn't change the background model. Anthropic's table scopes that change to "unless ANTHROPIC_DEFAULT_HAIKU_MODEL pins one". Could the sentence keep the approval prompt and drop the background-model clause?

12: docs/content/guides/claude-code-gateway.md:66

Nit: the shortcode renders manifest comments on the page, and they repeat the prose around them. The 1800s reasoning appears three times: here, in "Timeouts and retries" and in model-service.yaml. Gateway auth and the Anthropic schema repeat too (here and elsewhere). Please keep each explanation in one place. Also, "Setting the request timeout to 1800s here…" has no subject.

13: docs/manifests/guides/claude-code-gateway/model-endpoint.yaml:11

Nit: nothing selects modelplane.ai/external-provider. The services select on modelplane.ai/claude-model, and the header comment only explains that one. Please drop it, or say what it's for (here and on the other two endpoints).

Run 3

Summary

This looks close: the guide covers what #597 asks for, but a few of its steps won't work as written and need another look before it merges.

The base URL command, the port-forward and the managed settings advice each fail for the setup the page builds, which uses a TLS gateway and per-person keys. The reason given for naming services after the Claude ID and the example usage record don't match the code, and the retry advice endorses a trade-off #597 raised as a caveat.

The PR description and commit say the guide shows a managed settings env block, which it doesn't. The description also says Vale passes, but it reports 13 errors on the new page. The commit's trailer is Part of #597., where CONTRIBUTING uses Towards #597.

01: docs/content/guides/claude-code-gateway.md:63

Does Claude Code read the model ID from the response here? Each endpoint rewrites the model to claude-opus-5-5, and the response gives the model that answered (definition.yaml). If the response were what counted, a service named anthropic/opus would work too. #596's warning names the ID Claude Code sent (anthropic/opus[1m]), and Anthropic's rollout page says the same. Could "reads the model ID out of every response" say it's the configured ID, here and in the model-service.yaml comment?

02: docs/content/guides/claude-code-gateway.md:77

Should this snippet come first? Lines 81 to 83 cover the TLS case, but the gateway this page builds sets spec.tls. A reader who runs the steps in order gets an empty ADDRESS and exports an empty ANTHROPIC_BASE_URL. Could the guide lead with https://<name>/anthropic and drop the snippet, or point plain-HTTP readers to the Messages API guide?

03: docs/content/guides/claude-code-gateway.md:96

What happens when Claude Code asks for a Claude ID that has no service? Anthropic's docs say some background calls request a built-in ID whatever the overrides say, and #596 notes such an ID gets a 404. Could this section say so, and that each new model needs its own endpoint, service and client variable, with a link to #596?

04: docs/content/guides/claude-code-gateway.md:102

Should this block go in managed settings? It holds one person's key, and a managed settings file is the same on every machine, so every record would show the same caller. Anthropic's docs put ANTHROPIC_BASE_URL in the managed env block and get each person's key from an apiKeyHelper. Could the page show that env block, which #597 asked for, with the base URL and model variables, and say how each person's key reaches their machine?

05: docs/content/guides/claude-code-gateway.md:108

Does the port-forward work with a TLS gateway? The Messages API guide forwards 8080:80 and uses http://localhost:8080/anthropic. With spec.tls set, routes bind only to the HTTPS listener and compose_redirect answers :80 with a 301 to HTTPS (fn.py). Forwarding 443 instead fails certificate checks for localhost. Could we drop the sentence, or say what a TLS gateway needs?

06: docs/content/guides/claude-code-gateway.md:123

Is this what a reader wants from a shared key? The retry policy doesn't set a back-off and ignores retry-after (fn.py). With one endpoint, a 429 goes back three times to the same rate-limited key. Claude Code then retries up to 10 times (Anthropic), so one turn can make up to 44 upstream calls. #597 listed this as a caveat. Could the guide state the trade-off instead of endorsing it?

07: docs/content/guides/claude-code-gateway.md:137

Does this match what the proxy logs? The endpoint is the ModelRoute's backend in the mirrored namespace, which is mp-anthropic-<hash>/<route>-<endpoint>-<hash>, not anthropic/modelroute-... (_USAGE_RECORD). The e2e test shows the counts and status as integers, not strings, and the record also has a response_model field. Could the guide copy a real record's shape?

08: docs/content/guides/claude-code-gateway.md:150

Doesn't input_tokens include cache writes as well as reads? #597 asked the guide to say so, and the measurement in #535 shows a request with 2 uncached tokens and 25,768 written to cache recording input_tokens: 25770. Anthropic prices writes differently from reads, so whoever prices the records needs to know. Could the guide say reads and writes?

09: docs/manifests/guides/claude-code-gateway/model-service.yaml:22

Will a proxy rollout cut a long turn short of this 1800s timeout? _DRAIN_TIMEOUT is 300s, chosen to match the default request timeout "unless its service allows longer" (fn.py). So a rollout ends any Opus turn still running after five minutes. Could the timeout notes say so? #625 would make the drain configurable.

10: docs/manifests/guides/claude-code-gateway/model-service.yaml:7

Nit: the 300s default and the 1800s choice come up three times on one page: here, in "One service per model" and in "Timeouts and retries". The manifest comments also repeat the prose around them for service naming, the one-endpoint-per-model rule and the schema translation. Please keep each point in one place, since the page renders each manifest's comments right under the prose.

11: docs/manifests/guides/claude-code-gateway/model-endpoint.yaml:11

Nit: nothing selects on modelplane.ai/external-provider, since each service selects on modelplane.ai/claude-model. It looks carried over from concepts/model-endpoint.yaml, where it's the selector. Dropping it from all three endpoints keeps the example to what it uses.

Run 4

Summary

This looks close to mergeable once a few steps work on the TLS gateway the guide sets up, and its managed-settings advice no longer puts everyone under one caller.

The other questions are about claims that don't match the code or AI Gateway, such as the example usage record and what input_tokens counts. Draft #633 adds spec.auth.apiKey.header, which removes one of the two blockers the last section names, so that section will need an edit once #633 merges.

The PR description and commit message say the guide includes an env block for managed settings, but it has shell exports (see the comment on line 102).

01: docs/content/guides/claude-code-gateway.md:102

Won't distributing this block through managed settings give everyone Dennis's key? Managed settings apply one policy to everyone they reach, so every usage record would say "caller": "dennis". The block is also shell exports. Managed settings take a JSON env object, which is what #597 asked for. Could we show an env block with only the shared values (the base URL and the three model tiers), and have each person set their own key in their shell?

02: docs/content/guides/claude-code-gateway.md:77

Doesn't this return an empty string for the public gateway the page just created? It has tls, and the next paragraph says status.endpoints is only published without TLS. What if we lead with https://<name>/anthropic and keep the kubectl form as a note for plain-HTTP gateways?

03: docs/content/guides/claude-code-gateway.md:108

Does the Messages API guide's port-forward work for this gateway? It forwards 8080:80 and uses http://localhost:8080/anthropic. With tls set, the model routes attach only to the HTTPS listener (compose-model-route), and compose_redirect turns everything on :80 except /healthz into a 301 to HTTPS. Forwarding 443 instead would hit a certificate that doesn't match localhost. Could we drop the sentence, or say what an in-cluster TLS gateway needs?

04: docs/content/guides/claude-code-gateway.md:63

Is the response where Claude Code finds the model ID? The gateway doesn't rewrite the response's model on this path, so a response would say claude-opus-5-5 regardless of the service's name. The unrecognized_model warning in #596 shows anthropic/opus[1m], the configured name. The check seems to run on the name Claude Code sends from ANTHROPIC_DEFAULT_*_MODEL. Could we give that reason here, or just the observed behaviour (here and in the model-service.yaml comment)?

05: docs/content/guides/claude-code-gateway.md:124

Are the gateway's retries on top of Claude Code's "usually what you want from a shared key"? With one endpoint, the gateway resends a request that got a 429 to Anthropic up to three times without reading retry-after, so it spends more of the shared limit before Claude Code backs off. The same policy has a passive health check with consecutive5XxErrors: 5, maxEjectionPercent: 100 and panic mode off. Five consecutive 5xx responses, such as Anthropic's 529 overloaded, eject the only host, and the service returns 503 for at least 30s. Could this section state the retries as a limitation rather than recommend them, and mention the ejection?

06: docs/content/guides/claude-code-gateway.md:137

Can the gateway produce this endpoint value? _USAGE_RECORD says it's the AIServiceBackend, in the team's mirrored namespace (child_name("mp", "anthropic")), named child_name(<route>, <endpoint>). That gives mp-anthropic-<hash>/claude-opus-5-5-public-<hash>-anthropic-claude-opus-5-5-<hash>, as in fleet-gateway.md, with no modelroute- prefix. The record also has a response_model field, which the example leaves out. Could we match the example to the real record?

07: docs/content/guides/claude-code-gateway.md:150

Doesn't input_tokens include cache writes too? AI Gateway's ExtractTokenUsageFromExplicitCaching adds both cache_creation_input_tokens and cache_read_input_tokens to the input count, and #597 says "reads and writes". Since readers price usage from this number, could we say it counts both?

08: docs/content/guides/claude-code-gateway.md:66

Nit: the page explains the 1800s timeout in three places: here, the model-service.yaml comment and "Timeouts and retries". That section also restates the request and idle semantics from model-service.md. Please say it once and link to the ModelService page for the rest.

09: docs/manifests/guides/claude-code-gateway/model-endpoint.yaml:11

Nit: nothing selects modelplane.ai/external-provider (here and on the other two endpoints). The services select on modelplane.ai/claude-model alone, so a reader may look for what reads this label. Please drop it.

Run 5

Summary

This looks close: the approach fits the current API and #597, and what stands between it and merging is a few claims on the page that the code or the docs contradict.

Getting a base URL and port-forwarding behave differently on this guide's own TLS gateway than the page says, and the usage-record example and token counts don't match what the gateway logs. The rest are questions about the reasons and advice the page gives, such as the gateway's retries and how the per-person key reaches each machine.

The PR description says the page gives the client settings as a managed-settings env block, but the page shows shell exports.

01: docs/content/guides/claude-code-gateway.md:51

Doesn't "serves both client shapes on one name" contradict the ModelEndpoint XRD's api.schema description and Alternate APIs? Both say an OpenAI request routed to an Anthropic endpoint fails, so a mixed service would fail OpenAI callers whenever the gateway routes them to this endpoint. Could we link Alternate APIs for the translation rule and keep only the header-forwarding part here?

02: docs/content/guides/claude-code-gateway.md:63

Is it the response that Claude Code checks? The endpoint rewrites the model to claude-opus-5-5. The ModelEndpoint XRD says the response gives the model that answered, so it would contain the Claude ID whatever name the service has. #596 shows the warning on the model Claude Code sent (anthropic/opus[1m]), which suggests the name matters because it's what ANTHROPIC_DEFAULT_*_MODEL sends. Could the reason here and in model-service.yaml say that instead?

03: docs/content/guides/claude-code-gateway.md:66

Nit: this sentence repeats the 1800s point from the Timeouts section and the model-service.yaml comment, so it could go. It's also a fragment, with no main verb.

04: docs/content/guides/claude-code-gateway.md:77

Won't this print nothing for the public gateway the page just created? It sets spec.tls, and write_status only publishes status.endpoints without TLS (fn.py), so ANTHROPIC_BASE_URL ends up empty. Could we lead with https://<name>/anthropic, since that's the case this guide builds, and keep the status.endpoints snippet as the aside for a plain-HTTP gateway?

05: docs/content/guides/claude-code-gateway.md:102

Can this block go in managed settings as one file? The managed settings env block is the same on every machine, but ANTHROPIC_AUTH_TOKEN is different for each person. Anthropic's rollout steps pair managed settings with "your secrets tooling" for the credential. What if the page showed the shared values (ANTHROPIC_BASE_URL and the three model variables) as the JSON env block #597 asked for, and said the key comes from elsewhere, such as apiKeyHelper or the person's own settings?

06: docs/content/guides/claude-code-gateway.md:108

Does that port-forward work on a TLS gateway? The Messages API guide forwards 8080:80 and uses http://localhost:8080/anthropic. With spec.tls set, inference binds to the HTTPS listener alone and :80 answers with a 301 to https (fn.py, compose_redirect). Forwarding 443 instead meets a certificate for the gateway's DNS name. Could we drop the sentence, or say what reaching an in-cluster TLS gateway takes?

07: docs/content/guides/claude-code-gateway.md:123

Is this usually what you want from a shared key? The BackendTrafficPolicy doesn't set a backoff or handle retry-after (fn.py). With one endpoint, a 429 becomes up to four quick hits on the same key, and then Claude Code's own retries multiply that. Could the page state it as a limitation rather than endorse it?

08: docs/content/guides/claude-code-gateway.md:137

Could we paste a record captured from a proxy log here? This one differs from what the gateway writes in a few places:

• The endpoint is the AIServiceBackend in the team's mirrored mp-anthropic-<hash> namespace, named child_name(<route>, <endpoint>) (fn.py), not anthropic/modelroute-....
• The record also has response_model.
• The counts, status and duration are JSON numbers, as in fleet-gateway.md.

09: docs/content/guides/claude-code-gateway.md:150

Doesn't input_tokens count cache writes too? On the pinned AI Gateway v1.1.0, ExtractTokenUsageFromExplicitCaching sums input_tokens, cache_creation_input_tokens and cache_read_input_tokens for Anthropic responses (metrics.go). Anthropic bills reads and writes at different rates, and the record doesn't split them. Could the page say it includes both, as #597 asked?

10: docs/content/guides/claude-code-gateway.md:164

Nit: the closing sentence of this section promises future content, and goes stale if #596 changes direction. The link to #596 already does the job.

11: docs/manifests/guides/claude-code-gateway/model-endpoint.yaml:11

Nit: nothing in this guide selects modelplane.ai/external-provider, since every service selects on modelplane.ai/claude-model. Could we drop it from the three endpoints, so the reader doesn't look for the selector that uses it?

12: docs/manifests/guides/claude-code-gateway/model-service.yaml:22

Will a turn longer than 300s finish across a gateway proxy restart? compose-inference-gateway fixes the proxy's drain timeout at 300s to match the default request timeout (fn.py). A rollout then cuts streams that the 1800s timeout allows. #625 would make the drain timeout configurable. Until then, could the page mention the limit next to the 1800s advice?

reviews-625.md

Run 1

Summary

I think this needs a step back on the API before it's close: drainTimeout addresses #618, but the rest of spec.proxy either repeats what Envoy Gateway already does or doesn't take effect on the Deployment.

Passing drainTimeout through to shutdown may be the whole fix, which leaves open whether the API needs the rollout fields and whether the function should derive the drain default. Both answers would change the field before anyone depends on it.

The PR description says nix run .#build and nix flake check weren't run. nix build .#checks.x86_64-linux.docs-manifests fails on the integer maxSurge and maxUnavailable in docs/manifests/reference/inferencegateways.yaml.

01: apis/inferencegateways/definition.yaml:147

Where do we expect users to set the cluster gateway's drain timeout, which #618 also asks for? #626 covers that proxy's replicas, PDB and spread, but not its drain. A fleet gateway set to 1h would still have streams on the same request path cut at 300s by a cluster-gateway rollout. Deciding now, whether it's derived (see the comment on the default) or set on the ServingStack, lets both gateways' drain settings share a name and schema.

02: apis/inferencegateways/definition.yaml:164

#618 asks for these so a rollout adds a proxy before draining one, but do we need them in the API? Envoy Gateway's default strategy is a bare RollingUpdate (kubernetes_helpers.go), so Kubernetes applies 25%/25%. With _PROXY_REPLICAS = 2, that rounds to surge 1 and unavailable 0, which already brings a new proxy up before an old one drains. Could we drop both fields, or say which replica count or case needs them?

03: apis/inferencegateways/definition.yaml:177

Should the default come from the ModelServices this gateway serves, as #618 asks ("defaulting to the longest route timeout the gateway serves")? With a fixed 300s, the cut from #618 stays the default, and when a team raises a service's timeouts.request to 1h, every gateway serving it keeps cutting at 5 minutes until someone edits it too. It would slow rollouts, though: one 1h service would let each old proxy pod take up to an hour to drain during a rollout. A schema default fills in the field whether or not a user set it, so deriving it later means changing the API.

04: apis/inferencegateways/definition.yaml:179

If these fields stay, won't the documented maxSurge: "1" get the proxy Deployment rejected? Envoy Gateway copies the strategy into the Deployment (resource_provider.go), and Kubernetes accepts a string IntOrString only as a percentage (validation.go). Could these use x-kubernetes-int-or-string: true, as apis/modelreplicas/definition.yaml does? (Here and on maxUnavailable.)

05: apis/inferencegateways/definition.yaml:185

If these fields stay, what happens when someone sets both maxSurge and maxUnavailable to 0? Both patterns accept it, but Kubernetes rejects that Deployment (validation.go), so the proxy Deployment update would fail. Could a CEL rule on proxy reject it at admission?

06: functions/compose-inference-gateway/function/fn.py:578

Do we need to set terminationGracePeriodSeconds here? Envoy Gateway v1.8.4 already sets it to drainTimeout plus 300s whenever the EnvoyProxy sets shutdown.drainTimeout (resource_provider.go), so the docstring's "inert" case doesn't arise. The grace period computed here can also disagree with upstream's: _seconds matches m before ms, so 500ms reads as 500 minutes. Could we pass drainTimeout to shutdown and drop the patch, _seconds and _GRACE_MARGIN?

07: functions/compose-inference-gateway/function/fn.py:586

If these fields stay, will maxSurge and maxUnavailable reach the Deployment at this level? envoyDeployment.strategy is an appsv1.DeploymentStrategy (shared_types.go), which nests them under rollingUpdate. As composed, the API server would either reject the EnvoyProxy for unknown fields or prune them, leaving Envoy Gateway's default rollout. The new test passes because it expects the flat fields too. Could they go under rollingUpdate?

Run 2

Summary

I think this needs rework on everything but the drainTimeout pass-through before it's close to merging. That pass-through fixes the 300 s cut. The grace-period patch and the rollout fields around it look either redundant with what Envoy Gateway and Kubernetes already do or broken as written, and the default differs from the one #618 asks for.

The PR leaves the cluster-gateway proxy to #554, but #554 and #626 cover its replicas and PDB, not its drain timeout or rollout strategy. Could #618 or a new issue track the cluster gateway's drain timeout and rollout? The commit has no Signed-off-by or issue trailer, and CONTRIBUTING.md asks for "Part of #618" to open the description.

01: functions/compose-inference-gateway/function/fn.py:578

Do we need to set the grace period ourselves? Envoy Gateway v1.8.4 already sets the proxy pod's terminationGracePeriodSeconds to drainTimeout + 300 from shutdown.drainTimeout (resource_provider.go). So a raised drainTimeout isn't inert without this patch, contrary to the docstring. The patch also adds a bug: (h|m|s|ms) matches m before ms, so 500ms parses as 500 minutes and the grace period becomes 30300 s. Could we pass drainTimeout through and drop _seconds, _GRACE_MARGIN and the deep copy?

02: functions/compose-inference-gateway/function/fn.py:589

Won't these values get pruned? envoyDeployment.strategy is an appsv1.DeploymentStrategy, which only has type and rollingUpdate.{maxSurge,maxUnavailable}. On a kind cluster with the v1.8.4 EnvoyProxy CRD, {type, maxSurge, maxUnavailable} stores as {"type": "RollingUpdate"}, with warnings for the unknown fields, since provider-kubernetes doesn't set strict field validation. Should they go under rollingUpdate? The test expects the flat layout too, so it would need the same change.

03: apis/inferencegateways/definition.yaml:180

Will the Deployment accept "1" here? A Deployment takes a string maxSurge or maxUnavailable only as a percentage (validation.go). Once Envoy Gateway copies "1" into the proxy Deployment, the API server rejects it. With type: string, the reference manifest's maxSurge: 1 also fails the docs-manifests check. Could these use x-kubernetes-int-or-string: true, as apis/modelreplicas/definition.yaml does? (Here and on maxUnavailable.)

04: apis/inferencegateways/definition.yaml:177

Is the fixed 300 s default deliberate? #618 asks for drainTimeout to default to the longest route timeout the gateway serves. The model's author sets ModelService.spec.timeouts.request, but the platform team writes the InferenceGateway, so with a static default each raised timeout needs a matching edit here, or rollouts cut streams again. ModelRoutes have gatewayName and a timeouts.request taken from their ModelService, so this function could require the routes pinned to it and derive drainTimeout from them. Deciding now matters, because a literal default gets stored on every XR that sets proxy, and a derived default added later couldn't tell those from values users chose.

05: apis/inferencegateways/definition.yaml:179

What do maxSurge and maxUnavailable give a user while the proxy runs a fixed two replicas? Envoy Gateway's default strategy sets only type: RollingUpdate (kubernetes_helpers.go). The Deployment then takes the 25%/25% default. On two replicas that rounds to surge 1 and unavailable 0, which is the setting the PR's docs page recommends. Could this PR add only drainTimeout, and add rollout fields once replicas are configurable? If they stay, would nesting the XRD fields as strategy.rollingUpdate read closer to the Deployment?

Run 3

Summary

I think this needs another pass on its approach before the details matter.

Envoy Gateway v1.8.4 already sets terminationGracePeriodSeconds to drainTimeout plus 300 s, so passing drainTimeout through may be all the drain half needs, and the grace-period code can go. The rollout half has two failures I reproduced: the EnvoyProxy CRD rejects the strategy's nesting, and Kubernetes rejects string values like "1". The XRD's type: string also rejects the reference example's unquoted maxSurge: 1, so docs-manifests fails. I'd settle the field layout and the drain default in this PR too, since #554 will reuse them for the cluster gateway.

The commit message credits the grace-period patch with keeping the pod alive through draining, which Envoy Gateway already does. It also lacks the Signed-off-by line and issue trailer CONTRIBUTING asks for.

01: functions/compose-inference-gateway/function/fn.py:578

Do we need to set the grace period ourselves? Envoy Gateway v1.8.4 already sets it on the pod to drainTimeout plus 300 s (resource_provider.go#L623-L633). So a raised drainTimeout works without the patch, despite the docstring's "inert".

_seconds also misreads milliseconds: the regex tries m before ms, so 500ms parses as 500 minutes.

Could we pass drainTimeout through and drop _seconds, _GRACE_MARGIN and the patch? The PR's question about always setting the grace period then goes away.

02: functions/compose-inference-gateway/function/fn.py:587

Won't the API server reject this EnvoyProxy? envoyDeployment.strategy is an appsv1.DeploymentStrategy (shared_types.go), so maxSurge and maxUnavailable belong under strategy.rollingUpdate. Server-side applying this output against the v1.8.4 CRD fails with strategy.maxUnavailable: field not declared in schema, so the existing EnvoyProxy never gets the new drainTimeout either. The test expects the same un-nested keys, so it passes.

Could we nest them under rollingUpdate, and have the test expect that?

03: apis/inferencegateways/definition.yaml:179

Should these be real IntOrString fields? As type: string, the reference example's maxSurge: 1 fails validation (the docs-manifests check fails on it). If a user quotes it as "1", Envoy Gateway copies the string into the Deployment, and Kubernetes accepts a string IntOrString only as a percentage (a valid percent string must be a numeric string followed by an ending '%').

Could we use x-kubernetes-int-or-string: true, as apis/modeldeployments/definition.yaml and apis/modelreplicas/definition.yaml do?

04: apis/inferencegateways/definition.yaml:147

Should the rollout fields mirror the upstream DeploymentStrategy as a subset, as proxy.strategy.rollingUpdate.{maxSurge,maxUnavailable}, rather than sit beside drainTimeout? Users already know that layout, and the function could copy it into envoyDeployment.strategy unchanged. It does add two levels of nesting. #554 will add the same settings to the cluster gateway, so deciding the layout in this PR saves changing two APIs later.

05: apis/inferencegateways/definition.yaml:177

Is the fixed 300 s default a deliberate departure from what #618 asks, a default of "the longest route timeout the gateway serves"? With this default, a model author who raises timeouts.request to an hour still gets streams cut on rollout unless the platform team updates the gateway.

With a route-derived default, every route timeout change would roll the proxy. If you fixed the default for that reason, could the PR say so and note that #618 still needs this part?

06: apis/inferencegateways/definition.yaml:185

What happens with maxSurge: 0 and maxUnavailable: 0? Both patterns admit 0, and Kubernetes rejects a rolling update where both are zero (validation.go). The EnvoyProxy would apply, but Envoy Gateway's Deployment update would fail, and nothing on the XR would show it. Could a CEL rule on proxy reject that pair at admission?

07: docs/content/platform/inference-gateway.md:83

Does this example change anything for this gateway? The proxy runs two replicas, and Envoy Gateway's default strategy sets only type: RollingUpdate (kubernetes_helpers.go). Kubernetes' 25% defaults then round to a surge of 1 and 0 unavailable, which is already what this page recommends. Could the docs and the reference manifest say that, or pick an example that differs from the default?

Run 4

Summary

I think this needs a step back on its API before the details matter: most of what it adds beyond the drain timeout looks like it could go. Raising the drain timeout fixes the problem in #618. Of the rest, the grace-period patch repeats what Envoy Gateway already does, and the rollout fields leave today's two-replica rollout as it is, besides not reaching the Deployment in the form they're written. Dropping those would leave the drain timeout's default as the open design question.

The PR description's design question about when to set the grace period goes away if Envoy Gateway already derives it, as the first inline comment asks.

01: functions/compose-inference-gateway/function/fn.py:578

Do we need to set terminationGracePeriodSeconds ourselves? Envoy Gateway v1.8.4, which compose-serving-stack installs, already sets it to drainTimeout + 300 s (resource_provider.go). The patch overrides that with _seconds, which reads 500ms as 500 minutes because m matches before ms. Could we pass drainTimeout through and drop the patch, _seconds and _GRACE_MARGIN? That settles the "only when set or always" question too. Even if the patch stays, _seconds needs fixing.

02: functions/compose-inference-gateway/function/fn.py:587

Shouldn't maxSurge and maxUnavailable sit under strategy.rollingUpdate? envoyDeployment.strategy is an appsv1.DeploymentStrategy (shared_types.go), whose CRD schema only has type and rollingUpdate. Applied to a kind cluster with the v1.8.4 CRD, this EnvoyProxy fails with .spec.provider.kubernetes.envoyDeployment.strategy.maxSurge: field not declared in schema. The test expects the same flat nesting, so it passes.

03: apis/inferencegateways/definition.yaml:180

Won't the Deployment reject maxSurge: "1"? A Deployment's IntOrString accepts a string only as a percentage (ValidatePositiveIntOrPercent), so only the 25% form works here. The reference example's unquoted maxSurge: 1 also fails this schema and the docs-manifests check. Could these use x-kubernetes-int-or-string: true, as apis/modelreplicas/definition.yaml does for its rollout fields?

04: apis/inferencegateways/definition.yaml:166

Do we need maxSurge and maxUnavailable at all? Envoy Gateway's default strategy sets only type: RollingUpdate (kubernetes_helpers.go), so Kubernetes' 25% applies, and at the fixed two replicas that rounds to surge 1, unavailable 0. That's already the rollout these docs recommend opting into, and I don't see a setting that improves on it for streams. Could we drop them, or pin surge 1 and unavailable 0 in the composition the way #626 hardcodes the cluster gateway's defaults?

05: apis/inferencegateways/definition.yaml:177

Was dropping #618's default deliberate? #618 asks for drainTimeout to default to the longest route timeout the gateway serves, "so a rollout never cuts a request the route allows". With a fixed 300 s, a ModelService author who raises timeouts.request brings the original failure back unless the gateway owner notices. If reading every selected ModelService is too much for this function, could we raise the fixed default instead, say to the 3600 s #618 cites as a ModelService's longest request? That assumes a pod exits once its connections drain, which I haven't checked against v1.8.4.

06: apis/inferencegateways/definition.yaml:147

Could proxy take default: {}, as timeouts does in apis/modelservices/definition.yaml? Without it the "300s" default only applies when someone writes proxy:. The function then needs _DRAIN_TIMEOUT and the drain or _DRAIN_TIMEOUT fallback, which defines the default twice.

07: docs/content/platform/inference-gateway.md:76

Doesn't this make docs/content/models/model-service.md untrue? Its timeouts section says "A restarting gateway lets requests already in flight run for five minutes", which is now only the default. That's where readers set long request timeouts, so could it point at spec.proxy.drainTimeout and this section?

Run 5

Summary

I think this needs another look at what compose-inference-gateway composes from spec.proxy before it's ready to merge, since as written the rollout settings wouldn't reach the proxy Deployment.

Gateway-scoped proxy settings on the InferenceGateway look like the right place. Draft PR #626 configures the cluster gateway's proxy on the ServingStack side, so the two proxies could share field names once spec.proxy settles.

The commit message repeats the docstring's claim that the API server would kill the pod before Envoy finishes draining without the explicit grace period, and the commit has no Signed-off-by line.

01: functions/compose-inference-gateway/function/fn.py:587

Won't the EnvoyProxy CRD schema prune maxSurge and maxUnavailable here? envoyDeployment.strategy is an appsv1.DeploymentStrategy (shared_types.go), which nests them under rollingUpdate. Envoy Gateway copies it into the Deployment (resource_provider.go), so the proxy would keep rolling out at 25%/25% with nothing reporting it. Could both go under strategy.rollingUpdate?

02: apis/inferencegateways/definition.yaml:179

Should maxSurge and maxUnavailable be x-kubernetes-int-or-string: true, like the rollout fields in apis/modelreplicas/definition.yaml? As type: string, the reference manifest's maxSurge: 1 and maxUnavailable: 0 are YAML integers and fail validation, and the docs-manifests check fails on them. Quoting them doesn't help either: the function forwards "1" as a string IntOrString, and Deployment validation only accepts a string if it's a percentage (validation.go).

03: apis/inferencegateways/definition.yaml:185

Could a CEL rule reject maxSurge and maxUnavailable both being zero? The patterns accept 0 (or 0%) for both, and Kubernetes rejects that Deployment because the rollout could never make progress. Envoy Gateway then fails to apply the proxy Deployment. The InferenceGateway wouldn't show it, because envoy-proxy counts as ready once it exists.

04: functions/compose-inference-gateway/function/fn.py:579

Do we need to patch terminationGracePeriodSeconds at all? Envoy Gateway already sets it to drainTimeout + 300 s whenever the EnvoyProxy has shutdown.drainTimeout (resource_provider.go), so the docstring's claim that a raised drainTimeout is inert without the patch doesn't hold. Passing drainTimeout through by itself would let us drop _seconds, _GRACE_MARGIN and the deep copy, and it also settles the "only when set or always" question in the PR description.

05: functions/compose-inference-gateway/function/fn.py:201

If the patch stays, doesn't this read ms as minutes? The alternation tries m first, so 500ms matches m, parses as 500 minutes and returns 30000. With drainTimeout: 90000ms, the grace period becomes about 62 days, overriding Envoy Gateway's correct 390 s. Even if ms matched, the unit table weights it 0, so 3600000ms would get a 300 s grace period. Could the pattern drop ms, or the parser order ms first and divide by 1000?

06: apis/inferencegateways/definition.yaml:177

Is the fixed 300 s default a deliberate departure from #618, which wants drainTimeout to default to the longest route timeout the gateway serves? A derived default would roll the fleet gateway's proxy whenever one ModelService raised its timeout, because Envoy Gateway puts the drain timeout on the shutdown manager's command line (resource.go). If this is the reason for the fixed default, could the PR description say so?

07: docs/content/platform/inference-gateway.md:76

Should this section warn that changing drainTimeout itself rolls the proxy? The value is part of the pod's command (resource.go), so raising it from 300 s to 45 m replaces every pod. The old pods drain on their old 300 s timeout, so that first rollout still cuts longer streams. Teams like poc-2's would still need one maintenance window to adopt the setting.

reviews-632.md

Run 1

Summary

I think this needs a maintainer's call on direction before the details matter: whether hostPath belongs in an ML team's ModelDeployment, and whether this pass-through is an escape hatch beside the node-local storage #630 and #72 are working toward. Adding these fields fits design.md's "curated subset of a PodTemplateSpec", and user-authored-routing.md already expects volumeMounts. Apart from the generated models, I've left naming and test points until the direction settles.

The new fields come through to the ModelReplica and all three backends' pod specs, and the CEL rules compile on a real API server.

After you regenerate the models, please drop the commit message's note about editing them by hand.

01: apis/modeldeployments/definition.yaml:1068

Should a ModelDeployment be able to set a hostPath with any path? design.md gives platform teams "which clusters and hardware exist", and node paths look like theirs. Here a deployment author can mount the containerd socket on every cluster the deployment targets. It also ties the deployment to one node layout: #630 notes local-storage paths differ per cloud, and a replica on a cluster without /mnt/nvme never starts.

Could we leave hostPath out in favour of emptyDir or ephemeral plus a device selector from #630? Or keep it and document that platform teams should restrict it, such as with Pod Security admission? (Same in the ModelReplica XRD.)

02: docs/content/models/model-deployment.md:224

Does this get ahead of where #630 and #72 are heading? #630's question 4 assumes "ModelDeployment has no volumes or volumeMounts, so a KVOffloadTier or ModelCache would add the mount", and the #72 comment says each piece of the GLM workaround "is something Modelplane should own". This section documents the user-written hostPath and staging init container as the pattern, so staging from the ModelCache becomes per-deployment boilerplate.

I'd like a maintainer to confirm whether they want this as an escape hatch beside those abstractions, since #631 has no maintainer response yet.

03: apis/modeldeployments/definition.yaml:1044

Which trade-off do we want for volume sources? The schema allows only these 11, so another source such as iscsi or cephfs needs a Modelplane release. But each source is free-form inside, so hostPath: {} or a typo passes here and fails only when the workload cluster rejects the Deployment, LeaderWorkerSet or PodCliqueSet.

Could we make each item opaque apart from name? Or type the sources this is for (hostPath, emptyDir, ephemeral, persistentVolumeClaim) as a compatible subset, the way design.md describes template as a "curated subset of a Kubernetes PodTemplateSpec"? (Same in the ModelReplica XRD.)

04: apis/modeldeployments/definition.yaml:1077

Is it intended that a persistentVolumeClaim, configMap, secret or csi volume needs its object or driver on every InferenceCluster the deployment may target? design.md says this for secret references ("the referenced objects must exist on every InferenceCluster the deployment may target"). A claim also has to sit in the namespace Modelplane mirrors into. If that's the intended contract, could design.md and the docs say so for volumes too?

05: apis/modeldeployments/definition.yaml:570

Should design.md's description of template list these fields too? It lists the engine's "image, command, args, env, and ports, plus imagePullSecrets" (design.md), which is now incomplete. The reserved dshm and model-cache names and paths probably belong there as well.

06: apis/modeldeployments/definition.yaml:901

Is "passes them through as written" accurate? An init container keeps only name, image, command, args, env, resources and volumeMounts, and a mount only name, mountPath, subPath and readOnly. Someone copying an init container with securityContext or workingDir from a pod spec loses those fields without an error, which is the pruning #631 reports. The docs' "written as in a Kubernetes pod spec" has the same problem. Could we name the supported fields, or say it's a subset?

07: apis/modeldeployments/definition.yaml:854

Should validation check that each mount references a volume the pod will have? A sidecar or init container that mounts model-cache without a modelCacheRef, or any mistyped volume name, passes both XRDs. The workload cluster then rejects the pod for an undefined volume, and the error shows up only as a failing provider-kubernetes Object. Could a CEL rule at template.spec require each mount to reference a declared volume, dshm, or model-cache when the deployment sets a ref?

08: schemas/python/models/ai/modelplane/modelreplica/v1alpha1.py:181

Nit: please regenerate with nix run .#build, per CONTRIBUTING.md. It adds a """CPU and memory requests and limits.""" docstring here and updates the fs://apis hash in schemas/.lock.json. It keeps the hand-edited class names, so the tests won't need changes.

Run 2

Summary

I think the approach needs a step back on one point before this is close to merging: whether a ModelDeployment author should get hostPath, and how that fits with the node-local storage #630 provisions. The passthrough #631 asks for matches how design.md describes the template, a curated PodTemplateSpec subset.

The other comments, including a reserved-path rule that a trailing slash gets past, apply whatever that decision turns out to be. I've held back naming and test-style points until it's made.

01: apis/modeldeployments/definition.yaml:1068

Should the ML team be able to set hostPath? design.md gives the nodes to the platform team, and #72 calls node storage a platform-team concern. With this field, anyone who can write a ModelDeployment can mount / or the containerd socket into pods on shared GPU nodes (hostPath risks). Modelplane doesn't set Pod Security labels on the serving namespaces it derives, so the platform team has no Modelplane-managed place to stop it.

Is that deliberate? If not, could we leave out hostPath, or have the platform team opt in?

02: docs/content/models/model-deployment.md:251

Could #630 make hostPath unnecessary here? On EKS pools where #630 has nodeadm stripe the NVMe and move kubelet state onto it, I'd expect a disk-backed emptyDir with a sizeLimit to be on NVMe already. #630 also publishes the disk as a storage.modelplane.ai device the scheduler can select on. A hostPath is invisible to placement, so a replica on a pool without /mnt/nvme stays in ContainerCreating, and nothing fails at admission.

hostPath keeps weights across pod restarts. Does that outweigh placement on the device? If not, could the example use emptyDir and a node selector on the scratch device?

03: apis/modeldeployments/definition.yaml:893

Could the description and docs present init containers as a general escape hatch, and leave staging weights to ModelCache? In the PR's example the engine loads --model=/mnt/nvme/..., which bypasses the HF_HUB_CACHE mount ModelCache relies on. design/modelcache.md already sketches per-node local caches as a future ModelCache source, and #72 suggests ModelCache could use the same node storage capability. Or is staging weights in an init container meant to be the long-term path?

04: apis/modeldeployments/definition.yaml:570

Should design/design.md change with this? Its template bullet says the template "carries one container named engine ... plus imagePullSecrets", and other containers pass through as written. It doesn't mention volumes, init containers, or the dshm and model-cache names and /dev/shm and /mnt/models paths Modelplane now reserves.

05: apis/modeldeployments/definition.yaml:643

Won't a trailing slash get past this? The rule compares strings exactly, so /dev/shm/ and /mnt/models/ pass, and so does Kubernetes' own check for duplicate mount paths. On kind, a pod with dshm at /dev/shm and a hostPath at /dev/shm/ ran, and /dev/shm showed the host directory instead of Modelplane's shared memory. /mnt/models/ would hide the ModelCache the same way. The docs and the base.py comment both say this can't happen.

Could the rule drop a trailing slash before it compares, or also match paths that start with /dev/shm/? Same in the ModelReplica XRD.

06: apis/modeldeployments/definition.yaml:623

Should a mount that references a volume the pod doesn't have fail here rather than on the workload cluster? Right now {name: missing, mountPath: /data}, or model-cache with no modelCacheRef, passes admission. The remote API server then rejects the Deployment, LeaderWorkerSet or PodCliqueSet, and the error shows only on the provider-kubernetes Object, not on the ModelDeployment. A rule here could check that every mount references one of volumes, dshm or model-cache. The model-cache case needs modelCacheRef in scope, so it would sit higher up.

07: schemas/python/models/ai/modelplane/modelreplica/v1alpha1.py:181

Please regenerate these with nix run .#build rather than editing by hand. No CI check catches drift, so the next person to build would pick it up. Running it on this branch gives one difference here: the generator adds the """CPU and memory requests and limits.""" docstring under InitContainer.resources. It also updates the fs://apis hash in schemas/.lock.json. The ModelDeployment model came out identical.

08: docs/content/models/model-deployment.md:227

Is "written as in a Kubernetes pod spec" accurate? Only the volume sources hold to that. A mount takes only name, mountPath, subPath and readOnly, and an init container takes only name, image, command, args, env, resources and volumeMounts. So securityContext, workingDir, envFrom, mountPropagation and restartPolicy: Always get pruned without an error, which is the problem #631 reports. Could the page list what's accepted, or say it's a subset?

09: apis/modeldeployments/definition.yaml:935

Nit: two init container descriptions come from the engine container and don't apply. This env one cites VLLM_HOST_IP, but an init container exits before vLLM starts. The volumeMounts one at line 1008 says the engine container can't mount at /dev/shm or /mnt/models, which can't apply to an init container. Both recur in the ModelReplica XRD and the models.

Run 3

Summary

This looks close to mergeable, with one design question to settle first: whether any ModelDeployment author should be able to mount an arbitrary hostPath on a shared GPU node.

Passing these fields through as written follows how design/design.md treats the member template and its sidecars. The other comments are smaller, mostly admission gaps for bad mounts and descriptions or docs that need correcting.

01: apis/modeldeployments/definition.yaml:1068

Should any ModelDeployment author be able to mount any hostPath? With this, hostPath: {path: /} and an engine image that runs as root give write access to a GPU node other teams share. Crossplane creates the pod, so the author's RBAC doesn't limit it, and Modelplane doesn't label serving namespaces for Pod Security. design/design.md has platform teams decide who may use the hardware, but they can't limit this.

Could we drop hostPath for a platform team's StorageClass via persistentVolumeClaim or ephemeral, or allow it only where a platform setting permits? I don't mind which. If the exposure is deliberate, could the docs say so?

02: apis/modeldeployments/definition.yaml:860

Should admission check that each mount refers to a volume the pod has? A server-side dry run accepts an engine mount of nvmee (a typo), or of model-cache on a deployment without modelCacheRef. On LeaderWorkerSet and Grove the parent object applies, and the error only shows when their controllers create pods. The replica never gets Ready. A template.spec CEL rule checking each mount's name against volumes, dshm and model-cache would catch the typo. Tying model-cache to modelCacheRef would need a rule higher up.

03: apis/modeldeployments/definition.yaml:643

Doesn't this let /dev/shm/ through? The rule compares exact strings, and so does Kubernetes pod validation. On kind, an engine with dshm at /dev/shm plus a 1Mi emptyDir at /dev/shm/ ended up with a 1M /dev/shm, since the user's mount comes after Modelplane's. Could we match ^/dev/shm/?$ and ^/mnt/models/?$, or reject trailing slashes? The "never collide" comment in base.py would then hold. (Here and in the ModelReplica XRD.)

04: apis/modeldeployments/definition.yaml:1005

This description repeats the engine container's volumeMounts text. On an init container, "The engine container can't mount at /dev/shm or /mnt/models" reads as a rule on this field, and the PR's own example mounts model-cache at /mnt/models in an init container. The env description above has the same problem with its vLLM VLLM_HOST_IP example. Could both describe the init container instead?

05: docs/content/models/model-deployment.md:227

Is "written as in a Kubernetes pod spec" accurate beyond the volume sources? An init container takes only name, image, command, args, env, resources and volumeMounts, and a mount takes no mountPropagation or subPathExpr. Someone who copies an init container with securityContext or restartPolicy: Always will find the API server rejects or prunes those fields. Could the page say which fields it takes, or link the field reference?

06: docs/content/models/model-deployment.md:234

Nit: this page's opening introduces spec.template.spec as the deployment's template, so a reader may put this bare template: there. spec.template.spec has no volumes field, so the example won't work. Could it start from engines: and members:, as the earlier examples on the page do?

07: apis/modeldeployments/definition.yaml:570

This makes two committed designs out of date. design/design.md lists what the template contains (the engine's fields, imagePullSecrets and sidecars), and "More container fields" in design/user-authored-routing.md still says containers can't mount volumes. Could the PR update both, including the reserved dshm and model-cache names and the /dev/shm and /mnt/models paths?

08: schemas/python/models/ai/modelplane/modelreplica/v1alpha1.py:181

Could you run nix run .#build and commit schemas/ as it writes it? The generator gives this field the docstring "CPU and memory requests and limits.", and it changes the fs://apis hash in schemas/.lock.json, which the PR leaves stale. CI doesn't check that the models are fresh, so this drift would show up as unrelated churn in the next regeneration.

09: schemas/python/models/ai/modelplane/modelreplica/v1alpha1.py:206

Could compose-model-deployment get a test that its copy of the template into the ModelReplica keeps a volume, a mount and an init container? It copies the template with mrv1alpha1.Template.model_validate(...), and pydantic ignores fields this model lacks. A field missing here would drop without an error, the same failure #631 reports. test_fn.py already checks that a sidecar passes through, which a new case could follow.

10: functions/compose-model-replica/function/backends/base.py:140

Nit: the name template_pod_fields doesn't say what it holds, and other helpers pass sidecars and user volumes through as written too. An init_containers(member) helper assigned to its own key, the way backends use sidecar_containers, would read more clearly. Pull secrets could stay a conditional assignment or get their own helper.

Run 4

Summary

This looks close, with a few design questions to settle before it merges. The main one is how the fixed dshm and model-cache reservations fit with #574, which has the ModelCache publish its own mount.

The pass-through itself looks sound. Both XRDs install on a kind cluster. Each new CEL rule rejected the inputs it targets. The unit tests for all three backends pass. The other open questions are who gets to write a hostPath, and two kinds of mount that admission lets through but shouldn't.

Regenerating the models would also let the commit message drop its line about editing them by hand.

01: apis/modeldeployments/definition.yaml:1039

How should this reservation work alongside #574? There, each ModelCache entry publishes its own volumes and volumeMounts for replicas to copy. Its design says that with several caches per deployment (#281), "each cache would then need its own volume name and its own path". This rule, and the engine's /mnt/models rule, can't know those ahead of time.

Could we decide now which mechanism owns collision checks? The cache could always publish the reserved name and path. Or compose-model-replica could check for collisions and report one as a condition. Whichever PR merges second has to reconcile the two.

02: docs/content/models/model-deployment.md:258

Do we want dshm and model-cache to become API? Until now they were internal names. Once users write name: model-cache in an init container, renaming that volume or giving each cache its own name (#574, #281) breaks their deployments.

If that's intended, design/design.md (around line 452) could say so. It still lists only containers and imagePullSecrets as the template's contents. If not, could the docs leave out mounting these by name?

03: apis/modeldeployments/definition.yaml:860

What happens when a container mounts model-cache and the deployment has no modelCacheRef? pod_volumes only adds that volume when cache_mounts returns it. The deployment passes admission, but the workload cluster rejects the pod. The error then shows on a provider-kubernetes Object rather than on the ModelDeployment.

modelCacheRef and the members are both under spec.template.spec, so a CEL rule there can see both. Could one reject a model-cache mount when modelCacheRef is unset?

04: apis/modeldeployments/definition.yaml:1068

Is it intended that anyone who can write a ModelDeployment can mount any node path, / included, read-write? design.md (around line 103) has platform teams own who may use what, and #72 calls node-local NVMe a platform-team concern. modelexpress_security_context in base.py notes that Modelplane doesn't set Pod Security labels on the clusters it provisions, so on those clusters nothing refuses the mount.

If it's intended, could the docs say platform teams must restrict it themselves, with Pod Security Admission or a wrapping Composition? If not, should Modelplane limit hostPath, say to an allowed prefix?

05: apis/modeldeployments/definition.yaml:644

Does a trailing slash get past this rule? It compares exact strings, so an engine mount at /mnt/models/ or /dev/shm/ gets through. Kubernetes' duplicate-mount check is exact-string too. A pod mounting both /mnt/models and /mnt/models/ passes a server dry run. The user's volume can then hide the ModelCache or the memory-backed /dev/shm.

Could the rule match ^/(dev/shm|mnt/models)/*$ instead? Same in the ModelReplica XRD.

06: apis/modeldeployments/definition.yaml:1005

Nit: the init container fields copy the engine container's text, and some of it doesn't hold for an init container. This description says "The engine container can't mount at /dev/shm or /mnt/models", but an init container is never the engine container. The env description uses vLLM's VLLM_HOST_IP as its example, and the resources CEL message says a member gets devices from its node pool. Could the init container copies say only what's true of them? (Here and elsewhere, in both XRDs.)

07: schemas/python/models/ai/modelplane/modelreplica/v1alpha1.py:181

Please regenerate these with nix run .#build rather than editing them by hand. Running the generator adds a """CPU and memory requests and limits.""" docstring after InitContainer.resources here, and writes a new fs://apis hash to schemas/.lock.json, which this PR leaves stale.

08: functions/compose-model-replica/function/backends/base.py:140

Nit: template_pod_fields bundles two unrelated pass-throughs, init containers and image pull secrets, into a dict that callers spread with **. The neighbouring helpers each do one thing: sidecar_containers returns a list and place_pod sets fields on the spec. Could this be an init_containers(member) beside sidecar_containers, with image pull secrets handled the same way?

Run 5

Summary

I think this needs a step back on which volume sources an ML team may declare before it's close to merging. The PR description and this commit disagree on that design. The description rules out hostPath and adds rules on volume and mount names. This commit accepts any hostPath and has none of those name rules. Its commit message says the opposite of the description about hostPath. Could the code, the commit message and the description settle on one design? The commit message also says you edited the models by hand, which belongs in the PR's to-do rather than the history.

01: apis/modeldeployments/definition.yaml:1044

Do we mean for any ModelDeployment author to mount any host path on a shared workload cluster? The PR description says hostPath, nfs and image aren't accepted, but this rule accepts all three, and hostPath is free-form, so {path: /} passes. That puts node layout in the ML team's spec, against the split in design/design.md. Could we accept only the restricted profile's sources, as the description says? The hostPath examples in the volumes description and the docs would change with it.

02: apis/modeldeployments/definition.yaml:1038

Will these literals hold once #574 merges? Its future work gives each cache its own volume name, which CEL can't see, so each new name would need an XRD change, and a user volume with that name would clash. The docs also invite users to mount model-cache by name, which makes it a contract. The PR description reserves the modelplane- prefix for this, but I can't find that rule in this commit. Could we add it?

03: apis/modeldeployments/definition.yaml:854

Should a mount of an undeclared volume fail at admission? The PR description lists that rule, but this commit doesn't have it. A mount named nope passes a server dry run, and so does a model-cache mount with no modelCacheRef. The workload cluster then rejects the native Deployment (Not found: "model-cache"), with the error only on the provider-kubernetes Object. LeaderWorkerSet and Grove accept the object, but their controllers can't create the pods. Could a rule on template.spec check each mount name, plus one that can see modelCacheRef?

04: apis/modeldeployments/definition.yaml:643

Does a trailing slash get past this? It compares exact strings, so an engine mount at /dev/shm/ or /mnt/models/ passes. Kubernetes' unique-mountPath check compares exact strings too (validation.go). In a real pod the second mount goes on top of Modelplane's, and the engine loses its tmpfs /dev/shm or the cache without an error. Could the rule strip a trailing slash before comparing, or reject a mountPath that ends in one?

05: docs/content/models/model-deployment.md:251

If hostPath goes, is a disk-backed emptyDir on platform-provided ephemeral storage the contract for #631's NVMe case on every cluster type? It puts the weights on NVMe only on EKS, and only once #630 merges, and every pod replacement restages them, which #631 measured at 302 s. The description plans a platform-published node path as the follow-up. Should the docs say what other clusters get until then?

06: docs/content/models/model-deployment.md:227

Is "written as in a Kubernetes pod spec" true for mounts and init containers? Only the volume sources keep unknown fields. A mount takes name, mountPath, subPath and readOnly, and an init container takes name, image, command, args, env, resources and volumeMounts. So mountPropagation, securityContext or restartPolicy: Always don't get through. Could the page list what each takes, or narrow the claim to volume sources? The volumes description in the XRD makes the same claim.

runs.json

[
  {
    "pr": "607",
    "sha": "dd68374e9aac9f7f75b609fd447df067902f3bdb",
    "repo": "modelplaneai/modelplane",
    "run": 1
  },
  {
    "pr": "632",
    "sha": "f688619aeb904d7e617e7cb280236db4fa2b71cf",
    "repo": "modelplaneai/modelplane",
    "run": 1
  },
  {
    "pr": "625",
    "sha": "312cac413f9354feefeb2a408c26cae87fb3ddbb",
    "repo": "modelplaneai/modelplane",
    "run": 1
  },
  {
    "pr": "619",
    "sha": "0611f14548630c7ce0c4f7ead9784bb13218965f",
    "repo": "modelplaneai/modelplane",
    "run": 1
  },
  {
    "pr": "607",
    "sha": "dd68374e9aac9f7f75b609fd447df067902f3bdb",
    "repo": "modelplaneai/modelplane",
    "run": 2
  },
  {
    "pr": "632",
    "sha": "f688619aeb904d7e617e7cb280236db4fa2b71cf",
    "repo": "modelplaneai/modelplane",
    "run": 2
  },
  {
    "pr": "625",
    "sha": "312cac413f9354feefeb2a408c26cae87fb3ddbb",
    "repo": "modelplaneai/modelplane",
    "run": 2
  },
  {
    "pr": "619",
    "sha": "0611f14548630c7ce0c4f7ead9784bb13218965f",
    "repo": "modelplaneai/modelplane",
    "run": 2
  },
  {
    "pr": "607",
    "sha": "dd68374e9aac9f7f75b609fd447df067902f3bdb",
    "repo": "modelplaneai/modelplane",
    "run": 3
  },
  {
    "pr": "632",
    "sha": "f688619aeb904d7e617e7cb280236db4fa2b71cf",
    "repo": "modelplaneai/modelplane",
    "run": 3
  },
  {
    "pr": "625",
    "sha": "312cac413f9354feefeb2a408c26cae87fb3ddbb",
    "repo": "modelplaneai/modelplane",
    "run": 3
  },
  {
    "pr": "619",
    "sha": "0611f14548630c7ce0c4f7ead9784bb13218965f",
    "repo": "modelplaneai/modelplane",
    "run": 3
  },
  {
    "pr": "607",
    "sha": "dd68374e9aac9f7f75b609fd447df067902f3bdb",
    "repo": "modelplaneai/modelplane",
    "run": 4
  },
  {
    "pr": "632",
    "sha": "f688619aeb904d7e617e7cb280236db4fa2b71cf",
    "repo": "modelplaneai/modelplane",
    "run": 4
  },
  {
    "pr": "625",
    "sha": "312cac413f9354feefeb2a408c26cae87fb3ddbb",
    "repo": "modelplaneai/modelplane",
    "run": 4
  },
  {
    "pr": "619",
    "sha": "0611f14548630c7ce0c4f7ead9784bb13218965f",
    "repo": "modelplaneai/modelplane",
    "run": 4
  },
  {
    "pr": "607",
    "sha": "dd68374e9aac9f7f75b609fd447df067902f3bdb",
    "repo": "modelplaneai/modelplane",
    "run": 5
  },
  {
    "pr": "632",
    "sha": "f688619aeb904d7e617e7cb280236db4fa2b71cf",
    "repo": "modelplaneai/modelplane",
    "run": 5
  },
  {
    "pr": "625",
    "sha": "312cac413f9354feefeb2a408c26cae87fb3ddbb",
    "repo": "modelplaneai/modelplane",
    "run": 5
  },
  {
    "pr": "619",
    "sha": "0611f14548630c7ce0c4f7ead9784bb13218965f",
    "repo": "modelplaneai/modelplane",
    "run": 5
  }
]

trace-607.json

{
  "pr": 607,
  "findings": [
    {
      "id": "F1",
      "title": "fail() marks a running cluster's later stages Pending when an Accepted check fails",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F2",
      "title": "PR description wrongly says a Ready cloud cluster XR without the new conditions counts as having finished both stages; drop the sentence",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F3",
      "title": "Cloud cluster XR status.nodePools read as an untyped dict instead of through generated models",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "main.md lists it under dropped findings, no reason given"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F4",
      "title": "_pending docstring 'Pending while the stage after waits on is False' doesn't parse and ignores Unknown",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'Smaller wording nits'"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "high",
          "reason": "main.md: 'Smaller prose nits'"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose",
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F5",
      "title": "Shared gpu-observer makes one GPU pool's readiness depend on others (adding a pool flips Ready pools to Provisioning)",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F6",
      "title": "ServingStackReady never reports Failed; a broken Helm release in the serving stack reads Provisioning forever",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "main.md: goes beyond what the PR sets out to do"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "main.md: design questions rated low confidence"
        }
      }
    },
    {
      "id": "F7",
      "title": "Failed only derives from LastAsyncOperation; failures shown only on Synced (connect errors, Helm releases, Objects) read Provisioning forever",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low to medium",
          "reason": "main.md lists it under 'What I left out', no reason given"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "medium",
          "reason": "main.md: docs already say so, the issue's own example is covered, asking for more is beyond the PR's scope"
        }
      }
    },
    {
      "id": "F8",
      "title": "Docs say 'the cloud provider' sets LastAsyncOperation; should be 'the Crossplane provider'",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'Smaller wording nits'"
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'Smaller prose nits'"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'Smaller doc wording points'"
        }
      }
    },
    {
      "id": "F9",
      "title": "XRD status.nodePools description says it covers each pool in spec.nodePools, but role: System pools are excluded",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F10",
      "title": "Serving stack composition waits on node pools (cloud XR Ready) rather than ClusterReady, so one stuck GPU pool blocks the stack",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "main.md: low-confidence design points left out"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": ""
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "main.md: design questions rated low confidence"
        }
      }
    },
    {
      "id": "F11",
      "title": "ServingStackReady reads Provisioning before the stack is composed (NodePoolsReady True while ClusterReady still False)",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F12",
      "title": "Coordinate stage contract with #594 (UpCloud function would read Provisioning/Pending indefinitely)",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "medium",
          "reason": "main.md lists it under 'What I left out', no reason given"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low to medium",
          "reason": "main.md: low-confidence design points left out"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": ""
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "main.md: design questions rated low confidence"
        }
      }
    },
    {
      "id": "F13",
      "title": "Docs wrongly cite IAM role as a resource that stays Provisioning on failure; upjet IAM role is async and would report Failed",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F14",
      "title": "Docs condition table omits the FatalError reason relayed for ClusterReady/NodePoolsReady",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'A handful of minor naming and wording nits'"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'Smaller prose nits'"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'Smaller doc wording points'"
        }
      }
    },
    {
      "id": "F15",
      "title": "build-the-platform.md promises a stage reads Failed on errors, but Helm release/Object failures stay Provisioning",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'A handful of minor naming and wording nits'"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'Smaller wording nits'"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'Smaller doc wording points'"
        }
      }
    },
    {
      "id": "F16",
      "title": "Comment/docs describing when a stage is Pending ('only until it's composed') don't match NodePoolsReady/ServingStackReady rules",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F17",
      "title": "_pending returns a tuple though only fail() chains; a single-condition helper would be simpler",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: small craft nits left out"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F18",
      "title": "#603 asks for strict stage order, but NodePoolsReady can be True while ClusterReady isn't; update #603 to match",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "medium",
          "reason": "main.md: the PR's docs explain that choice"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F19",
      "title": "PR description credits moving reconcile checks to Accepted to the wrong commit",
      "kind": "process",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "main.md: 'Smaller wording nits'"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F20",
      "title": "Test fixtures for observed managed resources built two different ways across the six cloud functions",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "high",
          "reason": "main.md: small test issues (duplicate cases, names that differ between clouds) dropped"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        }
      }
    },
    {
      "id": "F21",
      "title": "Accepted=True reported before node pool checks run while provider activation is pending (and ClusterReady Provisioning with no message)",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F22",
      "title": "_first should be named for what it returns (a summary message)",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: 'A handful of minor naming and wording nits'"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "main.md: small craft nits left out"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        }
      }
    }
  ]
}

trace-619.json

{
  "pr": 619,
  "findings": [
    {
      "id": "F1",
      "title": "kubectl status.endpoints snippet yields empty ANTHROPIC_BASE_URL for the guide's own TLS gateway",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F2",
      "title": "Pointer to Messages API guide's port-forward doesn't work for a TLS gateway (:80 redirects, :443 cert mismatch)",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F3",
      "title": "Distributing the client block via managed settings gives everyone the same per-person key/caller",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F4",
      "title": "Page shows shell exports, not the managed-settings JSON env block #597 asks for and the PR description/commit promise",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F5",
      "title": "Wrong reason for naming services after the Claude ID: Claude Code checks the model ID it sends, not the response",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F6",
      "title": "Recommending gateway 429 retries for a single shared key is wrong (same key, no backoff/retry-after, multiplies with Claude Code retries)",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F7",
      "title": "Passive health check can eject the only endpoint after five 5xx (e.g. 529), failing requests; page should mention",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        }
      }
    },
    {
      "id": "F8",
      "title": "Example usage record doesn't match what the gateway logs (endpoint name/namespace, missing response_model, types)",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F9",
      "title": "input_tokens includes cache writes as well as reads; page should say so",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F10",
      "title": "Claim that a mixed service serves both OpenAI and Anthropic clients contradicts the XRD (OpenAI requests to Anthropic endpoint fail)",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F11",
      "title": "Page doesn't say a Claude ID with no service (via /model, new releases, or background built-in IDs) gets a 404",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F12",
      "title": "Unused modelplane.ai/external-provider label on the endpoints",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F13",
      "title": "1800s timeout reasoning (and other manifest-comment explanations) repeated in several places",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F14",
      "title": "'Setting the request timeout to 1800s here...' is a sentence fragment with no main verb",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "high",
          "reason": ""
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "high",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F15",
      "title": "Proxy drain timeout fixed at 300s cuts turns the 1800s request timeout allows during a rollout",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "low-medium",
          "reason": "Listed under 'Findings I left out'; notes #625, an open draft, would make the drain timeout configurable."
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "unrated (listed under 'Omissions worth a sentence')",
          "confidence": "low",
          "reason": ""
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "Left out 'since the reviewer had low confidence in it'."
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F16",
      "title": "Guide references cluster gw-gcp-us it never creates, and doesn't say where the TLS Secret goes",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "correctness",
            "prose",
            "craft"
          ],
          "reviewer_severity": "nit (correctness, craft); issue (prose)",
          "confidence": "high (correctness); medium (prose); low (craft)",
          "reason": "Main lists 'notes that the TLS certificate Secret's namespace and the anthropic namespace are never shown' as dropped, with no reason; the cluster part isn't mentioned."
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "Grouped under 'I left out the reviewers' low-confidence and precedent-only points', though craft rated it medium."
        }
      }
    },
    {
      "id": "F17",
      "title": "ANTHROPIC_API_KEY doesn't change the background model when ANTHROPIC_DEFAULT_HAIKU_MODEL is pinned",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F18",
      "title": "Closing sentence promises future content and will go stale",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft",
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F19",
      "title": "docs-vale reports 13 errors on the new page (PR description says Vale passes)",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F20",
      "title": "Commit trailer uses 'Part of #597.' instead of CONTRIBUTING's 'Towards #597.'",
      "kind": "process",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F21",
      "title": "'Generated with Claude Code' AI attribution line should be removed",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        }
      }
    },
    {
      "id": "F22",
      "title": "Draft #633 removes one of the subscription blockers the last section names; section will need an edit",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    }
  ]
}

trace-625.json

{
  "pr": 625,
  "findings": [
    {
      "id": "F1",
      "title": "terminationGracePeriodSeconds patch is redundant: Envoy Gateway v1.8.4 already sets it to drainTimeout + 300s, so the docstring's 'inert' claim is false; just pass drainTimeout through",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F2",
      "title": "_seconds regex matches 'm' before 'ms', so 500ms parses as 500 minutes",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F3",
      "title": "maxSurge/maxUnavailable are composed flat under envoyDeployment.strategy instead of strategy.rollingUpdate, so they're rejected or pruned; the test expects the same wrong layout",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F4",
      "title": "maxSurge/maxUnavailable typed as string: Deployment rejects non-percent strings like \"1\", and the reference manifest's unquoted integers fail the docs-manifests check; use x-kubernetes-int-or-string",
      "kind": "bug",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "craft",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F5",
      "title": "maxSurge and maxUnavailable can both be 0, which Kubernetes rejects; add a CEL rule",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "correctness",
            "design"
          ],
          "reviewer_severity": "correctness: nit; design: unrated (question)",
          "confidence": "correctness: medium; design: low-medium",
          "reason": "General only: main.md says it 'held back the naming, test and style nits because of the Rework rating'"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "high on behaviour, low on severity",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F6",
      "title": "Rollout fields add nothing at the fixed 2 replicas: Kubernetes' 25%/25% default already rounds to surge 1, unavailable 0; drop them or justify",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F7",
      "title": "drainTimeout defaults to a fixed 300s instead of #618's 'longest route timeout the gateway serves', so raising a ModelService timeout brings the cut back",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose",
            "correctness"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F8",
      "title": "Cluster gateway's drain timeout (and rollout) isn't covered by this PR, #554 or #626; decide where it lives / track it",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "craft"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "unrated (question)",
          "confidence": "low-medium",
          "reason": "main.md: 'I dropped two low-confidence design points: the deferred cluster-gateway setting and ...'"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        }
      }
    },
    {
      "id": "F9",
      "title": "Rollout fields should mirror upstream DeploymentStrategy as proxy.strategy.rollingUpdate.{maxSurge,maxUnavailable}, and the layout should be settled now since #554 will reuse it",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "unrated (question; 'this is minor')",
          "confidence": "low",
          "reason": "General only: main.md says it 'held back its naming, test and style findings' after the Rework rating"
        }
      }
    },
    {
      "id": "F10",
      "title": "proxy should take default: {} so the 300s default applies without writing proxy:, removing the duplicate _DRAIN_TIMEOUT fallback in the function",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "General only: main.md says it 'held back the reviewers' naming, test and style findings' after the Rework rating"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "General only: main.md says it 'held back the naming, test and style nits because of the Rework rating'"
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "General only: main.md says it 'left out its findings on naming, tests and style' after the Rework rating"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft",
            "prose"
          ],
          "reviewer_severity": "craft: issue; prose: nit",
          "confidence": "medium",
          "reason": "General only: main.md says it 'held back its naming, test and style findings' after the Rework rating"
        }
      }
    },
    {
      "id": "F11",
      "title": "docs/content/models/model-service.md still says in-flight requests get five minutes on gateway restart; should point at spec.proxy.drainTimeout",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F12",
      "title": "Docs should warn that changing drainTimeout itself rolls the proxy, and the first rollout still drains on the old 300s",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F13",
      "title": "Commit lacks Signed-off-by and issue trailer; PR description should open with 'Part of #618'",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "high",
          "reason": "main.md: 'Missing sign-off: CI's DCO check catches it.'"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "blocking",
          "confidence": "high",
          "reason": "main.md: 'Commit sign-off is left to the DCO check.'"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F14",
      "title": "Commit message falsely credits the grace-period patch with keeping the pod alive through draining",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "blocking",
          "confidence": "high",
          "reason": ""
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "blocking",
          "confidence": "high",
          "reason": ""
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F15",
      "title": "PR description says nix run .#build and nix flake check weren't run",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "high",
          "reason": ""
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "high",
          "reason": ""
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "high",
          "reason": ""
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "high",
          "reason": ""
        }
      }
    }
  ]
}

trace-632.json

{
  "pr": 632,
  "findings": [
    {
      "id": "F1",
      "title": "Any ModelDeployment author can mount any hostPath on shared nodes; node paths belong to platform teams and nothing restricts it",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F2",
      "title": "hostPath ties a deployment to a node layout and is invisible to placement; use emptyDir plus #630's device selection instead",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low-medium",
          "reason": "Low-confidence design questions: scheduling visibility for node-local disks"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "Low-confidence design points: nothing linking a hostPath to nodes that have the path"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F3",
      "title": "Documented hostPath + init-container weight staging gets ahead of #630/#72/ModelCache abstractions; escape hatch vs Modelplane-owned storage",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "Low-confidence design questions: a second cache tier written by hand"
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": ""
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F4",
      "title": "Volume source schema: fixed allowlist of 11 sources but each is free-form inside, so typos or hostPath: {} pass admission",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "design",
            "correctness"
          ],
          "reviewer_severity": "question (design); issue sub-point (correctness)",
          "confidence": "low",
          "reason": "Left out low-confidence design questions: the fixed list of volume sources"
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": ""
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low-medium",
          "reason": "Low-confidence design points: the free-form volume source schemas"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F5",
      "title": "PVC/configMap/secret/csi volumes need their objects or drivers on every target InferenceCluster; document that contract",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "low",
          "reason": "Left out low-confidence design questions: who creates PVCs"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F6",
      "title": "design/design.md (and user-authored-routing.md) out of date: template contents don't list volumes, init containers, reserved names/paths",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "design",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "design",
            "prose"
          ],
          "reviewer_severity": "question (design); nit (prose)",
          "confidence": "low",
          "reason": "Low-confidence design points: design.md now being out of date"
        }
      }
    },
    {
      "id": "F7",
      "title": "'Passes them through as written' / 'written as in a Kubernetes pod spec' is inaccurate: init containers and mounts are a pruned subset",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F8",
      "title": "Mounts referencing undeclared volumes (typos, model-cache without modelCacheRef) pass admission and fail only on the workload cluster",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "design",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F9",
      "title": "Python models edited by hand; regenerate with nix run .#build (missing docstring, stale fs://apis hash in .lock.json)",
      "kind": "process",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft",
            "correctness"
          ],
          "reviewer_severity": "blocking (craft); nit (correctness)",
          "confidence": "high",
          "reason": "The PR's own checklist already has an open item to regenerate them"
        }
      }
    },
    {
      "id": "F10",
      "title": "Commit message says models were edited by hand; drop that note from history",
      "kind": "process",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "3": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": ""
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F11",
      "title": "Reserved mountPath rule compares exact strings, so a trailing slash (/dev/shm/, /mnt/models/) bypasses it and shadows Modelplane's mount",
      "kind": "bug",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "correctness"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "Real, but unlikely to come up"
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F12",
      "title": "Init container field descriptions copied from the engine container don't apply (VLLM_HOST_IP env example, engine mount restriction, resources message)",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "prose",
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "high (prose); low (craft)",
          "reason": ""
        },
        "2": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "prose",
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft",
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "Rework rule: held back, including the init container descriptions copied from the engine container"
        }
      }
    },
    {
      "id": "F13",
      "title": "Docs example starts at a bare template:, which readers may put under spec.template.spec where volumes don't exist",
      "kind": "doc",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "prose"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "prose"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": ""
        },
        "5": {
          "status": "missed"
        }
      }
    },
    {
      "id": "F14",
      "title": "Add a compose-model-deployment test that copying the template into the ModelReplica keeps volumes, mounts and init containers",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "correctness",
            "craft"
          ],
          "reviewer_severity": "nit (correctness); issue (craft)",
          "confidence": "high (correctness); medium (craft)",
          "reason": "Held back because of the Rework rating: naming and test findings"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "Craft findings on test helpers and naming held back because of the Rework rating"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "Smaller craft nits: tests the PR could add"
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "Rework rule: naming, test and style nits held back"
        }
      }
    },
    {
      "id": "F15",
      "title": "template_pod_fields helper bundles init containers and pull secrets and is poorly named; split into init_containers(member)",
      "kind": "nit",
      "severity": "nit",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "issue",
          "confidence": "medium",
          "reason": "Held back because of the Rework rating: naming and test findings"
        },
        "2": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "medium",
          "reason": "Craft findings on test helpers and naming held back because of the Rework rating"
        },
        "3": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "craft"
          ],
          "main_only": false
        },
        "5": {
          "status": "dropped",
          "reviewers": [
            "craft"
          ],
          "reviewer_severity": "nit",
          "confidence": "low",
          "reason": "Rework rule: naming, test and style nits held back"
        }
      }
    },
    {
      "id": "F16",
      "title": "Hardcoded dshm/model-cache reservations conflict with #574/#281 per-cache volume names and paths; decide who owns collision checks",
      "kind": "design",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F17",
      "title": "Docs make dshm and model-cache public API by inviting users to mount them by name; renaming later breaks deployments",
      "kind": "design",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "dropped",
          "reviewers": [
            "design"
          ],
          "reviewer_severity": "question",
          "confidence": "medium-low",
          "reason": ""
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F18",
      "title": "PR description and commit disagree: description excludes hostPath/nfs/image and promises name rules (modelplane- prefix, mount-name check) the commit lacks",
      "kind": "process",
      "severity": "blocking",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design",
            "correctness",
            "prose"
          ],
          "main_only": false
        }
      }
    },
    {
      "id": "F19",
      "title": "If hostPath goes, disk-backed emptyDir only lands on NVMe on EKS after #630 and restages on every pod replacement; docs should say what other clusters get",
      "kind": "doc",
      "severity": "issue",
      "runs": {
        "1": {
          "status": "missed"
        },
        "2": {
          "status": "missed"
        },
        "3": {
          "status": "missed"
        },
        "4": {
          "status": "missed"
        },
        "5": {
          "status": "posted",
          "reviewers": [
            "design"
          ],
          "main_only": false
        }
      }
    }
  ]
}

trace-prompt.md

You're working out where an AI code review bot loses findings. The bot reviewed PR #{PR} five times independently. In each run, four reviewer subagents (design, correctness, craft, prose) each wrote a report of findings to a main agent. The main agent checked those findings, dropped the ones it judged not worth posting, and wrote the posted review.

Across the five runs the posted reviews raised the distinct findings listed in {DIR}/clusters-{PR}.json. Each finding's runs lists the runs whose posted review raised it. For each run N, {DIR}/review-{PR}-N/ holds:

• reports.md: the four reviewers' final reports to the main agent.
• main.md: the main agent's own text and reasoning, which is sparse.
• x/review/: the posted review (summary.md and comments/*.md).

For every finding, and every run 1 to 5, classify the run as one of:

• posted: the run is in the finding's runs.
• dropped: not posted, but at least one reviewer's report in that run raised the same underlying problem. Same problem means the same thing wrong, even if worded differently or placed on a different line. Be strict: a report that touches the same code for a different reason doesn't count.
• missed: not posted, and no report in that run raised it.

For posted runs, also note whether any report raised it, or whether the main agent added it itself (main_only). For dropped runs, give the reviewer, the severity and confidence the reviewer gave it, and any reason for dropping it that main.md gives (usually none).

Write JSON to {DIR}/trace-{PR}.json:

{
  "pr": 632,
  "findings": [
    {
      "id": "F1",
      "title": "copied from clusters",
      "kind": "copied from clusters",
      "severity": "copied from clusters",
      "runs": {
        "1": {"status": "posted", "reviewers": ["design"], "main_only": false},
        "2": {"status": "dropped", "reviewers": ["correctness"], "reviewer_severity": "issue", "confidence": "low", "reason": ""},
        "3": {"status": "missed"}
      }
    }
  ]
}

Then reply with a short table: finding, kind, severity, and the status for each run (P, D or M), followed by any classification you were unsure of. Don't judge whether findings are right. Don't edit anything outside {DIR}.