diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index ca538a8..dad8f59 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -482,7 +482,7 @@ An override is a PR comment from an authorized person: /oc-review override ``` -- The summary comment prints the exact line to copy, with the full 40-character SHAs, when the gate fails for waivable reasons only. +- The summary comment prints the exact line to copy, with the full 40-character SHAs, when the gate fails for waivable reasons only. The line is in the collapsed "Waiver" block (see "Summary comment" under "Publishing (code)"). The line format does not change. - Authority is bound to the exact SHA pair **and** to the PR's current push epoch. The push epoch is the server time of the newest of these PR timeline events ([timeline events](https://docs.github.com/en/rest/using-the-rest-api/issue-event-types)): `head_ref_force_pushed`, `head_ref_deleted`, `head_ref_restored`, `base_ref_changed`, `reopened`. A normal push always produces a new head SHA, so the SHA check covers it. A return to an earlier head SHA (A to B to A) needs a force push, which moves the epoch, so an old grant for A does not count in a new run ("Which run counts" covers an old run's pass). V16 tests that fork PR branches emit these events. `reopened` is in the list because a head branch reset while the PR is closed shows no other event (V16, K-12). - Immediately before it computes the gate, the publisher reads all comments and the full timeline again (every page). If the timeline cannot be read completely, no override is accepted. It accepts an override comment only when all of these hold: - both SHAs equal the snapshot's head SHA and merge base SHA exactly @@ -537,7 +537,7 @@ The gate record is a run of the central workflow on the PR head SHA. GitHub crea - **Cross-PR effect (V6 g2).** Two PRs with one head H share the runs on H. A failing or pending run on PR 2 blocks PR 1. A pass on PR 2 clears PR 1, also when PR 1 has no override. An override covers the code pair (head and merge base), not one PR, so an override on PR 2 can clear PR 1. Anyone who can open a PR with head H (a fork can fetch the commit) can start a run on H, so they can block PR 1 or start a new run on it. - For an immediate stop, a maintainer closes the PR. Reopen then starts a new run; it is not an atomic swap of the old result. -**Summary writes.** One sticky comment, best effort. Each gate job writes its own result into it, so the last writer wins, whichever run that is. The summary says "result for run R, attempt N", names its head SHA and merge base, and links to its run. A job that fails after its summary write can leave a `pass` summary next to a failed run, a normal cancel of the run before the gate job starts can leave a `pass` summary next to a `cancelled` run (the gate job still runs; a normal cancel while it runs was not tested), and a repo writer can post a bot comment with the marker. Neither changes the gate. The summary says: "The result that counts is the required workflow in the merge box." +**Summary writes.** One sticky comment, best effort. Each gate job writes its own result into it, so the last writer wins, whichever run that is. The summary names its run and attempt and links to its run (one link, in the provenance line), shows the short head SHA there, and holds the full head SHA, merge base SHA and base ref in the record. A job that fails after its summary write can leave a `pass` summary next to a failed run, a normal cancel of the run before the gate job starts can leave a `pass` summary next to a `cancelled` run (the gate job still runs; a normal cancel while it runs was not tested), and a repo writer can post a bot comment with the marker. Neither changes the gate. The provenance line says: "The required workflow in the merge box is the result that counts." ### Pilot and enforcement @@ -569,10 +569,29 @@ There is one build of the central workflow. Its gate job always applies every ru ## Publishing (code) - **Escape all untrusted text** before posting: strings from Claude and strings from the PR (title, paths, package names, URLs, prefilter evidence). Remove control characters, replace `@` with `@โ€‹` (no pings), render as text with no Markdown links, images, HTML or autolinks, and truncate to fixed limits. Redact credential-like strings. Escaping also breaks issue references (`#123`, `GH-123`) and runs of 7 or more hex characters, so no text becomes a cross-reference or a commit link. -- **Label model output:** each pass's text sits under a note that it is model output from untrusted input, to check before acting on it. +- **Label model output:** each pass's text sits under a note that it is model output from untrusted input, to check before acting on it. The note stands at the top of the finding details and above the model summaries, both in the collapsed record. A finding title in the visible findings table is model text without the note: its "Found by" cell names the model pass that reported it. - **Inline comments:** one review per run, with `event: COMMENT` and `commit_id` set to the bundle's head SHA, for verified and inline-eligible findings only. Each finding has two keys, both computed from the shown values: each string redacted and cut to its field limit as the publisher shows it, never the raw value, so a published key is not a guess check for a redacted secret. The D14 key is the SHA-256 of file, side, title and quote, joined by NUL. The full key is the SHA-256 of the JSON list of every field that the reader sees: pass, category, severity, confidence, file, side, line, title, quote and explanation (Python `json.dumps` with its default settings, which gives the same ASCII text for the same values). Two findings share a full key only when they show the same text in every one of these fields. Both keys are kept in a hidden `` at the start of each inline comment and read back only from review comments by `github-actions[bot]` whose body starts with that marker and holds no other; a marker with one key is not read, so its finding is posted again. The login proves only that some Actions workflow in the repo wrote the comment (as for the summary), and the keys are public hashes of shown text, so the marker alone proves nothing: the code also checks the text of the comment (review r-p3-cut-final Important 2). In one run, findings are deduped by full key, so each distinct finding gets its own inline comment, even when a cut or a redaction makes two findings equal in file, title and quote (P3-2). Across runs, a finding is not posted again when an earlier comment has its D14 key, the same path and side, and the layout of an inline comment (the marker, a finding line of the schema enums and a pass name, the finding's escaped title, an explanation and the fixed note); the line and the explanation can differ, because the D14 key covers neither. The model words an explanation again on each call and a push moves lines, so the full key of the same issue is rarely the same on two runs, and the D14 key is what keeps a re-run or a push from posting every finding again. The summary points to the earlier comment only when that comment is, in full, the body this run would write for the finding (so its full key matches too), at the same path, side and line (`line`, or `original_line` when GitHub marks the comment outdated). Otherwise the summary shows the finding in full (title, severity, place, explanation and quote), marked as different from the inline comment of an earlier run. So no explanation is hidden behind an earlier comment that says something else, and a forged comment with the D14 key can at most move a finding from its inline comment to its full summary block. That block is subject to the summary size limit below: a high or critical finding is never cut, but a low or medium one can be cut at the largest sizes (residual, see below). If nothing is new, no review is posted. If GitHub rejects the review (HTTP 422, or any other failure), the code does not retry or split it: no inline comment is posted for this run, and the summary shows all of its findings in full. The gate does not change. -- **Summary comment:** one sticky comment, found by the hidden marker `` and an author check (the `github-actions[bot]` identity). A comment with the marker from any other author is ignored. The author check proves only that some Actions workflow in the repo wrote it, so the summary is never a trust source for the gate (see "Summary writes"). It contains the run ID and attempt, the gate result, every reason with its waivable flag, the head SHA, merge base SHA and base ref it was computed for, the override (if any), prefilter results, coverage, the dependency delta, findings per pass, unverified findings, the token usage of the run per pass, and a link to the workflow run. Usage is token counts only (input, output, cache write, cache read, iterations); the code holds no price and the repo shows no currency. Findings that were posted inline show one line that points to the inline comment. The override command and the links to override comments are built from trusted values (server URL, repository, PR number, comment ID), never from a field the API returned. -- **Size limit and cut order:** a comment body is at most 65,536 characters, and the code aims at 64,000. If the body is too long, it cuts only these blocks, in this order, each from its end: unverified low and medium findings, low and medium findings, dependency delta, prefilter signals. Each cut says "truncated, see the run log". The result, the reasons, the SHAs, the override data, the high and critical findings (verified or not, so also one whose file path analyze cut), the model summaries and the hard block and coverage details are never cut. If those alone do not fit, the run fails closed: the comment becomes a short notice of fixed size with result `fail`, the run and attempt, the SHAs and base ref, the computed gate result, a count per reason code with its waivable flag, the number of valid overrides and the run link, and the job exits 1. This can turn a computed `pass` into a failed run. Residual (review r-p3-cut-final Important 1, coordinator ruling 2026-10-27): there is one summary comment and no continuation comment. So on a re-run whose findings changed keys at the largest allowed sizes, the cut can drop the explanation of a low or medium finding from the comment (it stays in the run log), and the never-cut high and critical blocks can exceed the limit, so the run fails closed even though the computed risk did not change. Bounded continuation comments that hold every summary-only finding are planned (build plan, phase 4). +- **Summary comment:** one sticky comment, found by the hidden marker `` and an author check (the `github-actions[bot]` identity). A comment with the marker from any other author is ignored. The author check proves only that some Actions workflow in the repo wrote it, so the summary is never a trust source for the gate (see "Summary writes"). The comment is built for a fast human judgment: a reader gets the result and the next step in about three seconds, and everything else is one click away. It never says "approved" and never implies approval; the provenance line says "This bot never approves a PR." The override command and the links to override comments are built from trusted values (server URL, repository, PR number, comment ID), never from a field the API returned. The layout, from top to bottom: + + ``` + hidden marker, first line + ## [light] RESULT: OC Claude review 1. result heading + > [!CALLOUT] how serious, who can clear it, next step 2. one colored callout + | Severity | Effect | Where | Finding | Found by | 3. findings table, worst first + | File checks | Review | Security | Supply chain | ... | 4. checks strip (never cut) + run link, short head SHA, never approves 5. provenance +
Waiver: the exact line to copy
6. only when waivable-only +
Full review record
7. everything else, collapsed + ``` + + - **Result heading:** `## ๐ŸŸข PASS: OC Claude review` for a pass with no findings, `## ๐ŸŸก PASS: OC Claude review, N finding(s) to consider` for a pass with non-blocking findings, and `## ๐Ÿ”ด FAIL: OC Claude review` for every fail. A fail caused by a pass, job or data that did not complete reads `... did not complete`. The result comes from the gate (status, reasons, override), never from model text. Every emoji has its word next to it, so color never carries meaning alone. + - **Callout rules:** `[!TIP]` (green) for a pass with no findings. `[!NOTE]` (blue) for a pass with non-blocking findings. `[!TIP]` for a pass by an applied waiver, with "A maintainer waiver was applied by LOGIN. Findings stay listed." **Every fail uses the red `[!CAUTION]` callout, waivable or not (owner ruling, decision P3-5).** `[!WARNING]` is never used for a fail result. Whether a waiver can clear the fail stays visible in the callout text ("A maintainer can waive it." or "No waiver can clear it.") and in the Effect column. The callout holds two bold sentences: how serious and who can clear it, then the next step (for the author, or for a maintainer when the author has nothing to change). The wording is a fixed table keyed by the worst reason, with fixed labels for reason codes and check IDs ("bidi-control" reads "Hidden text-direction control"); no internal ID shows. + - **Findings table:** one row per place, worst first: hard blocks (๐Ÿšซ), passes or jobs that did not run (โš ๏ธ), then model findings by severity (โ›” critical, ๐Ÿ”ด high, ๐ŸŸ  medium, ๐ŸŸก low), verified before unverified within a severity. Columns: severity, effect, where (file and line in a code span), finding (the title, with a pointer to the inline comment when there is one), found by. The Effect column comes from gate code: "Blocks, no waiver", "Blocks, waivable", "Blocks, waived" (an applied waiver) or "Does not block". A medium finding in a blocking category reads "Does not block" (gate truth: a blocking category blocks only at high or critical severity). An unverified finding shows "โŒ not checked" after its severity and still sorts by its severity. The table is left out when there are no rows. + - **Merge rule:** model findings merge into one row only when the shown file, side, line and gate effect are all the same. Findings with different effects stay on separate rows. A merged row shows the highest severity, the title of the highest-severity member (ties go to the pass order: review, security, supply-chain), and every pass that reported it, once each, in pass order, with its highest severity. The inline pointer shows only when every member has an inline comment; otherwise the cell says the other findings are in the record only. A merged row can be cut only when every member can be cut (one protected member protects the row). A hard block never merges with a model finding: the hard block is code and the finding is model output. The record keeps every title, one row per finding. + - **Checks strip:** one row with file checks (hard blocks and signals), each of the three passes (ran with its finding count, failed, skipped, missing), dependency review and coverage. It is never cut. + - **Collapsed Waiver block:** only when the gate fails for waivable reasons only. It holds the exact override line in a code block with the full SHAs, who may post it, the re-run method, the no-edit and no-reopen rules, and that a waiver covers only this head and merge base. A hard block, a failed pass and every other not-waivable reason show no waiver line. + - **Collapsed record:** the reviewed snapshot (full head SHA, merge base SHA, base ref, run), every reason with its waivable flag as a table with plain labels, the maintainer waivers (author, time, reason, comment link, applied, waived reasons), the code checks (hard block evidence, coverage with every unavailable file, dependency changes, dependency-review job), the signals (context only, never block; the first-time contributor signal keeps its caveat), the dependency changes, the finding details (a per-pass table with category, confidence, citation state, posting state and each pass's own title, plus the explanation and quote of every finding that has no inline comment and the posting state of each), the model summaries (one row per pass, with status, model and the escaped summary, or "No summary. Reason: ..."), and the token usage per pass. Usage is token counts only (input, output, cache write, cache read, attempts); a missing value reads "not recorded", never zero, and the total reads "Recorded total" when any value is missing. The code holds no price and the repo shows no currency. +- **Size limit and cut order:** a comment body is at most 65,536 characters, and the code aims at 64,000. The collapsed blocks count toward the limit. If the body is too long, it cuts only these blocks, in this order, each from its end: unverified low and medium findings, low and medium findings, dependency changes, prefilter signals. A cut removes whole table rows, list items or detail bullets, never part of one, and a merged findings row is cut only when every member is cuttable. A group that lost items shows one line with the exact phrase "N item(s) truncated, see the run log", as a table row inside a table so that the table stays whole, and the provenance line adds "Some details were truncated, see the run log." (its size is reserved up front). The `
` open and close lines are never cut, so the tags stay balanced. The heading, the callout, the findings rows for hard blocks and for high and critical findings (verified or not, so also one whose file path analyze cut), the checks strip, the provenance line, the snapshot, the reasons, the waiver block, the override data, the hard block evidence, the coverage rows, the model summaries and the token usage are never cut. If those alone do not fit, the run fails closed: the comment becomes a short notice of fixed size (heading `## ๐Ÿ”ด FAIL: OC Claude review could not publish`, a `[!CAUTION]` callout with the size note, the computed gate result, the snapshot, a table with a count per reason with its waivable flag, the number of valid overrides and the run link), and the job exits 1. This can turn a computed `pass` into a failed run. Residual (review r-p3-cut-final Important 1, coordinator ruling 2026-10-27): there is one summary comment and no continuation comment. So on a re-run whose findings changed keys at the largest allowed sizes, the cut can drop the explanation of a low or medium finding from the comment (it stays in the run log), and the never-cut high and critical blocks can exceed the limit, so the run fails closed even though the computed risk did not change. Bounded continuation comments that hold every summary-only finding are planned (build plan, phase 4). - **Full record:** whenever the comment is cut, the publisher prints the full redacted record (reasons, overrides, every finding, prefilter results, coverage, dependency delta) to the gate job log, between `::stop-commands::` and `::::` so that untrusted text cannot run workflow commands. The record also goes to the step summary when it is at most 512 KiB; over that, the step summary says that the record is in the job log. The step summary follows the comment as it stands at the end of the run: when the summary is updated after the final check and the update is not cut, the step summary has no full record; when that update fails, it keeps the record of the first, cut summary. "The run log" in each cut note means this job log. - **Gate:** the required run's result. No code calls the Checks API, and no job has `checks: write`. - **Order of writes:** inline comments, then the summary, then the final check, then the exit code. Every write, including the update of the summary after the final check, is preceded by the snapshot check. If the check shows a stale run, or cannot run (a failed read), the code writes nothing more and exits 1. The step summary line of such a run has `result: fail` and the `stale` reason code, whatever the gate result computed before. Only the code's own write targets exist: `POST .../pulls/{n}/reviews` with `event: COMMENT`, `POST .../issues/{n}/comments` and `PATCH .../issues/comments/{id}`. Any other write is refused in code. @@ -960,3 +979,4 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, - P3-2. Over-long strings (design change). Release eval [run 40517202830](https://github.com/OperationCode/.github/actions/runs/40517202830) at the candidate S1 (ccbae51) matched 12 of 13 cases: in `obfuscated-test-payload` the supply-chain pass failed with "response does not match the findings schema: 'findings'/1/'quote': [maxLength]". Phase 2 run 40445885087 matched the same case with the same prompts, so this is model variance. Cause: structured outputs cannot enforce `maxLength` (P0-12), so the API accepts a string of any length, and one long string made the whole pass `failed` and the gate fail on a real PR. Fix (coordinator ruling, 2026-10-27): before the full schema check, analyze cuts every string over its schema `maxLength` to that limit and keeps the finding. This covers `summary` (1500), `file` (400), `quote` (300), `title` (120) and `explanation` (1200), and any limit added to the schema later; a unit test checks that the cut reaches every `maxLength`. A file path is an identifier, not prose, so its cut differs: the first 399 characters and a final "/" (fix round 1, review r-p3-cut Important 1). A plain prefix of a 401-character path can be the path of another changed file, which could then verify the finding and place its inline comment on the wrong file. No file path ends in "/", and the file lookup never matches one, so a cut path names no file. The finding stays and still counts toward the gate. The system prompt also tells the model to copy the part of a longer line that shows the problem. Why it does not lower risk: a finding is never dropped, and its category, severity, confidence, side and line never change. Blocking never depends on the text, the quote or verification ("Citation check"). Whitespace normalization keeps a prefix of a quote inside the normalized text wherever the whole quote is, so a quote that verified still verifies. A cut quote that is not at that place, a cut quote that is only whitespace or holds a redacted credential, or a cut file path, stays unverified and still blocks. A cut can make two findings that differed past the cut share file, title and quote (review r-p3-cut Important 2). A redaction before the cut can do the same far below the limit: two titles that differ only in a credential both become `Token [REDACTED:jwt]` (fix round 2, review rr-p3-cut-fix1 Important A). Two findings below every limit can also share file, side, title and quote and differ in explanation or severity. So each inline comment carries two keys ("Publishing (code)", fix round 3, review rr-p3-cut-fix2): the D14 key (file, side, title and quote) and a full key of every field that the reader sees, both of the shown, redacted text. In one run, findings are deduped by full key, so each distinct finding gets its own inline comment. Across runs, a finding whose D14 key is already on the PR is not posted again; when its full key matches none of the earlier comments with that D14 key, the summary shows it in full. So none hides the explanation of another, and a re-run or a push does not post every finding again (fix round 2 keyed only on every shown field, which re-posted almost every finding on each run, because the explanation and the line change). Fix round 4 (review r-p3-cut-final, coordinator rulings 2026-10-27): the marker alone is not proof that a comment shows a finding, because any Actions workflow in the repo writes as `github-actions[bot]` and the keys are public hashes. The D14 skip needs the inline layout, path, side and title, and "see the earlier comment" needs the exact body at the same line; a forged comment can at most move a finding to its full summary block. A cut file path makes a finding unverified, and an unverified high or critical finding now keeps its never-cut rank in the summary, so the summary shows every such explanation or the run fails closed. The residual of one summary comment (a low or medium explanation can be cut on a changed-key re-run at the largest sizes; capacity can fail a run closed) is stated in "Publishing (code)"; continuation comments are planned for phase 4. Findings that are equal in every shown field after the cut show one inline comment; the publisher cannot know the text past the cut, and that comment holds all the text of each. A credential-like string in an over-long string is redacted before the cut, so the cut cannot leave part of a secret that the publisher would then not recognize. The publisher escapes every character of a cut string, so a cut inside Markdown, HTML or an escape still shows as text (unit test). A cut title, explanation or summary has no truncation mark, because the publisher shows one only for its own cuts. A new candidate with this change needs its own eval (P0-15). - P3-3. Phase 3 results and spec corrections (2026-10-27). Four spec claims did not survive the live tests, and each is now corrected in place. (1) The merge box counts only runs at the pinned SHA, not the newest run on the head SHA; a pin move back revives older passes at that SHA (PR 28 on 2026-10-27 at 18:34:53Z; PRs 28 and 37 at 21:38:44Z). A forward move fails closed (the box blocks at once). The rollback is a residual, covered by the rule to start a new run on each open PR after any pin move; it joins the residuals in rollout step 7. (2) Repo Actions settings do not affect the required run (V12 pass); the role table no longer lists them as an admin power against the gate. (3) A normal cancel before the gate job starts does not stop the `always()` gate job (run 40565538379), which then writes a `pass` summary next to a `cancelled` run; a normal cancel while the gate job runs was not tested; the run conclusion still blocks the box, and a force-cancel stopped the gate job after the summary (run 40565910355). (4) The artifacts of earlier attempts are gone after a re-run; the gate reads only its own attempt, so it still fails closed, and the residual is that earlier pass records cannot be read back. Release procedure: two Console edits per release (candidate SHA, then merge SHA), a tree check of the merge SHA, and a denied exchange at the merge SHA before the second edit. NOT YET RUN, pending account X: V8, V23, V6 f as a Write user, V25 and the web UI "Re-run all jobs" case. The exhausted-budget outage case stays covered by V9 and the unit tests (Q7). - P3-4. Model per pass, Sonnet 5.5 (2026-10-27). The owner proposed `claude-sonnet-5-5` for general use. The model is now a per-pass setting, and every pass uses Sonnet 5.5. The switch needed a full match of the 13 cases (P2-4 bar); Opus 5.5 would stay on any pass where Sonnet missed. Result: two eval runs at 302882f, [40589761270](https://github.com/OperationCode/.github/actions/runs/40589761270) and [40590079545](https://github.com/OperationCode/.github/actions/runs/40590079545), each matched 13 of 13 cases with no failed pass (Opus reference [run 40528717338](https://github.com/OperationCode/.github/actions/runs/40528717338), also 13 of 13). Against Opus, total tokens were 0.97x and 0.98x, output tokens 0.71x and 0.76x, and eval wall time 84 s and 82 s against 138 s. No currency figures were recorded. The switch takes effect at the next release: the production rule pins a SHA, so merging does not change production. +- P3-5. Summary comment redesigned for fast human judgment (owner and coordinator, 2026-10-27). The comment now opens with a result heading and one colored callout, then a findings table, a checks strip and a provenance line, with the waiver and the full record collapsed ("Summary comment" under "Publishing (code)"). The owner chose the Fable design with these rulings: every fail uses the red `[!CAUTION]` callout and the red heading, waivable or not (a waivable fail on a fake check was misread as a mild warning under `[!WARNING]`, so `[!WARNING]` is not used for a fail); the waiver line stays in the collapsed Waiver block; model summaries and token counts stay in the collapsed record; duplicate findings merge only at the same file, side, line and gate effect; a medium finding in a blocking category reads "Does not block"; the visible line shows the 7-character head SHA and the record shows the full SHAs; emoji are allowed, each next to its word. Machine markers, the override line format, the size limit and the overflow path are unchanged. diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index 64d09bc..2e27339 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -135,6 +135,12 @@ def strip_controls(text: str) -> str: return "".join(out) +def _defang(line: str) -> str: + """Steps 5 of `escape_text`: break references, autolinks and commit SHAs.""" + line = _HEX_RUN.sub(_break_hex, _DEFANG.sub(ZWSP, line)) + return "".join(ch + ZWSP if ch in _AT_SIGNS else ch for ch in line) + + def _escape_line(line: str) -> str: line = _HEX_RUN.sub(_break_hex, _DEFANG.sub(ZWSP, line)) out: list[str] = [] @@ -877,25 +883,49 @@ def build_review( CUT_ORDER = (CUT_UNVERIFIED, CUT_LOW_MEDIUM, CUT_DEPENDENCY, CUT_SIGNALS) +def _usage_numbers(meta: Mapping[str, Any]) -> dict[str, int]: + usage = meta.get("usage") + out: dict[str, int] = {} + if isinstance(usage, Mapping): + for k, v in usage.items(): + if isinstance(k, str) and isinstance(v, int) and not isinstance(v, bool) and v >= 0: + out[k] = v + return out + + +USAGE_FIELDS = ( + ("input_tokens", "input"), + ("output_tokens", "output"), + ("cache_creation_input_tokens", "cache write"), + ("cache_read_input_tokens", "cache read"), + ("iterations", "attempts"), +) + + @dataclass class _Block: text: str rank: int | None = None # None: never cut group: str = "" + cols: int = 0 # table columns of a table row; 0 for any other block -def _cut_note(n: int) -> str: +def _cut_note(n: int, cols: int = 0) -> str: + if cols: + return f"| {n} item(s) {TRUNCATED_NOTE}. |" + " |" * (cols - 1) return f"- {n} item(s) {TRUNCATED_NOTE}." def _fit(blocks: Sequence[_Block], limit: int) -> tuple[str | None, bool]: """Join blocks; cut by rank in `CUT_ORDER`, from the end of each rank, until it fits. Each group that lost blocks gets one note where its first - cut block was. The space left after the never-cut blocks is the budget of - the cut ranks. Returns (None, True) when the never-cut blocks alone do not - fit: the caller fails closed (D8).""" + cut block was (a table row for a table group, so the table stays whole). + The space left after the never-cut blocks is the budget of the cut ranks. + Returns (None, True) when the never-cut blocks alone do not fit: the + caller fails closed (D8).""" keep = [True] * len(blocks) cut: dict[str, int] = {} + cols_of: dict[str, int] = {} size = sum(len(b.text) + 1 for b in blocks) - 1 # joined with "\n" for rank in CUT_ORDER: for i in reversed(range(len(blocks))): @@ -906,8 +936,9 @@ def _fit(blocks: Sequence[_Block], limit: int) -> tuple[str | None, bool]: continue keep[i] = False n = cut.get(b.group, 0) + cols_of.setdefault(b.group, b.cols) size -= len(b.text) + 1 - size += len(_cut_note(n + 1)) + 1 - (len(_cut_note(n)) + 1 if n else 0) + size += len(_cut_note(n + 1, b.cols)) + 1 - (len(_cut_note(n, b.cols)) + 1 if n else 0) cut[b.group] = n + 1 if size > limit: return None, True @@ -916,86 +947,262 @@ def _fit(blocks: Sequence[_Block], limit: int) -> tuple[str | None, bool]: if k: out.append(b.text) elif b.group in cut: - out.append(_cut_note(cut.pop(b.group))) + out.append(_cut_note(cut.pop(b.group), b.cols)) return "\n".join(out), not all(keep) -def _waivable(r: verdict.Reason) -> str: - return "waivable by override" if r.waivable else "not waivable" +# --------------------------------------------------------------------------- +# Labels. Reason codes and check IDs never show: each maps to a fixed label. +# --------------------------------------------------------------------------- +REASON_LABELS: Mapping[str, str] = { + verdict.R_JOB_RESULT: "A required job did not succeed", + verdict.R_ARTIFACT: "Review data could not be read", + verdict.R_PASS_RECORDS: "A review pass failed", + verdict.R_STALE: "Stale review", + verdict.R_SUPERSEDED: "A newer required run exists", + verdict.R_BASE_NOT_DEFAULT: "Wrong base branch", + verdict.R_HARD_BLOCK: "Hard block from the file checks", + verdict.R_DEPENDENCY_REVIEW: "Dependency review did not succeed", + verdict.R_SIZE_GATE: "Change too large for the model review", + verdict.R_BLOCKING_FINDING: "Blocking finding", +} +REASON_FALLBACK = "A required check failed" + +CHECK_LABELS: Mapping[str, str] = { + "bidi-control": "Hidden text-direction control", + "bidi-control-in-path": "Hidden text-direction control in a file name", + "foreign-source": "Dependency from a new source", + "fresh-publish": "Recently published dependency", + "incomplete-coverage": "Incomplete file coverage", + "incomplete-dependency-data": "Incomplete dependency data", + "base-dependency-file-unparsed": "Dependency file could not be read", + "zero-width": "Hidden zero-width character", + "invisible-character": "Hidden invisible character", + "ai-directed-text": "Text aimed at an AI reviewer", + "credential-like-string": "Credential-like string", + "dependency-install-script": "Dependency with an install script", + "package-json-install-script": "Install script added to package.json", + "pypi-sdist-only": "Python package with source distribution only", + "manifest-without-lockfile": "Dependency manifest without a lock file", + "unsupported-dependency-file": "Dependency file type not checked", + "sensitive-path": "Sensitive path changed", + "pyproject-build-system": "Build system changed", + "binary-file": "Binary file added", + "submodule-change": "Submodule changed", + "became-executable": "File became executable", + "minified-js": "Minified script", + "long-line": "Very long line", + "encoded-blob": "Encoded blob", + "first-time-contributor": "First-time contributor", +} +# Signals that point at a sensitive place get the warning mark. All other +# signals are context only. +_WARN_SIGNALS = frozenset({"sensitive-path"}) + +PASS_LABELS: Mapping[str, str] = {"review": "Review", "security": "Security", "supply-chain": "Supply chain"} +CODE_ROW = "Workflow (code, no model)" +FILE_CHECKS_ROW = "File checks (code, no model)" + +SEVERITY_CELLS: Mapping[str, str] = { + "critical": "โ›” **Critical**", + "high": "๐Ÿ”ด **High**", + "medium": "๐ŸŸ  **Medium**", + "low": "๐ŸŸก **Low**", +} +_SEVERITY_ORDER = {"critical": 0, "high": 1, "medium": 2, "low": 3} +_PASS_ORDER = {name: i for i, name in enumerate(verdict.PASS_NAMES)} + +EFFECT_WAIVABLE = "Blocks, waivable" +EFFECT_NO_WAIVER = "Blocks, no waiver" +EFFECT_WAIVED = "Blocks, waived" +# Gate truth: a medium finding in a blocking category does not block, so it +# reads "Does not block" (coordinator ruling 6). +EFFECT_NONE = "Does not block" + +HARD_BLOCK_CELL = "๐Ÿšซ **Hard block**" +NOT_RUN_CELL = "โš ๏ธ **Not run**" +BLOCKED_CELL = "๐Ÿšซ **Blocked**" + +# Reasons that mean part of the review did not complete. +_INCOMPLETE_CODES = frozenset( + {verdict.R_PASS_RECORDS, verdict.R_ARTIFACT, verdict.R_JOB_RESULT, verdict.R_DEPENDENCY_REVIEW} +) -def _usage_numbers(meta: Mapping[str, Any]) -> dict[str, int]: - usage = meta.get("usage") - out: dict[str, int] = {} - if isinstance(usage, Mapping): - for k, v in usage.items(): - if isinstance(k, str) and isinstance(v, int) and not isinstance(v, bool) and v >= 0: - out[k] = v - return out +_WAIVER_NEXT = 'fix and push, or ask a maintainer to post the waiver line (under "Waiver" below).' -USAGE_FIELDS = ( - ("input_tokens", "input"), - ("output_tokens", "output"), - ("cache_creation_input_tokens", "cache write"), - ("cache_read_input_tokens", "cache read"), - ("iterations", "attempts"), -) +def _reason_label(code: str) -> str: + return REASON_LABELS.get(code, REASON_FALLBACK) -def _usage_text(nums: Mapping[str, int]) -> str: - parts = [f"{label} {nums[k]}" for k, label in USAGE_FIELDS if k in nums] - return ", ".join(parts) if parts else "no usage recorded" +def _check_label(check_id: Any) -> str: + label = CHECK_LABELS.get(check_id) if isinstance(check_id, str) else None + return label or _esc("label", check_id) -def _finding_where(f: verdict.CitedFinding) -> str: - return f"{_esc('file', f.file)} line {f.line} ({f.side})" +def _n(count: int, word: str) -> str: + return f"{count} {word}" + ("" if count == 1 else "s") -def _finding_block(f: verdict.CitedFinding, inline: InlineOutcome) -> _Block: - key = finding_key(f) - head = f"- {_finding_line(f)}, at {_finding_where(f)}: {_esc('title', f.title)}" - # A high or critical finding is never cut, also when it is unverified - # (a cut file path makes it so). - protected = f.severity in verdict.BLOCKING_SEVERITIES - rank = None if protected else CUT_LOW_MEDIUM - group = f"findings-{f.pass_name}" +def _code_span(kind: str, text: str, suffix: str = "") -> str: + """A code span that is safe in a table cell. `escape_field` backslash-escapes + punctuation, which would show inside a code span, so a span is cleaned + and cut here instead: redacted, cut to the field limit, no backticks.""" + return _span_of(*_clean_text(text, LIMITS[kind], False), suffix) + + +def _span_of(joined: str, cut: bool, suffix: str = "") -> str: + """The code span of text that `_clean_text` already cut: one marker only.""" + joined = _defang(joined).replace("`", "'").replace("<", "โ€น").replace("|", "\\|") + ("โ€ฆ" if cut else "") + return f"`{joined or 'unknown'}{suffix}`" + + +def _cell(text: str) -> str: + return text.replace("\n", " ") + + +def _row(*cells: str) -> str: + return "| " + " | ".join(_cell(c) for c in cells) + " |" + + +# --------------------------------------------------------------------------- +# Findings +# --------------------------------------------------------------------------- + +_POSTING_TEXT: Mapping[str, str] = { + "inline": "inline", + "earlier": "inline, earlier run", + "changed": "differs from the inline comment of an earlier run", + "not-posted": "inline comment not posted", + "meta": "PR title or body", + "outside": "outside the diff", + "unverified": "not posted", +} +_INLINE_STATES = frozenset({"inline", "earlier"}) + + +def _posting_state(f: verdict.CitedFinding, inline: InlineOutcome) -> str: if not f.verified: - return _Block( - "\\\n ".join( - ( - head, - f"Not verified: {_esc('reason', f.unverified_reason or 'unknown')}", - _esc("explanation", f.explanation), - f"Quote: {_esc('quote', f.quote)}", - ) - ), - None if protected else CUT_UNVERIFIED, - "unverified", - ) + return "unverified" + key = finding_key(f) if key in inline.posted: - return _Block(f"{head}. See the inline comment.", rank, group) + return "inline" if key in inline.earlier: - return _Block(f"{head}. See the inline comment from an earlier run.", rank, group) + return "earlier" if key in inline.changed: - where = "Differs from the inline comment of an earlier run, shown here." - elif key in inline.not_posted: - where = "Inline comment not posted, shown here." - elif f.file in verdict.META_PATHS: - where = "PR title or body, summary only." - else: - where = "Outside the diff, summary only." - return _Block( - "\\\n ".join((head, where, _esc("explanation", f.explanation), f"Quote: {_esc('quote', f.quote)}")), - rank, - group, + return "changed" + if key in inline.not_posted: + return "not-posted" + return "meta" if f.file in verdict.META_PATHS else "outside" + + +def _finding_effect(f: verdict.CitedFinding, result: verdict.GateResult) -> str: + if not f.blocking: + return EFFECT_NONE + if result.override_applied: + return EFFECT_WAIVED + return EFFECT_WAIVABLE if verdict.WAIVABLE[verdict.R_BLOCKING_FINDING] else EFFECT_NO_WAIVER + + +def _cuttable(f: verdict.CitedFinding) -> bool: + # A high or critical finding is never cut, also when it is unverified + # (a cut file path makes it so). + return f.severity not in verdict.BLOCKING_SEVERITIES + + +def _by_pass_then_severity(members: Sequence[verdict.CitedFinding]) -> list[verdict.CitedFinding]: + return sorted( + members, + key=lambda f: (_PASS_ORDER.get(f.pass_name, len(_PASS_ORDER)), _SEVERITY_ORDER.get(f.severity, 9)), ) -def _check_block(item: Any, rank: int | None, group: str) -> _Block | None: - if not isinstance(item, Mapping): - return None - text = f"- {_esc('label', item.get('id'))}: {_esc('evidence', item.get('detail'))}" +@dataclass +class _Row: + """One visible row of the findings table: a hard block, a pass or job that + did not run, or the model findings of one place that share one gate effect.""" + + sort: tuple + severity: str + effect: str + where: str + claim: str + found_by: str + rank: int | None = None + members: tuple[verdict.CitedFinding, ...] = () + is_model: bool = False + label: str = "" # a hard block row: its fixed label + + +def _found_by(pass_name: str, members: Sequence[verdict.CitedFinding]) -> str: + """One pass of a merged row: its name, its highest severity and, when it + reported more than one finding there, how many.""" + best = min(members, key=lambda f: _SEVERITY_ORDER.get(f.severity, 9)).severity + more = f", {len(members)} findings" if len(members) > 1 else "" + return f"{PASS_LABELS.get(pass_name, pass_name)} ({best}{more})" + + +def _merge_findings(result: verdict.GateResult, inline: InlineOutcome) -> list[_Row]: + """Model findings, merged into one row per (shown file, side, line, gate + effect), worst first. Different effects stay on separate rows (ruling 3). + A merged row can be cut only when every member can be cut.""" + groups: dict[tuple[str, bool, str, int, str], list[verdict.CitedFinding]] = {} + for f in result.findings: + file, cut = _clean_text(f.file, LIMITS["file"], False) + groups.setdefault((file, cut, f.side, f.line, _finding_effect(f, result)), []).append(f) + rows: list[_Row] = [] + for (file, cut, side, line, effect), members in groups.items(): + members = _by_pass_then_severity(members) + top = min(members, key=lambda f: _SEVERITY_ORDER.get(f.severity, 9)) # first of the best: pass order breaks ties + unverified = [f for f in members if not f.verified] + severity = SEVERITY_CELLS.get(top.severity, _esc("label", top.severity)) + if unverified: + severity += " โŒ not checked" + where = _span_of(file, cut, f":{line}") + (" (base side)" if side == "base" else "") + states = [_posting_state(f, inline) for f in members] + if all(s in _INLINE_STATES for s in states): + pointer = ( + "See the inline comment from an earlier run." + if all(s == "earlier" for s in states) + else "See the inline comment." + ) + elif any(s in _INLINE_STATES for s in states): + pointer = "See the inline comment. Other findings here are in the record only." + else: + names = [PASS_LABELS.get(f.pass_name, f.pass_name) for f in members] + pointer = "Summary only for " + ", ".join(dict.fromkeys(names)) + ". Details are in the record." + claim = f"{_esc('title', top.title)} {pointer}" + found_by = ", ".join(_found_by(name, [f for f in members if f.pass_name == name]) for name in dict.fromkeys(f.pass_name for f in members)) + if all(_cuttable(f) for f in members): + rank = CUT_UNVERIFIED if len(unverified) == len(members) else CUT_LOW_MEDIUM + else: + rank = None + rows.append( + _Row( + (2, _SEVERITY_ORDER.get(top.severity, 9), bool(len(unverified) == len(members)), file, line, effect), + severity, + effect, + where, + claim, + found_by, + rank, + tuple(members), + True, + ) + ) + return rows + + +def _hard_block_items(payload: Mapping[str, Any] | None) -> list[Mapping[str, Any]]: + pre = payload.get("prefilter") if isinstance(payload, Mapping) else None + items = pre.get("hard_blocks") if isinstance(pre, Mapping) else None + return [i for i in items or () if isinstance(i, Mapping)] + + +def _check_evidence(item: Mapping[str, Any]) -> str: + """The label, detail, file, subject and lines of a prefilter item, as one line.""" + text = f"{_check_label(item.get('id'))}: {_esc('evidence', item.get('detail'))}" if item.get("file"): text += f", in {_esc('path', item.get('file'))}" if item.get("subject"): @@ -1005,7 +1212,356 @@ def _check_block(item: Any, rank: int | None, group: str) -> _Block | None: nums = [str(n) for n in lines[:10] if isinstance(n, int) and not isinstance(n, bool)] if nums: text += ", lines " + " ".join(nums) + (" and more" if len(lines) > 10 else "") - return _Block(text, rank, group) + return text + + +def _hard_block_label_of(item: Mapping[str, Any]) -> str: + """The same text as `verdict._hard_block_label`, to match a reason to its item.""" + label = str(item.get("id")) + file = item.get("file") + return f"{label} in {file}" if isinstance(file, str) and file else label + + +def _hard_rows(result: verdict.GateResult, payload: Mapping[str, Any] | None) -> list[_Row]: + """A row for each hard block. Items come from the bundle; a hard block + reason with no item (missing evidence, unbound result) gets a row from the + reason, so no hard block is ever left out of the table.""" + items = _hard_block_items(payload) + out: list[_Row] = [] + for n, item in enumerate(items): + file = item.get("file") + lines = item.get("lines") + first = next((x for x in lines if isinstance(x, int) and not isinstance(x, bool)), None) if isinstance(lines, list) else None + if isinstance(file, str) and file: + where = _code_span("path", file, f":{first}" if first is not None else "") + else: + where = "Whole PR" + out.append( + _Row((0, n), HARD_BLOCK_CELL, EFFECT_NO_WAIVER, where, _check_evidence(item), FILE_CHECKS_ROW, + label=_check_label(item.get("id"))) + ) + known = {_hard_block_label_of(i) for i in items} + has_incomplete_item = any(i.get("id") == "incomplete-coverage" for i in items) + for n, r in enumerate(x for x in result.reasons if x.code == verdict.R_HARD_BLOCK): + if r.detail in known: + continue + if r.detail.startswith("incomplete-coverage") and has_incomplete_item: + continue + label = "Incomplete file coverage" if r.detail.startswith("incomplete-coverage") else "File check result" + out.append( + _Row((0, len(items) + n), HARD_BLOCK_CELL, EFFECT_NO_WAIVER, "Whole PR", + f"{label}: {_esc('reason', r.detail)}", FILE_CHECKS_ROW, label=label) + ) + return out + + +def _not_run_rows(result: verdict.GateResult) -> list[_Row]: + """Rows for every pass or job that did not complete, and for every other + reason that is not a finding or a hard block.""" + out: list[_Row] = [] + by_pass = {p.name: p for p in result.passes} + size_gate = any(r.code == verdict.R_SIZE_GATE for r in result.reasons) + pass_rows = 0 + for name in verdict.PASS_NAMES: + info = by_pass.get(name) + label = PASS_LABELS[name] + if info is None: + claim = f"No valid record of the {label} pass. The pass returned no result." + elif info.status == "ok" or (info.status == "skipped-size" and size_gate): + continue + else: + reason = info.meta.get("reason") + why = reason.strip().rstrip(".") if isinstance(reason, str) and reason.strip() else info.status + claim = f"{_esc('reason', why)}. The pass returned no result." + pass_rows += 1 + out.append(_Row((1, pass_rows), NOT_RUN_CELL, EFFECT_NO_WAIVER, f"{label} pass", claim, CODE_ROW)) + n = 0 + for r in result.reasons: + if r.code in (verdict.R_HARD_BLOCK, verdict.R_BLOCKING_FINDING): + continue + if r.code == verdict.R_PASS_RECORDS and pass_rows: + continue + n += 1 + if r.code == verdict.R_SIZE_GATE: + effect = EFFECT_WAIVED if result.override_applied else EFFECT_WAIVABLE + out.append( + _Row((1, 100 + n), NOT_RUN_CELL, effect, "Model passes", + "Too large for automatic review. File checks ran, model checks did not.", CODE_ROW) + ) + continue + cell = BLOCKED_CELL if r.code in (verdict.R_STALE, verdict.R_SUPERSEDED, verdict.R_BASE_NOT_DEFAULT) else NOT_RUN_CELL + out.append( + _Row((1, 100 + n), cell, EFFECT_NO_WAIVER, "Gate", f"{_reason_label(r.code)}: {_esc('reason', r.detail)}", CODE_ROW) + ) + return out + + +def _findings_table(rows: list[_Row]) -> list[_Block]: + if not rows: + return [] + out = [ + _Block("\n| Severity | Effect | Where | Finding | Found by |"), + _Block("|---|---|---|---|---|"), + ] + for r in rows: + out.append(_Block(_row(r.severity, r.effect, r.where, r.claim, r.found_by), r.rank, "findings-table", 5)) + return out + + +# --------------------------------------------------------------------------- +# Result heading, callout, checks strip, provenance +# --------------------------------------------------------------------------- + + +def _payload_of(bundle: Mapping[str, Any] | None) -> Mapping[str, Any] | None: + if bundle is not None and bundle.get("status") in verdict.BUNDLE_PAYLOAD_STATUSES: + payload = bundle.get("payload") + if isinstance(payload, Mapping): + return payload + return None + + +def _model_rows(rows: list[_Row]) -> list[_Row]: + return [r for r in rows if r.is_model] + + +def _status_heading(result: verdict.GateResult, model_rows: list[_Row]) -> str: + if result.status != verdict.FAIL: + if model_rows and not result.override_applied: + return f"## ๐ŸŸก PASS: OC Claude review, {_n(len(model_rows), 'finding')} to consider" + return "## ๐ŸŸข PASS: OC Claude review" + if result.codes & _INCOMPLETE_CODES: + return "## ๐Ÿ”ด FAIL: OC Claude review did not complete" + return "## ๐Ÿ”ด FAIL: OC Claude review" + + +def _blocking_phrase(model_rows: list[_Row]) -> str: + blocking = [r for r in model_rows if r.effect != EFFECT_NONE] + if len(blocking) == 1: + top = min(blocking[0].members, key=lambda f: _SEVERITY_ORDER.get(f.severity, 9)) + return f"One {top.severity}-severity {top.category.replace('-', ' ')} finding" + return f"{len(blocking)} findings" + + +def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has_payload: bool) -> str: + """The colored callout. Kind: TIP for a pass with no findings, NOTE for a + pass with findings, CAUTION for every fail, waivable or not (ruling 1). + The text says whether a waiver can clear it.""" + model = _model_rows(rows) + codes = result.codes + if result.status != verdict.FAIL: + if result.override_applied: + login = _esc("login", result.valid_overrides[0].author_login) if result.valid_overrides else "a maintainer" + lines = [f"**A maintainer waiver was applied by {login}. Findings stay listed.** Nothing to do for this review."] + elif model: + one = len(model) == 1 + lines = [ + f"**{_n(len(model), 'finding')} to consider. {'It does' if one else 'They do'} not block this PR.**", + f"**Next step for the author:** read the finding{'' if one else 's'} and fix what you agree with.", + ] + return "\n".join(f"> {x}" for x in ("[!NOTE]", *lines)) + else: + lines = ["**No findings. This review found no reason to block.** Nothing to do for this review."] + lines.append("The merge box shows whether the other required checks pass. This bot never approves a PR.") + return "\n".join(f"> {x}" for x in ("[!TIP]", *lines)) + + blocking = [r for r in model if r.effect != EFFECT_NONE] + also = f" {_blocking_phrase(model)} also blocks." if blocking else "" + failed = [ + PASS_LABELS[name] + for name in verdict.PASS_NAMES + if not any(p.name == name and p.status == "ok" for p in result.passes) + and not (any(p.name == name and p.status == "skipped-size" for p in result.passes) and verdict.R_SIZE_GATE in codes) + ] + if hard and has_payload: # with no bundle the hard rows are synthetic: the author cannot fix a missing artifact + label = hard[0].label + if len(hard) == 1: + first = f"**{label} blocks this PR. No waiver can clear it.**{also}" + else: + first = f"**{label} and {len(hard) - 1} other hard block(s) block this PR. No waiver can clear them.**{also}" + nxt = "**Next step for the author:** fix the cause named in the row, then push. The review runs again on the new commit." + elif codes & _INCOMPLETE_CODES: + if failed and verdict.R_PASS_RECORDS in codes: + who = " and ".join(failed) if len(failed) <= 2 else "Review, Security and Supply chain" + first = f"**The {who} {'review' if len(failed) == 1 else 'reviews'} did not run, so this PR could not be judged. No waiver can clear this.**{also}" + else: + first = f"**A required check did not complete, so this PR could not be judged. No waiver can clear this.**{also}" + nxt = "**Next step:** a maintainer re-runs the newest run of the required workflow once the cause is fixed. " + nxt += ( + f"The author also fixes or waives the finding{'' if len(blocking) == 1 else 's'}." + if blocking + else "The author does not need to change anything." + ) + elif verdict.R_BASE_NOT_DEFAULT in codes: + ref = _esc("ref", result.snapshot.base_ref) if result.snapshot else "another branch" + first = f"**This PR targets {ref}, not the default branch. No waiver can clear this.**{also}" + nxt = "**Next step for the author:** retarget the PR to the default branch." + elif verdict.R_SUPERSEDED in codes: + first = f"**A newer required run exists for this commit. No waiver can clear this.**{also}" + nxt = "**Next step:** read the newest run in the merge box." + elif verdict.R_STALE in codes: + first = f"**This review is out of date. No waiver can clear this.**{also}" + nxt = "**Next step:** read the newest run in the merge box." + elif verdict.R_SIZE_GATE in codes: + first = "**The change is too large for the model review. A maintainer can waive this.** File checks ran, model checks did not." + nxt = '**Next step for the author:** split the change, or ask a maintainer for a waiver (under "Waiver" below).' + else: + phrase = _blocking_phrase(model) + verb = "blocks" if phrase.startswith("One") else "block" + who = "it" if phrase.startswith("One") else "them" + first = f"**{phrase} {verb} this PR.** A maintainer can waive {who}." + nxt = f"**Next step for the author:** {_WAIVER_NEXT}" + return "\n".join(f"> {x}" for x in ("[!CAUTION]", first, nxt)) + + +def _checks_strip(result: verdict.GateResult, payload: Mapping[str, Any] | None, hard_count: int) -> _Block: + """One-row table: what ran. Never cut.""" + if payload is None: + file_cell, coverage_cell = "โŒ no result", "โŒ unknown" + else: + pre = payload.get("prefilter") if isinstance(payload.get("prefilter"), Mapping) else {} + signals = len([s for s in pre.get("signals") or () if isinstance(s, Mapping)]) + file_cell = f"๐Ÿšซ {_n(hard_count, 'hard block')}" if hard_count else "โœ… ok" + if signals: + file_cell += f", {_n(signals, 'signal')}" + cov = payload.get("coverage") + coverage_cell = "โœ… complete" if isinstance(cov, Mapping) and cov.get("complete") is True else "๐Ÿšซ incomplete" + by_pass = {p.name: p for p in result.passes} + unvalidated = payload is None and bool(result.codes & {verdict.R_ARTIFACT, verdict.R_PASS_RECORDS}) + cells = [] + for name in verdict.PASS_NAMES: + info = by_pass.get(name) + count = len([f for f in result.findings if f.pass_name == name]) + if info is None: + cells.append("โŒ missing") + elif info.status == "ok" and unvalidated: + cells.append("โš ๏ธ not validated") + elif info.status == "ok": + cells.append(f"โœ… ran, {_n(count, 'finding')}") + elif info.status == "skipped-size": + cells.append("โš ๏ธ skipped, too large") + else: + cells.append(f"โŒ {_esc('label', info.status)}") + dep_failed = any( + r.code == verdict.R_DEPENDENCY_REVIEW or (r.code == verdict.R_JOB_RESULT and verdict.DEPENDENCY_REVIEW_JOB in r.detail) + for r in result.reasons + ) + dep_cell = "โŒ failed" if dep_failed else "โœ… ok" + return _Block( + "\n".join( + ( + "\n| File checks | Review | Security | Supply chain | Dependency review | Coverage |", + "|:---:|:---:|:---:|:---:|:---:|:---:|", + _row(file_cell, *cells, dep_cell, coverage_cell), + ) + ) + ) + + +def _provenance(ctx: PublishContext, result: verdict.GateResult, truncated: bool) -> _Block: + snap = result.snapshot + run = f"Run [{ctx.run_id}, attempt {ctx.run_attempt}]({ctx.run_url})" + if snap is not None: + where = f"for head `{snap.head_sha[:7]}` into {_esc('ref', snap.base_ref)}." + else: + where = f"for event head `{ctx.head_sha[:7]}`, merge base unknown." + text = f"{run} {where} The required workflow in the merge box is the result that counts. This bot never approves a PR." + if truncated: + text += " Some details were truncated, see the run log." + return _Block(f"\n{text}") + + +# --------------------------------------------------------------------------- +# Collapsed sections +# --------------------------------------------------------------------------- + + +def _details(summary: str, inner: Sequence[_Block]) -> list[_Block]: + """`
` with never-cut open and close blocks, so a cut can never + unbalance the tags.""" + return [_Block(f"\n
\n{summary}"), *inner, _Block("\n
")] + + +def _waiver_blocks(result: verdict.GateResult) -> list[_Block]: + if result.override_command is None: + return [] + text = ( + "\nThis PR fails for waivable reasons only. A person with write or admin access, who is not the PR " + "author, posts this exact line as a new comment and replaces `` with the reason. Then they " + "re-run the newest run of the required workflow through the GitHub REST API " + "(`POST .../actions/runs/{run_id}/rerun`). Do not edit the comment afterwards. Do not close and " + "reopen the PR to apply the waiver.\n\n```\n" + result.override_command + "\n```\n\n" + "A waiver covers the code at this exact head and merge base. Any new push needs a new waiver." + ) + return _details("๐Ÿ”‘ Waiver: how a maintainer clears this", [_Block(text)]) + + +def _title(text: str) -> _Block: + return _Block(f"\n**{text}**\n") + + +def _snapshot_blocks(ctx: PublishContext, result: verdict.GateResult) -> list[_Block]: + snap = result.snapshot + run = f"{ctx.run_id}, attempt {ctx.run_attempt}" + if snap is None: + return [_title("Reviewed snapshot"), + _Block(f"No valid bundle. Event head SHA `{ctx.head_sha}`, merge base unknown. Run {run}.")] + return [ + _title("Reviewed snapshot"), + _Block("| Head SHA | Merge base SHA | Base branch | Run |"), + _Block("|---|---|---|---|"), + _Block(_row(f"`{snap.head_sha}`", f"`{snap.merge_base_sha}`", _esc("ref", snap.base_ref), run)), + ] + + +def _reason_blocks(result: verdict.GateResult) -> list[_Block]: + out = [_title("Reasons the gate failed")] + if not result.reasons: + return out + [_Block("None. No reason applies.")] + out += [_Block("| Reason | Waivable? | Detail |"), _Block("|---|---|---|")] + for r in result.reasons: + flag = ("โœ… yes, waived" if result.override_applied else "โœ… yes") if r.waivable else "๐Ÿšซ no" + out.append(_Block(_row(_reason_label(r.code), flag, _esc("reason", r.detail)))) + if result.status == verdict.FAIL and result.override_command is None: + out.append( + _Block( + "\nNo waiver line is shown. At least one reason cannot be waived, so a waiver would not " + "change the result." + ) + ) + return out + + +def _override_blocks(ctx: PublishContext, result: verdict.GateResult) -> list[_Block]: + valid = result.valid_overrides + if not valid: + return [] + waived = ", ".join(_reason_label(r.code) for r in result.waived) or "none" + out = [ + _title("Maintainer waivers"), + _Block("| Author | Time | Reason | Comment | Applied | Waived reasons |"), + _Block("|---|---|---|---|---|---|"), + ] + for c in valid: + out.append( + _Block( + _row( + _esc("login", c.author_login), + c.created_at.isoformat(), + _esc("reason", c.reason), + f"[comment]({ctx.comment_url(c.comment_id)})", + "yes" if result.override_applied else "no", + waived, + ) + ) + ) + return out + + +def _check_block(item: Any, rank: int | None, group: str, mark: str = "") -> _Block | None: + if not isinstance(item, Mapping): + return None + return _Block(f"- {mark}{_check_evidence(item)}", rank, group) def _delta_blocks(delta: Any) -> list[_Block]: @@ -1055,30 +1611,235 @@ def _delta_blocks(delta: Any) -> list[_Block]: return out -def _coverage_blocks(cov: Any) -> list[_Block]: +def _check_table_blocks(result: verdict.GateResult, payload: Mapping[str, Any] | None) -> list[_Block]: + """The "Checks (code, no model)" table: hard blocks, coverage, dependency changes + and the dependency-review job. Hard block evidence and unavailable files are never cut.""" + out = [_title("Checks (code, no model)"), _Block("| Check | Result | Detail |"), _Block("|---|---|---|")] + if payload is None: + return out + [_Block(_row("File checks", "โŒ no result", "No bundle payload was available."))] + items = _hard_block_items(payload) + if items: + out += [_Block(_row("Hard block", "๐Ÿšซ blocks", _check_evidence(i))) for i in items] + else: + out.append(_Block(_row("Hard blocks", "โœ… none", ""))) + cov = payload.get("coverage") if not isinstance(cov, Mapping): - return [_Block("Coverage: no coverage record.")] - complete = "complete" if cov.get("complete") is True else "NOT complete" - nums = [] - for k, label in (("files_in_comparison", "files in comparison"), ("files_collected", "files collected")): - v = cov.get(k) - if isinstance(v, int) and not isinstance(v, bool): - nums.append(f"{label} {v}") - trees = cov.get("trees_complete") - if isinstance(trees, bool): - nums.append("trees complete" if trees else "trees NOT complete") - out = [_Block(f"Coverage: {complete}" + (f" ({', '.join(nums)})." if nums else "."))] - files = cov.get("files") - for f in files if isinstance(files, list) else (): - if isinstance(f, Mapping) and f.get("coverage") == "unavailable": + out.append(_Block(_row("Coverage", "โŒ no record", ""))) + else: + nums = [] + for k, label in (("files_in_comparison", "files in comparison"), ("files_collected", "files collected")): + v = cov.get(k) + if isinstance(v, int) and not isinstance(v, bool): + nums.append(f"{label} {v}") + trees = cov.get("trees_complete") + if isinstance(trees, bool): + nums.append("trees complete" if trees else "trees NOT complete") + out.append( + _Block(_row("Coverage", "โœ… complete" if cov.get("complete") is True else "๐Ÿšซ NOT complete", ", ".join(nums))) + ) + files = cov.get("files") + for f in files if isinstance(files, list) else (): + if isinstance(f, Mapping) and f.get("coverage") == "unavailable": + out.append( + _Block(_row("Unavailable file", "โš ๏ธ unavailable", f"{_esc('path', f.get('path'))}: {_esc('reason', f.get('reason'))}")) + ) + has_delta = bool(_delta_blocks(payload.get("dependency_delta"))) + out.append(_Block(_row("Dependency changes", "โ„น๏ธ listed below" if has_delta else "โœ… none", ""))) + dep_failed = any( + r.code == verdict.R_DEPENDENCY_REVIEW or (r.code == verdict.R_JOB_RESULT and verdict.DEPENDENCY_REVIEW_JOB in r.detail) + for r in result.reasons + ) + out.append(_Block(_row("Dependency review job", "โŒ failed" if dep_failed else "โœ… success", ""))) + return out + + +def _signal_blocks(payload: Mapping[str, Any] | None) -> list[_Block]: + out = [_title("Signals (context only, never block)")] + pre = payload.get("prefilter") if isinstance(payload, Mapping) and isinstance(payload.get("prefilter"), Mapping) else {} + items = [] + for i in pre.get("signals") or (): + mark = "โš ๏ธ " if isinstance(i, Mapping) and i.get("id") in _WARN_SIGNALS else "โ„น๏ธ " + blk = _check_block(i, CUT_SIGNALS, "signals", mark) + if blk: + items.append(blk) + return out + (items or [_Block("None.")]) + + +def _finding_block(f: verdict.CitedFinding, inline: InlineOutcome) -> _Block | None: + """The record bullet of a finding that has no inline comment, with its + explanation and quote. None for a finding that has one (its row says so).""" + state = _posting_state(f, inline) + if state in _INLINE_STATES: + return None + where = f"{_esc('file', f.file)} line {f.line} ({f.side})" + head = f"- {_finding_line(f)}, at {where}: {_esc('title', f.title)}" + # A high or critical finding is never cut, also when it is unverified. + protected = not _cuttable(f) + if state == "unverified": + return _Block( + "\\\n ".join( + ( + head, + f"Not verified: {_esc('reason', f.unverified_reason or 'unknown')}", + _esc("explanation", f.explanation), + f"Quote: {_esc('quote', f.quote)}", + ) + ), + None if protected else CUT_UNVERIFIED, + "unverified", + ) + note = { + "changed": "Differs from the inline comment of an earlier run, shown here.", + "not-posted": "Inline comment not posted, shown here.", + "meta": "PR title or body, summary only.", + "outside": "Outside the diff, summary only.", + }[state] + return _Block( + "\\\n ".join((head, note, _esc("explanation", f.explanation), f"Quote: {_esc('quote', f.quote)}")), + None if protected else CUT_LOW_MEDIUM, + f"findings-{f.pass_name}", + ) + + +def _finding_detail_blocks(result: verdict.GateResult, inline: InlineOutcome) -> list[_Block]: + out = [_title("Finding details"), _Block(f"> **{UNTRUSTED_NOTE}** {VERIFIED_NOTE}\n")] + out.append( + _Block( + "Blocking is decided by code from category and severity, never by the model. " + "A finding in a blocking category blocks only at high or critical severity." + ) + ) + if inline.note: + out.append(_Block("\n" + _esc("reason", inline.note))) + by_pass = {p.name: p for p in result.passes} + ran = [n for n in verdict.PASS_NAMES if by_pass.get(n) is not None and by_pass[n].status == "ok"] + not_ran = [PASS_LABELS[n] for n in verdict.PASS_NAMES if n not in ran] + findings = _by_pass_then_severity(result.findings) + if findings: + if any(len(r.members) > 1 for r in _merge_findings(result, inline)): + out.append( + _Block("\nEach row above merges findings of several passes at the same file, line and gate effect.") + ) + out += [ + _Block("\n| Pass | Severity | Category | Confidence | Citation | Posted | Model title |"), + _Block("|---|---|---|---|---|---|---|"), + ] + for f in findings: + rank = None if not _cuttable(f) else (CUT_UNVERIFIED if not f.verified else CUT_LOW_MEDIUM) out.append( _Block( - f"- unavailable: {_esc('path', f.get('path'))}: {_esc('reason', f.get('reason'))}", + _row( + PASS_LABELS.get(f.pass_name, f.pass_name), + f.severity, + f.category.replace("-", " "), + f.confidence, + "โœ… checked" if f.verified else "โŒ not checked", + _POSTING_TEXT[_posting_state(f, inline)], + _esc("title", f.title), + ), + rank, + "detail-rows", + 7, ) ) + bullets = [x for x in (_finding_block(f, inline) for f in findings) if x is not None] + if bullets: + out.append(_Block("\nFindings with no inline comment, in full:\n")) + out += bullets + quiet = [PASS_LABELS[n] for n in ran if not any(f.pass_name == n for f in findings)] + for label in quiet: + out.append(_Block(f"\nThe {label} pass completed and reported no findings.")) + elif not not_ran: + out.append(_Block("\nNo findings. All three passes completed.")) + else: + out.append( + _Block( + f"\nNo findings from the passes that ran. The {', '.join(not_ran)} pass did not run, " + "so this is not a clean result." + ) + ) + if findings and not_ran: + out.append(_Block(f"\nThe {', '.join(not_ran)} pass did not run, so this is not a clean result.")) + return out + + +def _summary_table_blocks(result: verdict.GateResult) -> list[_Block]: + out = [ + _title("Model summaries"), + _Block(f"> **{UNTRUSTED_NOTE}** {VERIFIED_NOTE}\n"), + _Block("| Pass | Status | Model | Summary |"), + _Block("|---|---|---|---|"), + ] + by_pass = {p.name: p for p in result.passes} + for name in verdict.PASS_NAMES: + info = by_pass.get(name) + label = PASS_LABELS[name] + if info is None: + out.append(_Block(_row(label, "โŒ missing", "", "No valid pass record."))) + continue + model = info.meta.get("model") + status = "โœ… ok" if info.status == "ok" else f"โŒ {_esc('label', info.status)}" + if info.summary: + # One line: a multi-line summary would end the table row. + text = escape_text(info.summary, LIMITS["summary"]) + else: + reason = info.meta.get("reason") + text = "No summary." + (f" Reason: {_esc('reason', reason)}" if isinstance(reason, str) and reason else "") + out.append(_Block(_row(label, status, _esc("model", model) if isinstance(model, str) else "", text))) return out +def _token_blocks(result: verdict.GateResult) -> list[_Block]: + out = [ + _title("Token usage"), + _Block("| Pass | Input | Output | Cache write | Cache read | Attempts |"), + _Block("|---|---:|---:|---:|---:|---:|"), + ] + by_pass = {p.name: p for p in result.passes} + totals: dict[str, int] = {} + missing = False + for name in verdict.PASS_NAMES: + info = by_pass.get(name) + nums = _usage_numbers(info.meta) if info is not None else {} + cells = [] + for k, _label in USAGE_FIELDS: + if k in nums: + totals[k] = totals.get(k, 0) + nums[k] + cells.append(str(nums[k])) + else: + missing = True + cells.append("not recorded") + out.append(_Block(_row(PASS_LABELS[name], *cells))) + out.append( + _Block( + _row( + "**Recorded total**" if missing else "**Total**", + *(str(totals[k]) if k in totals else "not recorded" for k, _label in USAGE_FIELDS), + ) + ) + ) + return out + + +def _record_blocks(ctx: PublishContext, result: verdict.GateResult, payload: Mapping[str, Any] | None, + inline: InlineOutcome) -> list[_Block]: + inner: list[_Block] = [] + inner += _snapshot_blocks(ctx, result) + inner += _reason_blocks(result) + inner += _override_blocks(ctx, result) + inner += _check_table_blocks(result, payload) + inner += _signal_blocks(payload) + inner.append(_title("Dependency changes")) + delta = _delta_blocks(payload.get("dependency_delta")) if payload is not None else [] + inner += delta or [_Block("None.")] + inner += _finding_detail_blocks(result, inline) + inner += _summary_table_blocks(result) + inner += _token_blocks(result) + return _details( + "๐Ÿ“‹ Full review record: reasons, checks, signals, finding details, model summaries, token usage", inner + ) + + @dataclass(frozen=True) class RenderedSummary: """The sticky summary body. `cut`: D8 cuts were made (the full record goes @@ -1090,24 +1851,6 @@ class RenderedSummary: overflow: bool = False -def _header_lines(ctx: PublishContext, result: verdict.GateResult, status: str) -> list[str]: - snap = result.snapshot - if snap is not None: - computed = ( - f"Computed for head SHA `{snap.head_sha}`, merge base `{snap.merge_base_sha}`, " - f"base ref {_esc('ref', snap.base_ref)}." - ) - else: - computed = f"No valid bundle. Event head SHA `{ctx.head_sha}`, merge base unknown." - return [ - verdict.STICKY_MARKER, - f"## OC Claude review: {status}", - f"Result for run [{ctx.run_id}]({ctx.run_url}), attempt {ctx.run_attempt}.", - computed, - "The result that counts is the required workflow in the merge box.", - ] - - def _overflow_body(ctx: PublishContext, result: verdict.GateResult) -> str: """The fail-closed notice when the never-cut data does not fit (D8). It has a fixed size: the result, the SHAs, a count per reason code and the @@ -1115,23 +1858,58 @@ def _overflow_body(ctx: PublishContext, result: verdict.GateResult) -> str: counts: dict[str, int] = {} for r in result.reasons: counts[r.code] = counts.get(r.code, 0) + 1 - lines = _header_lines(ctx, result, verdict.FAIL) - lines += ["\n### Summary too large", OVERFLOW_NOTE] - lines.append( + lines = [ + verdict.STICKY_MARKER, + "## ๐Ÿ”ด FAIL: OC Claude review could not publish", + "", + "> [!CAUTION]", + f"> **The full review does not fit in one comment.** {OVERFLOW_NOTE}", + "> **Next step:** see the run log.", + "", f"The computed gate result was {result.status}. The full record (every reason, override, " - "finding and prefilter result) is in the run log." - ) - lines.append("\n### Reasons") + "finding and prefilter result) is in the run log.", + ] + lines += [b.text for b in _snapshot_blocks(ctx, result)] + lines += ["", "**Reasons**", ""] if not counts: lines.append("No reason applies.") - for code in sorted(counts): - waivable = "waivable by override" if verdict.WAIVABLE[code] else "not waivable" - lines.append(f"- {code} ({waivable}): {counts[code]} reason(s)") - lines.append(f"\n### Override\n{len(result.valid_overrides)} valid override(s).") - lines.append(f"\n[Workflow run]({ctx.run_url}). The result that counts is the required workflow in the merge box.") + else: + lines += ["| Reason | Waivable? | Count |", "|---|---|---|"] + for code in sorted(counts): + flag = "โœ… yes" if verdict.WAIVABLE[code] else "๐Ÿšซ no" + lines.append(_row(_reason_label(code), flag, f"{counts[code]} reason(s)")) + lines.append(f"\n{len(result.valid_overrides)} valid override(s).") + lines.append(f"\nRun [{ctx.run_id}, attempt {ctx.run_attempt}]({ctx.run_url}). " + "The required workflow in the merge box is the result that counts. This bot never approves a PR.") return "\n".join(lines) +def _summary_blocks( + ctx: PublishContext, + result: verdict.GateResult, + payload: Mapping[str, Any] | None, + inline: InlineOutcome, + truncated: bool, +) -> list[_Block]: + hard = _hard_rows(result, payload) + rows = sorted( + [*hard, *_not_run_rows(result), *_merge_findings(result, inline)], + key=lambda r: r.sort, + ) + model = _model_rows(rows) + b = [ + _Block(verdict.STICKY_MARKER), + _Block(_status_heading(result, model)), + _Block("\n" + _callout(result, rows, hard, payload is not None)), + ] + b += _findings_table(rows) + b.append(_checks_strip(result, payload, len(hard))) + b.append(_provenance(ctx, result, truncated)) + b += _waiver_blocks(result) + b += _record_blocks(ctx, result, payload, inline) + return b + + def render_summary( ctx: PublishContext, result: verdict.GateResult, @@ -1141,98 +1919,14 @@ def render_summary( limit: int = SUMMARY_LIMIT, ) -> RenderedSummary: """The sticky summary body (D8). Every untrusted string goes through - `escape_field`.""" - b: list[_Block] = [_Block(line) for line in _header_lines(ctx, result, result.status)] - add = b.append - - add(_Block("\n### Reasons")) - if not result.reasons: - add(_Block("No reason applies.")) - for r in result.reasons: - add(_Block(f"- {r.code} ({_waivable(r)}): {_esc('reason', r.detail)}")) - - if result.override_command is not None: - add(_Block("\n### Override")) - add( - _Block( - "The gate fails for waivable reasons only. A person with write access who is not " - "the PR author can post this comment, with a reason, then re-run the newest run " - "of the required workflow:\n\n```\n" + result.override_command + "\n```" - ) - ) - valid = result.valid_overrides - if valid: - if result.override_command is None: - add(_Block("\n### Override")) - waived = ", ".join(r.code for r in result.waived) or "none" - add(_Block(f"Override applied: {'yes' if result.override_applied else 'no'}. Waived reasons: {waived}.")) - for c in valid: - add( - _Block( - f"- by {_esc('login', c.author_login)} at {c.created_at.isoformat()}: " - f"{_esc('reason', c.reason)} ([comment]({ctx.comment_url(c.comment_id)}))" - ) - ) - - payload = None - if bundle is not None and bundle.get("status") in verdict.BUNDLE_PAYLOAD_STATUSES: - payload = bundle.get("payload") - if isinstance(payload, Mapping): - pre = payload.get("prefilter") if isinstance(payload.get("prefilter"), Mapping) else {} - add(_Block("\n### Prefilter (code, no LLM)")) - hard = [x for x in (_check_block(i, None, "hard-blocks") for i in pre.get("hard_blocks") or ()) if x] - sig = [x for x in (_check_block(i, CUT_SIGNALS, "signals") for i in pre.get("signals") or ()) if x] - add(_Block(f"Hard blocks: {len(hard)}.")) - b.extend(hard) - add(_Block(f"Signals: {len(sig)}.")) - b.extend(sig) - b.extend(_coverage_blocks(payload.get("coverage"))) - add(_Block("\n### Dependency delta")) - delta = _delta_blocks(payload.get("dependency_delta")) - b.extend(delta or [_Block("No dependency changes.")]) - - add(_Block("\n### Findings per pass")) - add(_Block(f"_{UNTRUSTED_NOTE} {VERIFIED_NOTE}_")) - if inline.note: - add(_Block(_esc("reason", inline.note))) - by_pass = {p.name: p for p in result.passes} - for name in verdict.PASS_NAMES: - info = by_pass.get(name) - if info is None: - add(_Block(f"\n#### {name}: no valid pass record")) - continue - line = f"\n#### {name}: {info.status}" - model = info.meta.get("model") - if isinstance(model, str) and model: - line += f", model {_esc('model', model)}" - reason = info.meta.get("reason") - if isinstance(reason, str) and reason: - line += f", {_esc('reason', reason)}" - add(_Block(line)) - if info.summary: - add(_Block(f"Model summary: {_esc('summary', info.summary)}")) - verified = [f for f in result.findings if f.pass_name == name and f.verified] - if not verified: - add(_Block("No verified findings.")) - for f in sorted(verified, key=lambda f: f.severity not in verdict.BLOCKING_SEVERITIES): - b.append(_finding_block(f, inline)) - - unverified = [f for f in result.findings if not f.verified] - if unverified: - add(_Block("\n### Unverified findings")) - add(_Block(f"_The citation of these findings did not check out. {UNTRUSTED_NOTE}_")) - b.extend(_finding_block(f, inline) for f in unverified) - - add(_Block("\n### Token usage")) - totals: dict[str, int] = {} - for info in result.passes: - nums = _usage_numbers(info.meta) - for k, v in nums.items(): - totals[k] = totals.get(k, 0) + v - add(_Block(f"- {info.name}: {_usage_text(nums)}")) - add(_Block(f"- total: {_usage_text(totals)}")) - add(_Block(f"\n[Workflow run]({ctx.run_url}). The result that counts is the required workflow in the merge box.")) - body, cut = _fit(b, limit) + `escape_field` (or `_code_span`). Layout: result heading, callout, findings + table, checks strip, provenance, then the collapsed waiver and record.""" + payload = _payload_of(bundle) + body, cut = _fit(_summary_blocks(ctx, result, payload, inline, False), limit) + if body is not None and cut: + # The provenance line says so when a cut was made. Its longer text is + # in the fit, so the body still fits. + body, cut = _fit(_summary_blocks(ctx, result, payload, inline, True), limit) if body is None: return RenderedSummary(_overflow_body(ctx, result), True, overflow=True) return RenderedSummary(body, cut) diff --git a/review/tests/unit/test_evals.py b/review/tests/unit/test_evals.py index cc5388c..87a18a9 100644 --- a/review/tests/unit/test_evals.py +++ b/review/tests/unit/test_evals.py @@ -524,7 +524,7 @@ def test_max_severity_counts_unverified_findings(tmp_path): def test_summary_is_written_per_case(tmp_path): r = run_with_file(tmp_path, {}, [HEAD_HIGH]) summary = (tmp_path / "out" / "case1" / evals.SUMMARY_FILE_NAME).read_text() - assert r["matched"] and "### Findings per pass" in summary + assert r["matched"] and "**Finding details**" in summary # nothing is posted inline, so the inline finding shows in full assert "Inline comment not posted, shown here." in summary and "Quote: eval\\(user\\_input\\)" in summary @@ -558,8 +558,8 @@ def test_summary_labels_a_suggested_shell_command_untrusted(tmp_path): r = run_with_file(tmp_path, {"findings": [{"category": "prompt-injection", "severity": "low"}]}, [f]) summary = (tmp_path / "out" / "case1" / evals.SUMMARY_FILE_NAME).read_text() assert r["matched"] - findings_part = summary.split("### Findings per pass", 1)[1] - assert findings_part.lstrip().startswith(f"_{publish.UNTRUSTED_NOTE}") + findings_part = summary.split("**Finding details**", 1)[1] + assert findings_part.lstrip().startswith(f"> **{publish.UNTRUSTED_NOTE}**") # escaped prose, not a code block: the command never shows as written assert cmd not in findings_part and cmd in evals._unescape(findings_part) - assert "```" not in findings_part.split("### Token usage")[0] + assert "```" not in findings_part.split("**Token usage**")[0] diff --git a/review/tests/unit/test_publish.py b/review/tests/unit/test_publish.py index b76949c..df8d339 100644 --- a/review/tests/unit/test_publish.py +++ b/review/tests/unit/test_publish.py @@ -355,8 +355,23 @@ def plain(text: str) -> str: return re.sub(r"\\(.)", r"\1", text, flags=re.S).replace(ZWSP, "") +LABEL_CODE = {label: code for code, label in publish.REASON_LABELS.items()} +_REASON_ROW = re.compile(r"^\| (.+?) \| (โœ… yes(?:, waived)?|๐Ÿšซ no) \| (.*) \|$", re.M) + + +def reason_rows(text: str) -> list[tuple[str, bool, str]]: + """The rows of the reasons table of the record, as (code, waivable, detail).""" + m = re.search(r"\*\*Reasons the gate failed\*\*\n\n(.*?)(?:\n\n|\Z)", text, re.S) + assert m, "no reasons section" + return [(LABEL_CODE[label], flag.startswith("โœ…"), plain(detail)) for label, flag, detail in _REASON_ROW.findall(m.group(1))] + + def reason_codes(text: str) -> set[str]: - return set(re.findall(r"^- ([a-z-]+) \((?:not waivable|waivable by override)\)", text, re.M)) + return {code for code, _, _ in reason_rows(text)} + + +def has_reason(text: str, code: str, waivable: bool, detail: str | None = None) -> bool: + return any(c == code and w == waivable and (detail is None or detail in d) for c, w, d in reason_rows(text)) # ---------------------------------------------------------------- happy path @@ -369,7 +384,7 @@ def test_clean_run_passes_and_posts_summary(tmp_path): assert not r.gh.bodies("POST", "/reviews") s = r.summary assert s.startswith(verdict.STICKY_MARKER) - assert "## OC Claude review: pass" in s + assert "## ๐ŸŸข PASS: OC Claude review" in s assert r.record["result"] == "pass" @@ -377,34 +392,34 @@ def test_summary_has_every_required_field(tmp_path): passes = clean_passes(security=[finding("security", "high", title="Shell call")]) gh = FakeGitHub(issue_comments=[override(1)]) r = run(tmp_path, gh, passes=passes, bundle=bundle_doc(signals=[ - {"id": "workflow-change", "level": "signal", "detail": "changes a workflow", "file": ".github/workflows/x.yml", + {"id": "sensitive-path", "level": "signal", "detail": "changes a workflow", "file": ".github/workflows/x.yml", "subject": None, "lines": [], "count": 1}])) s = r.summary assert r.code == 0 # waived by the override for text in ( verdict.STICKY_MARKER, - f"Result for run [{RUN_ID}]", - f"attempt {ATTEMPT}", - f"head SHA `{HEAD}`", - f"merge base `{MB}`", - "base ref main", - "### Reasons", - "blocking-finding (waivable by override)", - "Override applied: yes. Waived reasons: blocking-finding.", - f"by {MAINT}", - f"([comment](https://github.com/{REPO}/pull/{PR}#issuecomment-1))", - "### Prefilter (code, no LLM)", - "workflow\\-change", - "Coverage: complete", - "### Dependency delta", - "### Findings per pass", + f"Run [{RUN_ID}, attempt {ATTEMPT}](https://github.com/{REPO}/actions/runs/{RUN_ID}/attempts/{ATTEMPT})", + f"head `{HEAD[:7]}` into main", + f"| `{HEAD}` | `{MB}` | main |", # full SHAs and base ref, in the record + "**Reasons the gate failed**", + "| Blocking finding | โœ… yes, waived |", + "**Maintainer waivers**", + f"| {MAINT} |", + f"[comment](https://github.com/{REPO}/pull/{PR}#issuecomment-1)", + "| yes | Blocking finding |", + "**Checks (code, no model)**", + "โš ๏ธ Sensitive path changed: changes a workflow, in", + "| Coverage | โœ… complete | files in comparison 1, files collected 1, trees complete |", + "**Dependency changes**", + "**Finding details**", publish.UNTRUSTED_NOTE, publish.VERIFIED_NOTE, - "### Token usage", - "input 12000, output 900, cache write 11000, cache read 0, attempts 1", - "total: input 36000", - f"[Workflow run](https://github.com/{REPO}/actions/runs/{RUN_ID}/attempts/{ATTEMPT})", - "The result that counts is the required workflow in the merge box.", + "**Model summaries**", + "**Token usage**", + "| Review | 12000 | 900 | 11000 | 0 | 1 |", + "| **Total** | 36000 | 2700 | 33000 | 0 | 3 |", + "The required workflow in the merge box is the result that counts.", + "This bot never approves a PR.", ): assert text in s, text @@ -483,7 +498,7 @@ def test_base_not_default_branch_fails_not_waivable(tmp_path): gh = FakeGitHub(default_branch="develop", issue_comments=[override(1)]) r = run(tmp_path, gh) assert r.code == 1 - assert "base-not-default-branch (not waivable)" in r.summary + assert has_reason(r.summary, "base-not-default-branch", False) # ---------------------------------------------------------------- inline review @@ -511,10 +526,10 @@ def test_inline_review_only_verified_and_eligible(tmp_path): assert re.match(r"\n\n", c["body"]) assert publish.UNTRUSTED_NOTE in c["body"] s = r.summary - assert "in diff head. See the inline comment." in s + assert "in diff head See the inline comment." in s assert "Outside the diff, summary only." in s assert "PR title or body, summary only." in s - assert "### Unverified findings" in s and "Not verified: quote not found" in plain(s) + assert "Not verified: quote not found" in plain(s) def test_inline_rename_base_side_uses_new_path(tmp_path): @@ -556,7 +571,7 @@ def test_dedupe_by_bot_key_only(tmp_path): r = run(tmp_path, gh, passes=clean_passes(review=[f1, f2, dict(f2)])) (rv,) = r.gh.bodies("POST", "/reviews") assert [c["line"] for c in rv["comments"]] == [11] # f1 skipped; the spoofed f2 is not - assert "first. See the inline comment from an earlier run." in r.summary + assert "first See the inline comment from an earlier run." in r.summary SPOOFED = finding("malicious", "critical", title="Steals the token", explanation="Critical evidence not in the forged comment.") @@ -684,7 +699,7 @@ def test_rerun_with_the_same_findings_posts_nothing_new(tmp_path): assert len(posted_by(first)) == 1 second = run(tmp_path / "2", FakeGitHub(review_comments=posted_by(first)), passes=passes) assert second.gh.bodies("POST", "/reviews") == [] - assert "bug. See the inline comment from an earlier run." in second.summary + assert "bug See the inline comment from an earlier run." in second.summary assert "Same words." not in second.summary @@ -730,7 +745,9 @@ def test_same_d14_key_in_one_run_after_an_earlier_comment(tmp_path): r = run(tmp_path, gh, passes=clean_passes(review=[old, new])) assert r.gh.bodies("POST", "/reviews") == [] s = plain(r.summary) - assert "same. See the inline comment from an earlier run." in s + # Both findings share one row (same place and effect). The old one has an inline + # comment from an earlier run; the new one differs from it, so its text is shown. + assert "| inline, earlier run |" in s and "| differs from the inline comment of an earlier run |" in s assert "Differs from the inline comment of an earlier run, shown here." in s assert "Critical new words." in s and "Old words." not in s @@ -799,7 +816,7 @@ def test_finding_that_differs_past_the_cut_from_an_earlier_one_is_shown_in_full( r = run(tmp_path, gh, passes=clean_passes(review=[first, second])) assert r.gh.bodies("POST", "/reviews") == [] s = plain(r.summary) - assert "See the inline comment from an earlier run." in s + assert "| inline, earlier run |" in s assert "Differs from the inline comment of an earlier run, shown here." in s assert "Second distinct explanation: secret theft." in s @@ -884,7 +901,7 @@ def test_untrusted_text_is_escaped_in_summary(tmp_path): assert not re.search(r"(?") CUT_PATH = "abcde/" * 75 + "setup.js" # over the file limit: analyze cuts it to a final "/" @@ -1056,7 +1073,7 @@ def test_critical_findings_with_a_cut_path_are_never_cut_from_the_summary(tmp_pa r = run(tmp_path, passes=passes) assert r.code == 1 and r.gh.bodies("POST", "/reviews") == [] s = plain(r.summary) - assert publish.OVERFLOW_NOTE in s and "### Summary too large" in s + assert publish.OVERFLOW_NOTE in s and "could not publish" in s assert not any(c in s for c in claims(passes)) # no partial list assert r.record["result"] == "fail" and r.record["publish_error"] rec = full_record_from_log(capsys.readouterr().out) @@ -1095,9 +1112,9 @@ def test_reasons_that_do_not_fit_fail_closed(tmp_path, capsys): s = r.summary assert r.code == 1 assert len(s) < publish.SUMMARY_MAX_CHARS - assert "## OC Claude review: fail" in s and publish.OVERFLOW_NOTE in s - assert f"head SHA `{HEAD}`" in s and f"merge base `{MB}`" in s - assert "- prefilter-hard-block (not waivable): 400 reason(s)" in s + assert "## ๐Ÿ”ด FAIL: OC Claude review" in s and publish.OVERFLOW_NOTE in s + assert f"| `{HEAD}` | `{MB}` | main |" in s + assert "| Hard block from the file checks | ๐Ÿšซ no | 400 reason(s) |" in s # No partial reason list: either every reason or none. assert "p0000" not in s rec = full_record_from_log(capsys.readouterr().out) @@ -1125,8 +1142,8 @@ def test_many_overrides_that_fit_are_all_listed(tmp_path): s = r.summary assert publish.TRUNCATED_NOTE in s for i in range(1, 101): - assert f"reason {i:03d} ([comment]" in s, i - assert s.rstrip().endswith("The result that counts is the required workflow in the merge box.") + assert f"| reason {i:03d} | [comment](" in s, i + assert s.rstrip().endswith("
") def test_cut_data_is_in_the_run_log(tmp_path, capsys): @@ -1173,7 +1190,7 @@ def push(gh, req): kw["passes"] = clean_passes(supply_chain=[BLOCKING]) r = run(tmp_path, gh, **kw) assert r.code == 1 - assert len(r.gh.summary_bodies()) == 1 and "## OC Claude review: pass" in r.summary + assert len(r.gh.summary_bodies()) == 1 and "## ๐ŸŸข PASS: OC Claude review" in r.summary assert r.gh.paths("PATCH") == [] assert "stale" in r.record["reasons"] @@ -1208,7 +1225,7 @@ def test_override_from_fetched_data_and_permission_reads(tmp_path): assert r.code == 0 perms = sorted(p.split("/")[-2] for p in r.gh.paths() if "/collaborators/" in p) assert perms == sorted([MAINT, MAINT2] * 2) # before the gate and at the final check - assert f"by {MAINT}" in r.summary and "by admin\\-user" in r.summary + assert f"| {MAINT} |" in r.summary and "| admin\\-user |" in r.summary def test_timeline_not_read_fully_gives_no_override(tmp_path): @@ -1238,7 +1255,7 @@ def test_override_never_clears_hard_block(tmp_path): "subject": None, "lines": [], "count": 1}] r = run(tmp_path, FakeGitHub(issue_comments=[override(1)]), bundle=bundle_doc(hard_blocks=hb)) assert r.code == 1 - assert "prefilter-hard-block (not waivable)" in r.summary + assert has_reason(r.summary, "prefilter-hard-block", False) def test_grant_deleted_before_final_check(tmp_path): @@ -1250,7 +1267,7 @@ def delete_grant(gh, req): r = run(tmp_path, gh, passes=clean_passes(supply_chain=[BLOCKING])) assert r.code == 1 assert len(r.gh.summary_bodies()) == 2 # pass summary, then the update to fail - assert "## OC Claude review: fail" in r.summary + assert "## ๐Ÿ”ด FAIL: OC Claude review" in r.summary # ---------------------------------------------------------------- final check @@ -1261,8 +1278,8 @@ def test_newer_run_supersedes_and_updates_summary(tmp_path): r = run(tmp_path, gh) assert r.code == 1 first, last = r.gh.summary_bodies() - assert "## OC Claude review: pass" in first - assert "## OC Claude review: fail" in last and "superseded (not waivable)" in last + assert "## ๐ŸŸข PASS: OC Claude review" in first + assert "## ๐Ÿ”ด FAIL: OC Claude review" in last and has_reason(last, "superseded", False) assert r.gh.paths("PATCH") # the update edits the comment the run posted @@ -1296,7 +1313,7 @@ def test_run_list_read_in_part_is_superseded(tmp_path, kind): gh.runs = [{"id": RUN_ID, "path": WORKFLOW}, {"id": "x", "path": "ci.yml"}] r = run(tmp_path, gh) assert r.code == 1 - assert "superseded (not waivable)" in r.summary + assert has_reason(r.summary, "superseded", False) # ---------------------------------------------------------------- reasons and exit code @@ -1348,8 +1365,8 @@ def test_missing_needs_and_outcome_fail_with_summary(tmp_path): r = run(tmp_path, needs=needs) assert r.code == 1 s = r.summary - assert "job-result (not waivable): analyze: missing" in plain(s) - assert "dependency-review (not waivable): step outcome missing" in plain(s) + assert has_reason(s, "job-result", False, "analyze: missing") + assert has_reason(s, "dependency-review", False, "step outcome missing") def test_outcome_from_env_and_conflict(tmp_path): @@ -1363,7 +1380,7 @@ def test_outcome_from_env_and_conflict(tmp_path): def test_dependency_review_failure_with_job_success(tmp_path): r = run(tmp_path, needs=needs_ok("failure"), gh=FakeGitHub(issue_comments=[override(1)])) assert r.code == 1 - assert "dependency-review (not waivable): step outcome failure" in plain(r.summary) + assert has_reason(r.summary, "dependency-review", False, "step outcome failure") @pytest.mark.parametrize("missing", ["bundle", "review", "supply-chain", "all"]) @@ -1378,7 +1395,7 @@ def test_missing_artifact_dirs_fail_and_still_post_summary(tmp_path, missing): r = run(tmp_path, passes=passes) assert r.code == 1 s = r.summary - assert "artifact (not waivable)" in s + assert has_reason(s, "artifact", False) if missing in ("bundle", "all"): assert f"No valid bundle. Event head SHA `{HEAD}`" in s else: @@ -1404,9 +1421,9 @@ def test_spend_limit_reason_from_meta(tmp_path): r = run(tmp_path, passes=passes, gh=FakeGitHub(issue_comments=[override(1)])) assert r.code == 1 s = r.summary - assert "pass-records (not waivable): pass security failed: workspace spend limit reached" in plain(s) - assert "#### security: failed, workspace spend limit reached" in plain(s) - assert "- security: no usage recorded" in s + assert has_reason(s, "pass-records", False, "pass security failed: workspace spend limit reached") + assert "| Security | โŒ failed | | No summary. Reason: workspace spend limit reached |" in s + assert "| Security | not recorded | not recorded | not recorded | not recorded | not recorded |" in s rec = r.record assert rec["passes"]["review"]["usage"] == USAGE assert rec["passes"]["security"]["usage"] == {} diff --git a/review/tests/unit/test_summary_comment.py b/review/tests/unit/test_summary_comment.py new file mode 100644 index 0000000..6a48ffa --- /dev/null +++ b/review/tests/unit/test_summary_comment.py @@ -0,0 +1,449 @@ +"""The sticky PR summary comment: result heading, callout, findings table, +checks strip, collapsed waiver and record (spec "Publishing (code)"). No +network: the gate job runs against the fake GitHub of test_publish. + +Set OC_RENDER_OUT to a directory to also write the rendered Markdown of each +case there (`.md`). +""" + +from __future__ import annotations + +import os +import re +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from oc_review import publish, verdict + +from .test_publish import ( + BLOCKING, + HEAD, + MB, + FakeGitHub, + needs_ok, + bundle_doc, + clean_passes, + finding, + pass_doc, + plain, + reason_rows, + run, +) + +SPOOF = "Workflow spoofs the review status check with a fake pass." +TLS = "TLS certificate verification is disabled for the download." +BIDI_ITEM = { + "id": "bidi-control", "level": "hard_block", "detail": "bidi control characters U+202E, line 1", + "file": "bidi.txt", "subject": None, "lines": [1], "count": 1, +} +SIGNALS = [ + {"id": "sensitive-path", "level": "signal", "detail": "workflow file", "file": ".github/workflows/x.yml", + "subject": None, "lines": [], "count": 1}, + {"id": "first-time-contributor", "level": "signal", "detail": "author association NONE", "file": None, + "subject": None, "lines": [], "count": 1}, +] + + +def case_a(tmp_path): + return run(tmp_path) + + +def case_b(tmp_path): + low = finding("bug", "low", title="Setup command uses the old package name.", quote="shell=True") + return run(tmp_path, passes=clean_passes(review=[low])) + + +def case_c(tmp_path): + tls = finding("security", "high", title=TLS, quote="shell=True") + retry = finding("bug", "low", line=11, quote="line 11", title="Retry loop never sleeps between attempts.") + return run(tmp_path, passes=clean_passes(security=[tls], review=[retry])) + + +def case_d(tmp_path): + spoof_sec = finding("security", "high", title=SPOOF, quote="shell=True") + spoof_sc = finding("malicious", "high", title="Workflow spoofs the claude-review check with a fake pass.", + quote="shell=True") + bidi_sec = finding("security", "medium", line=11, quote="line 11", title="Bidirectional override committed.") + bidi_sc = finding("malicious", "medium", line=11, quote="line 11", title="Right-to-left override hidden.") + passes = clean_passes(security=[spoof_sec, bidi_sec], supply_chain=[spoof_sc, bidi_sc]) + return run(tmp_path, passes=passes, bundle=bundle_doc(hard_blocks=[BIDI_ITEM], signals=SIGNALS)) + + +def case_e(tmp_path): + passes = clean_passes() + meta = {"reason": "API unavailable after 3 attempts", "model": "claude-sonnet-5-5", "stop_reason": None, + "request_schema_sha256": "f" * 64, "usage": None} + passes["security"] = pass_doc("security", "failed", meta=meta) + return run(tmp_path, passes=passes) + + +def case_f(tmp_path): + title = "Job impersonates the review gate check name with a fake pass." + passes = clean_passes( + review=[finding("malicious", "high", title=title, quote="shell=True")], + security=[finding("security", "medium", title="Workflow spoofs the required review check name.", + quote="shell=True")], + supply_chain=[finding("supply-chain", "medium", title="Workflow job spoofs the review gate check name.", + quote="shell=True")], + ) + return run(tmp_path, passes=passes, bundle=bundle_doc(signals=SIGNALS)) + + +CASES = { + "a-pass-clean": case_a, + "b-pass-minor": case_b, + "c-fail-waivable": case_c, + "d-pr40-hard-block": case_d, + "e-could-not-run": case_e, + "f-pr39": case_f, +} + + +@pytest.fixture(params=CASES) +def rendered(request, tmp_path): + r = CASES[request.param](tmp_path) + out = os.environ.get("OC_RENDER_OUT") + if out: + Path(out).mkdir(parents=True, exist_ok=True) + (Path(out) / f"{request.param}.md").write_text(r.summary, encoding="utf-8") + return request.param, r + + +def callout_kind(s: str) -> str: + return re.search(r"^> \[!(\w+)\]$", s, re.M).group(1) + + +def table_rows(s: str, header_start: str) -> list[list[str]]: + """The body rows of the table whose header line starts with `header_start`, split into cells.""" + lines = s.split("\n") + i = next(n for n, line in enumerate(lines) if line.startswith(header_start)) + rows = [] + for line in lines[i + 2 :]: + if not line.startswith("|"): + break + rows.append([c.strip() for c in re.split(r"(?\n๐Ÿ”‘" not in s # no waiver block on a pass + + +def test_b_pass_with_a_minor_finding(tmp_path): + s = case_b(tmp_path).summary + assert "## ๐ŸŸก PASS: OC Claude review, 1 finding to consider\n" in s + assert callout_kind(s) == "NOTE" + assert "It does not block this PR." in s + [row] = table_rows(s, "| Severity |") + assert row[:2] == ["๐ŸŸก **Low**", "Does not block"] + assert row[2] == "`src/app.py:10`" and row[4] == "Review (low)" + assert "Setup command uses the old package name\\. See the inline comment." in row[3] + + +def test_c_waivable_fail_is_red_caution(tmp_path): + r = case_c(tmp_path) + s = r.summary + assert r.code == 1 + assert "## ๐Ÿ”ด FAIL: OC Claude review\n" in s + assert callout_kind(s) == "CAUTION" and "[!WARNING]" not in s # ruling 1 + assert "**One high-severity security finding blocks this PR.** A maintainer can waive it." in s + assert "ask a maintainer to post the waiver line" in s + rows = table_rows(s, "| Severity |") + assert [(x[0], x[1]) for x in rows] == [("๐Ÿ”ด **High**", "Blocks, waivable"), ("๐ŸŸก **Low**", "Does not block")] + assert "๐Ÿ”‘ Waiver: how a maintainer clears this" in s + assert f"/oc-review override {HEAD} {MB} " in s + + +def test_d_hard_block_comes_first_and_has_no_waiver(tmp_path): + r = case_d(tmp_path) + s = r.summary + assert "## ๐Ÿ”ด FAIL: OC Claude review\n" in s and callout_kind(s) == "CAUTION" + assert "**Hidden text-direction control blocks this PR. No waiver can clear it.**" in s + assert "One high-severity" in s and "also blocks." in s + rows = table_rows(s, "| Severity |") + assert [x[0] for x in rows] == ["๐Ÿšซ **Hard block**", "๐Ÿ”ด **High**", "๐ŸŸ  **Medium**"] + assert rows[0][1:3] == ["Blocks, no waiver", "`bidi.txt:1`"] and rows[0][4] == "File checks (code, no model)" + assert rows[1][1] == "Blocks, waivable" and rows[1][4] == "Security (high), Supply chain (high)" + # Gate truth: a medium finding in a blocking category does not block (ruling 6). + assert rows[2][1] == "Does not block" and rows[2][4] == "Security (medium), Supply chain (medium)" + assert "/oc-review override" not in s and "๐Ÿ”‘" not in s # a hard block cannot be waived + assert "| ๐Ÿšซ 1 hard block, 2 signals |" in s + assert "โš ๏ธ Sensitive path changed" in s and "โ„น๏ธ First-time contributor" in s + + +def test_e_pass_that_did_not_run(tmp_path): + r = case_e(tmp_path) + s = r.summary + assert "## ๐Ÿ”ด FAIL: OC Claude review did not complete\n" in s and callout_kind(s) == "CAUTION" + assert "The Security review did not run, so this PR could not be judged. No waiver can clear this." in s + [row] = table_rows(s, "| Severity |") + assert row[:3] == ["โš ๏ธ **Not run**", "Blocks, no waiver", "Security pass"] + assert "API unavailable after 3 attempts" in row[3] + assert "| โœ… ok | โœ… ran, 0 findings | โŒ failed | โœ… ran, 0 findings | โœ… ok | โœ… complete |" in s + assert "not recorded" in s and "**Recorded total**" in s # missing usage is never zero + assert "No summary. Reason: API unavailable after 3 attempts" in s + assert "this is not a clean result" in s + assert "/oc-review override" not in s + + +def test_f_pr39_waivable_fail_is_red_caution_with_two_rows(tmp_path): + s = case_f(tmp_path).summary + assert "## ๐Ÿ”ด FAIL: OC Claude review\n" in s + assert callout_kind(s) == "CAUTION" and "[!WARNING]" not in s # ruling 1: not the yellow signal + assert "A maintainer can waive it." in s + # Ruling 3: the high finding blocks, the two medium findings do not. Different effects, different rows. + rows = table_rows(s, "| Severity |") + assert [(x[0], x[1], x[4]) for x in rows] == [ + ("๐Ÿ”ด **High**", "Blocks, waivable", "Review (high)"), + ("๐ŸŸ  **Medium**", "Does not block", "Security (medium), Supply chain (medium)"), + ] + assert "| โœ… ok, 2 signals | โœ… ran, 1 finding | โœ… ran, 1 finding | โœ… ran, 1 finding | โœ… ok | โœ… complete |" in s + assert f"/oc-review override {HEAD} {MB} " in s + + +# ---------------------------------------------------------------- rules that hold in every case + + +def test_a_fail_never_uses_the_warning_callout(rendered): + name, r = rendered + s = r.summary + assert "[!WARNING]" not in s + if r.code == 1: + assert callout_kind(s) == "CAUTION" and "๐Ÿ”ด FAIL" in s + else: + assert callout_kind(s) in ("TIP", "NOTE") and "PASS" in s + + +def test_the_comment_never_implies_approval(rendered): + _, r = rendered + s = r.summary.lower() + assert "approved" not in s and "this bot never approves a pr" in s + + +def test_marker_first_and_details_balanced(rendered): + _, r = rendered + s = r.summary + assert s.startswith(verdict.STICKY_MARKER + "\n") + assert s.count("
") == s.count("
") >= 1 + assert s.count("") == s.count("") == s.count("
") + + +def test_short_sha_is_visible_and_full_shas_are_in_the_record(rendered): + _, r = rendered + s = r.summary + visible = s.split("
", 1)[0] + assert f"head `{HEAD[:7]}` into main" in visible and HEAD not in visible + record = s.rsplit("
", 1)[1] + assert f"| `{HEAD}` | `{MB}` | main |" in record + + +def test_every_table_row_has_the_columns_of_its_header(rendered): + _, r = rendered + lines = r.summary.split("\n") + header = None + for line in lines: + if not line.startswith("|"): + header = None + continue + cols = len(re.split(r"(?", 1)[1] + for title in ("Review title", "Security title", "Supply title"): + assert f"| {title} |" in record + + +def test_same_place_with_different_effects_stay_on_separate_rows(tmp_path): + passes = at_one_place( + ("review", "malicious", "high", "Blocks"), + ("security", "security", "medium", "Does not block"), + ) + s = run(tmp_path, passes=passes).summary + rows = table_rows(s, "| Severity |") + assert [(x[1], x[4]) for x in rows] == [("Blocks, waivable", "Review (high)"), + ("Does not block", "Security (medium)")] + + +def test_a_high_bug_does_not_block_and_does_not_merge_with_a_blocking_finding(tmp_path): + passes = at_one_place( + ("review", "bug", "high", "High bug"), + ("security", "security", "high", "High security"), + ) + s = run(tmp_path, passes=passes).summary + rows = table_rows(s, "| Severity |") + assert [x[1] for x in rows] == ["Blocks, waivable", "Does not block"] + + +def test_one_pass_with_two_findings_in_one_place_counts_them(tmp_path): + passes = at_one_place( + ("review", "bug", "low", "First"), + ("review", "bug", "low", "Second"), + ) + [row] = table_rows(run(tmp_path, passes=passes).summary, "| Severity |") + assert row[4] == "Review (low, 2 findings)" + + +def test_different_lines_and_sides_never_merge(tmp_path): + f = [finding(title="A", quote="line 11", line=11), finding(title="B", quote="line 12", line=12), + finding(title="C", side="base", quote="line 10", line=10)] + rows = table_rows(run(tmp_path, passes=clean_passes(review=f)).summary, "| Severity |") + assert len(rows) == 3 and any("(base side)" in x[2] for x in rows) + + +def test_a_waiver_applied_is_a_green_pass_and_the_findings_stay_listed(tmp_path): + from .test_publish import override + r = run(tmp_path, FakeGitHub(issue_comments=[override(1)]), passes=clean_passes(supply_chain=[BLOCKING])) + s = r.summary + assert r.code == 0 and "## ๐ŸŸข PASS: OC Claude review\n" in s and callout_kind(s) == "TIP" + assert "A maintainer waiver was applied by maintainer. Findings stay listed." in s + [row] = table_rows(s, "| Severity |") + assert row[1] == "Blocks, waived" + assert ("blocking-finding", True) in [(c, w) for c, w, _ in reason_rows(s)] + + +# ---------------------------------------------------------------- size and cuts + + +def test_a_cut_keeps_tables_and_tags_whole(tmp_path, monkeypatch): + real = publish.render_summary + + def small(ctx, result, **kw): + return real(ctx, result, **{**kw, "limit": 9000}) + + monkeypatch.setattr(publish, "render_summary", small) + # Distinct lines, so the rows do not merge. + review = [finding(title=f"Low {i}", file="src/app.py", line=1 + i, quote=f"line {1 + i}", + explanation="word " * 200) for i in range(9)] + r = run(tmp_path, passes=clean_passes(review=review)) + s = r.summary + assert len(s) <= 9000 + assert publish.TRUNCATED_NOTE in s + assert "Some details were truncated, see the run log." in s + assert s.count("
") == s.count("
") + # A note row inside a table keeps the column count of its table. + for line in s.split("\n"): + if publish.TRUNCATED_NOTE in line and line.startswith("|"): + assert len(re.split(r"(? ")) + assert "A required check did not complete" in callout + assert "a maintainer re-runs the newest run" in callout + assert "fix the cause named in the row" not in callout and "then push" not in callout + assert "blocks this PR" not in callout + + +def test_g_no_bundle_strip_does_not_say_ran(tmp_path): + s = case_g(tmp_path).summary + strip = next(line for line in s.split("\n") if line.startswith("| โŒ no result")) + assert "ran" not in strip + assert strip.count("โš ๏ธ not validated") == 3 + + +def test_hard_block_with_a_bundle_still_tells_the_author_to_fix(tmp_path): + s = case_d(tmp_path).summary + assert "fix the cause named in the row, then push" in s + + +def test_dependency_failure_with_blocking_finding_names_the_finding(tmp_path): + tls = finding("security", "high", title=TLS, quote="shell=True") + r = run(tmp_path, passes=clean_passes(security=[tls]), needs=needs_ok("failure")) + callout = "\n".join(line for line in r.summary.split("\n") if line.startswith("> ")) + assert "also blocks" in callout + assert "The author also fixes or waives the finding." in callout + assert "does not need to change anything" not in callout + + +def test_dependency_failure_alone_needs_no_author_change(tmp_path): + r = run(tmp_path, needs=needs_ok("failure")) + assert "The author does not need to change anything." in r.summary + + +def test_long_path_is_cut_once(monkeypatch): + """The schema caps `file` at 400, so a longer path reaches the renderer only + from a future schema change; the merge key and the span must still cut once.""" + f = verdict.CitedFinding("review", "bug", "high", "high", "src/" + "d" * 600 + "/app.py", "head", 10, + "q", "Long path finding.", "e", True, True, True, None) + result = SimpleNamespace(findings=[f]) + monkeypatch.setattr(publish, "_finding_effect", lambda *_: publish.EFFECT_NONE) + [row] = publish._merge_findings(result, publish.InlineOutcome()) + assert row.where.count("โ€ฆ") == 1 and "[truncated]" not in row.where