diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e8e8ca1..fef54b1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -55,6 +55,12 @@ jobs: - name: Context step end to end run: tests/context-step-test.sh + # Guards the comment style harness's corpus rather than the review workflow: the + # recorded totals in docs/comment-style-harness.md are only comparable while the + # committed corpus keeps the shape they were measured against. + - name: Comment style corpus + run: tests/comment-style-corpus-test.sh + # Runs claude-pr-review.yml itself, from this commit, with the review step skipped. The checks # above all read the workflow as text; this one hands it to Actions and asks whether it starts. # diff --git a/README.md b/README.md index 65887dc..7930cfe 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,10 @@ Automated code review on every pull request using [claude-code-action](https://g - **No issues** — silent approve The review prompt lives in [`docs/claude-pr-review-prompt.md`](docs/claude-pr-review-prompt.md). +Changes to its comment-style rules go through +[`docs/comment-style-harness.md`](docs/comment-style-harness.md) first — the prompt deploys +org-wide from `main` with no staging, and that procedure is the staging. It does not cover +the severity or summary-comment rules; that document says why. ### Frontloaded review context diff --git a/docs/comment-style-corpus.json b/docs/comment-style-corpus.json new file mode 100644 index 0000000..ac8bd20 --- /dev/null +++ b/docs/comment-style-corpus.json @@ -0,0 +1,196 @@ +{ + "fetched_utc": "2026-08-29T17:56:04Z", + "reviewer": "claude[bot]", + "comments": [ + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882309208, + "path": "README.md", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: (not blocking) The check is a third channel into this prompt, and it is the one left unexcluded.\n\n`CHECKS_JQ` (`scripts/gather-review-context.sh:552`) renders every rollup entry as ` / ` with no filter on name or author, so once `pullfrog-approval` has settled before the context step runs — a re-review, or any run where Pullfrog is faster — `FAILURE / pullfrog-approval` lands in the `## CI checks` block. That is the other arm's verdict reaching this reviewer before it forms its own, which is the independence the two exclusions above exist to protect. It also has a behavioural edge the comment threads do not: the review prompt says a failing check is a blocking issue and to name it, so the likely output is a request-changes citing `pullfrog-approval` — a blocking finding this reviewer cannot substantiate from the diff.\n\nAnd if #37 runs Pullfrog as an Actions workflow, `FAILING_JOBS_JQ` (`:555`) scans `/job/([0-9]+)` out of `detailsUrl` and `fetch_raw`s that job's log into a `### Failing job` excerpt, i.e. Pullfrog's own output verbatim, at up to an eighth of the context — the byte argument in the paragraph above, applied to the block that argument does not cover.\n\nExcluding it needs no new constant: the check's `name`/`workflowName` is already in the rollup entry, so both programs can drop it the way the comment programs drop `$skip`. If the verdict is deliberately kept visible, the paragraph is the place to say why the CI block is an exception — as written it reads as \"the check is harmless because it does not gate merge\", which is true of merge and silent about the prompt." + }, + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882310166, + "path": "tests/review-cycle-test.sh", + "line": 81, + "severity": "nit", + "baseline": true, + "body": "nit: (not blocking) The state this change makes ordinary is the one state left untested. After the exclusion, every PR in a trial repo carries a `pullfrog[bot]` review, so the drift warning's only remaining job is to fire when a *drifted* reviewer login sits beside the excluded one. The two assertions here cover pullfrog alone (silent) and `reviews-foreign-reviewer.json` covers a foreign bot alone (fires); nothing covers both in the same fixture.\n\nThat is exactly the gap a wrong-but-plausible predicate slips through — `all(.[][]; ...)`, or filtering `$skip` out of the array and then testing `.user.type` on what remains in a way that short-circuits — passes both existing cases and goes silent on the only case that matters in a trial repo. Adding a `pullfrog[bot]` entry to `reviews-foreign-reviewer.json` closes it without a new file (`LAST_REVIEW_JQ` in `pr-context-test.sh` still yields `\"\"` for that fixture, since it filters on `claude[bot]`), or a `reviews-drift-with-other-bot.json` keeps the two fixtures single-purpose." + }, + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882311081, + "path": "scripts/gather-review-context.sh", + "line": null, + "severity": "super nit", + "baseline": true, + "body": "super nit: (not blocking) Dropping by author leaves replies to dropped comments dangling. A human answering one of `$skip`'s findings is kept, so the block can render `Reply to #2103` with no `Thread #2103` anywhere in it — and the reply's text (\"Fixed in the next push\") reads as a settled finding whose subject this reviewer never sees, which is the \"not to be re-raised\" reading the block is written to produce. Your own `pull-comments-mixed-authors.json` has the shape (2102 → 2101), just with the parent kept.\n\nNot fixable by filtering — the reply is real human feedback and dropping it hides more than it protects — so this is only worth doing if the trial shows humans replying to Pullfrog inline. If they do, relabelling an orphaned reply costs one membership test: `($kept | map(.id)) as $ids` and then `if .in_reply_to_id then (if (.in_reply_to_id | IN($ids[])) then \"Reply to #\\(.in_reply_to_id)\" else \"Reply to a comment excluded from this block\" end) else ...`." + }, + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882389032, + "path": "scripts/gather-review-context.sh", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: (not blocking) The exclusion holds only while #37 names its job `pullfrog`/`pullfrog-*`, lowercase.\n\n`theirs` tests the entry's own name — `.name` for a CheckRun — against `slug` and `slug + \"-\"`, case-sensitively, and never looks at `.workflowName`, which `render` prints as the *first* half of the line it emits. `rollup-with-review-bot.json`'s own `detailsUrl` says these are Actions check runs, and for those `.name` is the job name while `.workflowName` is the workflow's `name:`. So the ordinary shape in the other PR —\n\n```yaml\nname: Pullfrog\njobs:\n review:\n```\n\n— lands in the rollup as `{name: \"review\", workflowName: \"Pullfrog\"}`, `theirs` is false in both programs, and both leaks come back whole: `FAILURE Pullfrog / review` in the list, and that job's log fetched into a `### Failing job` excerpt. The naming that closes the hole lives in #37 and nothing in this repo pins it; the fixture asserts a job that happens to be named `pullfrog`, which is the assumption rather than the guarantee. Same for case — a job named `Pullfrog` misses, since the slug is lowercase by construction and a job name is not.\n\nTwo clauses buy independence from the naming instead of depending on it: match `.workflowName` too (dropping a workflow named for the other reviewer is exactly the intent), and downcase before comparing.\n\n`def theirs: [(if .__typename == \"CheckRun\" then (.name // \"\") else (.context // \"\") end), (.workflowName // \"\")] | any(. != \"\" and (ascii_downcase | . == slug or startswith(slug + \"-\")));`\n\nwith the same predicate in `FAILING_JOBS_JQ` — where it is currently a second, narrower copy of the same rule, so the two can disagree about what \"theirs\" means. A fixture with `{\"name\": \"review\", \"workflowName\": \"Pullfrog\"}` pins it." + }, + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882389427, + "path": "README.md", + "line": null, + "severity": "super nit", + "baseline": true, + "body": "super nit: (not blocking) \"the four exclusions\" leaves the fifth program off the removal list, and it is the one where an incomplete removal fails silently.\n\n`DRIFT_JQ` also carries `$skip`. Delete the constant without touching that program and the call at `scripts/gather-review-context.sh:351` — `jq --arg skip \"$OTHER_REVIEW_BOT\" -e -s \"$DRIFT_JQ\"` with `2>&1` discarded — exits non-zero on an undefined `$skip`, which the `elif` reads as \"no drift\". The backstop against a reviewer-login change is then gone with no warning in either direction, which is the failure this paragraph's own argument is about. Since this section is what the removal will be worked from, worth saying \"the constant, the four exclusions and the drift predicate\"." + }, + { + "repo": "hotdata-dev/github-workflows", + "pr": 38, + "id": 3882438932, + "path": "scripts/gather-review-context.sh", + "line": 571, + "severity": "super nit", + "baseline": true, + "body": "super nit: (not blocking) This paragraph now describes the rule the next one replaces, and the two run together.\n\nLines 562-566 still say the match is \"by name … a CheckRun by `.name`, a StatusContext by `.context`\", which is exactly the name-only, case-sensitive rule cycle 2 established is not enough — a maintainer stopping here reads the current `theirs` as narrower than it is, and reads it as complete. Lines 577-585 say the true rule; the earlier paragraph is the superseded draft of it. Worth folding the two into one, or at least amending 565-566 to \"both names on the entry, downcased\".\n\nAlso missing the `#` separator between \"…gets read as green.\" (570) and \"One definition of…\" (571), so the silent/stated asymmetry and the shared-owner-test rationale render as one paragraph. Every other break in this block has one.\n\nSame class: the block says \"all five programs agree on\" `$skip` while `CHECKS_JQ` and `FAILING_JOBS_JQ` now reach it through `CHECK_OWNER_JQ`, and :302 still says \"three programs below\" (as does `extract_const`'s comment in `tests/lib.sh:45`). The README's removal list is the one that has to be right and it is; these are just counts that stopped counting." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1242, + "id": 3886508630, + "path": "src/catalog/postgres_manager.rs", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: this is the only query against `instances` that computes its cutoff from the pod clock (not blocking).\n\n`last_heartbeat` and `started_at` are both written with `NOW()` (`register_instance`, line 2193), and `migrations/postgres/v35.sql:10-11` states the reason: \"last_heartbeat / started_at are written with the DB clock so staleness comparisons never mix the pod clock with the DB clock.\" The two existing readers follow it — `sweep_orphaned_query_runs` uses `NOW() - (INTERVAL '1 second' * $1)` with a comment saying so, and `prune_dead_instances` does the same. This one binds `Utc::now() - staleness` instead.\n\nThe skew directions aren't symmetric, which matters given the PR's fail-open design:\n\n- pod clock **ahead** of the DB by > staleness → cutoff excludes everything → `None` → fail open. Harmless.\n- pod clock **behind** the DB by > staleness → the window widens to `staleness + skew`, so an instance that is already dead but not yet pruned (`instance_prune_after` ≫ staleness) can still satisfy the predicate and be returned as leader. Every live replica then compares against an owner id that no longer exists and skips its sweep, so maintenance stops fleet-wide until that row is pruned — fail *closed*, which is the one outcome `is_maintenance_leader` is written to avoid.\n\nWith the default 30s heartbeat this needs >120s of skew, so it's unlikely under NTP; I'm flagging it for the invariant rather than the probability. One-line fix, matching `prune_dead_instances`:\n\n```suggestion\n let row: Option<(String,)> = sqlx::query_as(\n \"SELECT owner_id FROM instances\n WHERE last_heartbeat > NOW() - (INTERVAL '1 second' * $1)\n ORDER BY started_at ASC, owner_id ASC\n LIMIT 1\",\n )\n .bind(staleness.num_seconds() as f64)\n```\n\n(The SQLite side is fine as-is — app-clock RFC3339 on both write and read is the documented single-node convention there.)" + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1242, + "id": 3886508712, + "path": "src/engine/jobs/maintenance.rs", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: the two doc blocks have merged (not blocking).\n\nThere's no separator between the end of `submit_maintenance_job`'s doc (lines 61-66, which used to sit directly above it) and the new one starting here, so rustdoc attaches the whole thing to `is_maintenance_leader`. Net effect: `is_maintenance_leader`'s docs open with \"Enqueue a periodic maintenance sweep, deduplicated cluster-wide. Returns the new job id when this call enqueued the sweep…\", which describes a different function, and the `pub` `submit_maintenance_job` at line 113 is left with no doc comment at all.\n\nMoving `is_maintenance_leader` below `submit_maintenance_job` (or above its doc block) fixes it." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1242, + "id": 3886508995, + "path": "src/engine/jobs/maintenance.rs", + "line": 744, + "severity": "nit", + "baseline": true, + "body": "nit: this drops the first-tick catch-up on multi-replica deployments, and `start_ticker`'s doc above (lines 683-686) still claims it (not blocking).\n\nDuring a rolling deploy the *old* pod is both the oldest `started_at` and still heartbeating — `start_heartbeat_worker`'s handle is deliberately aborted last, after in-flight work drains (`instance_liveness.rs:18-21`). So while new pods boot, the leader is a pod whose own ticker has already broken out on the shutdown token. Every new replica's immediate first tick is skipped, and because the gate is only re-evaluated on the next tick, the next attempt is one full interval later — up to an hour for vacuum/compaction.\n\nBounded and not a stall (the last-run guard already spaces sweeps at 0.9× interval, so worst case is ~2 intervals between vacuums after a deploy), but the PR description's \"the immediate first tick stays as it is\" only holds for the single-replica case. Either re-check leadership on a short backoff when the first tick is skipped, or just amend the `start_ticker` doc comment so the catch-up claim isn't left stale." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1242, + "id": 3886509142, + "path": "src/engine/jobs/maintenance.rs", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: the fail-open branches aren't tested (not blocking).\n\n`oldest_live_instance_picks_the_longest_lived_replica` covers the SQL well on both backends, but nothing exercises `is_maintenance_leader` itself: the zero-interval early return, `Ok(None)` → `true`, `Err` → `true`, and `Ok(Some(other))` → `false`. Those four branches are the part where a mistake is silent — an inverted comparison or an accidental `false` on the error path stops maintenance fleet-wide with no failing job to alert on, which is strictly worse than the bug being fixed here.\n\n`FailingCatalog` in `tests/integration/result_persistence_tests.rs` already forwards `oldest_live_instance`, so a variant that returns `Err` (or a fixed owner id) would cover the table cheaply." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3884595787, + "path": "config-compose.toml", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: the stated reason for this block isn't accurate (not blocking).\n\nThe egress guard is only wired into the *guarded outbound HTTP client* — `crate::outbound_http::guarded_client`, constructed at `src/engine/builder.rs:1279` and `src/embedding/openai.rs:320`. Its module doc (`src/core/egress.rs:3`) scopes it to \"every outbound HTTP call made on a caller's behalf\". Object storage does not go through it: `src/storage/s3.rs` and `src/storage/multipart_presign.rs` contain no reference to `EgressPolicy`/`GuardedClient`, and neither do the Postgres catalog or Redis clients.\n\nSo \"without this, the server cannot resolve or reach `minio:9000`\" is not what happens — MinIO, Postgres and Redis are reachable with `[egress]` absent entirely. `scripts/run-local.sh:104-107` is the corroborating precedent: it allowlists only loopback, and its comment says explicitly that it's for a local *embeddings* endpoint.\n\nTwo consequences worth acting on:\n\n- The block as written opens all three RFC1918 ranges, which CLAUDE.md calls out specifically (\"a specific range, not all of RFC1918\").\n- A reader who trusts this comment will carry the same allowlist into a deployment to \"make storage work\", where it does open real reach.\n\nEither drop `[egress]` from this file (nothing in the stack needs it), or keep it with a comment saying what it actually enables — e.g. pointing an embedding provider at a container on this network." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3884596296, + "path": "scripts/compose-smoke.sh", + "line": null, + "severity": "nit", + "baseline": false, + "body": "nit: this comment is stale as of this same commit (not blocking).\n\nThe diff fixes `src/datafusion/bm25_table_function.rs:46` to read `'catalog.schema.table'`, so line 46 no longer says what this paragraph says it says. A reader who follows the pointer finds the opposite of the claim and has to work out which one is current.\n\nThe useful half is the two-identifier distinction above it (lines 154-158), which stands on its own. Suggest dropping the \"Note the doc comment ... says\" paragraph, or rewording to the positive form: `bm25_search` takes the catalog name (`default_catalog`), while the index routes take the connection id." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3884597044, + "path": "Dockerfile.compose", + "line": null, + "severity": "nit", + "baseline": false, + "body": "nit: `set -e` makes the failure-path cleanup unreachable (not blocking).\n\n`cargo build` on line 81 is a simple command in an `set -e` shell, so a non-zero exit aborts the `RUN` right there. `status=$?` never executes, and neither does the `git config --unset-all` / `rm -f /root/.gitconfig` pair the comment on lines 64-67 describes as \"belt and braces\". On the success path `$status` is always 0, so line 85's guard never does anything either.\n\nThe outcome is still correct — a failed build fails the stage and the layer is discarded, so the token never lands anywhere — but the code reads as if it handles the failure case and doesn't. Either drop the `status` dance and let `set -e` do the work:\n\n```\n cargo build --release --bin server; \\\n git config --global --unset-all url.\"https://x-access-token:$(cat /run/secrets/github_token 2>/dev/null)@github.com/\".insteadOf 2>/dev/null || true; \\\n rm -f /root/.gitconfig; \\\n cp target/release/server /build/server\n```\n\nor keep the cleanup on both paths by making the build's status observable, e.g. `if cargo build --release --bin server; then status=0; else status=$?; fi`." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3884597515, + "path": "Dockerfile.compose", + "line": null, + "severity": "super nit", + "baseline": true, + "body": "super nit: lld is installed but never used (not blocking).\n\nCI reaches lld via `RUSTFLAGS: \"-C debuginfo=0 -C link-arg=-fuse-ld=lld\"` (`.github/workflows/ci.yml:63`, `:102`); this stage sets no `RUSTFLAGS`, so cargo links with the default `cc`/`ld`. \"lld matches CI's linker\" describes an intent the file doesn't carry out.\n\nSince a cold build here is the 30-60 min the header advertises, adding the flag is probably the version you want rather than dropping the package:\n\n```dockerfile\nENV RUSTFLAGS=\"-C debuginfo=0 -C link-arg=-fuse-ld=lld\"\n```" + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3884598006, + "path": "docker-compose.yml", + "line": null, + "severity": "nit", + "baseline": true, + "body": "nit: on the question you raised in the description — I'd take the narrow profile (not blocking).\n\nYour reasoning for a disposable local rig is sound and I'm not going to block on it. The argument for doing it anyway is that this is the one line most likely to get copied out of context: it's short, it's the fix for an opaque startup failure, and the surrounding \"NOT FOR SHARED USE\" caveat lives 100 lines up in the header rather than next to the knob. A three-syscall allowlist costs one JSON file and keeps the rest of the default profile.\n\n```yaml\n security_opt:\n - seccomp=./docker/seccomp-io-uring.json\n```\n\nwhere the file is Docker's default profile plus `io_uring_setup`, `io_uring_enter`, `io_uring_register` in the allowed names list. Your call — the diagnosis in the comment is the valuable part either way, and it's right." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3885278816, + "path": "Dockerfile.compose", + "line": null, + "severity": "blocking", + "baseline": true, + "body": "The builder's distro is below the PROJ floor this crate documents.\n\n`Cargo.toml:58` states the constraint explicitly: *\"PROJ 9.4 == the proj-sys 0.25 floor — the crate pins proj 0.29 for exactly that\"*, and CI honours it by building on Ubuntu 24.04 (`.github/workflows/ci.yml:41,61,100,204` are all `ubuntu-2404`, with `libgeos-dev libproj-dev` installed at `:81,:125,:231` — PROJ 9.4 there).\n\nThis stage is `rust:1.94.0-bookworm`. Debian bookworm's `libproj-dev` is PROJ **9.1.1**, so `pkg-config --atleast-version=9.4.0 proj` fails and `proj-sys` has no system PROJ to bind against. `libgeos-dev` is fine (bookworm is 3.11.1, which clears the `geos-3_11` floor) — it's only PROJ that's short.\n\nI could not run the build to confirm the failure mode, and the description says a cold build completed, so one of these is true and it's worth pinning down in the comment either way:\n\n- it fails at `proj-sys`, and `docker compose build` — the headline feature — is broken for everyone; or\n- `proj-sys` falls back to building PROJ from source, which needs `cmake`, `libsqlite3-dev`, `libtiff-dev` and `libcurl4-openssl-dev` that this stage doesn't install, and would add materially to the 30-60 min the header advertises.\n\nThe cheap fix is a base whose PROJ clears the floor — Debian trixie ships 9.6:\n\n```suggestion\n# lld matches CI's linker; libgeos-dev/libproj-dev are the build-time halves of\n# the libgeos-c1t64/libproj25 runtime packages the server dynamically links.\n# The base must be trixie, not bookworm: proj-sys 0.25 needs PROJ >= 9.4 (see\n# Cargo.toml) and bookworm ships 9.1.1. CI builds on Ubuntu 24.04 for the same\n# reason.\nRUN export DEBIAN_FRONTEND=noninteractive && apt-get update && apt-get install -y \\\n lld \\\n libgeos-dev \\\n libproj-dev \\\n && rm -rf /var/lib/apt/lists/*\n```\n\n(paired with a `-trixie` tag on the `FROM` above)." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3885279197, + "path": "Dockerfile.compose", + "line": null, + "severity": "nit", + "baseline": false, + "body": "nit: the base tag and `rust-toolchain.toml` don't actually agree (not blocking).\n\n`rust-toolchain.toml:2` is `channel = \"1.95.0\"`; this is `rust:1.94.0-bookworm`. Because the file is copied in below, rustup honours it and silently downloads 1.95.0 on the first cargo invocation — so the build works, but the base pin buys nothing (a second toolchain is fetched every cold build) and the comment above describes a coupling the file doesn't have.\n\n```suggestion\n# Pinned to rust-toolchain.toml's channel. rust-toolchain.toml is copied first\n# so a channel bump invalidates from here rather than silently building on the\n# base image's default toolchain.\nFROM rust:1.95.0-bookworm AS builder\n```\n\nWorth keeping in step with `rust-toolchain.toml` when the channel moves, since nothing here fails loudly if it drifts." + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3886931333, + "path": "Dockerfile.compose", + "line": 119, + "severity": "nit", + "baseline": false, + "body": "nit: the git-dependency clones aren't cached, so every rebuild re-fetches them (not blocking).\n\n`CARGO_HOME=/usr/local/cargo`, and only `$CARGO_HOME/registry` has a cache mount. Cargo puts git dependencies in `$CARGO_HOME/git/db` and `$CARGO_HOME/git/checkouts`, which sit in this RUN's own layer — discarded and re-created whenever the layer re-executes. None of the parent layers (apt, rustup, the `COPY`s) contain that directory, so a rebuild after any `src/` edit re-clones the whole git set from scratch: `datafusion-functions-json`, `geodatafusion-hotspatial`, `datafusion-ducklake`, `datafusion-vector-search-ext`, `hotquery`, `liquid-cache`, plus the `geoarrow-rs` repo behind the three `[patch.crates-io]` entries. That works against the \"a rebuild after a source edit is a small fraction of that\" claim in the header, and it re-exercises the private-repo auth path every time.\n\nOne extra mount fixes it:\n\n```suggestion\n --mount=type=cache,target=/usr/local/cargo/registry \\\n --mount=type=cache,target=/usr/local/cargo/git \\\n```\n" + }, + { + "repo": "hotdata-dev/runtimedb", + "pr": 1236, + "id": 3886931591, + "path": "docker-compose.yml", + "line": 256, + "severity": "super nit", + "baseline": false, + "body": "super nit: `>/dev/null 2>&1` makes an `apk` failure silent (not blocking).\n\nIf the package fetch fails — no outbound network, a mirror hiccup — the `&&` short-circuits, `bash /smoke.sh` never runs, and `docker compose --profile smoke run --rm smoke` exits non-zero having printed nothing at all. That is exactly the failure mode the script's own header argues against (\"a smoke test that only says 'failed' costs more time than it saves\"), and it reads as if the stack is broken rather than the setup step.\n\nDropping the redirect costs a few lines of apk progress output before step 1:\n\n```suggestion\n - \"apk add --no-cache bash curl jq && bash /smoke.sh\"\n```\n" + } + ] +} diff --git a/docs/comment-style-harness.md b/docs/comment-style-harness.md new file mode 100644 index 0000000..75b0c22 --- /dev/null +++ b/docs/comment-style-harness.md @@ -0,0 +1,161 @@ +# Comment style harness + +A procedure for testing changes to the review prompt's `## Comment Style` section before +they reach `main`. It is not a CI check and cannot be one: it needs model calls, and its +output is prose that another model judges. Run it by hand, as a gate on the pull request. + +## Why it exists + +The prompt deploys org-wide from `main` with no staging, so the first time a rule change +meets a real pull request is in production, across every repository at once. Three rule +changes have been through this harness. It caught a defect in each, and none of the three +had been predicted by the people who wrote the rules: + +- A `
`-folding ruleset that read as a 40% reduction was cutting **2%** of real + volume (the `
` row in `## Baseline`: 2,338 → 2,290). It relocated text rather + than deleting it, and folded text returns whole through `` on + later review cycles. +- Folding scope-limiting facts made comments read **more** severe than the originals they + replaced: the finding stayed visible while the bound on it did not. +- Coupling proof to convention-existence ("write a failure scenario only for blocking + findings, or where no convention covers the issue") dropped the consequence from the two + highest-impact findings in the corpus, because a convention happened to exist for both. + A convention existing means the fix is uncontroversial. It says nothing about how bad + the bug is. + +## When to run it + +Any edit to `## Comment Style` in `docs/claude-pr-review-prompt.md`. + +Not `## Severity Classification`, and not the summary-comment half of `## Output Rules`. +The harness cannot exercise either — see below. + +## What it cannot tell you + +The harness measures how comments are **written**, given findings that already exist. It +rewrites real posted comments under a candidate ruleset; it does not review any code. + +It therefore says nothing about finding-rate, false positives, or missed bugs. A rule +change that made the reviewer notice less would pass this harness cleanly. Do not cite it +as evidence of review quality in that sense. + +Two narrower gaps follow from how the corpus is built, and both look like coverage until +you check: + +- **Severity rules are not exercised.** Each comment's severity is derived once, from the + prefix it was posted with (`scripts/fetch-comment-style-corpus.sh`), and the agents are + told to preserve every claim. Nothing re-classifies anything, so a change to + `## Severity Classification` would pass without ever being applied. +- **Summary comments are not in the corpus.** It holds inline review comments only. The + summary-comment rules in `## Output Rules` — the format block, and the rule that a + summary appears only when requesting changes — have no example to act on. + +Extending the corpus to summary bodies is the cheaper of the two to fix, and would need +the review bodies as well as the inline comments. + +## The corpus + +`docs/comment-style-corpus.json`, refreshed by `scripts/fetch-comment-style-corpus.sh`. +The bodies are committed rather than fetched live, so an edited or deleted comment cannot +move the baseline silently. + +19 comments over three pull requests, of which 14 carry `baseline: true` and are the ones +every recorded number was measured against. They were chosen for adversarial shape, not +coverage: + +| Source | Exercises | +|---|---| +| `github-workflows#38` | shell and jq; one comment fusing three findings; one whose finding *is* its mechanism | +| `runtimedb#1242` | Rust and SQL; long mechanism chains; a low-likelihood, high-consequence nit | +| `runtimedb#1236` | Docker and TOML; the only blocking finding; two absence-claims | + +The five non-baseline comments are corpus but not baseline. Rewriting them produces a +total that no recorded number compares to. + +## Running it + +Dispatch two agents in parallel, one per repository, so the two reports are independent. +Each gets: + +1. The candidate `## Comment Style` text **verbatim**, with a note on what changed. +2. Its slice of the corpus. +3. The brief below. +4. Acceptance criteria, when there is a specific regression to check. + +### The brief + +Two lines in it do most of the work. Keep both. + +> Apply the style rules faithfully. Do not optimise for shortness beyond what the rules +> require. +> +> Preserve every technical claim. You are restyling, not re-reviewing. Do not add findings +> and do not drop findings. +> +> Where a rule forced a genuinely bad tradeoff, say so explicitly rather than silently +> working around it. + +The last line is what produces the useful signal. Without it an agent resolves an +ambiguous rule quietly and the ambiguity never surfaces. + +Rewriting rather than re-reviewing is deliberate: re-reviewing a past pull request changes +which findings appear, and the difference between rulesets is then unreadable against that +variance. + +### Ask for + +- The rewritten comments in full. +- A word-count table: original, previous run, this run. +- An assessment naming, per comment: whether it reads more severe, less severe or the same + as the original; whether the author can act on it; and any rule that was ambiguous. + +### Acceptance criteria + +State them as pass-or-fail before the run, and make each name a specific comment. Vague +criteria produce vague reports. The gate run for the current rules used: + +1. Consequence restored on the comments that had lost it (name them). +2. The comments that were unactionable are actionable (name them). +3. No distortion in either direction — "same" is the pass, on every comment. +4. No regression on the comments that had improved (name them). + +Criterion 4 matters more than it looks. Two of the three defects above were introduced by +a change that fixed something else. + +## Counting method + +Pin it, or runs are not comparable. The two agents that ran the gate disagreed about the +size of the same text: 3% on the runtimedb slice, and 1.7% over the whole baseline (2,377 +against 2,338 — see the note under `## Baseline`). Both are the same disagreement measured +over different denominators, which is the reason to derive every percentage from a table +row rather than restate one. + +Prose words only: strip fenced code blocks, strip the `nit:` / `super nit:` / +`(not blocking)` prefix, then count whitespace-separated tokens. Folded `
` content +counts as prose — a ruleset that hides text has not removed it. + +## Baseline + +Measured over the 14 baseline comments, by the two agents that ran each ruleset. + +| Ruleset | Total prose words | Change | +|---|---|---| +| As posted | 2,338 | — | +| Sentence caps + `
` folds | 2,290 | −2% | +| Proof-scaling, first draft | 938 | −60% | +| Shipped (#39) | 1,256 | −46% | + +The shipped row measures the section as the gate run saw it. Two fixes landed on #39 after +that run — `One finding per comment` scoped to inline comments, and the context disclosures +exempted from the self-subject rule — so the merged text is a little longer than 1,256 words +of output implies. Neither changes what the rules ask for on the corpus, which holds no +summary comment and no disclosure. + +The first draft of proof-scaling scored best and was rejected: it bought the extra 14 +points by dropping consequences, which is the second defect listed above. + +Recomputing the "as posted" row by the pinned method above gives 2,377 rather than 2,338 — +a 1.7% disagreement between two agents counting the same text. The rows here are internally +consistent because one agent counted each, but a future ruleset measured by the pinned +method will not be exactly comparable to them. Re-measure the "as posted" row alongside any +new one. diff --git a/scripts/fetch-comment-style-corpus.sh b/scripts/fetch-comment-style-corpus.sh new file mode 100755 index 0000000..b73da77 --- /dev/null +++ b/scripts/fetch-comment-style-corpus.sh @@ -0,0 +1,98 @@ +#!/usr/bin/env bash +# Snapshots the review comments the comment-style harness measures against. +# +# The harness compares rewritten comments to what the reviewer actually posted, so its +# numbers only mean something against a fixed corpus. Re-fetching on every run would let +# an edited or deleted comment move the baseline silently, which is why the bodies are +# committed rather than pulled live. Run this only to add a PR to the corpus or to +# refresh it deliberately, and say so in the commit that changes the numbers. +# +# Usage: scripts/fetch-comment-style-corpus.sh [output-path] +set -euo pipefail + +OUT="${1:-docs/comment-style-corpus.json}" + +# owner/repo:pr. Chosen for adversarial shape rather than coverage -- see +# docs/comment-style-harness.md for what each one exercises. +SOURCES=( + "hotdata-dev/github-workflows:38" + "hotdata-dev/runtimedb:1242" + "hotdata-dev/runtimedb:1236" +) + +# The shape the recorded numbers were measured against. Baseline membership is derived +# from whichever comments exist at fetch time, and #38 and #1242 contribute all of theirs, +# so one comment added, deleted, or edited to lose its `nit:` prefix silently rewrites the +# set that docs/comment-style-harness.md says its totals came from. The guard at the end +# refuses to leave that unremarked; tests/comment-style-corpus-test.sh reads these two +# constants rather than repeating them. Changing the corpus on purpose means changing them +# in the same commit as the table. +EXPECTED_COMMENTS=19 +EXPECTED_BASELINE=14 + +# The 14 comments the baseline in docs/comment-style-harness.md was measured against. +# Every comment on #38 and #1242 qualified; #1236 contributes four of its nine, picked +# to add a blocking finding and a second domain. The rest of #1236 is corpus but not +# baseline -- rewriting it would produce a total that no recorded number compares to. +BASELINE_IDS='[3884595787, 3884597515, 3884598006, 3885278816]' + +# The reviewer's login. Comments from anyone else are the PR conversation, not review +# output, and restyling them would measure the wrong thing. +REVIEWER='claude[bot]' + +tmp=$(mktemp) +# All three, not just $tmp: .page and .merged are removed inline at the end of each +# iteration, so an abort between them -- a gh 404 on a repo the token cannot see is the +# likely one -- leaves them behind under set -e. +trap 'rm -f "$tmp" "$tmp".page "$tmp".merged' EXIT +echo '[]' > "$tmp" + +for source in "${SOURCES[@]}"; do + repo="${source%%:*}" + pr="${source##*:}" + echo "fetching ${repo}#${pr}" >&2 + # --paginate because a long review history spans pages, and a partial corpus is a + # baseline that quietly drifts from the one the numbers were taken against. + # --slurp so multi-page histories arrive as one array of pages rather than + # concatenated arrays, which jq cannot read as a single document. + gh api "repos/${repo}/pulls/${pr}/comments" --paginate --slurp \ + | jq --arg repo "$repo" --arg pr "$pr" --arg who "$REVIEWER" \ + --argjson baseline_extra "$BASELINE_IDS" ' + [ .[][] + | select(.user.login == $who) + | { repo: $repo, + pr: ($pr | tonumber), + id: .id, + path: .path, + line: .line, + # The severity the rules key off. Anything without a prefix is a blocking + # finding: the prompt requires the marker on non-blocking comments only. + severity: (if (.body | startswith("super nit:")) then "super nit" + elif (.body | startswith("nit:")) then "nit" + else "blocking" end), + # Whole-PR membership for #38 and #1242; per-comment for #1236. + baseline: (($pr | tonumber) as $n + | if $n == 1236 then (.id | IN($baseline_extra[])) else true end), + body: .body } + ]' > "${tmp}.page" + jq -s '.[0] + .[1]' "$tmp" "${tmp}.page" > "${tmp}.merged" + mv "${tmp}.merged" "$tmp" + rm -f "${tmp}.page" +done + +jq --arg who "$REVIEWER" '{ + fetched_utc: (now | todate), + reviewer: $who, + comments: . +}' "$tmp" > "$OUT" + +count=$(jq '.comments | length' "$OUT") +baseline=$(jq '[.comments[] | select(.baseline)] | length' "$OUT") +if [ "$count" != "$EXPECTED_COMMENTS" ] || [ "$baseline" != "$EXPECTED_BASELINE" ]; then + echo "corpus changed shape: ${count} comments / ${baseline} baseline" \ + "(expected ${EXPECTED_COMMENTS}/${EXPECTED_BASELINE})." >&2 + echo "The totals in docs/comment-style-harness.md were measured against the expected" \ + "shape. Re-measure the baseline table, or restore the corpus." >&2 + exit 1 +fi +echo "wrote ${count} comments to ${OUT} (${baseline} baseline)" >&2 diff --git a/tests/comment-style-corpus-test.sh b/tests/comment-style-corpus-test.sh new file mode 100755 index 0000000..e53967b --- /dev/null +++ b/tests/comment-style-corpus-test.sh @@ -0,0 +1,93 @@ +#!/usr/bin/env bash +# +# The corpus is committed so the numbers in docs/comment-style-harness.md stay checkable, but +# committing it does not by itself hold the baseline still: the fetch script derives baseline +# membership from whichever comments exist on the source pull requests at fetch time, and two +# of the three sources contribute every comment they have. One comment added to #38, one +# deleted from #1242, or one edited to lose its `nit:` prefix, and a re-fetch writes a corpus +# the recorded totals were never measured against, with nothing failing. +# +# So the shape is asserted here and declared in the script, and the script's own guard reads +# the same two constants. Changing the corpus deliberately means changing them, which is the +# "say so in the commit" step made mechanical. +set -euo pipefail + +cd "$(dirname "$0")/.." + +CORPUS=docs/comment-style-corpus.json +FETCH_SCRIPT=scripts/fetch-comment-style-corpus.sh +failures=0 + +expect() { + if [ "$1" = "$2" ]; then + echo "ok $3" + else + echo "FAIL $3" + printf ' expected: %s\n got: %s\n' "$2" "$1" + failures=$((failures + 1)) + fi +} + +# Read out of the script rather than repeated here, for the reason tests/lib.sh extracts the +# shipped jq: a second copy of a constant drifts from the first, and a test asserting 19 while +# the script guards 14 is a test that passes over the bug. +# +# Deliberately not lib.sh's extract_const, which is the same extraction: it searches +# JQ_SOURCES -- the workflow and the context script -- and widening that array for two +# integers would put the fetch script in front of every other test's extractions, where +# extract_jq rejects any name it finds in more than one source. The duplicate-assignment +# guard is worth carrying across, though; without it two assignments yield a two-line value +# and the failure surfaces as a puzzling `expected: 19\n20` further down. +const_of() { + local name=$1 value + value=$(sed -n "s/^$name=\([0-9]*\)\$/\1/p" "$FETCH_SCRIPT") + if [ -z "$value" ]; then + echo "FAIL: no $name= assignment in $FETCH_SCRIPT" >&2 + exit 1 + fi + if [ "$(printf '%s\n' "$value" | wc -l)" -ne 1 ]; then + echo "FAIL: more than one $name assignment in $FETCH_SCRIPT" >&2 + exit 1 + fi + printf '%s' "$value" +} + +EXPECTED_COMMENTS=$(const_of EXPECTED_COMMENTS) || exit 1 +EXPECTED_BASELINE=$(const_of EXPECTED_BASELINE) || exit 1 + +expect "$(jq '.comments | length' "$CORPUS")" "$EXPECTED_COMMENTS" \ + "the committed corpus holds the number of comments the fetch script expects" +expect "$(jq '[.comments[] | select(.baseline)] | length' "$CORPUS")" "$EXPECTED_BASELINE" \ + "the committed corpus holds the number of baseline comments the fetch script expects" + +# The baseline rows in the harness document are per-source, so a source dropping out of the +# baseline entirely would still satisfy the totals above. +expect "$(jq '[.comments[] | select(.baseline) | .pr] | unique | length' "$CORPUS")" "3" \ + "all three sources still contribute to the baseline" + +# An empty body restyles to nothing and would quietly shrink every total measured against it. +expect "$(jq '[.comments[] | select(.body | length == 0)] | length' "$CORPUS")" "0" \ + "no comment in the corpus has an empty body" + +# Only reachable by hand-editing the JSON: the fetch script's if/elif/else emits one of these +# three and nothing else. Kept as the guard against that hand-edit, not against drift. +expect "$(jq -r '[.comments[] | select(.severity | IN("nit", "super nit", "blocking") | not)] | length' "$CORPUS")" "0" \ + "every comment carries one of the three known severities" + +# The drift the paragraph at the top describes. severity is derived from the posted prefix, so +# a comment edited to lose its `nit:` prefix re-fetches as blocking -- a valid value, leaving +# every count above unchanged while the corpus quietly gains a second blocking comment, and the +# rules treat that tier least like a nit. The split is what moves, so pin the split. +expect "$(jq '[.comments[] | select(.severity == "blocking")] | length' "$CORPUS")" "1" \ + "the corpus still holds exactly one blocking comment" +expect "$(jq '[.comments[] | select(.severity == "nit")] | length' "$CORPUS")" "13" \ + "the corpus still holds thirteen nits" +expect "$(jq '[.comments[] | select(.severity == "super nit")] | length' "$CORPUS")" "5" \ + "the corpus still holds five super nits" + +if [ "$failures" -eq 0 ]; then + echo "all tests passed" +else + echo "$failures test(s) failed" + exit 1 +fi