[kevinchannon]

This PR is being opened prematurely to allow assessment of the state of CI tests against this work.

Eventually thë whole feature will be present and we can merge it.

[claude]

Review

This PR is explicitly a WIP checkpoint (slices 0–3 of the schema-validation-pipeline plan, issue #43), so I focused the review on the code that's actually landed rather than on the plan's not-yet-built slices.

Summary of the change

• Unifies runner/interpreter field substitution onto the Jinja renderer (Executor._render_runner_field), deleting the old regex-based _substitute_builtin/_substitute_env wrappers in executor.py.
• Adds a generic, regex-based reference walker (src/tasktree/template_refs.py: collect_template_refs, expand_variable_refs) for discovering {{ prefix.name }} references across a raw recipe subtree.
• Adds check_runner_template_refs (parser.py), called from build_recipe_runner and _parse_inline_interpreter, which rejects arg.*/dep.*/self.* and per-task tt.* builtins (task_name, working_dir, timestamp, timestamp_unix) inside runner/interpreter definitions at parse time, since those definitions are shared across tasks and rendered once.
• task_config.build_runner_config assembles the restricted (env + tt only) rendering context for runner fields.

Code quality / best practices

• Nice adherence to the project's stated philosophy: small commits, tests alongside each increment, and a documented "reference gate" comparing behaviour against v1.3.2 with an explicit expected-divergences list. The plan doc is kept in sync with what actually landed.
• check_runner_template_refs and build_runner_config are small, well-named, single-purpose functions with docstrings that state pre/post-conditions — matches the repo's "small named functions over comments" guidance.
• Minor redundancy: build_recipe_runner calls check_runner_template_refs(config, ...) on the entire raw runner config (which already includes a nested interpreter: subtree), and then parse_interpreter_spec → _parse_inline_interpreter calls it again on just the interpreter subtree. Harmless (recipes are small, parsed once), but worth a one-line comment noting it's intentional double coverage, or a follow-up to avoid re-walking the same nodes.
• The plan doc (docs/plans/schema-validation-pipeline.md) keeps each "✅ done" slice's completion summary directly above the original (now historical) planning prose for that slice ("Original scope follows."). Intentional per the doc's own convention, but it reads as duplicated content on a first pass — worth a note for future readers unfamiliar with the convention.
• The plan records that "Kevin has granted per-increment local commits for this work (supersedes CLAUDE.md's never-commit rule)" — good that the exception is explicit and documented in the plan rather than silently diverging from CLAUDE.md.

Potential bugs / issues

• template_refs._REFERENCE's name pattern ([A-Za-z_][A-Za-z0-9_-]*(?:\.[A-Za-z_][A-Za-z0-9_-]*)*) requires each dotted segment to start with a letter/underscore, so a positional reference like {{ self.inputs.0 }} only captures self.inputs, dropping the numeric index. This doesn't affect the current use (check_runner_template_refs only needs to know a forbidden prefix was used, not the exact dotted path), but it's a latent under-matching gap if collect_template_refs is later reused for hashing/pruning (per the plan's slice 5 notes), where the full dotted path matters more. Worth a test/fix before that reuse happens.
• Defense-in-depth is solid: even if the regex walker under-matches (e.g. bracket-style Jinja access like {{ arg['x'] }}), the restricted build_runner_config context (no arg/dep/self keys) plus StrictUndefined means such a reference would still fail at render time — just with a less specific error message than the parse-time check produces. Not a correctness bug, just a slightly worse error message in that edge case.

Performance

• No concerns. The added regex walking runs once at parse time over small YAML trees; no hot-path impact.

Security

• No concerns. This is pure validation/rendering logic operating on trusted recipe YAML — no new subprocess/eval/network surface, and the regexes have no catastrophic-backtracking shape (no nested quantifiers).

Test coverage

• Strong coverage for what's landed: tests/unit/test_template_refs.py covers the walker (all prefixes, nested dicts/lists/keys, Jinja conditionals, cycles/fixpoint closure); tests/unit/test_runner_template_restriction.py covers end-to-end parse-time rejection/acceptance across runners, inline task runners, and interpreters, and is deliberately kept import-minimal so it doubles as the v1.3.2 reference-gate fixture; tests/unit/test_executor.py adds parity + rejection tests per runner field (volumes, env_vars, ports, working_dir, run args, preamble).
• One small regression in assertion strength: tests/integration/test_builtin_variables.py's TASK_NAME_VAR → USER_NAME_VAR swap changed the assertion from checking an exact expected value ("docker-test") to just assertNotIn("{{", ...). That's a reasonable trade-off since tt.user_name is environment-dependent, but it's worth knowing the test now only proves "something got substituted," not "the right thing got substituted."

Action items

• Consider avoiding the double check_runner_template_refs walk (once over the whole runner config in build_recipe_runner, again over just the interpreter subtree in _parse_inline_interpreter) — e.g. by having build_recipe_runner skip the interpreter key before checking, or documenting why the double-check is intentional.
• Add a test (or fix) for positional self.inputs.N/self.outputs.N references in collect_template_refs, since the current regex drops the trailing numeric segment — flag this now so it doesn't silently bite slice 5's planned reuse of the walker for reachability/pruning.
• Optional: strengthen the updated USER_NAME_VAR integration test (e.g. mock/patch user_name to a known value) so it verifies the correct value was substituted, not just that substitution happened.

[kevinchannon]

Review action items addressed in d2b44e4:

1. Double check_runner_template_refs walk — kept, now documented as intentional at the call site in build_recipe_runner. The outer walk covers the whole raw config; the inner check in _parse_inline_interpreter is what protects interpreters defined outside any runner (the interpreters: section and task-level overrides). Skipping the interpreter key in the outer walk would couple the function to that call graph for no measurable saving.

2. Numeric segments dropped from positional refs — fixed. _REFERENCE continuation segments may now start with digits, so {{ self.inputs.0 }} extracts inputs.0 (and {{ dep.build.outputs.0 }} keeps its index). Tests added for both. As the review noted, this didn't affect the current prefix-only consumer, but it closes the under-matching gap before slice 5/7 reuse the walker for pruning/hashing.

3. USER_NAME_VAR assertion strength — strengthened. The test now pins os.getlogin to a known value and asserts the exact substituted string, restoring the pre-change assertion strength without depending on the environment. Reference gate: the updated test passes against v1.3.2 (parity).

Full pyramid run after the changes: unit+integration 1647 passed / 4 conditional skips; e2e 71 passed / 5 conditional skips.

[claude]

Review of PR #212 — "Use JSON schema to validate incoming tasks" (slices 0-3, WIP)

Thanks for the clear per-slice commit history and the self-documenting plan doc - it made this easy to follow even without the originating conversation. This is explicitly an in-progress WIP PR per the description, so I focused on the substance of slices 1-3 that has landed rather than the overall roadmap.

Summary of what's in this PR

• Slice 1: runner/interpreter field substitution (volumes, ports, env_vars, working_dir, dockerfile, context, docker build/run args, interpreter cmd/preamble) is unified onto the Jinja rendering.render() path via a new Executor._render_runner_field, replacing the old regex-based _substitute_builtin/_substitute_env helpers (deleted).
• Slice 2: a new, generic collect_template_refs/expand_variable_refs walker in template_refs.py that finds {{ prefix.name }} references anywhere in a raw recipe subtree.
• Slice 3: check_runner_template_refs (parser.py) uses the slice-2 walker to reject arg.*/dep.*/self.* and per-task tt.* builtins (task_name, working_dir, timestamp, timestamp_unix) inside runner/interpreter definitions, since runners are shared across tasks and rendered once.

Correctness issue: the "runners can't see per-task values" guarantee doesn't actually hold everywhere

The core safety property this slice is building towards is that runner/interpreter fields must not depend on per-task state, because a runner is shared across tasks. Two gaps undermine that today:

