Skip to content

test(parity): run the corpus with Reflect views of the contexts (0322) - #722

Merged
rmorozov merged 1 commit into
masterfrom
claude/project-thread-k2hevb
Nov 1, 2026
Merged

rmorozov merged 1 commit into
masterfrom
claude/project-thread-k2hevb

Conversation

@rmorozov

@rmorozov rmorozov commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

Requested by Ruslan · project thread

Problem

Since 0311 S7, the parity corpus passes every context through jinja2::ToValue, which makes a copy. The view path, jinja2::Reflect, answers lookups, iteration, comparisons and filters through the generic map and list adapters. Until now only the ~100 rows of test/binding/conformance.h checked it (task 0322).

Change

  • ParityReflect/* in test/parity/parity_test.cpp renders every case with its context as items of one Reflect view of a shared ordered_json copy. The items keep the document alive, not the harness. The result is held to the copy's result, the same way the wide run is held to the narrow one, so a divergence the copy already has is listed only once. The pass runs only when JINJA2CPP_PARITY_CONTEXT=reflect is set. Without it the suite is empty (GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST), so the per-PR ctest -R parity does not change.
  • test/parity/divergences/reflect.txt is one allow-list for the whole pass, with ids reflect.<id>, loaded together with the area files. DivergencesNameExistingCases strips the prefix. update_divergences.py leaves alone any line that has no recorded outcome, so normal runs do not touch this file.
  • In bindings.yml, the ASan job (gcc-14, address+undefined, Boost bigint) runs the pass after the binding tests. It runs nightly, on demand, and on PRs that match the job's path filter. The filter now also includes parity_test.cpp and reflect.txt.
  • test/parity/README.md gets a "reflect run" section.

What the pass found (11 lines)

  • 5 mutation cases, already under 0049 P5: the methods.alias_* cases (m.update on a loop item or on a set alias of a JSON list item, and nd['k'].append). When a JSON value is read twice, each read gets its own accessor, so a write lands in one cell and the other read does not see it. 0049 already records this as "no stable identity until P5's IJsonNode marker".
  • 6 non-mutation cases, filed as 0370: a JSON object seen through Reflect gets MapAttrPolicy::KeysFirst, the policy for reflected structs. The copy gets MethodsFirst, and Python treats the object as a dict. As a result, d.items returns the key items instead of the method, attr('name') finds keys, and is sequence is false. The fix belongs in GenericMapAdapter plus a dict marker on the JSON adapter, ideally the same IJsonNode that 0049 P5 plans (D7 option c). Those files are outside this group's ownership, so the fix is filed rather than made here.

0322's done-when says reflect.txt holds only mutation cases. The six 0370 lines are an exception until 0370 lands, so 0322 stays in-progress.

Validation

  • Debug (gcc, nlohmann): Parity* passed 5704 tests without the switch and 8554 with JINJA2CPP_PARITY_CONTEXT=reflect.

  • ASan+UBSan (clang-18 RelWithDebInfo, nlohmann_json;boost_json, detect_leaks=1, detect_stack_use_after_return=1): ParityReflect/*:ParityRegistry.*:*Json* passed 2973 tests with no sanitizer reports.

  • scripts/preflight.sh: everything passed (clang-format, clang-tidy 22.1.8 on changed lines, null comparisons, task files).

  • No rendering code changed, so this PR cannot make a line in test/chat_templates/divergences.txt (test: Hugging Face chat template suite against transformers (0360 phase 1) #720) stale.

  • Not done: a wide reflect run (only narrow is covered), and MSVC/Apple Clang builds of the new code (CI only).

  • CI on d5d7829: 40 checks green, including bindings address+undefined (gcc-14), which runs the pass. Three macos-14 jobs were cancelled by GitHub's macOS 14 image brownout (the image retires 2026-11-02). That is not caused by this PR, and it was flagged for a separate CI change.

Roles: main session only. The verifier was not run because this change touches only tests and CI, not src/ or include/. Red CI pushes: 0 of 1. Codex: 0 findings, 0 fixed, 0 declined.

https://claude.ai/code/session_01AWNL7nqXUvCsDpYRatAhes


Generated by Claude Code

ParityReflect/* renders every case with its context as items of one
jinja2::Reflect view of a shared copy of the document, held to the
ToValue copy's result as the wide run is held to the narrow one. It runs
only with JINJA2CPP_PARITY_CONTEXT=reflect: nightly in bindings.yml,
under ASan, where it checks the owner chain of every reflected item.

divergences/reflect.txt lists 11 cases: five mutation cases that need
0049 P5's stable JSON identity, and six where a JSON view is looked up
keys-first like a reflected struct while Python treats it as a dict,
filed as 0370.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AWNL7nqXUvCsDpYRatAhes
@rmorozov rmorozov self-assigned this Nov 1, 2026
rmorozov pushed a commit that referenced this pull request Nov 1, 2026

rmorozov commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

CI on d5d7829: 40 checks green, including bindings address+undefined (gcc-14), which now runs the Reflect pass. The three macos-14 jobs (macos-14 AppleClang C++17/C++23 Release and bindings macos-14 AppleClang C++17 Release) were cancelled by GitHub's scheduled macOS 14 runner-image brownout. Their annotation says the image retires on 2026-11-02 (actions/runner-images#13518). This PR does not cause those cancellations, and they will hit every PR. Moving the jobs off macos-14 is a CI change that also touches macos-build.yml, so it goes to its own thread instead of this PR. The macos-15 AppleClang jobs passed on this head.

@codex review


Generated by Claude Code

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T16:54:36.897354Z d5d7829 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: d5d7829d91

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@rmorozov
rmorozov marked this pull request as ready for review November 1, 2026 16:54
@rmorozov
rmorozov merged commit bfd006f into master Nov 1, 2026
41 of 44 checks passed
rmorozov pushed a commit that referenced this pull request Nov 1, 2026
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTQTriEsFrrBgcYNKDUUF4
rmorozov pushed a commit that referenced this pull request Nov 3, 2026
- 0322 done: #722 merged and the nightly bindings run is green.
- reflect.txt: the five mutation lines that named 0049 (done) now name 0212,
  which took 0049 P5 (JSON accessor identity); 0212 says so.
- docs/parity.md summary regenerated: 3043 cases, 3016 match.
- release-2.0.md G1: 0186's O2 version macros are done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017r4qbSd8vaW6bzt8m4g9it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants