Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 172 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,61 @@ This binds every reviewer — Codex, the implementation agent's own
maintainer comments, and any third model added later. A finding relayed
without its class is not usable by the next reader.

### Look at CI before approving

**Check the status rollup on the head you are approving. Cancelled,
pending, queued, and absent are all "not green."** Say which you saw.

This is the cheapest control available and the easiest to skip, because
a PR page shows a review box and a checks box and only one of them asks
for your attention.

Measured on #296, from the review and workflow timestamps:

```
head CI approvals green first?
82c9f27e cancelled 11:06Z 10:46Z, 15:18Z no
4a32d142 never completed 16:39Z, 17:18Z no
5e6a2e04 success 17:56Z 19:13Z, 19:18Z yes
```

Four of the six approvals landed against a matrix that was cancelled or
still running, and no review on those two heads cited a check status.
`4a32d142` was approved twice while its run sat `in_progress` — on a
defect that made the suite hang forever on two of the three supported
runtimes, which is why that run never finished. The last head is what
the rule looks like when it is followed.

Approving ahead of CI is sometimes right; a fork PR whose workflow needs
maintainer authorisation cannot go green before someone acts. Then say
so — *"approved with CI pending, on the following local run"* — so the
next reader knows the checkmark was not part of the evidence.

### A local run is evidence only for the runtime it ran on

**State the runtime beside the count.** `1543/1543` is not a result;
`1543/1543 on node v24.11.1` is.

When the package declares an `engines` range, exercise the **floor**
before approving, not just whatever is on your PATH. A version-specific
defect is invisible to any number of runs on one version — repetition
measures flakiness, not portability.

Measured on #296, and reproducible from the PR thread: the full suite
passed `1543/1543` on node v24.11.1 — repeatedly, and for both
independent reviewers. On node v20.20.2 all 36 CA tests pass and **the
process never exits**, because a positive control the tests depend on
cannot be established below v22.15. `npx node@20 --test` took under a
minute and would have caught it. `package.json` declares
`engines: >=18`, and the CI matrix is 18/20/22 — so the runtime that
everyone measured on was the one runtime CI does not cover.

This is an Evidence Class failure of exactly the shape this section
exists for: the measurement was real, honestly reported, and certified
nothing about two thirds of the supported surface. Same family as
running a genuine TLS handshake through the API production does not
call.

### Why this rule exists

On PR #270 the reviewer **endorsed** a claim about agent-id availability
Expand All @@ -150,6 +205,123 @@ When several reviewers are on one PR, the classes are what make their
findings cheap to reconcile — *Measured* from one and *Read* from
another on the same point is a signal, not a contradiction.

## Predicates That Predict Another Program

**When a change decides trust, admission, or rejection — or more
generally when any predicate's job is to predict another program's
behavior — the review must run adversarial inputs through the real
decision path and compare against the oracle production actually uses.
A code read is not sufficient evidence for a claim about which inputs
it accepts.**

Two corollaries. They are the load-bearing part; the rule above them
would not have caught the case that produced it.

- **A. The oracle must be the *same API* production calls, not merely
a real one.** A real check through the wrong entry point is a green
test that certifies nothing.
- **B. Before relying on a test as evidence, verify it reaches the
shipped code — mutate the code and confirm the test fails.**

**"We have tests" is the most common form the reassurance takes, and
it is the one that failed here.** Measured on merged `23346ac9`:
mutating the launcher's CA guard to accept unconditionally left
`test/proxy-forward-ca.test.mjs` passing **12/12**, because the test
exercised a hand-copied twin of the logic rather than the shipped
function. Both reviews cited the suite's pass count; neither established
that it reached the shipped guard.

### Phrasing

Claims of the form *"conservative, never permissive"* / *"fails
closed"* / *"cannot accept X"* are universal quantifications over an
input space. State them **Measured with the input set named**, or
downgrade to a floor: *"these shapes were checked; the set is not
known to be complete."*

### Why this rule exists

PR #283's `ca-trust` guard merged with two approvals and independent
verification of every blocker. It is wrong in **both** directions, and
both reproduce on `main`:

- **False reject** — `new X509Certificate(block)` runs on every PEM
block and the throw escapes; one CRL in a bundle voids the whole
file. Rejection is not the safe direction: the fallback drops every
sibling CA, which is the failure the contract exists to prevent.
- **False accept** — `X509Certificate` ignores the PEM label, so our
CA relabelled `TRUSTED CERTIFICATE` yields byte-identical DER and
passes, while node's loader skips any block not labelled exactly
`CERTIFICATE`.

We were not careless. We measured a write→rename race at 0.88 ms over
5,000 iterations, grepped the rendezvous path, checked file modes —
**and never fed the guard a realistic bundle.** The shape to watch for
is *verifying the checkable parts and reasoning about the deciding
part*, and it is invisible from inside because the deciding function
usually looks readable.

That predicate shipped in #283 and was still being corrected in #296 —
two PRs, and the defects above were found after both had been approved.
When a single function keeps producing new defects after review has
signed off on it, stop asking whether reviewers were diligent and ask
whether the design is a model of an oracle that already exists.

### Read the README before reviewing the code

**When a change depends on what some other program does — a runtime, a
client, an upstream API — read this repo's own README and CHANGELOG
history for that program before reviewing the diff.** The project's
accumulated knowledge of it lives there, and a reviewer who skips it
re-derives from the code alone and will re-derive wrong.

Concretely, and measured against the review bodies themselves: **no
review on #283 mentions Bun or BoringSSL** (0 of 3 — two written Codex
rounds and one empty-bodied approval), while both written rounds reason
about node's `X509Certificate` and node's CA loader. The client stopped
being node at CC v2.1.113, which is documented in this file, in
`README.md`, in `CHANGELOG.md`, and is the reason the `NODE_OPTIONS`
preload was abandoned and this proxy exists in its current form.

The guard documented its own limitation too. It carried the comment
*"Still only a pre-flight guard, not proof… never that **Node** will
verify a given leaf with it. Only a handshake shows that, and the
launcher does not perform one"* (`23346ac:bin/claude-via-proxy.mjs`,
replaced by #296). That comment was **partly answered and partly
misread**: #283 round 1 credits the new tests with verifying "the guard
against real TLS authorization outcomes," so the handshake gap was
noticed — but the handshake in question ran through `tls.connect({ca})`,
which is not the API the launcher uses. The limitation was read, and
answered with the wrong oracle.

The runtime fact stayed unexamined for three further rounds, until it
was measured directly against the shipped binary.

The failure is not that the fact was hidden. It is that reviewing a
diff invites reasoning from the diff, and project history is exactly
the context a diff does not carry.

### The expectations are part of what gets checked

Corollary B says mutate the code to prove the test reaches it. That is
not sufficient on its own: **when a test asserts what another program
does, the expectation itself must have come from that program.**

A row in the CA guard's shape table recorded the predicate's own
behaviour as the expected value. It survived every review that reached
it, and was found only when @codeslake ran the table against the real
loader while building #296's oracle — reported on that PR. The suite was
green throughout.

### Where else it applies

The CA guard is one instance. The unifying property is that **the
oracle exists and we chose to model it instead of calling it.** Also
in this class: `git push --dry-run` as a test of a branch ruleset (it
reports success against a ruleset the server never consulted — read
`gh api repos/<o>/<r>/rulesets` instead); a schema pre-check ahead of
a strict parse; any validator predicting a downstream parser.

## Anti-Bloat Lens (no-directive PRs)

The global baseline's bloat bar is *"larger than the directive's
Expand Down
32 changes: 32 additions & 0 deletions docs/code-reviews/pr-300-round-1-codex.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Review: PR #300

Date: 2026-08-04
Reviewed: `AGENTS.md` at `0a92e8dc81e5b72c2fc0a99be4abdae3eaa5bd4d`
Round: 1
Label applied: changes-requested

