feat: Add validation infrastructure and review skills (v3)

[rafdoodle]

Hello @yulric,

I have brought all the relevant skills and validation files from PR #181 for your review, so there should not be anymore merge conflicts.

Here is Doug's text from the original PR:

Summary

• Add 4 Claude Code skills for cchsflow development: review, validation, worksheets, and derive
• Add worksheet validation infrastructure: check_worksheet(), fix_worksheet(), scope_worksheets(), YAML schemas, and CLI scripts (exec/check-worksheets.R, exec/fix-worksheets.R)
• Add CEP-015 (variable discovery and project tools)

Test plan

• [] Rscript exec/check-worksheets.R runs without errors
• [] Rscript exec/fix-worksheets.R runs without errors
• [] devtools::load_all() loads successfully
• [] Scoped validation works: Rscript exec/check-worksheets.R --variables "SDCGCGT"

Please let me know what you think or if you need anything else.

Sincerely,
Rafidul

[yulric]

When running the quarto preview ./ceps/cep-015-variable-tools/cep-015-variable-tools.qmd its giving an error in another CEP that needs to be fixed.

I only reviewed the one file for now but I'll add more reviews as I go through the others.

[yulric]

This text PUMF pack-years have \~15-20% relative error versus Master is not a coverage problem but a limitation that users should be aware of when choosing that variable. Recommend moving it to a new section.

[yulric]

This file is missing

[yulric]

I don't see how any of the resources mentioned here implement variable selection. If I understand correctly from this CEP, variable selection would mean tools that would help a researcher choose the variables that would help them answer a research questions. All the tools in this sections are used to help with the dependency problem mentioned above.

[yulric]

Suggested change

	Requirements were scored against seven criteria. The current project context: 381 variables, 3,698 variable_details rows, 225 DerivedVar entries, 17 recommended tags (smoking only), \~2,900 CRAN downloads/year, 7+ downstream repositories within big-life-lab (bllflow, cvd-trends-Canada, phiat-yll, raiflow, huipain, chmsflow, calibrationTutorial).
	Requirements were scored against seven criteria. The current project context: 381 variables, 3,698 variable details rows, 225 derived variables entries, 17 recommended tags (smoking only), \~2,900 CRAN downloads/year, 7+ downstream repositories within big-life-lab (bllflow, cvd-trends-Canada, phiat-yll, raiflow, huipain, chmsflow, calibrationTutorial).

[yulric]

I would rename this column from Req (Requirement) to Feature since it lists things that we're considering building as opposed to a requirement which is something that has to be built. This change also has downstream effects that will need to addressed.

[yulric]

There is not concept of a cycle within cchsflow, just database. How would you extract the cycle?

[yulric]

By coverage do you mean database?

[yulric]

I assume these are errors that should be reported? Would also give examples.

[yulric]

Where do these come from?

[yulric]

Would move the content within this section to the Requirements section since that's what it is. No reason to split them.

[yulric]

This just reviews the cchsflow-derived folder.

1. All the Gold templates are missing Roxygen documentation. Is there a reason for that?
2. The foundations.md document mentions that the Bronze tier should have minimal Roxygen comments but none of the pattern documents do that.

[yulric]

"1-2 years" is an example of a category and not a categorical variable. Recommend rewording.

[yulric]

Suggested change

	- Input is categorical with ordered ranges
	- Input is ordinal with ranges for each category

[yulric]

Is there a reason for the three templates as opposed to just going with the Gold? I assume that's the best one?

[yulric]

Can you expand on the comment 'always tagged_na for Step 2'?

[yulric]

I don't think this is in the branch. In any case, I would not reference other places in the repo unless we've set up something that gives an error if they've been removed.

[yulric]

Isn't this all done in the worksheets rather than through a function.

[yulric]

Isn't this all possible in the worksheet without the need for a custom function?

[yulric]

The example mentioned is not in the codebase.

[yulric]

Can you go over what you mean by domain logic? Because otherwise it's the same as L2?

[yulric]

Can you explain the reason behind this file? As opposed to limiting the examples for each pattern to the definition for each one? I can see this file getting out of sync really fast.

[yulric]

This reviews the files in the .claude/skills/cchsflow-worksheets folder.

[yulric]

The categorical Func:: bullet reads Func:: row, then N category rows (ascending recEnd), then NA::a + NA::b with no up to qualifier, implying both NA rows are mandatory. But the exception text on line 108 (NA::a / NA::b may be omitted if the function never returns the corresponding tagged_na) is written generically and applies to both continuous and categorical Func:: blocks. Line 105 (continuous) was updated to up to 3 rows, but line 106 was left unchanged — same asymmetry exists in csv-conventions.md:171-180. Suggest rewording line 106 to make the NA::a/NA::b rows optional (parallel to line 105), or making the exception paragraph explicitly cover both row types.

[yulric]

The §9 reference is ambiguous because SKILL.md links into multiple docs (csv-conventions.md, pumf-master-harmonization.md, etc.) and §9 alone doesn't say which file's section 9 is meant. Suggest converting it to a markdown link pointing at docs/csv-conventions.md §9 (Derived variable inputs in variables.csv variableStart).

[yulric]

Keeping a canonical list is useful but would require work to maintain everytime the list changes. I also think it can be built by the LLMs by following the rules outlined in this section and with the full list of supported databases. Keeping the reasoning in mind, I would instead clearly outline the three rules and provide a minimal example that showcases them.