1. Machine-config runners silently pick up per-task tt.* values instead of failing. check_runner_template_refs is only wired into the recipe path (build_recipe_runner in parser.py:2096, and _parse_inline_interpreter for interpreter definitions). config.py's default-runner loader (~line 298) builds machine-config runners straight through runner_from_config, which never calls check_runner_template_refs. The plan doc (docs/plans/schema-validation-pipeline.md, slice 3 notes) states this is fine because "their forbidden refs still fail at render via slice 1's strict Jinja" - but that's only true for arg/dep/self (absent from the render context entirely, so Jinja's StrictUndefined raises). It is not true for tt.*: task_config.build_runner_config (task_config.py:173-176) does "tt": dict(builtins or {}), forwarding the entire builtins dict - including task_name, working_dir, timestamp, timestamp_unix - unfiltered into the Jinja context (confirmed via Executor._collect_builtin_variables, executor.py:239-297, which always includes those four per-task keys, and _run_task_in_docker, executor.py:1399, which passes that dict straight into _substitute_builtin_in_runner/_render_runner_field). So a machine-config runner with e.g. env_vars: {TASK: "{{ tt.task_name }}"} will render successfully - silently varying its value depending on which task happens to be running - instead of erroring, which is exactly the class of bug this feature is meant to close off. There's no test exercising this path (grep -rn "runner_from_config" tests/ turns up nothing checking tt.task_name/per-task rejection there).
2. Recipe runners can bypass the check via variable indirection. check_runner_template_refs calls collect_template_refs(subtree) directly on the raw config - it never calls expand_variable_refs (the transitive-closure helper slice 2 built specifically for this kind of chasing). So runners: { docker: { env_vars: { X: "{{ var.mount }}" } } } with variables: { mount: "{{ arg.thing }}" } passes the parse-time check (it only sees var.mount), and only fails later at render time with a generic "Undefined variable: 'arg' is undefined" Jinja error instead of the intended, more actionable message pointing at the runner/interpreter definition. Minor by comparison to (1) since it does still fail, just with a worse error and later than intended.

Suggest: for (1), either call check_runner_template_refs from config.py's runner-building path too, or (better, since it closes the hole for both paths at once) filter build_runner_config's tt dict down to the allowed four global names always, so silently-wrong values become fail-fast even for uncovered call sites. For (2), run the check against expand_variable_refs(collect_template_refs(subtree), raw_variables) rather than the raw refs, so indirection through a variable doesn't hide a forbidden namespace.

Smaller notes

