Merge pull request #95 from fiddlerwoaroof/docs/read-the-reviews

docs: read the reviews before merging; authorship will not identify them

AGENTS.md

@@ -153,6 +153,28 @@ worth it on a repo with other collaborators.
 One PR per wave, built by a **dedicated PR agent in its own worktree**, so waves
 pipeline against it: the next wave runs while the PR absorbs review fixes.
 
+**Read the PR's reviews before merging it.** This is separate from CI and from
+the adversarial agent, and it has been missed: two ChatGPT Codex reviews landed
+on merged PRs and were not read until afterwards, one of them a live P2
+(improvise-x1ns). Both were correct.
+
+```bash
+gh pr view <n> --json reviews -q '.reviews[]|"[\(.state)] \(.submittedAt) \(.commit.oid[0:8])\n\(.body)"'
+```
+
+Two things make a Codex review easy to miss. It arrives as a **review**, not a
+comment, so a glance at the comment thread does not show it. And it posts under
+the repository owner's account — the same account our own agents post under —
+so **authorship does not distinguish it**. The tell is the trailing line
+`_Automated review by ChatGPT Codex (…)_`; ours open with
+`_Posted by Claude (…)_`. Check the `commit.oid` too: a review may target an
+earlier head, and the finding may already be fixed — verify rather than assume
+in either direction.
+
+Reviews arrive **as the owner's quota allows**, so absence is not evidence.
+Wait where a review is plausible, and when one is found, answer it on the
+thread even if the PR has already merged: that is where the next reader looks.
+
 **Copilot is not reviewing this repository, and the request is a silent no-op.**
 Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61,
 #66, #68 and #70 carry none. Both request paths report success and produce zero

prolog-sessions/kb.pl

@@ -287,6 +287,8 @@ doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild,'pro
 doctrine(a_conflicting_pr_runs_no_ci_because_it_has_no_merge_ref,'A pull_request workflow runs against refs/pull/N/merge, and GitHub cannot build that ref for a CONFLICTING pull request -- so NO RUN IS EVER CREATED and the PR simply shows zero checks. That is indistinguishable from a dropped webhook if you only look at the runs list. PR #92 sat with zero checks across two pushes and a close/reopen; the cause was that PR #91 had merged and both had appended to the same four context/ files. DIAGNOSIS ORDER: before theorising about event delivery, ask \`gh pr view N --json mergeable\` -- one call, and it answers first. The orchestrator ran a genuinely good control (push to another PR, watch a run appear in five seconds) which correctly proved delivery works repo-wide, and then drew the WRONG conclusion from a sound experiment, because the hypothesis space had two entries and the truth was a third. A control discriminates only between the hypotheses you listed.').
 doctrine(no_checks_reported_is_a_merge_state_symptom,'A pull_request-triggered workflow builds refs/pull/N/merge, and a CONFLICTING pull request HAS NO MERGE REF, so no run is ever created -- \"no checks reported\" then looks IDENTICAL to a dropped webhook. Measured on PR #92, 2026-09-21: 34 minutes of polling actions/runs?head_sha=, a close/reopen and an empty commit all produced zero runs; one git merge main produced a run in seconds. ASK gh pr view --json mergeable,mergeStateStatus FIRST -- one call answers what no amount of polling the runs API can distinguish. Both the agent and the orchestrator hypothesised a dropped event and a stalled delivery; NEITHER LISTED THE THIRD CASE, and the orchestrator control -- pushing to another PR and watching a run appear in five seconds -- is consistent with all three. A control discriminates only between the hypotheses you listed.').
 doctrine(grep_over_git_diff_is_silently_empty_under_an_external_differ,'This repo configures diff.external = difftastic, which prints a SIDE-BY-SIDE format carrying no unified-diff markers. So \`git diff | grep \"^-\"\` matches nothing REGARDLESS OF CONTENT, and an agent verifying \"nothing was dropped in the merge\" read three consecutive empty outputs as three clean bills. Verified by the orchestrator: on a file with one real deletion, git diff | grep -c \"^-\" gives 0 and git diff --no-ext-diff | grep -c \"^-\" gives 1. Use --no-ext-diff in any programmatic read of a diff, or --numstat / --name-only, which are unaffected. The general shape, and it is the sharpest form of check_the_checker: A CHECK THAT RETURNS EMPTY FOR THE WRONG REASON IS INDISTINGUISHABLE FROM ONE THAT PASSES -- and an empty result is the one outcome nobody investigates.').
+doctrine(read_the_reviews_before_merging_and_authorship_will_not_identify_them,'Twenty PRs were merged in one campaign without reading their REVIEWS -- CI green plus a dispatched adversarial agent felt like the whole gate, and it was not. Two ChatGPT Codex reviews were found afterwards on merged PRs; both were correct and one was a live P2 (improvise-x1ns). Three things make a Codex review easy to miss. It arrives as a REVIEW, not a comment, so scanning the comment thread does not show it. It posts under the repository OWNER account -- the same account our own agents post under -- so AUTHORSHIP DOES NOT DISTINGUISH IT; the tell is the trailing line \"Automated review by ChatGPT Codex\", where ours OPEN with \"Posted by Claude\". And it may target an EARLIER head, so a finding can be already fixed or still live and you must verify which rather than assume from the date. Reviews arrive as the owner quota allows, so absence is not evidence of approval. Answer a review on the thread even after the PR has merged: that is where the next reader looks.').
+doctrine(bounded_by_a_later_check_does_not_bound_the_cost,'improvise-x1ns: encoded_width is a PREFLIGHT whose whole purpose is to refuse an expansion BEFORE allocating it, and its categorical branch under-counted by the missing level that build_categorical actually emits. The wave own review had seen the shape and reasoned it away as \"bounded by the exact check\". Codex rebuttal is the keeper: THE EXACT CHECK RUNS AFTER THE ALLOCATION, so it bounds the error MESSAGE, not the cost. When a guard exists to prevent work rather than to report a wrong answer, a downstream correctness check is not a backstop for it -- the thing you were protecting has already happened. Second instance on this path: improvise-zv8t own round found forcing by NAME while budgeting by INDEX, repaired as a data dependency so the ordering became a compile error. Both are the same shape -- two halves of one decision disagreeing about what a column costs.').
 work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result<String,String>; three tests in ui/effect.rs.').
 work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.').
 work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.').