• The list of supported databases should live at
  inst/metadata/documentation/database_metadata.yaml referenced from here. This would fix a dangling reference in inst/metadata/documentation/metadata_registry.yaml (line 168) that declares that database_metadata.yaml exists at that path, but the file itself is missing. As well, I can see the metadata being useful for other purposes like programmatically checking if the sheets are using the right databases.
• To be clear, the three rules are,
  1. PUMF databases (_p suffix) come before the Master databases (_m) and
     both should be listed in chronological order;
  2. Child databases come after parent databases; and
  3. cchs2021_p is not a valid database name

[yulric]

Same issue as the canonical cycle list. The parent → child mapping table (lines 41-48) and the per-case rule table (lines 50-60) both need to be updated every time a new parent-child pair is added.

• Move the parent → child mapping itself to
  inst/metadata/documentation/database_metadata.yaml alongside the canonical cycle list, so it becomes a single source of truth that R code and validation tooling can also consume.
• In this doc, replace both tables with the two underlying rules plus a
  minimal example:
  1. If a parent cycle is in databaseStart, also include all of its
     children (as declared in database_metadata.yaml).
  2. If a child appears without its parent, remove it.

  # Parent present → add children
  databaseStart: cchs2009_2010_m   →   add cchs2009_m, cchs2010_m
  
  # Child present without parent → remove child
  databaseStart: cchs2012_m  (no cchs2011_2012_m)   →   remove cchs2012_m
  

• Keep the conceptual sentence on line 39 (children are sub-releases sharing
  source variables with the parent) and the variableStart child-inheritance example on lines 62-72 — those are authoring behaviors, not reference data.

[yulric]

The parenthetical (exception: cycles explicitly deferred as "UNMATCHED" pending new era blocks) references a convention that isn't defined anywhere. "UNMATCHED" does not appear in any other skill doc, in the worksheets (variables.csv / variable_details.csv), or in the R package. Without a concrete definition the exception clause can't be applied or validated. Either:

• Remove the clause if no such convention exists, simplifying the rule to
  "every cycle in databaseStart must have a matching era block".
• Or define it explicitly — what does "UNMATCHED" look like in a worksheet
  (a comment? a placeholder era block? a specific recEnd value?), how should validation tooling treat it, and how does a cycle transition out of the UNMATCHED state once an era block is authored?

[yulric]

The example function signature on this line:

calculate_cigs_per_day <- function(SMKDSTY_A, SMK_204, SMK_208, ...)

uses SMKDSTY_A as an acceptable "well-known source name" — but the same name appears in the "Avoid (source-specific)" column of the parameter naming table on line 102, where smoking_status is the recommended replacement. The same name is in both the "use" and "avoid" lists.

On top of the contradiction, SMKDSTY_A isn't a real variable (see the entire-file comment above). The actual calculate_cigs_per_day signature uses SMKDSTY_original:

calculate_cigs_per_day <- function(SMKDSTY_original, ...)  # R/smoke-intensity.R:121

Fix both issues by updating the example to match reality and reconcile the table — e.g., have the line 102 "Avoid" column show a truly avoidable example (a raw CCHS source name like SMKADSTY from 2001), and keep SMKDSTY_original here as an example of an acceptable well-known harmonized source name.

[yulric]

The "Pattern" column uses category names that don't match the cchsflow-derive/ docs/patterns/ filenames, so a reader who wants to drill into a pattern can't trivially find its full documentation.

This doc's name	cchsflow-derive/docs/patterns/ filename
Pass-through	pass-through.md ✓
Domain routing	multi-source-routing.md
Multi-input calculation	formula-calculation.md
Categorical binning	category-grouping.md or cat-to-continuous.md (ambiguous)
Source combining	(no direct equivalent in cchsflow-derive)
Doc stub	(not in cchsflow-derive)

Two skills using different vocabularies for the same patterns is the kind of drift the line 41 consolidation comment is meant to prevent. If this table stays in cchsflow-worksheets rather than moving to cchsflow-derive, at least align the pattern names with the cchsflow-derive filenames and link each row to the corresponding pattern doc.

[yulric]

The row reads:

| Source combining | calculate_time_quit_smoking() | R/smoking-cessation.R |

but calculate_time_quit_smoking() actually lives in R/smoking.R:851, not R/smoking-cessation.R. Worse, that function is flagged as legacy / v2 to be deprecated in cchsflow-derive/docs/function-inventory.md:49-60, where it's listed with Bronze tier and modern replacements (calculate_time_quit_smoking_complete/daily — both in R/smoking-cessation.R).

So the table points to a deprecated function while naming the new location. Suggest updating to the modern equivalent so readers don't copy a legacy pattern as a "reference implementation":

| Source combining | calculate_time_quit_smoking_complete() | R/smoking-cessation.R |

[yulric]

derive_passthrough() is defined in R/clean-variables.R (line 493), not R/utility-functions.R.

[yulric]

The current wording ("The only exception is derive_passthrough(), where input and output are the same variable") describes the condition but not why it matters. It also reads as if pass-through functions skip Step 1 / Step 3, which contradicts the immediately preceding sentence ("A function that skips Step 3 is a bug, not a simplification.").

Clarify that the helper encapsulates Step 1 and Step 3 rather than skipping them — the work still happens, it's just inside the helper instead of in the caller's function body. Suggested rewrite:

A function that skips Step 3 is a bug, not a simplification. The
only exception is derived pass-through variables that use the
derive_passthrough() helper — in that case Step 1 and Step 3 are
still happening, just encapsulated inside the helper rather than
written explicitly in the function body. Since input and output are
the same variable, a single clean_variables() lookup is enough.