• docs/plans/schema-validation-pipeline.md's slice-3 completion note asserting per-task tt.* refs "fail at render" for machine-config runners should be corrected/removed once (1) above is addressed - right now it documents behaviour that isn't actually true, which could mislead whoever picks up slice 4+.
• Good instinct writing tests/unit/test_runner_template_restriction.py self-contained (only importing parse_recipe) for the v1.3.2 reference-gate comparisons - matches the plan's own stated gate practicalities.
• Test coverage for the new Jinja-based runner-field rendering (slice 1) and the walker (slice 2) both look solid and well-targeted at the field list actually in play (including the wider list found in slice 0's extra finding: dockerfile/context/build+run args/interpreter cmd+preamble).

Performance / security

No concerns - collect_template_refs's regex is non-greedy and bounded per {{ ... }} block, no ReDoS risk from the patterns used. No injection concerns; this is parse-time validation logic only.

---

Action items

• Close the machine-config runner gap: either call check_runner_template_refs from config.py's runner-loading path, or make build_runner_config's tt namespace always restrict to the four global builtins (project_root, recipe_dir, user_home, user_name) regardless of caller, so per-task values can never silently leak into a shared runner's rendered fields.
• Add a regression test that a machine-config runner referencing {{ tt.task_name }} (or another per-task builtin) either fails to parse/load or fails to render - currently nothing catches this.
• Have check_runner_template_refs use expand_variable_refs (not just collect_template_refs) so a forbidden namespace referenced indirectly through a {{ var.name }} in a runner/interpreter field is caught at parse time with the intended error message, not a generic Jinja "undefined" error at render time.
• Update the "forbidden refs still fail at render via slice 1's strict Jinja" note in docs/plans/schema-validation-pipeline.md once the above is fixed, since it currently overstates what's actually guaranteed for tt.* in machine-config runners.

[claude]

Review of PR #212: Use JSON schema to validate incoming tasks

I reviewed the new modules (recipe_schema.py, raw_merge.py, and parts of pyproject.toml and the schema) and skimmed the rest. The diff is large (~9.7k additions, ~1.5k deletions), so I did not read every test file. At the time of review, CI was still running except lint, which passed. The PR description says it was opened early for CI, so I haven't treated unfinished pieces as defects.

What's good

• Separating the raw merge (raw_merge.py), validation (recipe_schema.py) and error rendering gives small, single-purpose modules with well-named helpers. This fits the CLAUDE.md style.
• merged_tree_schema derives the merged-tree schema from the file schema, so there is one source of truth. It raises on name patterns it doesn't know how to namespace, instead of silently rejecting valid imported names.
• Pruning unreachable tasks before validation is a sensible design. collect_reachable_task_names is deliberately tolerant of malformed data, and there is a unit test for that (test_malformed_task_and_deps_are_tolerated).
• jsonschema moves from dev to runtime dependencies, which is required now that validation happens at runtime. The force-include of the schema in the wheel, plus test_packaging.py, covers the install path.
• The README note on var.*/env.* hash tracking is clear about its limits.

Concerns and suggestions

1. Reachability and dep rewriting disagree on dict deps.

   • _rewrite_deps (raw_merge.py:501) handles any dict dep, including multi-key ones.
   • collect_reachable_task_names (raw_merge.py:277) only follows dicts with len(dep) == 1.
   • A multi-key dict dep is probably invalid anyway. If so, say so in a comment. If it's reachable in practice, its dependency would be pruned silently and surface as a confusing "unknown task" error. Consider sharing one dep-name extractor between the two functions.

2. Deleted root __init__.py. Check that nothing relies on the repo root being a package (pytest rootdir/conftest behaviour, packaging config). If it's a cleanup, mention it in the PR description.

3. Schema relaxation.

   • Dropping {"type": "integer"} from oneOf in the variables schema and keeping only number is correct. In oneOf, integer and number both match an integer, so the old schema would have rejected it.
   • Please confirm a unit test covers an integer variable value, e.g. x: 1.
   • The new description keys next to $ref in the runner/interpreter oneOf are fine in draft 2020-12 but ignored in older drafts, so check which $schema is declared.

4. Schema lookup in schema_candidates().

   • Path(__file__).parents[2] / "schema" assumes a src/tasktree layout. In an editable install this works, and in a wheel the first candidate wins.
   • A stale tasktree/schema/ directory in a source tree would shadow the authored copy and drift silently. Consider a test that asserts the two copies are identical when both exist.

5. lru_cache on load_file_schema returns a shared mutable dict. merged_tree_schema deep-copies, which is good. A caller that mutates the cached dict would affect the whole process, so it's worth a docstring note or returning a deep copy.

6. Validation performance. Is the schema re-validated for every get_recipe call (including in the LSP)? Consider caching the compiled validator and the derived merged-tree schema, since copy.deepcopy of the schema runs on every parse.

7. Error messages. _remediation_hint and _plain_reason are string-heuristic based on jsonschema error internals. These can change between jsonschema versions. The >=4.0.0 lower bound with no upper bound is loose, so confirm the test_schema_errors.py expectations hold on the minimum and the latest version.

8. Docs.

   • docs/plans/schema-validation-pipeline.md is large (~1,350 lines of diff). Consider whether it belongs in the repo or should be trimmed before merge.
   • CLAUDE.md's component list (line counts, parser.py description, the new modules raw_merge, recipe_schema, field_order, template_refs, task_config, rendering) should be updated to match.

9. Commit size. CLAUDE.md asks for small incremental commits, and this PR is very large. If it's not already split into logical commits (merge, schema, rendering, hashing, pruning), please make sure the history is reviewable.

10. Tests.

    • Coverage looks thorough on paper: unit tests for each new module, plus integration tests for schema validation, pruning, and hash sensitivity.
    • Please confirm that no tests are skipped unconditionally. I did not complete that check.
    • Add an e2e check that a defect in an unreachable task is tolerated via the real tt subprocess, and that a defect in a reachable task fails with the new message format.
    • Wait for the macOS and Windows jobs, since path handling in schema_candidates and _load_yaml is platform-sensitive.

Security

Nothing concerning. The schema is read from package or repo paths only, YAML is loaded via the existing loader (confirm it still uses safe_load), and no new subprocess or eval paths appear in the modules I read.

Action items

• Align dep-name extraction between _rewrite_deps and collect_reachable_task_names (multi-key dict deps), or document why they differ
• Explain or revert the root __init__.py deletion; verify pytest/packaging are unaffected
• Confirm a test covers integer variable values after the oneOf change, and check the declared $schema draft for $ref siblings
• Add a test that the packaged and authored schema copies are identical
• Return a copy from (or document the shared dict in) load_file_schema
• Cache the validator and merged-tree schema rather than rebuilding on every parse
• Verify test_schema_errors.py against the minimum and latest jsonschema versions; consider an upper bound
• Update CLAUDE.md architecture section for the new modules
• Trim or relocate docs/plans/schema-validation-pipeline.md before merge
• Split into small logical commits if not already done
• Add an e2e subprocess test for unreachable-defect tolerance and reachable-defect failure
• Confirm no unconditional test skips, and that CI is green on macOS and Windows

[claude]

Review of PR #212: runtime schema validation pipeline

Scope of this review. The PR is very large (about 25 source and test files, +~7k lines, including a 1,244-line plan doc). I read the PR description and file list, the plan document, recipe_schema.py, the pyproject.toml changes and the state.py pruning logic. I did not read the parser.py, executor.py, graph.py or raw_merge.py diffs line by line, and I did not run the test suites. Treat the points below as targeted observations, not a full audit.

What looks good

• Architecture. The raw-dict merge followed by schema validation is a cleaner shape than interleaving object construction with validation. merged_tree_schema is derived from the file schema, so the two can't drift. It raises on an unknown name pattern instead of silently rejecting imported names.
• Error messages. schema_error_message turns jsonschema's page-long dumps into short, located messages.
• Packaging. The wheel force-include, the schema_candidates() fallback and an e2e test that builds the wheel and looks inside it give a proper safety net. Deleting the stray root __init__.py, which shadowed src/tasktree, is a good catch.
• Tests. Coverage is broad: unit, integration and e2e. Reference-gate verdicts against v1.3.2 are recorded, and TestFixtureCorpus validates every recipe under tests/fixtures.
• State pruning. The state entries now carry task_name, and un-invoked tasks are no longer pruned. This fixes a real pre-existing thrash bug, with a sensible legacy fallback.

Concerns

1. PR size versus the project's "small, incremental commits" rule. This is one PR covering slices 0–10e plus a plan rewrite. It is hard to review meaningfully. The PR body says it was opened prematurely, which is fine for CI. Before merge, consider splitting along the plan's own slice boundaries (merge module, schema wiring, hash change, rendering move). At minimum, make sure the commit history is slice-by-slice so reviewers can go commit by commit.
2. Breaking behaviour changes. The plan lists several intentional ones:
   • Unknown or misspelled fields are now hard errors.
   • Template strings in private, pin_runner and task_output are rejected.
   • Every task with a var.* or env.* reference re-runs once after upgrade.
   • Environment changes now trigger re-runs.
   • These are reasonable, but they are user-visible. They need a changelog or release-note entry and probably a version-bump decision. I couldn't confirm either exists.
3. jsonschema runtime dependency. It is promoted to a runtime dependency with a >=4.0.0 floor. Check that the floor is realistic for the APIs used (best_match, the draft in $schema). If a draft 2020-12 feature is used, the floor should be higher (about 4.18).
4. Schema lookup failure mode. schema_path() raises FileNotFoundError on a broken install. load_file_schema is lru_cached, so that is cheap. Confirm the CLI turns this into a clean message rather than a traceback. Also confirm the LSP, which reuses the parser, degrades gracefully if the schema is missing (a CLAUDE.md requirement).
5. Schema is read on every parse and validated on the merged tree. For large recipes, jsonschema validation has a cost. A quick timing check on the biggest fixture would be reassuring. --list validates the entire file, so it pays the full cost.
6. Validation order. The plan puts schema validation after the hand-written checks and before evaluate_variables(), so structural validation happens before any eval: runs a shell command. This is the important property. Please make sure a test pins it, for example an eval: variable in a structurally invalid recipe must never execute.
7. Documentation of the plan doc. docs/plans/schema-validation-pipeline.md is now a 1,200+ line status log containing local details (~/repos/tasktree-ref, personal names, dates). It is useful during development, but it shouldn't ship as-is. Trim it, or move it out of the repo, before merging. Related: the plan says "pushing needs explicit go-ahead" and "supersedes CLAUDE.md's never-commit rule". CLAUDE.md should be updated if that is now policy.
8. Test hygiene.
   • TestParityWithObjectPath in test_raw_merge.py is noted in the plan as tautological now. Drop or repurpose it.
   • The helper interpreter_yaml is deliberately duplicated across test files. That is acceptable for the gate files, but do remove the duplication once the reference-gate workflow ends.
   • Confirm no test relies on a bare bash hard-coding, since the plan itself says this has broken Windows CI three times.
9. CLAUDE.md drift. The architecture section lists line counts and components that no longer match (parser.py shrank by about 300 lines; raw_merge.py, recipe_schema.py, template_refs.py and field_order.py are new). Update it, and describe the new pipeline order (merge, prune, validate, evaluate variables, build, render at point of use).
10. Open design items the plan itself flags. Slices 11 and 12 are open: runner fields are still the last textual substitution, and two defects came out of an earlier review. If these are merge blockers, say so in the PR. If not, link tracking issues.

Actions

• Split or restructure the PR/commit history by slice, or confirm the history is already slice-by-slice, so it can be reviewed incrementally
• Add a changelog / release-note entry covering the intentional behaviour changes: stricter schema rejection, one-time re-run after upgrade, and env-change re-runs
• Verify the jsonschema minimum version against the APIs and draft actually used, and raise the floor if needed
• Confirm the CLI and LSP handle a missing schema file with a clean error or graceful degradation, and add a test
• Add or confirm a test that a structurally invalid recipe never executes an eval: variable
• Time schema validation on the largest fixture and confirm the overhead is acceptable for --list and normal runs
• Trim docs/plans/schema-validation-pipeline.md (remove local paths and personal workflow notes) or move it out of the repo before merge
• Update CLAUDE.md: new modules, new pipeline order, line counts, and the commit policy if it has changed
• Remove or repurpose the tautological TestParityWithObjectPath
• Check that no new test hard-codes bash or POSIX paths, so Windows CI stays green
• Link tracking issues for slices 11 and 12, or state whether they block this merge

🤖 Generated with Claude Code

[claude]

Review of PR #212: schema validation pipeline

I read the non-test source in the diff (recipe_schema.py, template_refs.py, rendering.py, state.py, substitution.py, execute_dynamic_task.py, pyproject.toml) and skimmed the test layout. I did not read all of parser.py, raw_merge.py, executor.py or graph.py line by line, and I did not run the test suite. Treat those areas as less reviewed.

Overall

The design is coherent. The merged-tree schema is derived from the file schema, so the two can't drift. Rendering is sandboxed and happens at the point of use. Pruning is name-aware. collect_template_refs is a single shared implementation for pruning, hashing and the runner restriction. Test coverage looks broad (unit, integration, e2e), including test_packaging.py for the wheel-bundled schema.

Process and conventions

• PR size. The diff is about 13k lines, roughly 6k of it tests, plus about 1.2k lines of plan doc and about 1k lines rewritten in parser.py. This conflicts with the "small, incremental commits" rule in CLAUDE.md. The description says the PR was opened early to see CI, so this may be known. Please make sure the commit history is split by slice (the plan doc already names them).
• Stale CLAUDE.md. parser.py shrinks by about 300 lines net and there are new modules (raw_merge.py, recipe_schema.py, template_refs.py, field_order.py). The "Core Components" list and line counts don't mention them; the CLAUDE.md edit only touches the dependencies list.
• Root __init__.py deleted. Probably right, but unexplained in the description or plan. It could affect pytest rootdir and import resolution, so add a line on why.

Potential bugs and risks

1. rewrite_var_refs / _REFERENCE over-match (template_refs.py). The regex \b(var|arg|env|tt|dep|self)\s*\. runs inside whole {{ ... }} blocks. Over-matching is fine for discovery, as the docstring says, but not for rewriting. A string literal or attribute path such as {{ "var.x" }} or {{ foo.var.bar }} would be rewritten (\b matches after a .). Add a negative lookbehind for . and quotes, or tokenise, and test it.
2. {% ... %} blocks aren't scanned. _TEMPLATE_BLOCK only matches {{ }}. A reference in {% if var.x %} or {% set y = env.FOO %} is missed, so pruning won't evaluate that variable and hashing won't fold it in. That means a stale cache hit or an undefined-variable error. Scan statement blocks too (or use jinja2.meta.find_undeclared_variables on the AST), or confirm they're rejected elsewhere, and add a test.
3. State pruning. Entries for defined-but-not-invoked tasks are kept even if their hash is stale. That is a reasonable tradeoff. Legacy entries (task_name == "") fall back to the hash-only rule and never get a name until re-run. Worth a test for a renamed task that is never re-run.
4. Sandbox scope. SandboxedEnvironment blocks access to Python internals, which is good, but it doesn't bound resources: {{ 'a' * 10**10 }} from an imported recipe can still exhaust memory. Document that the sandbox isn't a DoS boundary.
5. schema_candidates fallback. parents[2] / "schema" assumes the src/tasktree/ layout and would look outside site-packages if the wheel copy were missing. Harmless, since the error lists both paths.
6. Private API reach-through. execute_dynamic_task.py calls executor._render_pattern. Make it public or inject a callable.
7. State ordering. prune() then save() before execute_task is pre-existing, but pruning now depends on reachable_task_names. Check a failed run can't drop entries for skipped tasks.
8. jsonschema is now a runtime dependency. The schema load is cached, but validation cost on large multi-import recipes is unmeasured.

Code quality

• The nested resolve_specs def in execute_dynamic_task.py just wraps executor.resolve_arg_specs; functools.partial would do.
• _walk and _extract_from_string lack docstrings, unlike the rest of the module.
• The plan doc reads as a changelog; trim it to current state after merge.

Tests

• Good coverage of cutover, hash sensitivity, unreachable-task tolerance and the sandbox.
• Missing as far as I can see: tests for items 1 and 2, and a CLI-path test that a Jinja SecurityError reports the task name.
• Confirm nothing is unconditionally skipped (CLAUDE.md rule).

Action list

• Split or confirm the commit history is by slice, with small commits.
• Update CLAUDE.md "Core Components" for the new modules and line counts.
• Explain the root __init__.py deletion in the PR description.
• Tighten _REFERENCE for rewrite_var_refs so string literals and attribute paths (foo.var.x, "var.x") aren't rewritten; add unit tests.
• Decide how {% ... %} blocks are handled by collect_template_refs (scan or reject) and test it.
• Make Executor._render_pattern public (or inject a callable) instead of calling it from execute_dynamic_task.py.
• Replace the nested resolve_specs def with functools.partial.
• Document the sandbox's lack of resource bounds.
• Add a pruning test for a renamed, never-re-run task.
• Add docstrings to _walk and _extract_from_string.
• Optionally benchmark schema validation on a multi-import recipe.

[claude]

Review of PR #212: JSON-schema validation pipeline

Scope caveat: this is a partial review. I could read the PR metadata, the file list, all of recipe_schema.py, and the CLAUDE.md and plan-doc diffs. Several gh/diff commands were blocked in my environment, so I did not read the diffs of raw_merge.py, parser.py, executor.py, graph.py, state.py, hasher.py or pyproject.toml. I also could not see CI results. Treat the comments below as targeted observations, not a full audit. The PR is about 8k lines across 50 files, and a human or a fuller pass should cover the rest.

Code quality and conventions

• recipe_schema.py is well structured. It has small named functions, comments that explain why, and a loud ValueError when the schema gains a patternProperties key the transform doesn't know. That guard stops imported names being silently rejected.
• Size versus the "small, incremental changes" rule. The plan doc says the work was built in slices with per-increment commits, which is good. The PR itself is very large (parser.py +712/−1024, raw_merge.py +600, executor.py +365/−159). The description says it was opened early to see CI, so this is fine as a draft. Keep it as a draft until the feature is complete, and make sure the commit history stays one logical change per commit.
• Local CLAUDE.md is out of sync with the PR. In my checkout, CLAUDE.md is modified relative to the PR's version. It has the old component list, so it is missing raw_merge.py, recipe_schema.py, rendering.py, field_order.py, template_refs.py and others. Make sure the PR's updated version is what lands, with no stale copy committed over it.
• Stray root __init__.py deleted. The file was empty, so this looks right. Confirm nothing (packaging, test discovery) relied on it. tests/e2e/test_packaging.py is a good addition for that.
• Plan doc. docs/plans/schema-validation-pipeline.md grows by about 1,200 lines. It is useful, but it reads as a work log (dated progress entries, gate verdicts). Consider trimming it to the decisions and the remaining slices before merge, or moving the log to the PR/issue JSON schema should be used to validate recipes at runtime #43.
• Style nit (recipe_schema.py): the "Args:/Raises:" docstring sections aren't indented under their headings. Match whatever the rest of the codebase does.

Potential bugs and risks

1. lru_cache on load_file_schema() returns a shared mutable dict. The docstring warns about this, and merged_tree_schema deep-copies first, so it is safe today. A defensive option is to return a deep copy, or to wrap the schema in types.MappingProxyType or copy.deepcopy at the call sites. At minimum, add a test that fails if the cached schema is mutated by merged_tree_schema.

2. schema_candidates() ordering. The installed (in-package) copy is preferred over the repo-root copy. In a source checkout where a stale built copy exists under src/tasktree/schema/, the stale one would win over the authored file. Check that the build force-include can't leave a stale copy in the working tree.

3. _remediation_hint assumes path[-1] == "args". That also matches a runner's args: mapping (the Docker build args are a mapping, per CLAUDE.md), where "Arguments are a list" would be wrong advice. Restrict it to task-level args (for example path[-3:-1] == ("tasks", ...) or check the parent is a task).

4. _accepted_forms_reason uses error.instance!r. For a large value (a whole task dict) this reintroduces the "pages of output" problem the function is meant to avoid. Truncate the repr.

5. _wrong_kind_reason message always says to set type/engine. That is right for the runner-kind not.required cases, but the not keyword may be used elsewhere in the schema later. Fine today, but add a test that pins every not in the schema to the runner context.

6. Behaviour changes users will notice (documented in the plan as expected divergences):

   • Unknown or misspelled keys are now hard errors.
   • Env-var changes now trigger re-runs.
   • State entries now carry task names, so the first run after upgrading re-runs tasks.

   Please make sure the user-facing changelog or README calls these out. The stricter schema is a breaking change for anyone with typos that were silently ignored.

7. Machine-level config (config.py → runner_from_config) is explicitly not schema-validated, and the slice 3 template restriction doesn't cover it. It is a known gap, but it means two validation standards for runners. Track it as a follow-up issue.

Performance

• Validation runs on every parse, and merged_tree_schema deep-copies and rewrites the schema each time. Memoise the derived merged-tree schema, since it is a pure function of the cached file schema. Also consider reusing a compiled Draft*Validator instead of calling jsonschema.validate each time. Measure it on a large recipe.
• jsonschema best_match on oneOf-heavy schemas can be slow on big recipes. A quick benchmark in a test or note would help.

Security

• Jinja rendering uses SandboxedEnvironment with StrictUndefined, which is the right choice. Confirm that no new code path renders recipe-supplied strings with an unsandboxed environment.
• read:/eval: variables are unchanged in nature (they execute recipe-defined commands). The schema now validates their shape, which is an improvement.

Test coverage

• Coverage is extensive: about 4k lines of new tests across unit, integration and e2e, including reference-gate parity tests, packaging and schema-error tests.
• Check that nothing is skipped unconditionally (per CLAUDE.md). I couldn't verify this because the test diffs were blocked. A quick grep for @unittest.skip( and bare return in tests would confirm.
• Add direct unit tests, if they aren't already there, for: the args-as-mapping hint on a runner args (item 3), repr truncation (item 4), and the cached-schema immutability guarantee (item 1).
• CI status wasn't visible to me. Since the PR was opened specifically to see CI, make sure Windows and macOS runs are green. The plan doc notes that the local test pyramid can't catch Windows path-separator bugs or hardcoded bash in tests.

Suggested actions

• Confirm CI is green on all platforms (Linux, macOS, Windows) and fix any failures
• Restrict _remediation_hint so it fires only for task-level args, not runner build-args mappings; add a test
• Truncate error.instance!r in _accepted_forms_reason for large values; add a test
• Memoise merged_tree_schema (and reuse a validator instance) rather than rebuilding on every parse; add a quick benchmark or note
• Add a test that the cached load_file_schema() result is not mutated by merged_tree_schema (or return a copy)
• Verify the build force-include can't leave a stale in-package schema that shadows schema/tasktree-schema.json in a source checkout
• Grep the new and changed tests for unconditional skips or bare early returns and remove any
• Make sure the final CLAUDE.md is the PR's updated version (component list including raw_merge.py, recipe_schema.py, etc.)
• Add a user-facing changelog or README note for the breaking and behavioural changes: unknown keys rejected, env changes trigger re-runs, one-time re-run after upgrade
• Trim docs/plans/schema-validation-pipeline.md to decisions and remaining slices before merge
• Open a follow-up issue for schema or validation parity on machine-level config runners
• Do a fuller review pass of raw_merge.py, parser.py, executor.py, state.py, hasher.py and pyproject.toml. This review did not cover them.

🤖 Generated with Claude Code

[claude]

Review of PR #212 (schema validation pipeline)

I read the new modules (recipe_schema.py, field_order.py, template_refs.py), rendering.py, state.py, hasher.py, the packaging changes and the packaging test. I did not read the parser.py and executor.py rewrites line by line (+712/-1024 and +365/-159), and I did not run the test suite. The PR description says this is opened early for CI, so I've focused on design and risk rather than polish.

What looks good

• Sandboxed Jinja (rendering.py): switching to SandboxedEnvironment closes a real hole, because {{ ''.__class__.__mro__ }} runs on the host in an imported recipe. The SecurityError is mapped to a clear ValueError.
• Schema derivation (recipe_schema.py): merged_tree_schema is derived from the file schema rather than copied, and it fails loudly on an unrecognised patternProperties key. That guards against drift.
• Packaging: force-include keeps the raw-GitHub schema URL working. tests/e2e/test_packaging.py builds a wheel and checks its contents, which is the right level of test for this, and it uses a conditional skipIf as CLAUDE.md allows. jsonschema is now a runtime dependency.
• Hash stability: referenced_values is only added to the hash when non-empty, so existing tasks keep their hashes.
• field_order.py: it is small, single-purpose and renders through a callback. Cycle detection uses graphlib.
• Tests: coverage is thorough and spread across unit, integration and e2e.

Issues and risks

1. PR size breaks the CLAUDE.md "small, incremental commits" rule. The PR is about 14k diff lines, including a 1,278-line plan doc, and it rewrites parser.py and executor.py. Reviewing it as one unit is hard. If the work is already split into commits, the PR description should list the stages. It should also say which bits are done and which are pending, since the description currently says only "premature".

2. template_refs._REFERENCE over-matches, and rewrite_var_refs mutates text. The regex \b(var|arg|env|tt|dep|self)\s*\.\s*name runs on the whole {{ ... }} block, including string literals and attribute chains.

   • Example: {{ arg.x ~ 'see var.readme' }}, or {{ foo.var.bar }}, will register a var reference.
   • For collection this is harmless, as the docstring says (over-matching is safe).
   • For rewrite_var_refs it is not safe. It would rewrite the literal text to var.<ns>.readme in an imported recipe's command, silently changing output.
   • Suggestion: use a negative lookbehind for . and quotes, or use jinja2.meta.find_undeclared_variables or the Jinja AST for the rewrite path. Add a test with a literal containing var..

3. field_order.render_own_patterns: raise ValueError(...) inside except CycleError has no from e. Add from e (or from None) so the chaining is intentional. e.args[1] is also an undocumented index into CycleError; consider a short comment.

4. state.prune: entries for defined-but-unreachable tasks are never pruned by hash. This is a deliberate design choice and it is documented. But if a task is edited and then never invoked again, its old entries persist until the task is deleted. Consider noting this in the docs. Also confirm TaskState.task_name round-trips through a state file written by an older version (there is a from_dict default; I couldn't see a test for a legacy entry lacking task_name at integration level).

5. recipe_schema.py details:

   • schema_path().read_text() has no encoding="utf-8". This matters on Windows, where CI may use cp1252, if the schema ever contains non-ASCII text.
   • load_file_schema is lru_cached and returns a shared mutable dict. The docstring warns about this, and merged_tree_schema deep-copies, so it's fine today.
   • _NAMESPACED_NAME_PATTERNS is keyed on exact regex text, which is brittle. That is intended, and the error message is good.

6. Hashing raw env values. referenced_values folds the resolved values of env.* references into the task hash. Only the hash is stored, so there is no secret leak. But tasks referencing volatile env vars (e.g. {{ env.CI_JOB_ID }}) will now never be considered fresh. Worth a line in the docs.

7. CLAUDE.md is stale after this change. It still lists parser.py as ~3,540 lines. It also doesn't mention raw_merge.py, recipe_schema.py, field_order.py or template_refs.py in "Core Components". The PR touches CLAUDE.md, so update it.

8. Deleted root __init__.py: this looks like a good cleanup. Please confirm nothing relied on the root being a package (e.g. pytest rootdir/conftest imports).

9. Docs plan (docs/plans/schema-validation-pipeline.md, +1,278 lines): consider trimming it or moving the finished parts into src/tasktree/README.md, so the plan doesn't rot after merge.

Security

No new concerns. The sandbox is a net improvement. SandboxedEnvironment is defence in depth, not a hard boundary. { eval: ... } variables and cmd already execute arbitrary shell by design, so the threat model is mostly about template expressions escaping into the tt process.

Performance

Schema load is cached, and collect_template_refs / expand_variable_refs are linear in recipe size. Nothing concerning.

Actions

• Make rewrite_var_refs / _REFERENCE ignore matches inside string literals and after a . (or use the Jinja AST), and add unit tests for {{ 'var.x' }} and {{ foo.var.bar }}
• Add from e to the ValueError raised in field_order.render_own_patterns
• Add encoding="utf-8" to schema_path().read_text()
• Add a test that loads a state file lacking task_name and prunes it (legacy fallback)
• Document that stale state for edited-but-never-reinvoked tasks persists, and that volatile env.* references defeat incremental skipping
• Update CLAUDE.md "Core Components" (new modules, new line counts) and the Schema Validation section
• Confirm removing the root __init__.py doesn't affect test discovery or imports
• Expand the PR description with the commit/stage breakdown and what remains
• Trim or relocate docs/plans/schema-validation-pipeline.md once its stages are complete

[claude]

Review of PR #212 (schema validation pipeline)

Scope of this review: I read recipe_schema.py, template_refs.py, field_order.py and a few spots in parser.py. I could not run the test suite in this environment, so I can't say whether CI is green. I did not review the +368/-159 change in executor.py, raw_merge.py (600 lines), or most of the parser.py rewrite (+712/-1024).

Summary

This replaces the object-level merge and validation with a raw-dict pipeline: merge imports, prune, schema-validate, evaluate variables lazily, build objects, then render with Jinja at the point of use. The design is sound, and the module docstrings explain why the pieces are shaped as they are. The stage ordering (validate before any eval: runs) is the right security property, and it has its own test in test_validation_ordering.py.

Code quality

• field_order.py, template_refs.py and recipe_schema.py are small, single-purpose and well named. They follow the project's "small named functions" guidance.
• merged_tree_schema derives the merged-tree schema from the file schema. That avoids drift between the two. The ValueError on unknown name patterns makes future schema edits fail loudly, which is good.
• field_order.py:132: raise ValueError(...) inside except CycleError should use from e, or from None if you want a clean message.
• field_order.py:24: _Slot is a frozen dataclass, so it is hashed by all fields, including text: Any. If a pattern value is ever a list or dict (for example, if schema validation lets one through), TopologicalSorter raises TypeError: unhashable. Either set field(compare=False) on text, or key the sorter on (field, position).
• field_order.py:123: if two entries in one list share a name, the later alias silently overwrites the earlier. Check that the schema or the merge rejects duplicates, and test it.
• parser.py:2935,2948,3308,3337 and execute_dynamic_task.py:163: except Exception is broad. Where the call is validator.convert, catching (click.BadParameter, ValueError, TypeError) would be tighter. Also add from e when re-raising.

Potential bugs

• template_refs.py over-matches inside Jinja blocks. _REFERENCE is applied to the whole block text, so it also matches inside string literals and after attribute access. Examples are {{ "see var.x" }} and {{ item.env.foo }}. For collection this is harmless, and the docstring says it is deliberate. For rewrite_var_refs it is not harmless, because it rewrites text. A string literal such as {{ "var.x" }} in an imported file would be changed to var.<ns>.x. Consider a negative lookbehind for . and quote characters, or tokenising with Jinja's lexer (jinja2.Environment().lex) for the rewrite path. At minimum add a test that pins the current behaviour.
• _REFERENCE and rewrite_var_refs handle {{ var['x'] }} and {{ var | ... }} differently from {{ var.x }}. Confirm that subscript access is either rejected or covered. A reference the walker can't see is never evaluated and never hashed, which is the failure mode the _TEMPLATE_BLOCK comment describes.
• Schema lookup (schema_candidates) depends on the wheel force-include in pyproject.toml. Please add a packaging test, or a CI step that builds the wheel and runs tt --list from an installed copy outside the checkout. Otherwise a broken install only shows up for end users.
• load_file_schema is lru_cached and returns a shared mutable dict. The docstring warns about this. Returning MappingProxyType, or a deepcopy in the one caller that needs to mutate it, would enforce the rule instead of relying on discipline.

Performance

• Schema validation of the pruned tree on every invocation is fine. Make sure the validator is constructed once. The Draft*Validator and merged_tree_schema(...) result should be cached the way load_file_schema is. Currently merged_tree_schema deep-copies the schema each call. I did not confirm whether the caller caches it.
• collect_template_refs compiles nothing per call, which is good. expand_variable_refs re-walks definitions, but the chased set bounds it.

Security

• Validate-before-evaluate is the right order for eval: variables.
• rendering.py uses SandboxedEnvironment with StrictUndefined. That is good for an untrusted-recipe threat model. Add a test that attribute escapes such as {{ ''.__class__.__mro__ }} are rejected.

Test coverage

• New unit tests exist for field_order, raw_merge and template_refs, plus test_merged_schema, test_schema, test_schema_errors and an integration test for ordering. That is good coverage of the new modules.
• Missing tests I'd expect: the unhashable and duplicate-name cases above; rewrite_var_refs on string literals and on the {% if %} statement form; and the wheel-packaging test.
• tests/e2e/test_example.py:161,190 use an unconditional @unittest.skip. If those are pre-existing (they reference issue Remove reference to dockerised runners from user readme, MCP and elsewhere #193), ignore this. If this PR added them, they break the project rule against unconditional skips. Use a condition, or fix the example recipe.

Housekeeping

• PR size. About 11k lines of source, including a 1,282-line plan doc, across a parser rewrite. That is far from the "small incremental commits" philosophy. Since the PR says it is opened early, please make sure the final history is split into reviewable steps (merge, prune, validate, render).
• The root __init__.py is deleted. Confirm that nothing relied on it (packaging or pytest rootdir).
• CLAUDE.md in the PR adds the pipeline and module docs. Line-count figures such as parser.py (~3,540 lines) are already stale: parser.py is now ~3,460 lines and executor.py ~2,235. Drop the counts or refresh them.
• docs/plans/schema-validation-pipeline.md is 1.3k lines. Once the feature is done, collapse it into the CLAUDE.md pipeline section and delete the plan, so it doesn't rot.

Actions

• Add from e (or from None) to the ValueError raised in field_order.render_own_patterns
• Make _Slot hashing safe (field(compare=False) on text, or key on (field, position)) and add a test with a non-string pattern value
• Reject or test duplicate names within one inputs/outputs list
• Stop rewrite_var_refs from rewriting var.* inside string literals and after attribute access, or add a test pinning the current behaviour
• Test var subscript forms (var['x']) and {% if var.x %} in collect_template_refs and rewrite_var_refs
• Narrow the except Exception in parser.py (lines ~2935, 2948, 3308, 3337) and chain with from e
• Confirm the merged-tree validator and schema are built once per process, not per parse
• Add a wheel-build and installed-run check that the packaged schema is found
• Add a sandbox-escape rendering test (__class__, __mro__)
• Confirm the two @unittest.skip calls in tests/e2e/test_example.py are pre-existing, or make them conditional
• Confirm the deleted root __init__.py is not needed
• Refresh or remove the stale line counts in CLAUDE.md
• Split the work into small commits before merge, and fold the plan doc into CLAUDE.md
• Review executor.py, raw_merge.py and the parser.py rewrite in detail once the PR is out of draft. I did not cover them here.

🤖 Generated with Claude Code

[claude]

Review of PR #212 (schema-validation pipeline)

I read the new modules (raw_merge.py, recipe_schema.py, template_refs.py) and the new parse_recipe flow in full. I only skimmed the executor, graph and state changes and the tests, and I did not run the test suite. The PR body says it is opened early for CI, so I've kept this to things that matter before merge.

Summary

The design is sound. Merging imports as raw dicts, pruning to reachable tasks, validating against a schema derived from the file schema, and only then building objects is a clear improvement over the old object-level import path. The new modules are small, well documented and mostly single-purpose, and the test volume is high. The points below are mainly robustness gaps on unvalidated input and a few things that cut against the repo's conventions.

Potential bugs / robustness

1. Circular import detection compares unresolved paths (raw_merge.py:108). file_path in import_stack compares Paths built from parent / child_file. A cycle written as ../x/a.yaml or ./a.yaml never compares equal, so it recurses until RecursionError instead of raising CircularImportError. Resolve the path (.resolve()) before the check and the push.

2. The merge runs before schema validation, but assumes valid shapes.

   • _load_yaml returns whatever safe_load gives. A file containing a list or a scalar makes data.pop("imports", None) raise AttributeError.
   • import_spec["file"] and import_spec["as"] raise a bare KeyError, and a non-dict import_spec raises TypeError. Both skip the friendly schema message the PR is introducing.
   • imports: "foo.yaml" (a string) iterates characters.

   Check these shapes in _merge_file and raise a ValueError naming the file, or validate each file's imports section against the file schema.

3. rewrite_var_refs can rewrite more than it should (template_refs.py:30,127). _REFERENCE starts with \b, which also matches after a .. So {{ foo.var.x }} or {{ "a.var.b" }} gets its text rewritten, and the same pattern in a string literal is rewritten too. Over-matching is fine for collection, as the module says, but it is not fine for rewriting. Add a negative lookbehind such as (?<![\w.]) for the rewrite path, or use a separate stricter pattern, and add a test for it.

4. Nested imports and run_in. The docstring says the blanket does not cascade into an import's own imports. The CLAUDE.md "Runner Override" section doesn't mention this. Confirm it matches the old behaviour. If it does, document it. If not, it's a regression.

5. _rewrite_dep_name leaves a dotted dep unchanged unless its root segment is a local import alias. A short test pinning the intended rule for ambiguous cases (an absolute name whose root collides with a local alias) would help.

Performance

• _schema_validate calls merged_tree_schema(load_file_schema()) on every parse. That is a deepcopy plus a pattern rewrite each time, and a new Draft7Validator is built too. Wrap the derived schema (or the validator) in an lru_cache the same way load_file_schema is.
• import jsonschema is deferred inside the function, but jsonschema is now a hard runtime dependency used on every run. That is fine if deliberate, but say so in a comment.
• best_match reports a single error. That's a reasonable UX choice, but multiple independent errors need several runs.

Security

• No new concerns. Schema validation now runs before eval: variables execute, which is a good ordering. test_validation_ordering.py covers it.
• schema_path() falls back to parents[2]/schema. For an installed package this resolves to a location outside it (e.g. site-packages/../schema), which could pick up an unrelated file with that name. Low risk, but restrict the fallback to a source checkout (e.g. only when pyproject.toml sits next to it).

Style / conventions (CLAUDE.md)

• Commit size. This PR is ~7k added lines across ~55 files, which is far from "small, incremental changes". If it merges as one unit, make sure the history is a series of reviewable commits.
• Comments that reference process. Phrases like "slice 4", "slice 6/8" in docstrings (raw_merge.py:2, parser.py:2421) will rot once the plan is finished. Move them to docs/plans/ and keep docstrings about behaviour.
• Root __init__.py deleted. Confirm this was intentional and that nothing (tests, packaging) relied on it.
• CLAUDE.md should list the new modules (raw_merge, recipe_schema, template_refs, field_order) under "Core Components", and the line counts for parser.py/executor.py are now stale.
• _validate_top_level_keys hand-rolls checks that the schema also performs. Fine as transitional code, but make sure a follow-up ticket exists.

Test coverage

Coverage is strong: unit tests for merge, template refs, schema errors and field order, plus integration tests for ordering, pruning and hashing. Gaps I noticed:

• Circular import via non-normalised paths (item 1).
• Malformed imports entries and non-dict top-level YAML in an imported file (item 2).
• rewrite_var_refs on foo.var.x and on string literals (item 3).
• Please confirm tests/e2e/test_packaging.py is not skipped unconditionally in CI (CLAUDE.md forbids blanket skips).

Actions

• Resolve paths in _merge_file so circular imports with non-normalised paths raise CircularImportError; add a test
• Validate shape of loaded YAML and imports entries in raw_merge._merge_file (non-dict file, missing file/as, string imports) and raise ValueError with the file name; add tests
• Tighten the _REFERENCE regex for rewrite_var_refs (don't match after . or inside string literals); add tests
• Cache merged_tree_schema(load_file_schema()) / the validator instead of rebuilding on every parse_recipe
• Restrict the schema_candidates() source-checkout fallback so an installed package can't pick up an unrelated ../../schema file
• Document (or fix) the non-cascading run_in behaviour for nested imports in CLAUDE.md, and add a test pinning it
• Replace "slice N" references in docstrings/comments with behaviour-focused wording
• Confirm the deletion of root __init__.py is intentional
• Update the CLAUDE.md "Core Components" list and line counts for new/changed modules
• Verify tests/e2e/test_packaging.py has no unconditional skips
• Consider splitting the PR or ensuring commit history is in small, reviewable slices

🤖 Generated with Claude Code

[claude]

Review of PR #212 (runtime schema validation pipeline)

Scope note: I could only partly review this. The PR is very large (parser.py +712/−1024, executor.py +368/−159, plus new raw_merge.py, recipe_schema.py, field_order.py, template_refs.py, and a 1,293-line plan doc). The checkout is shallow, so I couldn't diff against main. My permission mode also blocked running the test suite and gh pr checks. I read the PR file list and metadata, the first ~40KB of the diff (CLAUDE.md and the plan doc), pyproject.toml, and recipe_schema.py. I did not read the diffs for parser.py, executor.py, raw_merge.py, graph.py or the tests, and I have not run any tests. Treat the points below as limited to what I read.

What looks good

• recipe_schema.py is small and well documented. merged_tree_schema deep-copies before mutating. load_file_schema is cached, and its docstring warns about the shared dict. _rewrite_name_patterns fails loudly on patterns it doesn't know instead of silently accepting names.
• The ordering guarantee (merge → prune → schema-validate → evaluate eval: variables) is sensible and is pinned by a test.
• The schema ships via hatch force-include, with a source-checkout fallback. tests/e2e/test_packaging.py covers this.
• Promoting jsonschema to a runtime dependency and adding jinja2 is declared in pyproject.toml.

Concerns

1. PR size vs. the project's "small, incremental changes" rule. CLAUDE.md asks for small commits. This PR is a multi-slice rewrite of the parser and executor. The description says it's opened early to check CI, so that's understandable. Before merge, confirm the history is still split into reviewable commits per slice, and consider whether slices 10–12 should land separately.
2. CLAUDE.md diff. In my checkout the working tree's CLAUDE.md differs from the PR head: it reverts the PR's architecture section to the older description. Check that the PR's CLAUDE.md is what you intend to merge, and that line counts such as "~3,460 lines" are still accurate. These drift quickly.
3. Stray root __init__.py deleted. That looks right (it was an empty docstring file). Confirm nothing, such as packaging or test discovery, relied on it.
4. Behaviour changes need release notes. The plan lists many "expected divergences": stricter rejection of unknown or misspelled fields, desc: 42, template strings in private/pin_runner, per-task tt.* in runner fields, and a one-time full re-run from the hash change. These are intentional but user-visible. Add them to the changelog or release notes, and ideally state them in the PR description, which is currently only two sentences.
5. Security: Jinja rendering uses SandboxedEnvironment with StrictUndefined, which is the right choice. Check that eval: and read: variables are only evaluated after schema validation on every entry point. config.py's machine-level runners are explicitly not schema-validated, so a second path stays unvalidated.
6. Performance: load_file_schema is cached, but merged_tree_schema deep-copies the schema and validation runs on every parse. This is probably cheap, but cache the derived merged schema (it is a pure function of a cached input) if parse time shows up in profiling.
7. Test coverage: The new test files (hash sensitivity, imported Jinja expressions, schema corpus, validation ordering) look thorough from the file list. I couldn't confirm CI status or that unit, integration and e2e all pass. The plan itself notes the local pyramid can't catch Windows path-separator or hardcoded-bash problems, so check the Windows CI results before merging.
8. Plan doc. docs/plans/schema-validation-pipeline.md has grown to roughly 1,300 added lines and mixes plan, status log and session notes. Consider trimming it, or moving the completed-slice history out once the work lands.

Actions

• Wait for CI to go green on all platforms (especially Windows) and confirm unit, integration and e2e all pass
• Expand the PR description with the user-visible behaviour changes (new rejections, hash change causing a one-time re-run, runner-field template restrictions) and add release notes
• Verify the PR's CLAUDE.md matches the architecture described, and that the line counts are current
• Confirm the commit history is split per slice, and consider splitting slices 10–12 into separate PRs
• Confirm eval: and read: variables can never evaluate before schema validation on any entry point (CLI --list, --show, --tree, --runner)
• Decide whether to validate machine-config runners (config.py) or document the gap
• Optionally cache merged_tree_schema(load_file_schema()) if parse-time profiling shows it matters
• Trim or archive the plan doc before merge
• Get a fuller review of parser.py, executor.py and raw_merge.py, which I did not read

🤖 Generated with Claude Code

[claude]

Review of PR #212 (schema validation pipeline)

This is a large PR: about 40 files, roughly +3k/-1.3k in source, plus very large test additions. I read the new modules (recipe_schema.py, raw_merge.py) and the packaging changes closely. I only skimmed the parser.py, executor.py, graph.py and state.py changes, and I haven't run the test suite. The PR description says it is opened early to see CI, so treat this as interim feedback.

Strengths

• recipe_schema.py derives the merged-tree schema from the file schema. The unknown-pattern ValueError in _namespaced_pattern makes drift fail loudly, and merged_tree_schema deep-copies the cached schema.
• Error rendering (schema_error_message) is concise and gives task-specific hints. The comments explain why, which fits CLAUDE.md.
• Module split is clean. raw_merge, template_refs, field_order and recipe_schema each have a single responsibility. Cycle detection now compares resolved paths.
• Test coverage is broad across unit, integration and e2e, including a packaging test for the wheel-bundled schema.

Issues / risks

1. Non-mapping recipe files crash the merge (raw_merge.py, _load_yaml / _merge_file). _load_yaml only maps None to {}. A file whose root is a list or scalar (for example - foo) reaches data.pop("imports", None) and raises AttributeError, instead of a friendly "recipe must be a mapping" error. This applies to imported files too. Please add a guard and a test.
2. PR scope and size. CLAUDE.md asks for small, incremental commits. This PR mixes the schema pipeline, a rewrite of the merge, the state-pruning feature, hash-sensitivity changes, the executor refactor and a plan doc. Please make sure the commit history is split along those lines, and consider splitting the PR if that is still practical.
3. Docs drift. CLAUDE.md still lists approximate line counts (parser.py ~3,540, executor.py ~1,940, and so on) and has no entries for raw_merge, recipe_schema, template_refs, field_order or freshness. Actual sizes are now parser.py 3,462 and executor.py 2,235. The Core Components section should be updated.
4. Stale wording. _validate_top_level_keys says "until slice 6/8 replaces this with the schema". Refer to a ticket, or remove it once the slice lands, so it doesn't rot.
5. Schema location. The schema is force-included into the wheel and has two lookup candidates. This is reasonable, but schema_path() does an is_file() check on every call. It is cheap, and load_file_schema is already cached. Please check that sdist builds also carry schema/ (the e2e packaging test covers only the wheel, as far as I can tell).
6. dep_task_names silently ignores multi-key dict deps. This is documented and the schema catches them later. Confirm there is an integration test where the pruning walk plus validation produces the "too many properties" error end to end.
7. Root __init__.py deleted. This looks intentional. Please confirm nothing (tooling, test discovery, imports) relied on it.
8. Security. I found no concerns. YAML is loaded with safe_load, and the schema is read from package or repo paths only.
9. Performance. Per-parse validation against the jsonschema validators is fine. Consider building the validator once, with Draft*Validator(schema) cached alongside load_file_schema, if it isn't already. It is likely noticeable on large recipes with many imports.

Actions

• Handle non-mapping YAML roots in raw_merge._load_yaml / _merge_file (main and imported files) with a clear error, and add unit and integration tests
• Update CLAUDE.md "Core Components" for the new modules (raw_merge, recipe_schema, template_refs, field_order, freshness) and refresh the line counts
• Replace the "slice 6/8" comment in _validate_top_level_keys with something durable, or remove it
• Confirm the sdist includes schema/tasktree-schema.json (add a packaging test if not)
• Add or confirm an end-to-end test for multi-key dict deps (prune, then schema error)
• Confirm the root __init__.py removal breaks nothing
• Cache the compiled jsonschema validator for the file and merged-tree schemas if it isn't already
• Tidy the commit history into small, reviewable commits (or split the PR) before marking ready
• Get CI green on all platforms before merging, with no unconditional skips

[claude]

Review of PR #212 (schema validation pipeline)

Scope of this review: I read the new modules raw_merge.py, recipe_schema.py and template_refs.py, and checked that matching unit test files exist. I did not review the large rewrites in parser.py, executor.py, graph.py, state.py or field_order.py line by line. I did not run the test suite. The PR description says it was opened early to see CI, so treat this as an early review.

What's good

• Doing the merge, pruning and schema validation on the raw dict before any Task/Runner is built is a clean design. merged_tree_schema derives the merged-tree schema from the file schema, and raises on an unknown name pattern. That stops the two schemas drifting silently.
• The comments explain why (cycle detection by resolved path, the (?<![\w.]) lookbehind, statement blocks {% %} counting as references). Functions are small and named for what they do, as CLAUDE.md asks.
• Lazy name errors keyed by merged name keep unreferenced bad items from breaking a run. That matches the old behaviour.
• Each new module has a test file (test_raw_merge, test_template_refs, test_merged_schema, test_schema_errors, test_field_order, and others).

Potential bugs / issues

1. Malformed imports crashes before validation (raw_merge.py:137-141). import_spec["file"] and ["as"] raise a bare KeyError/TypeError if an entry is not a mapping or lacks a key. A string imports: value iterates character by character. Validate the shape here, or validate each file against the file schema before merging, so the user gets the friendly schema message.
2. rewrite_var_refs rewrites inside string literals. {{ "see var.x" }} has its literal text changed on import, because the regex matches anywhere in the block. The over-match is fine for discovery but not for rewriting. Consider restricting rewrites to identifier positions, or document this as a known limitation with a test.
3. Inconsistent interpreter: {use: ...} namespacing. _namespace_interpreter_use handles named runners in imported files. An inline runner dict on an imported task, and a task-level interpreter: {use: x}, are left un-namespaced (the comment says this is deliberate for task-level). Imported interpreters are namespaced, so a use: inside an imported inline runner may fail to resolve. Please add a test that pins the intended behaviour, and fix it if it is not intended.
4. Dep rewriting heuristic (_rewrite_dep_name). A dotted dep is treated as "absolute" unless its root segment is a local import alias. A task in an imported file that depends on a dotted name that is neither an alias nor a real absolute path gets no error until graph build. Worth a test for the error message.
5. Hyphen in the reference regex. The name pattern allows -, so {{ var.count-1 }} is read as the variable count-1. Harmless for discovery, but another place where the regex can differ from Jinja's grammar. A comment or test would help.
6. Schema file lookup. schema_candidates() checks Path(__file__).parent/"schema" and then parents[2]/"schema". Please add a test or CI step that builds the wheel and checks the schema is present in the installed package, since the force-include mapping in pyproject.toml is the only thing keeping an installed tt working.

Performance

• collect_template_refs and expand_variable_refs are linear, and the schema load is lru_cached. _merge_file calls resolve() on every import-stack entry at each level, which is trivial at realistic depths.
• merged_tree_schema deep-copies on every call. Consider caching it alongside load_file_schema.

Security

• Nothing concerning. YAML is read with safe_load. Import paths are joined onto the importing file's directory with no containment check. That matches existing behaviour, but it means ../ imports are allowed. Please confirm this is intended.

Process / conventions

• The PR is very large (parser.py +712/-1024, executor.py +368/-159, a 1,293-line plan doc, CLAUDE.md edits). CLAUDE.md asks for small incremental commits, so please make sure the history is split into reviewable steps.
• The CLAUDE.md "Core Components" section should list raw_merge.py, recipe_schema.py, template_refs.py and field_order.py, and its parser.py line count is now stale.
• The deleted root __init__.py looks right, but please mention it in the PR description.
• Please update the PR description before merge with the feature summary and migration notes.

Actions

• Validate imports entry shape in _merge_file (non-mapping entry, missing file/as, non-list imports) and add unit tests for the error messages
• Decide and test the behaviour of rewrite_var_refs for var. text inside string literals
• Add tests (and a fix if needed) for interpreter: {use: ...} in imported inline runners and task-level interpreters
• Add a test for the error message when an imported task has a dotted dep that is not a local import alias
• Add a test or CI step that the built wheel contains tasktree/schema/tasktree-schema.json
• Cache merged_tree_schema(load_file_schema()) so it is not deep-copied on every parse
• Confirm that ../ import paths are intentionally allowed
• Update the CLAUDE.md architecture section for the new modules and the shrunken parser.py
• Update the PR description with a summary and migration notes before merge
• Confirm the full suite (uv run --extra dev python -m pytest tests/) passes in CI, including the e2e tests, as I did not run it