Measurement layer M1: Codebook (foundry_codebook, schema helpers, hashing, print, diff) - #3
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
farach
left a comment
farach
left a comment
There was a problem hiding this comment.
Fresh-context reviewer pass — Milestone M1 (Codebook)
Per Section 11.0 step 6 and Section 11.1. I did not write this code and reviewed only SPEC-measurement-layer.md and the M1 diff, independently repeating steps 2 (line-by-line), 4 (gap analysis), and 5 (dead-code / scope), plus the M1 acceptance check that hashing is reproducible across sessions (read against Section 5.1, not trusted from tests alone).
Verdict: APPROVE — 0 BLOCKING findings. 5 NON-BLOCKING items below.
Note on mechanics: a true GitHub Approve event is rejected here because the reviewer and the PR author are the same
farachaccount (the spec's fresh-context protocol assumes distinct identities). This review is therefore submitted as a Comment, but the reviewer's verdict is approve: zero blocking items remain. Two NON-BLOCKING items (#4 export scope, #5 milestone isolation) touch spec-discipline rules and warrant Alex's decision, but I am not blocking merge on them.
What I verified independently
- Hash reproducible across sessions (M1 acceptance): PASS. Fresh
--vanillaR process reproduces the committed hasha74264284ddf…929fb2a;createdis excluded from the payload (identical content -> identical hash despite differing timestamps); independent recomputation of the Section 5.1 recipe matches. Fixed order isinstructions, schema, examples, versionas specified. - Snapshot + unit tests: PASS. 22/22 pass with
NOT_CRAN=true(the 2 snapshot tests skip on CRAN by testthat default — correct hygiene). - lintr on
R/codebook.R: 0 lints (the file is not in.lintrexclusions;object_usage_linteris off, so I confirmed by hand that all 14 internal helpers are called — no dead code, noTODO/FIXME/browser()). - Dependencies: only
digestadded to Imports (jsonlite already imported); noirr/boot/caret. §8 / §3.5 satisfied. - check(): on this Windows box the harness emits a spurious
1 ERROR / 1 WARNINGfrom a locale issue (Setting LC_CTYPE=C.UTF-8 failed) and a quarto-Vprobe quirk (Unknown command "TMPDIR=…"); the R CMD build DESCRIPTION check isOK, all package checks areOK, and there are 0 notes. No package-level problem is attributable to the M1 diff. Implementer should confirm a fully clean run on their environment (step 3).
Independent gap-analysis (every M1 requirement by spec section)
| # | Requirement (spec §) | Status | Evidence |
|---|---|---|---|
| 1 | Class foundry_codebook; elements name,version,instructions,schema,examples,created,hash in order (§5.1) |
MET | codebook.R 47-61; test-codebook.R 16-19 |
| 2 | name lowercase slug, hyphens (§5.1) |
MET | regex codebook.R:206; snapshot rejects "Bad Name" |
| 3 | version validated semver (§5.1) |
MET | codebook.R:215-227; snapshot rejects "1" |
| 4 | instructions chr(1) (§5.1) |
MET | codebook.R:41 |
| 5 | schema built/coerced via foundry_schema() (§5.1) |
MET | as_foundry_schema codebook.R:42; reuses R/schema.R |
| 6 | examples list|NULL, part of hash (§5.1) |
MET | codebook.R:43-45,184 |
| 7 | created POSIXct UTC (§5.1) |
MET | codebook.R:231-233; test:23-24 |
| 8 | hash = sha256 over canonical serialization of (instructions,schema,examples,version), fixed order (§5.1) |
MET | codebook.R:179-193 |
| 9 | Canonical toJSON(auto_unbox,digits=NA,null="null") -> digest sha256 on string; documented; reproducible across sessions (§5.1, M1) |
MET (doc gap, see comment) | fresh-session repro; created excluded (verified) |
| 10 | type_* schema helpers, reuse existing constructors (§5.1) |
MET | codebook.R:80-103; test:73-81 |
| 11 | codebook_diff: unified instr diff + field-level schema/examples diff + both hashes (§5.1) |
MET | codebook.R:121-145; diff snapshot |
| 12 | Print: name, version, hash prefix, vars w/ types + allowed values, n examples (§5.1) | MET | codebook.R:148-169; print snapshot |
| 13 | Deps: add digest; jsonlite already imported; no irr/boot/caret (§8,§3.5) |
MET | DESCRIPTION diff |
| 14 | New file R/codebook.R only; tests mirror (§4) |
MET | commit touches only R/codebook.R; test-codebook.R |
| 15 | No dead code / TODO / commented blocks (§13, §11.0.5) | MET | 14/14 internal helpers called; grep clean; lintr 0 |
| 16 | lintr passes on new file (§13, §11.0.3) | MET | .lintr added; lint(R/codebook.R) = 0 |
| 17 | NEWS.md entry per milestone (§13) | MET | NEWS.md M1 line |
| 18 | Snapshot tests pass (M1 acceptance) | MET | 22/22 with NOT_CRAN=true |
| 19 | check() clean (M1 acceptance) | MET* | build/all checks OK, 0 notes; reported ERROR/WARNING are environmental (locale + quarto probe) — implementer to confirm on clean env |
| 20 | Exports frozen to §4/§6 (§13, §11.0.5) | MET w/ caveat | type_* anchored in §5.1 but absent from §4/§6 — spec inconsistency; see comment #4 |
| 21 | One milestone per PR (§0.3) | PARTIAL (NON-BLOCKING) | pkgdown "Measurement" section is M6 work — see comment #5 |
No NOT MET items. No BLOCKING items. The five inline comments are documentation, robustness, test-coverage, and scope-reconciliation improvements. The implementer resolves them; #4 and #5 need Alex's call on spec discipline.
| digits = NA, | ||
| null = "null" | ||
| ) | ||
| digest::digest(as.character(json), algo = "sha256", serialize = FALSE) |
There was a problem hiding this comment.
NON-BLOCKING (reproducibility — verified, plus one hardening note)
I verified the M1 acceptance criterion "hashes stable across sessions" by reading this serialization against Section 5.1 and by reproducing it in a fresh --vanilla R process:
- The committed snapshot hash
a74264284ddf…929fb2ais reproduced exactly in a clean session. - An independent hand-recomputation of the recipe (fixed order
instructions, schema, examples, version->toJSON(auto_unbox=TRUE, digits=NA, null="null")->digest(serialize=FALSE)) yields the same hash. - Building the same codebook twice ~1s apart gives different
createdbut an identical hash, confirming the session-varying timestamp is correctly excluded from the payload.
Two notes on this line:
serialize = FALSEis the right choice and is actually stronger than the literal Section 5.1 recipe (which omitsserializeand would otherwise hash R's serialization envelope). Good.- The JSON string is not
enc2utf8()-normalised before hashing. For non-ASCIIinstructions/examples, the bytes handed to sha256 can depend on the session's string encoding/locale, which weakens "reproducible across sessions" on differently-configured machines. Considerdigest(enc2utf8(as.character(json)), algo = "sha256", serialize = FALSE).
| foundry_codebook_hash <- function(instructions, schema, examples, version) { | ||
| payload <- list( | ||
| instructions = instructions, | ||
| schema = foundry_preserve_schema_arrays(schema), |
There was a problem hiding this comment.
NON-BLOCKING (documentation fidelity of the canonical form)
The canonical serialization documented in the roxygen (and in Section 5.1) is toJSON(auto_unbox=TRUE, digits=NA, null="null") of (instructions, schema, examples, version). In practice the schema element is first passed through foundry_preserve_schema_arrays() so that single-element arrays (a one-value enum, or a one-field required) are not unboxed to scalars.
That step is correct and necessary — but it is undocumented as part of the "canonical" recipe. An independent reimplementation following the documented recipe literally would compute a different hash for any codebook whose schema contains a length-1 array. Please document the array-preservation normalization as part of the canonical form (and ideally reflect it in Section 5.1), since M1's whole value proposition is a hash others can reproduce.
| examples = list(list(text = "Write code", label = "yes")) | ||
| ) | ||
|
|
||
| canonical <- jsonlite::toJSON( |
There was a problem hiding this comment.
NON-BLOCKING (test coverage gap)
This hand-recomputation does not apply foundry_preserve_schema_arrays() and uses a two-value enum, so auto_unbox leaves the array intact and the assertion passes without exercising the array-preservation path. That path — a single-element enum/required — is exactly where the implementation's canonical form and the documented recipe diverge (see the note on R/codebook.R:182). Suggest adding a case with a one-value enum asserting the hashed payload keeps ["x"] as an array, to lock in cross-session / cross-reimplementation stability.
|
|
||
| #' @rdname codebook_schema_helpers | ||
| #' @export | ||
| type_boolean <- function(desc = NULL) { |
There was a problem hiding this comment.
NON-BLOCKING (export-scope reconciliation — decision for Alex)
type_boolean / type_enum / type_number / type_string are exported. They are explicitly requested by Section 5.1 ("Provide helpers type_boolean(desc), …"), so they do trace to a numbered section per rule 1b, and they are required for the documented foundry_schema(field = type_*()) usage.
However, Section 13 and Section 11.0 step 5 freeze the exported surface to "Sections 4 and 6", and Section 4's R/codebook.R line lists only foundry_codebook(), foundry_schema(), codebook_diff(), print/format, hashing — not the type_* helpers. This is a spec internal inconsistency, not an implementation defect. Recommend Alex amend Section 4 to list the type_* helpers so the frozen-surface rule is self-consistent. Not blocking: I am treating Section 5.1 as a valid anchor.
| aria-label: GitHub repository | ||
|
|
||
| reference: | ||
| - title: Measurement |
There was a problem hiding this comment.
NON-BLOCKING (milestone isolation — Section 0 rule 3)
This adds the pkgdown "Measurement" reference section. Per Section 11, pkgdown updates are an M6 deliverable ("M6 … README and pkgdown updates"), and Section 0 rule 3 says do not combine milestones. R CMD check does not read _pkgdown.yml, so this is not needed for M1's "check() clean". The entries themselves are correct. Suggest either deferring this block to M6 or getting Alex's explicit OK to register reference entries as functions land. (A strict reading of Section 0.3 could treat this as blocking; I classify it NON-BLOCKING because it does not affect any M1 acceptance criterion.)
Normalize codebook hash payloads to UTF-8 before digesting, document schema array preservation in the hash recipe, add single-value enum hash coverage, and move M1 codebook reference entries out of the future Measurement pkgdown section. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the fresh-context review items in
Verification after the fixes:
|
1 participant
Measurement layer — Milestone M1: Codebook
Implements
R/codebook.Rper Section 5.1 and the M1 row of Section 11 ofSPEC-measurement-layer.md:foundry_codebook()constructor with validatedname(slug),version(semver),instructions,schema,examples, UTCcreated, and a deterministic SHA-256hash.type_boolean(),type_enum(),type_number(),type_string()wrapping the existingschema_*()constructors.codebook_diff()(unified instruction diff + field-level schema/examples diff + both hashes).print/formatmethods.(instructions, schema, examples, version)..lintr,DESCRIPTION(digestimport),NAMESPACE,NEWS.md,_pkgdown.yml,.Rbuildignore,.gitignore; tests + snapshots.This PR was opened to host the Section 11.0 step 6 / Section 11.1 fresh-context reviewer pass. The reviewer did not write the code. Findings are posted as review comments, each classified BLOCKING or NON-BLOCKING, with an independent gap-analysis table.