## What Is Correct
- Measured: the new predicate rule itself is justified by the historical test gap. I checked merged `23346ac9`, installed that tree's dependencies in a disposable checkout, ran `node --test test/proxy-forward-ca.test.mjs` (`12/12` pass), then mutated the inline launcher guard in `bin/claude-via-proxy.mjs` to accept unconditionally and reran the same file; it still passed `12/12`. That supports [AGENTS.md:214-219](AGENTS.md#L214) as written.
- Read + Measured: the historical launcher quote is anchored correctly now. `git show 23346ac9:bin/claude-via-proxy.mjs` contains the quoted comment at the cited commit, so the switch from a live line number to a historical commit anchor in [AGENTS.md:270-274](AGENTS.md#L270) is the right repair.
- Read: the repo-history premise for the Bun switch is supported. The same `v2.1.113` / Bun fact appears in [README.md:7](README.md#L7), [README.md:694](README.md#L694), [CHANGELOG.md:471](CHANGELOG.md#L471), and [AGENTS.md:52](AGENTS.md#L52), so [AGENTS.md:264-268](AGENTS.md#L264) is grounded in repo artifacts rather than in reviewer memory.
- Read: the pre-existing agent-id figures in [AGENTS.md:176-180](AGENTS.md#L176) are sourceable to the directive history at [docs/directives/proxy-session-budget-attribution.md:102](docs/directives/proxy-session-budget-attribution.md#L102) and [docs/directives/proxy-session-budget-attribution.md:104](docs/directives/proxy-session-budget-attribution.md#L104). I did not independently rerun those external log queries here, so I am treating them as sourced, not freshly re-measured.
- Measured: the runtime-floor paragraph is directionally supported. `package.json` declares `engines.node: ">=18"` at [package.json:25](package.json#L25), and the #296 issue thread records `node 24.11.1 1543/1543, exits clean` plus `node 20.20.2 36/36 pass, NEVER EXITS` in the discussion that led to these rules.

## Blockers
- Measured + Read: the new CI paragraph overstates the historical record and is false as written. [AGENTS.md:140-143](AGENTS.md#L140) says that on PR #296 "every approval on that PR ... was granted while CI was either cancelled or still running." `gh api repos/cnighswonger/claude-code-cache-fix/pulls/296/reviews --paginate` and `gh pr view 296 --json statusCheckRollup` show two final approvals on `2026-08-04`: `vsits-codex-review-agent[bot]` at `19:13:15Z` and `cnighswonger` at `19:18:28Z`, both after the matrix had completed successfully at `17:56Z`. The rule is sound; the example is not. A document about evidence discipline cannot keep a measured example that fails measurement.
- Read: several of the added justifications state reviewer thought-processes as facts when the artifacts only show outcomes. "Nobody looked" in [AGENTS.md:142](AGENTS.md#L142), "none consulted it" in [AGENTS.md:268-269](AGENTS.md#L268), and "Five rounds read past" in [AGENTS.md:274](AGENTS.md#L274) are not mechanically verifiable from repo or GitHub artifacts. At most, the artifacts show approvals before green, missing mention of the Bun switch, or arguments focused on node semantics. That distinction matters because this PR is adding rules about evidence classes and falsifiability; it should not itself rely on mind-reading. Rephrase these to observable claims.

## What Needs Attention
- Read: the CI escape hatch in [AGENTS.md:146-149](AGENTS.md#L146) is acceptable only because it requires the reviewer to say explicitly that CI was pending and to name the local run. Keep that reporting requirement if you revise the section; without it, the exception would become too permissive.
- Read: anti-bloat is mostly fine here. The added sections are not redundant with the earlier Evidence Class text; the predicate/oracle rule, README-history rule, and expectation-source rule each close a distinct failure mode. My concern is accuracy, not size.

## Bloat / Non-Functional
None.

## Recommendations
- Rewrite the #296 CI example to the narrower claim the artifacts actually support. For example: earlier approvals/labels were granted while CI was cancelled or running, but the final approvals on August 4, 2026 landed after green. That still teaches the rule without misstating the timeline.
- Replace mind-reading phrases with observable ones: "the reviews did not cite the check status," "the review thread argued node-loader semantics despite the Bun switch being documented," or similar.
- If you want the agent-id figures to carry "Measured" weight in this file rather than merely sourced weight, add a pointer to the underlying query artifact or log note. Right now they are traceable, but not reproducible from this branch alone.

## Bottom Line
Revise. Most of the new rules survive scrutiny, and the strongest new example — the `23346ac9` mutation staying green at `12/12` — reproduced exactly. But one of the added measured examples is false as written, and several surrounding sentences assert reviewer mental state rather than observable evidence. For a PR whose whole point is to raise the bar on verification language, those are blocking problems. — Codex review
30 changes: 30 additions & 0 deletions docs/code-reviews/pr-300-round-2-codex.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Review: PR #300

Date: 2026-08-04
Reviewed: `AGENTS.md` at `7053366daa60180f6267744fed68a63281a5cf46`
Round: 2
Label applied: changes-requested

## What Is Correct
- Measured: the revised CI table in `AGENTS.md:140-154` is materially better than round 1 and its cells match the GitHub artifacts I checked. `gh api repos/cnighswonger/claude-code-cache-fix/commits/<sha>/check-runs` shows `82c9f27e` had `test (18)` / `test (20)` cancelled at `11:06Z`, `4a32d142` had `test (18)` / `test (20)` still `in_progress` at the two approval times `16:39Z` and `17:18Z`, and `5e6a2e04` had all three test jobs completed successfully by `17:56Z` before the two approvals at `19:13Z` and `19:18Z`.
- Measured: the narrower CI prose at `AGENTS.md:149-154` now tracks that table instead of repeating the false “every approval” claim from round 1. I confirmed six approval events on PR #296 via `gh api repos/cnighswonger/claude-code-cache-fix/pulls/296/reviews --paginate`.
- Read: the third row does not undercut the rule. Keeping one compliant head in the example helps because it shows the contrast the rule is trying to enforce rather than leaving the section as a pile of failures with no positive comparator.
- Measured: PR #300 itself is reviewable on this head. `gh pr checks 300 --repo cnighswonger/claude-code-cache-fix` is green on `7053366d`, and `gh api graphql` for review threads returned no open threads to resolve.

## Blockers
- Read + Measured: `AGENTS.md:275-287` still makes two artifact claims that the written review record does not support. First, “**no round's written findings mention the Bun switch**” is false as written against the review bodies you are invoking: the approved PR #296 review at `5e6a2e04` says the head “adds the missing Bun/BoringSSL veto.” Second, “**No round's findings quote or answer it**” is not safe: PR #283 round 1 explicitly says the new tests “verify the guard against real TLS authorization outcomes,” which is at least an attempted answer to the comment’s “Only a handshake shows that” limitation, even if it later proved to be the wrong oracle. `quote` is grep-able; `answer` is both semantic and, on this record, substantively false. Because this section is itself a rule about falsifiable claims, it needs another wording pass that stays inside what the review texts demonstrably say.

## What Needs Attention
- Read: if you want to preserve the point, I would split the current sentence into claims with different evidence standards. “No #283 review body mentions the Bun switch” is cheaply checkable. “No round quoted the launcher comment” is also checkable. The broader “or answer it” formulation is where the evidence slips.

## Bloat / Non-Functional
- None. The delta is a tight wording correction in one document, and the retained table is proportionate because it replaces an earlier incorrect universal claim with inspectable evidence.

## Recommendations
- Replace `AGENTS.md:275-287` with phrasing limited to the artifacts you can actually cite, for example by separating “did not mention the Bun switch” from the weaker claim that the reviews reasoned from the guard/tests rather than from the project-history context.
- Keep the third CI row. It strengthens the section by showing the compliant case on the same PR timeline.

## Bottom Line
Revise once more. The round-1 blockers about the CI example and reviewer mental-state assertions are mostly fixed, and the CI table now stands up to direct verification. But one replacement paragraph still overstates what the historical review bodies did not say, in exactly the document that tells reviewers not to do that. I would merge this after one more tightening pass on `AGENTS.md:275-287`, not before.

— Codex review
Loading
Loading