Wrap informal definitions/theorems in individual crossref divs - #78
Wrap informal definitions/theorems in individual crossref divs#78d-morrison wants to merge 5 commits into
Conversation
Both subsections stated a definitional-precision claim ("A **static
strategy** assigns...", "A **dynamic strategy** adapts...") but rendered
as plain numbered subsections instead of boxed Definition environments,
unlike every other definition in the chapter.
Part of #77.
Chapters 1, 3-13, 15-18, 20-23: 53 additional definitions, theorems,
and one algorithm were stated with definitional precision (a bolded
term plus defining language, or a formula-naming sentence) but never
wrapped in the book's `::: {#def-...}` / `{#thm-...}` / etc. crossref
div, so they rendered as plain numbered subsections instead of boxed,
numbered environments like their sibling definitions.
Each fix wraps only the core defining statement, leaving surrounding
motivation, bullet-list examples, and "why this matters" commentary
as plain prose outside the div, matching the existing convention
(see ch19's Static/Dynamic Strategies from the prior commit). No new
`{#exm-...}` divs were added and no `@def-...` crossref links were
introduced, since neither pattern exists anywhere else in the book.
Chapter 14 needed no changes (already fully boxed); chapter 2 likewise.
Closes #77.
|
Warning Claude review skipped — API credential or quota unavailable. No |
Self-review (Claude review was skipped — quota/credential unavailable per the automated notice above)I've independently reviewed this diff at the same standard the automated reviewer would apply, since it didn't produce a verdict this round. Content-preservation check (the highest-risk failure mode for a "wrap existing prose in a div" change): diffed every removed line against every added line across all 20 files. All "orphan" removals (lines that don't reappear byte-identical) were verified to be legitimate reformatting only — mainly inline math converted to display math to match the existing div convention (e.g. Structural checks (scripted, not eyeballed):
Build verification: full Scope check: confirmed no new No findings. Requesting a human look given the automated review didn't run this round — happy to address anything raised. |
|
/review |
|
Warning Claude review skipped — API credential or quota unavailable. No |
|
/review |
Both were stated with defining precision (naming sentence + display equation) but never boxed, despite "treatment history" and "covariate history" being cited by name throughout the rest of Part III. Found via a second, more targeted sweep of the whole book for bolded terms with defining language sitting outside any theorem div. Every other candidate from that sweep was already covered (boxed nearby, inside a deliberately-informal .notes div, a pedagogical label like "Step 1"/"Setting", or a comparison-list restatement of an already-defined term) -- consistent with the judgment the original six auditing passes already applied. Part of #77.
|
Follow-up pass: swept the whole book again for bolded terms with defining language sitting outside any theorem div, per a request to look for anything the first pass missed. Found and fixed one genuine gap — ch. 19's Treatment History ( Everything else in that sweep (~100 raw candidates, mostly false positives from a broad regex) was already accounted for: boxed nearby, sitting inside a deliberately-informal |
…ve bar The repo owner reviewed the first two waves of fixes and corrected the standard applied: "any term being introduced for the first time should get boxed -- same with new results... let's err on the side of more boxing, not less." This overturns the earlier "is it thin / is it just an instance / is it in a list" filter, which had wrongly left real definitions unboxed (caught concretely: ch23's direct/indirect effect, ch01's associational risk difference/ratio/odds ratio). Re-dispatched the same file-groups to re-audit every chapter (including 02 and 14, previously judged to need no changes at all) under the new bar: box every first-time-introduced term or result, including short one-liners, estimator/instance restatements of an already-general concept, and bulleted list items -- skipping only pure structural labels, genuine re-mentions of a term already boxed elsewhere, and (per the same permissive instinct) as little `.notes`/callout content as possible. This added ~140 more theorem-family divs across 21 files, including the book's second and third `#alg-` divs (g-estimation, the g-formula Monte Carlo procedure, target trial emulation steps, etc., joining ch16's 2SLS as the first). Fixed one real collision the two batches independently produced (`#alg-g-estimation` claimed by both ch14 and ch21 -- disambiguated the time-varying one to `#alg-g-estimation-tv`, matching this chapter's existing `-tv` suffix convention). Fixed a genuine Quarto rendering constraint neither raw pandoc nor the first render attempt caught: a theorem-family div cannot be nested inside a `.callout-note` (Quarto implements crossref theorems as callouts internally, so nesting one inside another callout is a fatal "nested callout" error at render time, even though pandoc alone parses it as valid markdown). Three ch17 divs were nested this way; moved each out to sit alongside its callout instead of inside it. Content-preservation verified: every file's word count grew (never shrank) relative to main, and every "orphan" diff line (removed text with no byte-identical re-addition) was checked to be legitimate reflow/copyediting, not deletion. Part of #77.
Self-review (round 2, covering the "err on the side of more boxing" pass)Independent review of the second wave (commit 963b138), at the same standard the automated reviewer would apply, since it hasn't produced a verdict yet at this head either. What changed since the last self-review: ~140 more theorem-family divs across 21 files, driven by a corrected, more permissive standard after review feedback (see PR description). This round's specific risks and how I checked them:
Build verification: full No findings. Flagging this round's rendering gotcha (theorem divs can't nest inside callouts) as worth knowing for any future edits to this book's Fine Point/Technical Point sections. |
|
Claude Code's Bash tool caps inline output at ~30,000 characters; past that a valid result comes back as a preview plus a harness-saved file path, not the full text. This is true of the very PR this fix cites as its motivating evidence (ucdavis/win#78, 114,587 bytes / 2,464 lines). Tell the reviewer to Read that harness-saved path instead of claiming the bare command always returns everything inline -- the file itself is fine to read, since it's the harness's own write, not the agent's. Addresses review finding on PR #542.
…b denials (#1789) * memory: download the claude-review execution artifact to diagnose stub denials The job log alone shows only a summary line around a stub-review failure (the permission_denials_count, the guard warning) with no detail on what was actually denied -- that detail lives in the uploaded execution artifact (claude-execution-output.json), which has to be downloaded and parsed separately. Documents the technique with a worked case: Morrison-Lab/gha#541, where downloading the artifact for a 33-denial no-verdict run on ucdavis/win#78 revealed the reviewer looping ~15 times trying to save a large PR diff to a file, denied every time by either a compound-command approval requirement or a hard block on writing outside a narrow "allowed working directory" (including /tmp and mkdir'd repo subdirectories). Also notes that Morrison-Lab/gha#370 hit the identical denial-count signature but couldn't confirm the mechanism because that session's egress proxy blocked the artifact download -- worth retrying from an unrestricted session before assuming the artifact is unreachable. * Address review: fix wrong artifact-name pattern, dedupe, fix wording Consolidates into the existing claude-bot-workflows.md entry (which already documented this technique in more depth) instead of leaving a duplicate section in github.md -- the wrong scope per that file's own purpose (general gh CLI usage vs. the bot's runtime behavior). While there, fixed that existing entry's artifact-name pattern, which was missing the attempt-label component and wrongly appended .zip (not part of the artifact's own name). Also: stopped calling permission_denials_count 33 and 32 "identical" (they aren't -- "comparably high" is accurate), cross-referenced the show-full-output re-run as a simpler alternative to artifact download (claude-review-dispatch.md already documents it, so the earlier "only working way" claim was wrong), clarified the run-attempt vs attempt-label naming ambiguity a reviewer flagged as a real footgun, and applied semantic line breaks to the new content. * Reword to avoid a false semantic-line-break trigger on 'E.g.'
… diff (#542) * fix(claude-code-review): tell the reviewer not to redirect/pipe gh pr diff On a large PR, the reviewer would reach for `gh pr diff ... > file; wc -l file` (or a pipe) to chunk the diff, and every such compound command was denied wholesale, while writing to any file (including /tmp) is a hard sandbox block rather than a permission prompt. It burned through the denial budget on variant after variant with no verdict ever produced (33 denials measured on ucdavis/win#78, run 32415477507). Fixes #541. * fix: semantic line breaks in changelog fragment * fix: correct false "however large it is" claim in gh pr diff guidance Claude Code's Bash tool caps inline output at ~30,000 characters; past that a valid result comes back as a preview plus a harness-saved file path, not the full text. This is true of the very PR this fix cites as its motivating evidence (ucdavis/win#78, 114,587 bytes / 2,464 lines). Tell the reviewer to Read that harness-saved path instead of claiming the bare command always returns everything inline -- the file itself is fine to read, since it's the harness's own write, not the agent's. Addresses review finding on PR #542. * fix: scope the file-write hard-block claim to the observed sandbox Non-blocking review note on PR #542: "anywhere ... no retry or different path will get past" read as a universal property of every runs-on runner, when it's only confirmed for the observed sandbox. Reworded to state what was actually tried and observed blocked. * fix: scope changelog fragment's hard-block claim, matching action.yml Non-blocking observation from the latest review round on PR #542: the changelog fragment still stated the file-write block as a general fact rather than scoped to what was observed, the same overclaim 72b7dda already fixed in action.yml's own prompt text.
Closes #543. The reviewer was not attempting something forbidden. It wanted the diff in a FILE so it could chunk or count a large one, and had no route to one: a command chaining `gh pr diff` with a redirect is rejected as a whole, and the agent cannot write a file itself. So it retried variants until the denial count crossed the stub-retry threshold and the run ended with no verdict -- 33 denials on ucdavis/win#78, 6 on gha#543's reproduction, 8 and $4.95 on gha#555, the last with #542's prompt paragraph forbidding exactly that construction already live at @v2. A new step writes the diff to the workspace before the agent starts, and the prompt names that absolute path -- absolute because Claude Code's Read tool requires one. Bash(gh pr diff:*) stays allowed, so a denial afterwards means something has genuinely gone wrong, which is what the threshold is trying to measure. Failure to save is not fatal: the partial file is removed, the path is left empty, and the reviewer keeps today's route. Verified against a stub gh across four shapes, including gh writing bytes then failing. Not verifiable on its own PR -- claude-review.yml pins @v2, so both review rounds ran the pre-fix workflow. #568 tracks extracting the step into a tested composite action once the tag has moved.
1 participant
Closes #77
Summary
Every chapter had prose that stated a definition, theorem-like result, or named
algorithm with real defining precision but never wrapped it in the book's formal
crossref div (
::: {#def-...},{#thm-...},{#alg-...}, etc. --- see.github/copilot-instructions.md's "Theorems and Proofs" section), so it renderedas a plain numbered subsection instead of a boxed, numbered
Definition N/Theorem N/Algorithm Nenvironment like its sibling definitions.Confirmed example that prompted this: ch. 19's "Dynamic Strategies" ---
before
rendered as plain subsection
2.2; now renders asDefinition 4 (Dynamic Strategies).Two passes, at two different standards
This PR went through two full audit passes:
confirmation checklist that filtered out anything judged "too thin" or "just
an instance of an already-defined concept."
is now "any term being introduced for the first time should get boxed --
same with new results ... err on the side of more boxing, not less." Every
chapter (including two judged "no changes needed" in pass 1) was re-audited
under this permissive bar, adding ~140 more divs -- among them the book's
2nd and 3rd
#alg-(Algorithm) environments, and all five namedselection-bias subtypes in ch. 8 (healthy worker bias, differential loss to
follow-up, etc.) individually boxed.
Combined: ~240 definition/theorem/algorithm divs added across 21 of 23
chapters (ch. 2 and 14 needed real fixes only in the second pass; every
chapter has real fixes now).
Verification
one real collision the two independent batches produced (
#alg-g-estimationclaimed by both ch. 14 and ch. 21) was caught and disambiguated.
:::fences balanced per file, no theorem-family divnested inside another, no theorem-family div nested inside a
.callout-note(a genuine Quarto rendering constraint --- fatal at render time even though
raw pandoc parses the nesting as valid markdown; three ch. 17 instances found
and fixed).
to
main, and every diff line that looked like a deletion was checked to belegitimate reflow/copyediting, not lost content.
quarto render(all 23 chapters x 3 formats: HTML, RevealJS, PDFhandout) completes clean; spot-checked rendered HTML repeatedly to confirm
new divs actually render as boxed, numbered environments.
{#exm-...}example divs added (26 before, 26 after), and no@def-...crossref links introduced --- neither pattern exists anywhereelse in the book yet, so adding either now would be scope creep beyond this
pass's ask.
Follow-ups filed separately
duplicate crossref ids (
#def-positivityin ch. 2 & 3;#def-standardizationin ch. 2 & 13), unrelated to this pass.
Claude Code Reviewworkflow failed to produce a verdict on this PR three times in a row (two
quota-skips, one "no verdict" job failure); a self-review is posted below in
lieu of the automated one.
Types" heading actually lists five, noticed in passing while boxing that
section.