Skip to content

Pin that selected verification calls back through a plain function - #323

Merged
Deicyde merged 1 commit into
mainfrom
test/pin-plain-verify-callback
Oct 15, 2026
Merged

Deicyde merged 1 commit into
mainfrom
test/pin-plain-verify-callback

Conversation

@Deicyde

@Deicyde Deicyde commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up from the post-merge review of #307 (#307 (comment)): one small test, no code change.

  • Slim the Lean capture code and tests #257 added, without changing what they pin #307 passes verify_recorded to _recheck_scanned_directory as a callback. A lambda there adds a Python frame per folder level, which test_selected_verification_recurses_in_three_frames_per_folder already pins. A C-implemented wrapper such as functools.partial adds no Python frame but spends CPython's C recursion budget at every level, so deep Lean trees fail with "nested too deeply" where main captured them. fd731bd removed the partial; nothing caught putting it back.
  • test_selected_verification_calls_back_through_a_plain_function records the type of every verify_kept callback in a Lean capture and requires a plain Python function.

Tests

  • The new test fails with verify_kept=functools.partial(verify_recorded) restored and passes on main; the frame test passes either way, which is the gap.
  • tests/test_lean_sources.py and tests/test_tree_snapshot_limits.py pass on Python 3.13; the new test passes on 3.10. ruff check is clean.

#307 passes verify_recorded to _recheck_scanned_directory as a callback. Its review history found two ways a wrapper costs capture depth: a lambda adds a Python frame per folder level (pinned by test_selected_verification_recurses_in_three_frames_per_folder), and a C-implemented callable such as functools.partial adds no Python frame but spends CPython's separate C recursion budget per level, so a deep Lean tree fails with "nested too deeply" where main captured it. fd731bd removed the partial, but nothing caught putting it back: the frame count cannot see it.

The new test records the type of every verify_kept callback in a Lean capture and requires a plain Python function. It fails with functools.partial(verify_recorded) restored and passes on main.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 14, 2026
@Deicyde

Deicyde commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Merge readiness: ready to merge (reviewed at e82523d)

  • CI is green on every job. On the Linux test jobs the new test runs, not skips: one more pass than main, skips unchanged.
  • An independent review mutated the callback:
    • The new test fails for every C-implemented callable it tried: functools.partial(verify_recorded), the C method-wrapper verify_recorded.__call__, an lru_cache-wrapped function and a staticmethod. It passes with the plain callback on main, on Python 3.13 and 3.10.
    • It does not interfere with test_selected_verification_recurses_in_three_frames_per_folder: 30 runs in both orders all passed.
    • It is stable under pytest-randomly (4 seeds), and it skips correctly where descriptor capture is unavailable.
  • Worth its lines: 26 lines guard a regression that Slim the Lean capture code and tests #257 added, without changing what they pin #307's review history introduced once and that the frame test cannot see.

Optional: the body says a C-implemented wrapper makes deep trees fail. That only happens on CPython 3.12 and later, with a raised recursion limit or little C budget left at the call site.

It is a draft, so it needs to be marked ready before merging.

@Deicyde
Deicyde marked this pull request as ready for review October 15, 2026 17:20
@Deicyde
Deicyde merged commit 3ec4e59 into main Oct 15, 2026
11 checks passed
Deicyde added a commit that referenced this pull request Oct 15, 2026
Main brings #331, #332, #335, #341, #338, #336, #323, #343 and #337.
lean.py, render.py and tests/test_render.py conflicted, and four
changes were needed outside the conflicts:

- lean.py: snapshot_project_sources and _open_stable_project_sources
  lose the opaque_markers parameter #336 removed and keep this
  branch's names= keyword. The publication constants #336 moved here
  take this branch's autoform-publication/v3, and _REPLACEABLE_SCHEMAS
  moves with _is_complete_publication, since lean.py cannot import
  render.py. Every Lean capture skips a complete v1, v2 or v3 site;
  with main's v2 alone it would index this branch's sites and their
  mkdocs copies.
- render.py: imports the constants and _REPLACEABLE_SCHEMAS from
  lean.py, drops its own _is_complete_publication, and its Lean capture
  passes names= and relies on the marker every capture now has.
- tests/test_render.py: keeps both sides' tests. #336's audit test
  forwards the names= keyword audit passes, and this branch's revision
  test builds TreeSnapshot without root_identity and placeholders,
  which #341 deleted.
- graph.py: _catalog_has_local_ledger takes #337's guards (a link
  urlsplit rejects is skipped, RuntimeError from resolve is caught,
  os.path.isfile replaces Path.is_file), so a catalog's Sources link
  can no longer crash load_graph. A new test in tests/test_graph.py
  covers an invalid URL, an over-long name and a symlink loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant